lwIP - 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: |
Dirk Ziegelmeier <dziegel> |
Mon 08 Jan 2018 10:30:32 AM UTC, comment #19:
|
Dirk Ziegelmeier <dziegel> |
Mon 08 Jan 2018 08:52:22 AM UTC, comment #18: Thank you for morning it forward.
|
Douglas <ourairquality> |
Sun 07 Jan 2018 01:06:32 PM UTC, comment #17: sys_untimeout needed updating to not adjust the times |
Douglas <ourairquality> |
Sun 07 Jan 2018 07:48:00 AM UTC, comment #16:
|
Simon Goldschmidt <goldsimon> |
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:
|
Douglas <ourairquality> |
Sat 06 Jan 2018 01:35:59 PM UTC, comment #13: I like your idea for sys_restart_timeouts.
|
Simon Goldschmidt <goldsimon> |
Sat 06 Jan 2018 01:00:08 PM UTC, comment #12: Put the LWIP_TESTMODE code back in. |
Douglas <ourairquality> |
Sat 06 Jan 2018 09:53:21 AM UTC, comment #11:
|
Douglas <ourairquality> |
Sat 06 Jan 2018 09:40:59 AM UTC, comment #10: FWIW:
|
Douglas <ourairquality> |
Sat 06 Jan 2018 07:21:54 AM UTC, comment #9:
|
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.
|
Simon Goldschmidt <goldsimon> |
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> |
Fri 05 Jan 2018 01:10:55 PM UTC, comment #6: Find attached a variant that checks sys_now() in every loop.
|
Dirk Ziegelmeier <dziegel> |
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.
|
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 :-) |
Dirk Ziegelmeier <dziegel> |
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.
|
Dirk Ziegelmeier <dziegel> |
Wed 03 Jan 2018 06:49:13 PM UTC, comment #2: See also patch #7855 |
Simon Goldschmidt <goldsimon> |
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. |
Douglas <ourairquality> |
Wed 27 Dec 2017 02:09:41 PM UTC, original submission:
|
wangcheng <wangcheng> |
Depends on the following items: None found
Items that depend on this one: None found
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 | Status | None | Fixed | |
Open/Closed | Open | 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 to | None | dziegel | |
2018-01-02 | goldsimon | Planned Release | None | 2.1.0 | |
2017-12-28 | ourairquality | Attached File | - | Added 0001-sys_timeout-correct-insertion-of-new-timeouts.patch, #42744 |
Powered by Savane 3.13-02a9.
Corresponding source code
Applied your patch for absolute timeouts with a small modification for cyclic timer "overload". Thanks!
IMHO, this bug is fixed now.