buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52748, the bug in timeouts.c

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52748: the bug in timeouts.c

Submitter:  wangcheng <wangcheng>
Submitted:  Wed 27 Dec 2017 02:09:41 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  dziegel
Open/Closed:  Closed Planned Release:  2.1.0
lwIP version:  git head

Jump to the original submission

Mon 08 Jan 2018 11:55:32 AM UTC, comment #20: 

Applied your patch for absolute timeouts with a small modification for cyclic timer "overload". Thanks!

IMHO, this bug is fixed now.

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 08 Jan 2018 10:30:32 AM UTC, comment #19: 


> That is not a fair attribution, my version used: [...] which is equivalent to my version but more convoluted.


You are right, my bad, apologies!

Your patch with does not correctly handle the cyclic timer overload situation. Run unit test "test_cyclic_timers", it fails in the last line "fail_unless((*list_head)->time == ..."
I gracefully handle "overload" situations, when a restarted cyclic timer would immediately expire again. See if condition in lwip_cyclic_timer()

You are correct, my cyclic timers are not 100% precise, if a tick occurs between the calculation of the relative timeout value in lwip_cyclic_timer and the calculation of the absolute time in sys_timeout, the timeout still drifts away! So something like sys_timeout_abs is needed...

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 08 Jan 2018 08:52:22 AM UTC, comment #18: 

Thank you for morning it forward.

> Fix that one of Douglas' optimizations broke the timers


That is not a fair attribution, my version used:
TIME_LESS_THAN(now, tmptimeout->time)
which was broken when it was replaced with:
TIME_LESS_THAN_OR_EQUAL_THAN(now, tmptimeout->time)
and then just fixed by changing it to:
TIME_LESS_THAN_OR_EQUAL_THAN(tmptimeout->time, now) == 0
which is equivalent to my version but more convoluted.

A similar problem was introduced in sys_timeout, where if the times are equal the new timeout is place first, not FIFO, which does not seem best and is a change from the prior code.

The use in sys_timeouts_sleeptime does not care if it returns true or false when they are equal, so it can be TIME_LESS_THAN as-is.

So perhaps consider taking another look at just using the simpler TIME_LESS_THAN.

> Make cyclic timers interval more deterministic - next timeout is calculated from last due time instead of relative to current time. This eliminates the cyclic timer jitter


The cyclic timer fixes do not appear to have made it, they are still broken. Need to be able to set a timeout at an absolute time, not just at an offset in msec from sys_now(), so need a new function.


Douglas <ourairquality>
Sun 07 Jan 2018 01:06:32 PM UTC, comment #17: 

sys_untimeout needed updating to not adjust the times

