Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve server/client stuck at the test end before results-exchange #1527

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Resolve server/client stuck at the test end before results-exchange #1527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e396362
Reimplement for Multi-thread and remove write_set
davidBar-On Dec 10, 2023
fcf1c46
Completely remove serevr write_set per comment received
davidBar-On May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/iperf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ struct iperf_test
/* Select related parameters */
int max_fd;
fd_set read_set; /* set of read sockets */
fd_set write_set; /* set of write sockets */

/* Interval related members */
int omitting;
Expand Down
74 changes: 46 additions & 28 deletions src/iperf_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,9 +1698,6 @@ iperf_parse_arguments(struct iperf_test *test, int argc, char **argv)
} else if (test->role == 'c' && (test->server_skew_threshold != 0)){
i_errno = IESERVERONLY;
return -1;
} else if (test->role == 'c' && rcv_timeout_flag && test->mode == SENDER){
i_errno = IERVRSONLYRCVTIMEOUT;
return -1;
} else if (test->role == 's' && (server_rsa_private_key || test->server_authorized_users) &&
!(server_rsa_private_key && test->server_authorized_users)) {
i_errno = IESETSERVERAUTH;
Expand Down Expand Up @@ -1857,13 +1854,28 @@ void iperf_close_logfile(struct iperf_test *test)
int
iperf_set_send_state(struct iperf_test *test, signed char state)
{
int l;
const char c = 0;

if (test->debug_level >= DEBUG_LEVEL_INFO)
fprintf(stderr, "Setting and sending new test state %d (changed from %d).\n", state, test->state);

if (test->ctrl_sck >= 0) {
test->state = state;
if (Nwrite(test->ctrl_sck, (char*) &state, sizeof(state), Ptcp) < 0) {
i_errno = IESENDMESSAGE;
return -1;
}

if (state == IPERF_DONE || state == CLIENT_TERMINATE) {
// Send additional bytes to complete the sent size to JSON length prefix,
// in case the other size is waiting for JSON.
l = sizeof(uint32_t) - sizeof(state);
while (l-- > 0)
Nwrite(test->ctrl_sck, &c, 1, Ptcp);
}
}

return 0;
}

Expand Down Expand Up @@ -2386,6 +2398,9 @@ get_parameters(struct iperf_test *test)
static int
send_results(struct iperf_test *test)
{
if (test->debug_level >= DEBUG_LEVEL_INFO)
fprintf(stderr, "Sending results.\n");

int r = 0;
cJSON *j;
cJSON *j_streams;
Expand Down Expand Up @@ -2487,6 +2502,7 @@ send_results(struct iperf_test *test)
}
cJSON_Delete(j);
}

return r;
}

Expand All @@ -2495,6 +2511,9 @@ send_results(struct iperf_test *test)
static int
get_results(struct iperf_test *test)
{
if (test->debug_level >= DEBUG_LEVEL_INFO)
fprintf(stderr, "Getting results.\n");

int r = 0;
cJSON *j;
cJSON *j_cpu_util_total;
Expand Down Expand Up @@ -3106,14 +3125,7 @@ iperf_free_test(struct iperf_test *test)
free(test->remote_congestion_used);
if (test->timestamp_format)
free(test->timestamp_format);
if (test->omit_timer != NULL)
tmr_cancel(test->omit_timer);
if (test->timer != NULL)
tmr_cancel(test->timer);
if (test->stats_timer != NULL)
tmr_cancel(test->stats_timer);
if (test->reporter_timer != NULL)
tmr_cancel(test->reporter_timer);
iperf_cancel_test_timers(test);

/* Free protocol list */
while (!SLIST_EMPTY(&test->protocols)) {
Expand Down Expand Up @@ -3193,22 +3205,9 @@ iperf_reset_test(struct iperf_test *test)
SLIST_REMOVE_HEAD(&test->streams, streams);
iperf_free_stream(sp);
}
if (test->omit_timer != NULL) {
tmr_cancel(test->omit_timer);
test->omit_timer = NULL;
}
if (test->timer != NULL) {
tmr_cancel(test->timer);
test->timer = NULL;
}
if (test->stats_timer != NULL) {
tmr_cancel(test->stats_timer);
test->stats_timer = NULL;
}
if (test->reporter_timer != NULL) {
tmr_cancel(test->reporter_timer);
test->reporter_timer = NULL;
}

iperf_cancel_test_timers(test);

test->done = 0;

SLIST_INIT(&test->streams);
Expand Down Expand Up @@ -3252,7 +3251,6 @@ iperf_reset_test(struct iperf_test *test)
test->no_delay = 0;

FD_ZERO(&test->read_set);
FD_ZERO(&test->write_set);

test->num_streams = 1;
test->settings->socket_bufsize = 0;
Expand Down Expand Up @@ -3339,6 +3337,26 @@ iperf_reset_stats(struct iperf_test *test)
}
}

