Variable changes by main and work threads without lock protection

[davidBar-On]

Hi, the steam's green_light flag is changes by both the main and the worker thread, but without locking during the change. This may not a problem, as this is not a counter, etc., so my question is whether this was evaluated and found to be a non-issue?

The green_light for a stream may be changed when calling iperf_check_throttle():

• On the worker threads it is called from iperf_send_mt(().
• On the main thread is in called periodically from send_timer_proc() which is set by iperf_create_send_timers(). (It seems that the periodic call to send_timer_proc() should have been set by each of the worker threads, and not by the main thread.)

[bmah888]

You might be on to something here. The send_timer_proc() function and things around it are leftover from the original single-threaded implementation used it for pacing all of the connections. This might not have gotten completely converted to work correctly in a multi-threaded environment. Note this primarily affects the --bitrate form of pacing, but probably doesn't have any effect on --fq-rate (for systems that support it).

More study is needed.

[bmah888]

So let's see. Your summary about where green_light gets accessed (from iperf_check_throttle()) looks right to me.

The periodic calls to send_timer_proc() only run in the context of the main thread, because there's only one timer structure which is global to the process (and it's serviced in the main thread from the calls to tmr_run() in iperf_run_client() and iperf_run_server()). Other actions that fire from the global timer structure are the per-interval reports, the end of the test for time-based tests, and the omit timer. So that global timer structure is pretty firmly tied to the main thread.

As you also noted, iperf_check_throttle (and green_light) are specific to one test and hence one thread. So we probably shouldn't be creating timers that call send_timer_proc() from the main thread (that might even mean that send_timer_proc() and iperf_create_send_timers() could go away altogether). In the single-threaded iperf3, we needed this arrangement with periodic checks to pace the streams. In the multi-threaded iperf3, the individual threads are (for better or worse) spinning in iperf_client_worker_run() and iperf_server_worker_run(), and this causes iperf_check_throttle() to get called fairly frequently (maybe too often?). So at least the way to code stands right now, we might be able to get rid of everything that manipulates green_list in the main thread, maybe after some preliminary initialization.

However I just noticed your PR #1743 (haven't read it yet though). Does that affect this discussion?

[davidBar-On]

I just noticed your PR #1743 (haven't read it yet though). Does that affect this discussion?

Yes, PR #1743 is very relevant to this discussion and one of its changes it the obsolete send_timer_proc() as you suggested. I started making the changes because of issue #1741, and not because of this discussion. However, since the root cause of the issue is the infinite loop that checks the green_light without the select() (that is used only by the main thread), the suggested approach is using nanosleep (when supported) instead of of looping until green_light green light is set. The PR also makes the pacing timer redundant, and I believe that also burst/multisend can be removed.

Regarding this discussion, there can also be a problem with the interval statistics. This is because the statistics are collected by the streams threads, but the statistics are handled by the main thread, per the -i timer interrupts. Not sure if this is a big issue but may also need to be checked.

[bmah888]

The interval statistics are working as designed...all of the counters involved are atomic variables and it's intended that one thread writes while the other reads.

I like the general thought behind PR #1743, although I still haven't really had a chance to give it an in-depth review or test it. Most of the old pacing code should probably go away because it was really made for a single-threaded process. I notice that PR #1743 has a number of cases of conditional compilation dependent on nanosleep(), et al. Is the code intended to work in places where nanosleep() doesn't exist, or is this defensive coding? (On the other hand, nanosleep() is a POSIX.1 system call and I think every modern host supports this.)

[davidBar-On]

conditional compilation dependent on nanosleep(), et al. Is the code intended to work in places where nanosleep() doesn't exist, or is this defensive coding? (On the other hand, nanosleep() is a POSIX.1 system call and I think every modern host supports this.)

The intention is that the the code will work if nanosleep() and/or clock_nanosleep() don't exist. iperf2 has the same conditional compilation. I don't know what really exist in different systems. The approach of the PR is that clock_nanosleep() has cleaner implementation than nanosleep(), so it is preferred. If both don't exist than behavior is as currently implemented (using pacing).

[bmah888]

So with the nanosleep() / clock_nanosleep() changes merged (thank for working with me on those!) I guess we can revisit this particular question. So, if nanosleep() or clock_nanosleep() are available, we don't need send_timer_proc() (or at least we don't need to call it).

[davidBar-On]

So with the nanosleep() / clock_nanosleep() changes merged (thank for working with me on those!) I guess we can revisit this particular question. So, if nanosleep() or clock_nanosleep() are available, we don't need send_timer_proc() (or at least we don't need to call it).

Unless you already plan to implement this, I will try to come up with a suggested implementation.

[davidBar-On]

Submitted PR #1773 which eliminates the use of send_timer_proc() (and simplifies iperf_ms_send()).

[bmah888]

PR #1773 has been merged!