buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19222, timeout in sys_mbox_fetch

 
 

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

bug #19222: timeout in sys_mbox_fetch

Submitter:  Dmitry Potapov <dpotapov>
Submitted:  Tue 06 Mar 2007 08:04:37 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Wed 11 Apr 2007 11:04:30 PM UTC, comment #19: 

I'm far of my office, but I think your patch is ok...

Frédéric Bernon <fbernon>
Group Member
Wed 11 Apr 2007 04:47:55 AM UTC, comment #18: 

I have just two little comments on this change to sys_mbox_fetch. This function is now hard to follow for sure, so I may have slipped up. Nevertheless, in this block:

      if ((timeout) && (timeout<timeouts->next->time)) {
      /* If time == SYS_ARCH_TIMEOUT, and we have wait "fetch's timeout" and not "timer's timeout",
         The timeout variable is the number of milliseconds we have waited for the message. */
        if (timeout < timeouts->next->time) {
          timeouts->next->time -= timeout;
        } else {
          timeouts->next->time = 0;
        }

The last 'else' clause can never happen because the second 'if' is a subset of the first 'if'. That test just shouldn't be there I think.

I have also added a little comment.

Is this ok by you Frederic? I don't use this code myself, but it's a tiny change.


(file #12462)

Jonathan Larmour <jifl>
Group Member
Fri 09 Mar 2007 10:43:48 AM UTC, comment #17: 


> Can I close this item for you?


Sure

Dmitry Potapov <dpotapov>
Fri 09 Mar 2007 09:40:23 AM UTC, comment #16: 

Sorry, look "bug #1902 : Timeouts and semaphores/mailboxes are too tightly integrated"

Frédéric Bernon <fbernon>
Group Member
Fri 09 Mar 2007 09:39:20 AM UTC, comment #15: 

Hi Dimtry,

Look also "patch #5067 : Fix err_to_errno() macro for sockets API" (I talk about my tests).

I join to this item the sample I used during my tests, based on your idea (hoping respect what you wanted, else, tell me).

Measures give really same results in generic scenarios (in my target, a dsp@300Mhz, some with slow targets, perhaps it's not the case), but better with mine when I don't use sys_timeout in my application (I add one to check the code, but my real application doesn't use it). And better agin when I directly use sys_arch and not sys in api_lib.c.

So, I don't know. Test it, change it if you need SO_RCVTIMEO and this method is not good for you. I will be happy to get your feelings about that.

Last, sorry again about this problem, and sorry about the late fix...

Can I close this item for you?

Frédéric Bernon <fbernon>
Group Member
Thu 08 Mar 2007 10:36:45 PM UTC, comment #14: 

Hi Frédéric,

I have studied  sys_mbox_fetch_timeout and it seems to be correct, but looking at it now (with some tricky #if LWIP_SO_RCVTIMEO inside), I still believe that my solution that employed sys_timeout() to implement sys_mbox_fetch_timeout over existing sys_mbox_fetch() was more simple ;)

On the other hand, when we move to new timeout handling based on expiration timestamp (which was discussed in BUG #19167), I believe the code of sys_mbox_fetch_timeout should become much more simple.

Thank you

Dmitry Potapov <dpotapov>
Thu 08 Mar 2007 09:07:26 PM UTC, comment #13: 

Hi Dimtry,

Hoping this solution will be good for you...


Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 10:08:10 AM UTC, comment #12: 

Frédéric,

Besides, the expression to find minimum as we discussed yesterday:

 (timeout!=0) ? min( timeout, timeouts->next->time) : timeouts->next->time)

you also need to add the following 5 lines
(I marked them with '+' at the beginning):

      /* If time != SYS_ARCH_TIMEOUT, a message was received before the timeout
   occured. The time variable is set to the number of
   milliseconds we waited for the message. */
      if (time <= timeouts->next->time) {
  timeouts->next->time -= time;
      } else {
  timeouts->next->time = 0;
      }
+     if(timeout) {
+        timeout -= time;
+        if(timeout == 0)
+           break;
+     }
    }

Dmitry Potapov <dpotapov>
Wed 07 Mar 2007 09:45:52 AM UTC, comment #11: 

I currently work to enable/disable the code with LWIP_SO_RCVTIMEO, a provide a "fast" solution to Dmitry (and others). I will do real measures with both sys_mbox_fetch_with_timeout's implementations to know what is the best performance...


Frédéric Bernon <fbernon>
Group Member
Wed 07 Mar 2007 08:42:48 AM UTC, comment #10: 

You two had a real good and long conversation yesterday... :)

I agree that, since sys_mbox_fetch() seems to be called only from socket/netconn thread with a timeout, it's probably a faster solution if fetch_with_timeout was based on fetch_without_timeout because fetch_with_timeout is not needed often.

This whole timer-callback thing in sys_sem_wait seems like re-inventing something you already have. You have the user-given timeout and you convert it to an lwip-timer just to put it in the list of timers.

On the other hand, if you don't do it this way, you have to be sure the value sys_arch_mbox_fetch() returns as the time it has waited for a message to arrive MUST be correct to have accurate timing, and I don't think you can count on that.

So the way sys_sem_wait_timeout() goes probably leads to more accurate timing.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 07 Mar 2007 12:06:08 AM UTC, comment #9: 

Frédéric,

I fully agree that to keep lwIP as simple as possible is important.  It appears we have a slightly different view what is the most simple solution here :) but it is not so importart for me. Your last proposal sounds good to me. Thank you for your time and efforts.

Dmitry Potapov <dpotapov>
Tue 06 Mar 2007 11:30:39 PM UTC, comment #8: 

Don't worry Dmitry, I will fix that tomorrow morning (I don't want this new feature cause you any problems), but I hope other comments about that.

I will propose a configuration option to avoid increase footprint. Perhaps LWIP_RCVTIMEO to enable/disable this feature ?

About sys_mbox_fetch implementation, perhaps that :

#define sys_mbox_fetch(m,d,t) sys_mbox_fetch_with_timeout(m,d,t,0)

footprint not increased, no sensible performance lost, source code compatible, easy to change with LWIP_RCVTIMEO.

About sys_mbox_fetch_with_timeout implementation, some points to talk :

- I don't "like" sys_sem_wait_timeout, because it's only use per "select", and sys_msleep (and in this last, implementation isn't efficent - already done measures), and really increase footprint.

- I will test each one and will do cycles measures, to see if a there is a real sensible difference.

- lwIP design seems to keep things as simple as possible, and I think an implementation based on sys_arch_mbox_fetch's timeout is simple and efficient. This is my personnal point of view...

So, is it possible to others developers to give their comments?

Good night and "see" you in some hours...

Frédéric Bernon <fbernon>
Group Member
Tue 06 Mar 2007 10:42:11 PM UTC, comment #7: 

Hi Frédéric,

I like having sys_mbox_fetch_with_timeout, but I don't like implementation of sys_mbox_fetch based on it.

My idea was that sys_mbox_fetch_with_timeout should be based on sys_mbox_fetch, in the same way sys_sem_wait_timeout is based on sys_sem_wait.

I don't think it is worth to introduce sys_mbox_fetch_with_timeout just to avoid passing one extra parameter. You hardly win anything in footprint, and certainly you will lose in performance.

Anyway, I would prefer to have this problem fixed in one way or another. Because I am not happy about its current behavior.

Dmitry Potapov <dpotapov>
Tue 06 Mar 2007 09:59:33 PM UTC, comment #6: 

Hey Dimtry, "overhead" is perhaps a too big word ? :)

But, you're right, adding a parameter to a function is an overhead! I just think the real overhead is to don't use the sys_arch_mbox_fetch's timeout parameter, which can be based on optimized operating system calls (in my case, pSos).

Fetching a message from a mailbox with a timeout is simplest - but it's my point of view - with sys_arch_mbox_fetch.

I can understand that some users (or perhaps lot of them, I don't really know all lwip's users, and how they use lwIP?) want to use lwip's internal features, to reduce fooprint, but most of time, sys_arch.c is a wrapper of operating system functions, so, we can suppose that application level will also use these operating systems calls, and not sys.h calls.

But ok, we have to found a solution which can be accept by most of users.

I propose you that (in fact, Simon already propose it, but he was the only one to answer me when I have submit the patch...) :

- Add a sys_mbox_fetch_with_timeout, implement sys_mbox_fetch with it

- Remove any extra parameters from tcpip.c and api_lib.c (except netconn_recv of course, which call sys_mbox_fetch_with_timeout).

- Force NULL inside sys_mbox_fetch_with_timeout when we get SYS_ARCH_TIMEOUT, and use a "min" with 0 checking.

More, to avoid extra recv_timeout in netconn struct, I can set a define to enable to include or not this code.

Ok for you ? Do you prefer this solution ?


Frédéric Bernon <fbernon>
Group Member
Tue 06 Mar 2007 09:07:24 PM UTC, comment #5: 

BTW, who said sys_arch_mbox_fetch() sets `msg' to NULL when it returns SYS_ARCH_TIMEOUT? I don't think that all ports do that. As result, you can end up with undefined value in `msg'. Of course, it is easy to fix by adding *msg = NULL at the beginning... but it just another indication that this approach is wrong...

In fact, if you look at sys_sem_wait_timeout, it uses sys_timeout() to implement timeout. So, it would be more consistent to implement sys_mbox_fetch_timeout() in the same way using sys_timeout().

Dmitry Potapov <dpotapov>
Tue 06 Mar 2007 08:56:13 PM UTC, comment #4: 

I know that timeout handlers created per thread, but I don't like this implicit assumption that there is no timeout handlers in the thread that calls sys_arch_mbox_fetch with a finitive timeout.

> time = sys_arch_mbox_fetch(mbox, msg, min( timeout, timeouts->next->time));


And you also need to verify that timeout is not 0. So, it should be something like this:

time = sys_arch_mbox_fetch(mbox, msg, (time!=0) ? min( timeout, timeouts->next->time) : imeouts->next->time));