void
iperf_cancel_test_timers(struct iperf_test *test)
{
if (test->stats_timer != NULL) {
tmr_cancel(test->stats_timer);
test->stats_timer = NULL;
}
if (test->reporter_timer != NULL) {
tmr_cancel(test->reporter_timer);
test->reporter_timer = NULL;
}
if (test->omit_timer != NULL) {
tmr_cancel(test->omit_timer);
test->omit_timer = NULL;
}
if (test->timer != NULL) {
tmr_cancel(test->timer);
test->timer = NULL;
}
}

/**************************************************************************/

Expand Down
8 changes: 7 additions & 1 deletion src/iperf_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,12 @@ int iperf_defaults(struct iperf_test * testp);
*/
void iperf_free_test(struct iperf_test * testp);

/**
* iperf_cancel_test_timers -- cancel test timers
*
*/
void iperf_cancel_test_timers(struct iperf_test * testp);

/**
* iperf_new_stream -- return a net iperf_stream with default values
*
Expand Down Expand Up @@ -415,7 +421,7 @@ enum {
IESKEWTHRESHOLD = 29, // Invalid value specified as skew threshold
IEIDLETIMEOUT = 30, // Invalid value specified as idle state timeout
IERCVTIMEOUT = 31, // Illegal message receive timeout
IERVRSONLYRCVTIMEOUT = 32, // Client receive timeout is valid only in reverse mode
// [DELETED] IERVRSONLYRCVTIMEOUT = 32, // Client receive timeout is valid only in reverse mode
IESNDTIMEOUT = 33, // Illegal message send timeout
IEUDPFILETRANSFER = 34, // Cannot transfer file using UDP
IESERVERAUTHUSERS = 35, // Cannot access authorized users file
Expand Down
35 changes: 26 additions & 9 deletions src/iperf_client_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,6 @@ iperf_connect(struct iperf_test *test)
return -1;
}
FD_ZERO(&test->read_set);
FD_ZERO(&test->write_set);

make_cookie(test->cookie);

Expand Down Expand Up @@ -499,6 +498,9 @@ iperf_connect(struct iperf_test *test)
int
iperf_client_end(struct iperf_test *test)
{
if (test->debug_level >= DEBUG_LEVEL_INFO)
fprintf(stderr, "Ending client test.\n");

if (NULL == test)
{
iperf_err(NULL, "No test\n");
Expand Down Expand Up @@ -533,7 +535,7 @@ iperf_run_client(struct iperf_test * test)
{
int startup;
int result = 0;
fd_set read_set, write_set;
fd_set read_set;
struct iperf_time now;
struct timeval* timeout = NULL;
struct iperf_stream *sp;
Expand All @@ -544,6 +546,7 @@ iperf_run_client(struct iperf_test * test)
int64_t t_usecs;
int64_t timeout_us;
int64_t rcv_timeout_us;
int64_t rcv_timeout_value_in_us;
int i_errno_save;

if (NULL == test)
Expand Down Expand Up @@ -580,8 +583,9 @@ iperf_run_client(struct iperf_test * test)

/* Begin calculating CPU utilization */
cpu_util(NULL);
rcv_timeout_value_in_us = (test->settings->rcv_timeout.secs * SEC_TO_US) + test->settings->rcv_timeout.usecs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change creates a scenario with an early timeout.
Given:

  • test->mode == SENDER
  • rcv_timeout_value_in_us > 0
  • test->state == TEST_END (or EXCHANGE_RESULTS or DISPLAY_RESULTS)

This implies that rcv_timeout_us = 0. Then on line 642, the if statement will evaluate to something like if (t_usecs > 0); always being true since t_usecs will pretty much always be greater than 0.