(file #42863)

Douglas <ourairquality>
Sun 07 Jan 2018 07:48:00 AM UTC, comment #16: 


> See pros and cons either way.


Right. On an idle system, it's probably better to have them running all at once to be able to sleep longer, but when messages are available, it might be better to space them apart. Maybe we could do it depending on if there are messages to process...

Simon Goldschmidt <goldsimon>
Group administrator
Sat 06 Jan 2018 10:58:49 PM UTC, comment #15: 

Observation: with the cyclic timers now running true, there are often a bunch of them to handle at a time. They appear light weight, but with this change there is now the option to stagger them. E.g I tried at 15ms offset between them and that broken them up. See pros and cons either way.

Douglas <ourairquality>
Sat 06 Jan 2018 09:16:44 PM UTC, comment #14: 


> Returning true when equal is both better for cyclic timers and is what the old implementation did. I don't see a need to change this. The macro name is wrong in this case, of course.


Probably best to have a look at the actual uses of this macro. When inserting a new timeout it now places the new time after others with an equal time, so a FIFO, which seems good. When checking the timeouts if returns when 'now' is less than the next timeout (so handlers the next timeout if equal), and it seems best not to return if 'now' is equal. etc

Douglas <ourairquality>
Sat 06 Jan 2018 01:35:59 PM UTC, comment #13: 

I like your idea for sys_restart_timeouts.

> TIME_LESS_THAN - do not return true when equal; simplify


Returning true when equal is both better for cyclic timers and is what the old implementation did. I don't see a need to change this. The macro name is wrong in this case, of course.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 06 Jan 2018 01:00:08 PM UTC, comment #12: 

Put the LWIP_TESTMODE code back in.

(file #42858)

Douglas <ourairquality>
Sat 06 Jan 2018 09:53:21 AM UTC, comment #11: 


> the only bug I see is that if we sleep more than ~25 days (31 bits ms), we would not know that all timers have surely expired ...


Sorry, it was trivial to rebase the times as they are in an ordered list. Attached updated suggestion.

(file #42857)

Douglas <ourairquality>
Sat 06 Jan 2018 09:40:59 AM UTC, comment #10: 

FWIW:

  • remove LWIP_TESTMODE code


  • TIME_LESS_THAN - do not return true when equal; simplify.


  • sys_timeout - move the body to a new function sys_timeout_abs that takes an absolute time; flatten and simplify.


  • sys_check_timeouts: sample sys_now() only at the start of the function; flatten and simplify.


  • lwip_cyclic_timer: use sys_timeout_abs to set the new time. It did not look correct to simply subtract sys_now and then call sys_timeout, the sys_now in sys_timeout might be different and they might not cancel out.


> the only bug I see is that if we sleep more than ~25 days (31 bits ms), we would not know that all timers have surely expired ...


Is that a practical problem for any deployment? Would it be sound to assume that no timeout were more that 1/4 of that and so scan them to find the earliest and then rebase them to the current time?

(file #42856)

Douglas <ourairquality>
Sat 06 Jan 2018 07:21:54 AM UTC, comment #9: 


> I don't know if calling sys_now() in every loop is a good thing to do ...


Looking at it again it does seem appropriate to sample sys_now() only at the start of the loop. So on each call it  processes all timeouts that had expired at the start of the call. This gives some priority to processing mbox messages which seems appropriate.

The caller, sys_timeouts_mbox_fetch, calls sys_timeouts_sleeptime() and only waits that long so will loop around again quickly if there are now more expired timeouts, but will have processed a mbox message first.

Douglas <ourairquality>
Fri 05 Jan 2018 08:02:56 PM UTC, comment #8: 

Keeping that aside, the only bug I see is that if we sleep more than ~25 days (31 bits ms), we would not know that all timers have surely expired but we would instead wait another ~25 days for all of them to expire.

To me it seems like somehow sys_restart_timeouts() should handle this.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Jan 2018 08:01:27 PM UTC, comment #7: 

Hmm, I don't know if calling sys_now() in every loop is a good thing to do: this way, a too short timer can keep us in this tight loop forever. What do we gain? Timers are allowed to jitter, the only thing we have to watch out for is that cyclic timers must stay accurate (i.e. next timeout = last timeout + period, not now + period).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Jan 2018 01:10:55 PM UTC, comment #6: 

Find attached a variant that checks sys_now() in every loop.
Additionally, the cyclic timer drift has been removed.

(file #42849)

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 05 Jan 2018 10:14:52 AM UTC, comment #5: 

fwiw it might still be beneficial to adopt the change from patch #9533 to sample sys_now() each time around the sys_check_timeouts loop. Consider the case that the code in the handler installs a new timeout and if that is already expired on return from the handler (in the limit the delay might be zero). Consider the case of a slow handler, it might be best to bring forward the handling of some pending timeouts if they have already expired.

Could it now do better with the cyclic timers, so note the last expired time, and add the period to that, rather than to sys_now(), to calculate then next expired time. This way the times for a cyclic timer should note drift wrt the system time.

So perhaps create a separate version of sys_timeout that takes an absolute expiry time, and have sys_timeout add sys_now to the msec and then call this new function to install the timeout. This new entry point could be used to reload the cyclic timeouts.

Douglas <ourairquality>
Fri 05 Jan 2018 08:03:27 AM UTC, comment #4: 

Attach whole timeouts.c file so people that are interested do not need to apply patches :-)

(file #42844)

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 05 Jan 2018 07:59:45 AM UTC, comment #3: 

I rewrote the timer implementation to fix and simplify it by using absolute timer expiration values.
This also mostly obsoletes patch #9533.

The function sys_restart_timeouts() is now empty - due to absolute expiration values, the timers will all fire just once. The previous implementation would have fired timers several times due to the time difference diff handling (if sys_restart_timeouts() is not called after long sleep times).

Request for comments :-)

Calling PBUF_CHECK_FREE_OOSEQ() when no timers are active will be implemented soon.

(file #42843)

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 03 Jan 2018 06:49:13 PM UTC, comment #2: 

See also patch #7855

Simon Goldschmidt <goldsimon>
Group administrator
Thu 28 Dec 2017 06:19:16 AM UTC, comment #1: 

Good catch. Saw reports of poor timing but had no clues. There was another piece of logic that did not look useful and was hard to understand being correct, so the attached patch removes that too.

(file #42744)

Douglas <ourairquality>
Wed 27 Dec 2017 02:09:41 PM UTC, original submission:  


in the function "sys_timeout":

the code snippet as follows:

if (next_timeout->time > msecs) {
   next_timeout->time -= msecs;
   timeout->next = next_timeout;
   next_timeout  = timeout; 
}


the "msecs" should be "timeout->time".

otherwise, the actually time for the timer pointed by "next_timeout" will longer "diff" than expect.

wangcheng <wangcheng>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #42863:  timeouts.c added by ourairquality (13KiB - text/x-csrc)
file #42858:  timeouts.c added by ourairquality (13KiB - text/x-csrc)
file #42857:  timeouts.c added by ourairquality (13KiB - text/x-csrc)
file #42856:  timeouts.c added by ourairquality (13KiB - text/x-csrc)
file #42849:  timeouts.c added by dziegel (13KiB - text/plain)
file #42844:  timeouts.c added by dziegel (12KiB - text/plain)
file #42843:  absolute_timeouts.patch added by dziegel (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by goldsimon (Updated the item)
  • -email is unavailable- added by ourairquality (Updated the item)
  • -email is unavailable- added by wangcheng (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-08 dziegel StatusNone Fixed
        Open/ClosedOpen Closed
    2018-01-07 ourairquality Attached File- Added timeouts.c, #42863
    2018-01-06 ourairquality Attached File- Added timeouts.c, #42858
    2018-01-06 ourairquality Attached File- Added timeouts.c, #42857
    2018-01-06 ourairquality Attached File- Added timeouts.c, #42856
    2018-01-05 dziegel Attached File- Added timeouts.c, #42849
    2018-01-05 dziegel Attached File- Added timeouts.c, #42844
    2018-01-05 dziegel Attached File- Added absolute_timeouts.patch, #42843
    2018-01-03 goldsimon Assigned toNone dziegel
    2018-01-02 goldsimon Planned ReleaseNone 2.1.0
    2017-12-28 ourairquality Attached File- Added 0001-sys_timeout-correct-insertion-of-new-timeouts.patch, #42744

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code