Of course, it is possible to fix the bug in this way. But I don't understand why should we add this extra parameter and overhead for anyone who does not use UDP socket with the receive timeout? I think it is a better solution to use sys_timeout() for recv_timeout as I did in my patch.


Dmitry Potapov <dpotapov>
Tue 06 Mar 2007 08:39:56 PM UTC, comment #3: 

Perhaps it will be better to change in sys.c, in sys_mbox_fetch, the line:

time = sys_arch_mbox_fetch(mbox, msg, timeouts->next->time);

by this one :

time = sys_arch_mbox_fetch(mbox, msg, min( timeout, timeouts->next->time));

Do you prefer that ?

Frédéric Bernon <fbernon>
Group Member
Tue 06 Mar 2007 08:29:01 PM UTC, comment #2: 

In fact, no, the behavior is correct: timeouts have a processing priority but and except for netconn_recv, there is no change. At netconn or socket layer, there is no real timeouts handler defined (remember that timouts have to be created per thread).

If you grep inside lwip sources, you will see that the only case where timeouts can be defined is in tcpip_thread: this thread have to handle timeouts (for ARP timer, IP reassembly timer, etc...) as soon as possible (some timers, like SNMP, have to be as accurate as possible).

All other sys_mbox_fetch() calls are at api_lib level, and are used to wait an acknowledge from tcpip_thread, after they have post to it a command to execute.

