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

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Sat 30 Jan 2010 03:17:56 PM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Change RequestStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by slesage (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by goldsimon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 07 Mar 2010 06:46:08 PM UTCgoldsimonItem GroupNone=>Change Request
      StatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
      Planned Release=>1.4.0

    Back to the top


    Powered by Savane 3.1-cleanup1