buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28775, select/event_callback: only check...

 
 

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

bug #28775: select/event_callback: only check select_cb_list on change

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Sat 30 Jan 2010 03:17:56 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Sun 07 Mar 2010 06:46:08 PM UTC, comment #6: 

Checked in:
- using SYS_LIGHTWEIGHT_PROT to protect the socket code (carefully chosen to not disable interrupts for too long)
- added a counter to each socket that tells event_callback if anyone is waiting using select, which speeds up receiving when noone is waiting

Tested with the tests-functions in contrib/apps/socket_examples.c.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Feb 2010 05:29:31 PM UTC, comment #5: 


> (for the same reasons as Frederic Bernon IIRC)


Could you tell us which reasons? I didn't hear from Frédéric for a while, now...

I don't know about yet another option: the time the socket array-protection would disable interrupts is very short! And I just shortened it more. By unlocking and relocking step in the for-loop, this should really not be a problem.

Did you mean locking the selectsem-list using LIGHTWEIGHT_PROT is not a good idea? For this, it's the same as above, though.

The general idea is to not wait on a semaphore for every received packet - that would not at all be lightweight to me!

Simon Goldschmidt <goldsimon>
Group administrator
Sun 07 Feb 2010 10:42:59 PM UTC, comment #4: 

Maybe it's a bit late, but I'm not in favor of using SYS_LIGHTWEIGHT_PROT for the socket array.
It's generally implemented by masking interrupts, and it's not good in my case (for the same reasons as Frederic Bernon IIRC), so I'd rather stick to semaphore or mutex.
We should let the choice to the user with an option, like LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT.

Stephane Lesage <slesage>
Sun 07 Feb 2010 12:51:42 AM UTC, comment #3: 

You're right the wake-up test can be moved before the loop.

I think there's another optimization here.
Now, we parse the select_cb_list in all cases, and specifically the whole list when nobody is waiting for that socket.

I think we could add a counter in the socket structure to count how many threads are waiting for a particular socket in select().
If it's 0, no need to parse the list.

Stephane Lesage <slesage>
Mon 01 Feb 2010 03:51:38 PM UTC, comment #2: 

That would have been covered by the initial check (lwip_selscan() is called before actually waiting).

If no socket is initially ready, it would indeed be necessary to add a second lwip_selscan() between adding the new semaphore to the list and releasing selectsem. But I guess that's OK since I'd rater improve the common case (RX packets) instead of improving the runtime of select().

I'll prepare a patch.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 01 Feb 2010 09:53:29 AM UTC, comment #1: 

We have to support the case where someone calls select on a socket that has already had notification of a change in state.

Kieran Mansley <kieranm>
Group Member
Sat 30 Jan 2010 03:17:56 PM UTC, original submission:  

Currently, every call to API_EVENT/event_callback leads to scanning the select_cb_list for waiting selects.

I'd change that to only scan the list when there has actually been a change (i.e. recvevent changes from <=0 to >0 or back, or sendevent/erreven actually changes).

Together with this test being protected by SYS_LIGHTWEIGHT_PROT only, this should speed up socket communication as it would reduce the semaphore usage.

This implicates that there is only someone actually waiting if the socket hasn't had the corresponding event before. Hardening lwip_select() against that would also be necessary.

Does anyone expect problems with that?

Simon Goldschmidt <goldsimon>
Group administrator

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2010-03-07 goldsimon Item GroupNone Change Request
        StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
        Planned Release 1.4.0

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code