Except in "netconn_recv" in UDP case, there is no behavior change. sys_timeout is "only" use to implement timers. I think, but it's my point of view, that the job is the same, but in this case, I always prefer my solution...



Frédéric Bernon <fbernon>
Group Member
Tue 06 Mar 2007 08:13:56 PM UTC, comment #1: 

I am sorry for previous patch. I inadvertently upload a wrong (old) patch.

(file #12131)

Dmitry Potapov <dpotapov>
Tue 06 Mar 2007 08:04:37 PM UTC, original submission:  

The recently added `timeout' parameter to sys_mbox_fetch function is ignored when there are timeouts handlers, which seems to be incorrect behavior. Moreover, this parameter is used only in netconn_recv, where it can easily be implemented using sys_timeout().

So, I propose to remove this parameter completely. I attached the patch to that effect -- it removes `timeout' parameter and uses sys_timeout() to implement UDP recv_timeout.

Dmitry Potapov <dpotapov>

 

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

Attached Files
file #12462:  sys.c.tweak.patch added by jifl (1KiB - text/x-patch)
file #12131:  sys_mbox_fetch-fix.patch added by dpotapov (6KiB - text/x-patch)
file #12130:  sys_mbox_fetch-fix.patch added by dpotapov (6KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by dpotapov (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-11 jifl Attached File- Added sys.c.tweak.patch, #12462
    2007-03-09 fbernon StatusWorks For Me Fixed
        Open/ClosedOpen Closed
    2007-03-08 fbernon StatusNone Works For Me
    2007-03-06 fbernon Assigned toNone fbernon
    2007-03-06 dpotapov Attached File- Added sys_mbox_fetch-fix.patch, #12131
    2007-03-06 dpotapov Attached File- Added sys_mbox_fetch-fix.patch, #12130

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code