patchlwIP - A Lightweight TCP/IP stack - Patches: patch #8995, timers: fix wrong timings for...

 
 

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

patch #8995: timers: fix wrong timings for !NO_SYS targets

Submitter:  Sylvain Rochet <gradator>
Submitted:  Sun 08 May 2016 01:58:01 AM UTC
   
 
Category:  None Priority:  6
Status:  Done Privacy:  Public
Assigned to:  gradator Open/Closed:  Closed
Planned Release:  2.0.0 Beta2

Jump to the original submission

Wed 20 Jul 2016 10:10:24 AM UTC, comment #8: 

I created a bug entry instead with all related information about the issue.

Sylvain Rochet <gradator>
Group Member
Wed 20 Jul 2016 09:30:25 AM UTC, comment #7: 

The patch didn't change that, this is an issue which already existed for timers previously used for NO_SYS targets.

This could be fixed by moving to absolute target timers instead of the currently used delta-diff timers (which I dislike, due to the problem mentioned below, at least). The main problem is our current support of timers restart on wake up event for devices that sleep to prevent firing all timers at once, that's hard to achieve with absolute target timers.


Sylvain Rochet <gradator>
Group Member
Wed 20 Jul 2016 06:25:13 AM UTC, comment #6: 

I found a problem with this patch: with the current code, timeouts get delayed when you're in a debugger breakpoint and your sys_now() continues running while you're stopped.

This is because sys_now() actually continued, say 50 seconds, but sys_check_timeouts() increases timeouts_last_time only by the expired timeout's time. If that handler restarts a timeout (as is the case for all lwIP-internal timers), it gets an addditional delay of 50 seconds.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 27 Jun 2016 01:29:19 PM UTC, comment #5: 

Thanks :-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 21 Jun 2016 10:02:21 AM UTC, comment #4: 

Applied :)

By the way, we do not use the time based sys_arch_sem_wait() return value anymore, should we remove the burden from ports for doing so ?

We can keep compatibility by telling in the documentation it should return 0 (i.e. anything != SYS_ARCH_TIMEOUT) if a message arrived or SYS_ARCH_TIMEOUT.

Also, having a 0 value meaning "infinite timeout" is quite uncommon and misleading, the main issue about that is there is no way to have messages having priority over timeouts, which is what we might want to prevent being stuck in timeouts (ideally an ~equal priority should be given to messages and timeouts to prevent any lockup condition). We could as well use MAX_INT (or MAX_SHORT) value and it might return from time to time (each 49.7 days or 65.5s (is it still possible to use short ?)) with no message, which is acceptable. The other issue about the 0 special value is what we need to handle this case in a different code path in sys_timeouts_mbox_fetch() AND in sys_arch_mbox_fetch(), which is not necessary if a 0 value means "try to fetch a message from the queue and return immediately if no message is pending".

Sylvain Rochet <gradator>
Group Member
Mon 20 Jun 2016 12:39:19 PM UTC, comment #3: 

Go ahead, the patch looks good and it also reduces duplicate code...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Jun 2016 08:28:22 AM UTC, comment #2: 

We should first apply my patch, which doesn't add a new feature but only
fix the main design issue. If you look at the patch you'll see that it
is surprisingly straightforward for what it actually does.

I am willing to port the other patchs (#7855, #8712) on top of my change
once mine is applied and once you acked them for inclusion.

Sylvain Rochet <gradator>
Group Member
Mon 23 May 2016 08:10:56 AM UTC, comment #1: 

This is the 3rd try to fix this problem (see patches #7855 and #8712). I'd love to fix this problem for 2.0.0, but I think I need some time to catch up with the other 2 entries...

I also though of putting the lwIP-internal timers on a "public" array so that timers could easily implemented externally (which is what patch #8712 is targeted at, I think)...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 08 May 2016 01:58:01 AM UTC, original submission:  

issue 1:

sys_arch_sem_wait() is supposed to return an elapsed time in ms, what could
happen given a > 1 kHz calling rate for high throughput systems is that it
might always returns 0 ms. This is a problem for systems which compute the
elapsed time from a high precision clock source.

This is what is currently happening in the unix port in sys_arch_sem_wait():

start time -> 1000000000;  // ns
-- less than a ms before an event arrive --
end time   -> 1000xxxxxx;  // ns
return value -> (end time - start time)/1000000 -> 0

The return value is used to reduce the next timer interval, if
sys_arch_sem_wait() always return 0 no more timers are fired anymore


issue 2:

The current timer implementation for !NO_SYS targets only count elapsed
time while -waiting- for semaphore and doesn't count at all the time
spent by the stack to process packets. For CPU bound traffic patterns no
more timers are fired anymore.

Both are serious design issues which cannot be easily fixed without reworking
everything. This patch uses the properly implemented timers for NO_SYS targets
for !NO_SYS targets and merge them both into one single timers implementation.

Sylvain Rochet <gradator>
Group Member

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by gradator (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-20 gradator StatusIn Progress Done
        Open/ClosedOpen Closed
    2016-07-20 goldsimon Priority5 - Normal 6
        StatusDone In Progress
        Assigned togoldsimon gradator
        Open/ClosedClosed Open
        Planned ReleaseNone 2.0.0 Beta2
    2016-06-27 goldsimon StatusNone Done
        Open/ClosedOpen Closed
    2016-05-08 gradator Attached File- Added 0001-timers-fix-wrong-timings-for-NO_SYS-targets.patch, #37099

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code