It might make more sense to split the blocks so the correct timeout is used:
(I've also renamed rcv_timeout_value_in_us -> end_rcv_timeout and rcv_timeout_us -> running_rcv_timeout to hopefully make their use more clear)

    if (result < 0 && errno != EINTR) {
  	    i_errno = IESELECT;
	    goto cleanup_and_fail;
    } else if ( result == 0 && (running_rcv_timeout > 0 && test->state == TEST_RUNNING)) {
        /*
            * If nothing was received in non-reverse running state
            * then probably something got stuck - either client,
            * server or network, and test should be terminated./
            */
        iperf_time_now(&now);
        if (iperf_time_diff(&now, &last_receive_time, &diff_time) == 0) {
            t_usecs = iperf_time_in_usecs(&diff_time);
            if (t_usecs > running_rcv_timeout) {
                /* Idle timeout if no new blocks received */
                if (test->blocks_received == last_receive_blocks) {
                    i_errno = IENOMSG;
                    goto cleanup_and_fail;
                }
            }

        }
    } else if (result == 0 && (end_rcv_timeout > 0 && (test->state == TEST_END 
                                        || test->state == EXCHANGE_RESULTS 
                                        || test->state == DISPLAY_RESULTS))) {
        iperf_time_now(&now);
        if (iperf_time_diff(&now, &last_receive_time, &diff_time) == 0) {
            t_usecs = iperf_time_in_usecs(&diff_time);
            if (t_usecs > end_rcv_timeout) {
                /* Idle timeout if no new blocks received */
                if (test->blocks_received == last_receive_blocks) {
                    i_errno = IENOMSG;
                    goto cleanup_and_fail;
                }
            }

        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only rcv_timeout_us value is used for timeout (in the select statement). The value of rcv_timeout_value_in_us is only used to initialize rcv_timeout_us and to indicate for for the ending states whether receive timeout was requested (as there is no "timeout requested" setting). Therefore, the current code is correct in that respect.

I agree that probably rcv_timeout_value_in_us may have a better name, like rcv_timeout_setting_us but I won't make a change just for that.

if (test->mode != SENDER)
rcv_timeout_us = (test->settings->rcv_timeout.secs * SEC_TO_US) + test->settings->rcv_timeout.usecs;
rcv_timeout_us = rcv_timeout_value_in_us;
else
rcv_timeout_us = 0;

Expand All @@ -591,12 +595,17 @@ iperf_run_client(struct iperf_test * test)
startup = 1;
while (test->state != IPERF_DONE) {
memcpy(&read_set, &test->read_set, sizeof(fd_set));
memcpy(&write_set, &test->write_set, sizeof(fd_set));
iperf_time_now(&now);
timeout = tmr_timeout(&now);

// In reverse active mode client ensures data is received
if (test->state == TEST_RUNNING && rcv_timeout_us > 0) {
// In reverse/bidir active mode client ensures data is received.
// Same for receiving control messages at the end of the test.
if ( (rcv_timeout_us > 0 && test->state == TEST_RUNNING)
|| (rcv_timeout_value_in_us > 0
&& (test->state == TEST_END
|| test->state == EXCHANGE_RESULTS
|| test->state == DISPLAY_RESULTS)) )
{
timeout_us = -1;
if (timeout != NULL) {
used_timeout.tv_sec = timeout->tv_sec;
Expand All @@ -621,16 +630,22 @@ iperf_run_client(struct iperf_test * test)

result = select(test->max_fd + 1,
&read_set,
(test->state == TEST_RUNNING && !test->reverse) ? &write_set : NULL,
NULL,
NULL,
timeout);
#else
result = select(test->max_fd + 1, &read_set, &write_set, NULL, timeout);
result = select(test->max_fd + 1, &read_set, NULL, NULL, timeout);
#endif // __vxworks or __VXWORKS__
if (result < 0 && errno != EINTR) {
i_errno = IESELECT;
goto cleanup_and_fail;
} else if (result == 0 && test->state == TEST_RUNNING && rcv_timeout_us > 0) {
} else if ( result == 0 &&
((rcv_timeout_us > 0 && test->state == TEST_RUNNING)
|| (rcv_timeout_value_in_us > 0
&& (test->state == TEST_END
|| test->state == EXCHANGE_RESULTS
|| test->state == DISPLAY_RESULTS))) )
{
/*
* If nothing was received in non-reverse running state
* then probably something got stuck - either client,
Expand Down Expand Up @@ -748,6 +763,8 @@ iperf_run_client(struct iperf_test * test)
test->done = 1;
cpu_util(test->cpu_util);
test->stats_callback(test);
// Timers not needed at test end and may interrupt with select() receive timeout
iperf_cancel_test_timers(test);
if (iperf_set_send_state(test, TEST_END) != 0)
goto cleanup_and_fail;
}
Expand Down
4 changes: 0 additions & 4 deletions src/iperf_error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,10 +377,6 @@ iperf_strerror(int int_errno)
case IEUDPFILETRANSFER:
snprintf(errstr, len, "cannot transfer file using UDP");
break;
case IERVRSONLYRCVTIMEOUT:
snprintf(errstr, len, "client receive timeout is valid only in receiving mode");
perr = 1;
break;
case IEDAEMON:
snprintf(errstr, len, "unable to become a daemon");
perr = 1;
Expand Down
38 changes: 12 additions & 26 deletions src/iperf_server_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ iperf_server_listen(struct iperf_test *test)
}

FD_ZERO(&test->read_set);
FD_ZERO(&test->write_set);
FD_SET(test->listener, &test->read_set);
if (test->listener > test->max_fd) test->max_fd = test->listener;

Expand Down Expand Up @@ -236,7 +235,6 @@ iperf_handle_message_server(struct iperf_test *test)
test->stats_callback(test);
SLIST_FOREACH(sp, &test->streams, streams) {
FD_CLR(sp->socket, &test->read_set);
FD_CLR(sp->socket, &test->write_set);
close(sp->socket);
}
test->reporter_callback(test);
Expand Down Expand Up @@ -266,8 +264,8 @@ iperf_handle_message_server(struct iperf_test *test)
iperf_err(test, "the client has terminated");
SLIST_FOREACH(sp, &test->streams, streams) {
FD_CLR(sp->socket, &test->read_set);
FD_CLR(sp->socket, &test->write_set);
close(sp->socket);
sp->socket = -1;
}
test->state = IPERF_DONE;
break;
Expand All @@ -294,6 +292,7 @@ server_timer_proc(TimerClientData client_data, struct iperf_time *nowP)
sp = SLIST_FIRST(&test->streams);
SLIST_REMOVE_HEAD(&test->streams, streams);
close(sp->socket);
sp->socket = -1;
iperf_free_stream(sp);
}
close(test->ctrl_sck);
Expand Down Expand Up @@ -444,7 +443,6 @@ cleanup_server(struct iperf_test *test)
SLIST_FOREACH(sp, &test->streams, streams) {
if (sp->socket > -1) {
FD_CLR(sp->socket, &test->read_set);
FD_CLR(sp->socket, &test->write_set);
close(sp->socket);
sp->socket = -1;
}
Expand All @@ -463,28 +461,13 @@ cleanup_server(struct iperf_test *test)
close(test->prot_listener);
test->prot_listener = -1;
}

/* Cancel any remaining timers. */
if (test->stats_timer != NULL) {
tmr_cancel(test->stats_timer);
test->stats_timer = NULL;
}
if (test->reporter_timer != NULL) {
tmr_cancel(test->reporter_timer);
test->reporter_timer = NULL;
}
if (test->omit_timer != NULL) {
tmr_cancel(test->omit_timer);
test->omit_timer = NULL;
}

iperf_cancel_test_timers(test); /* Cancel any remaining timers. */

if (test->congestion_used != NULL) {
free(test->congestion_used);
test->congestion_used = NULL;
}
if (test->timer != NULL) {
tmr_cancel(test->timer);
test->timer = NULL;
}
}


Expand All @@ -497,7 +480,7 @@ iperf_run_server(struct iperf_test *test)
#if defined(HAVE_TCP_CONGESTION)
int saved_errno;
#endif /* HAVE_TCP_CONGESTION */
fd_set read_set, write_set;
fd_set read_set;
struct iperf_stream *sp;
struct iperf_time now;
struct iperf_time last_receive_time;
Expand Down Expand Up @@ -560,7 +543,6 @@ iperf_run_server(struct iperf_test *test)
}

memcpy(&read_set, &test->read_set, sizeof(fd_set));
memcpy(&write_set, &test->write_set, sizeof(fd_set));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this means that write_set may have an uninitialized values on line 581. Should you just remove the variable entirely since it it unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment. Forgot to remove all occurances of write_set... Now removed (with rebase).


iperf_time_now(&now);
timeout = tmr_timeout(&now);
Expand All @@ -572,7 +554,11 @@ iperf_run_server(struct iperf_test *test)
used_timeout.tv_usec = 0;
timeout = &used_timeout;
}
} else if (test->mode != SENDER) { // In non-reverse active mode server ensures data is received
} else if (test->mode != SENDER // In non-reverse active mode server ensures data is received.
|| test->state == TEST_END // Same for receiving control messages at the end of the test.
|| test->state == EXCHANGE_RESULTS
|| test->state == DISPLAY_RESULTS)
{
timeout_us = -1;
if (timeout != NULL) {
used_timeout.tv_sec = timeout->tv_sec;
Expand All @@ -590,7 +576,7 @@ iperf_run_server(struct iperf_test *test)
timeout = &used_timeout;
}

result = select(test->max_fd + 1, &read_set, &write_set, NULL, timeout);
result = select(test->max_fd + 1, &read_set, NULL, NULL, timeout);
if (result < 0 && errno != EINTR) {
cleanup_server(test);
i_errno = IESELECT;
Expand Down
Loading
Loading