tasklwIP - A Lightweight TCP/IP stack - Tasks: task #14420, Remove sys_sem_signal from inside...

 
 

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

task #14420: Remove sys_sem_signal from inside SYS_ARCH_PROTECT crit section

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Fri 24 Mar 2017 03:21:46 PM UTC
   
 
Category:  socket/netconn Should Start On:  Fri 24 Mar 2017 12:00:00 AM UTC
Should be Finished on:  Fri 24 Mar 2017 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  jcunningham Percent Complete:  100%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Tue 25 Jul 2017 08:12:39 PM UTC, comment #14: 

Patch pushed in c5db278746daedcc6566822992f3f33ce3801470, let me know if there is any additional questions/discussion

Joel Cunningham <jcunningham>
Group Member
Mon 24 Jul 2017 09:18:47 PM UTC, comment #13: 

With item 3 I was trying to describe the following execution flow in event_callback (for !LWIP_TCPIP_CORE_LOCKING case):

1) SYS_ARCH_PROTECT
2) Update sock events, i.e. sock->rcvevent++
3) Save sock events to local, i.e. has_recvevent = sock->rcvevent > 0;
4) Loop over select_cb_list, process scb
5) SYS_ARCH_UNPROTECT/SYS_ARCH_PROTECT
6) Possibly restart if select_cb_ctr changed
7) Goto 4 until scb == NULL

With the refactor, there is now an UNPROTECT/PROTECT between saving the events (#3) and starting the first iteration of the loop (#4)

Joel Cunningham <jcunningham>
Group Member
Mon 24 Jul 2017 07:24:23 PM UTC, comment #12: 

I don't get the 3) thing. Other than that, the patch looks fine.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 21 Jul 2017 09:15:17 PM UTC, comment #11: 

Simon,

Re-opening issue for refactor to improve readability.  Attached is a patch that does some event_callback() refactoring.  Changes were mainly moving code, not too invasive.  I would appreciate a review before pushing :)

From commit message:

This refactors event_callback() to separate updating socket event
state from processing the select list (to apply socket event change)

Refactoring changes:

1) select_list_cb processing has been moved to a new local function called
select_check_waiters()
2) goto no_select_wakeup has been removed and now we use a flag
to track whether to call select_check_waiters()
3) There is a small functional change for !LWIP_TCPIP_CORE_LOCKING.
We call SYS_ARCH_UNPROTECT after saving events but before calling
select_check_waiters() (which now calls PROTECT before starting the loop).
Before the code held the PROTECT across saving the events and the first
loop iteration, but this didn't protect against anything because each loop
iteration we do an UNPROTECT/PROTECT
4) Better documentation for both LWIP_TCPIP_CORE_LOCKING and
!LWIP_TCPIP_CORE_LOCKING


(file #41283)

Joel Cunningham <jcunningham>
Group Member
Thu 20 Jul 2017 07:12:46 PM UTC, comment #10: 

Joel, please feel free to change the code to make it more readable! goto is often not the best solution...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 19 Jul 2017 02:56:04 PM UTC, comment #9: 

Ignore previous comment, NETCONN_EVT_RCVMINUS cases hit the "goto no_select_wakeup" and never touch the select_cb_list code, so while core lock is not held
for these instances of event_callback, it's ok.  Sorry for the noise, closing again :)

Joel Cunningham <jcunningham>
Group Member
Wed 19 Jul 2017 02:42:44 PM UTC, comment #8: 

I was auditing the changes for this task again and I believe there is an outstanding synchronization issue for the TCPIP_CORE_LOCKING case.

The premise of the change was that the core lock would be held when event_callback is executed and then select code in sockets.c could use the core lock to also protect select_cb_list

The problem is there are some places where event_callback (called via API_EVENT) is not within core locking code.

These are for cases where sockets/netconn is taking things from their mboxes.  Here's the list I'm seeing:

netconn_accept
netconn_recv_data
netconn_recv_data_tcp

Joel Cunningham <jcunningham>
Group Member
Tue 11 Apr 2017 08:27:57 PM UTC, comment #7: 


> is this even the start of not investing time into improving
> the non-core locking case because it's not the default locking
> mechanism and performs worse?


It's exactly that. It performs worse unless you have a really strange system. Even with COMPAT_MUTEX it should be better as the traditional message-passing way doesn't fix priority inversion, too. I have a vague plan of getting rid of the non-core-locking code some day but haven't planned it yet ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Apr 2017 08:19:11 PM UTC, comment #6: 

Reviewed the change, exactly what I was thinking, thanks :)

For non-core locking support, are you thinking the use of sys_sem_signal while inside SYS_ARCH_PROTECT can be fixed at a later time (possibly when requested by another dev)?

Or is this even the start of not investing time into improving the non-core locking case because it's not the default locking mechanism and performs worse?

Joel Cunningham <jcunningham>
Group Member
Tue 11 Apr 2017 07:00:30 PM UTC, comment #5: 

Done (comment #2).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Apr 2017 02:44:15 PM UTC, comment #4: 

Yes, good idea!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 Apr 2017 02:41:39 PM UTC, comment #3: 


> easier to understand because we have one only have the select list...


Should say "easier to understand because we have only the select list..."

Joel Cunningham <jcunningham>
Group Member
Tue 11 Apr 2017 02:37:43 PM UTC, comment #2: 

Simon,

Thanks for the changes here!  I reviewed the changes and had one idea/suggestion that I wanted to mention:

In event_callback(), we could remove the use of SYS_ARCH_PROTECT when iterating the select list and signaling the waiters for TCPIP_CORE_LOCKING case if we copied the values of sock->rcvevent, sock->sendevent, and sock->errevent, before starting the loop.  See lwip_selscan() for an example of this

This would improve performance in the TCPIP_CORE_LOCKING case as we wouldn't acquire/release SYS_ARCH_PROTECT per iteration.  Also, this makes the loop's synchronization easier to understand because we have one only have the select list to synchronize (rather than also socket access)

Joel Cunningham <jcunningham>
Group Member
Tue 11 Apr 2017 10:43:43 AM UTC, comment #1: 

Fixed by protecting select_cb_list using LOCK_TCPIP_CORE() instead of SYS_ARCH_PROTECT().

Simon Goldschmidt <goldsimon>
Group administrator
Fri 24 Mar 2017 03:21:46 PM UTC, original submission:  

In event_callback(), the select infrastructure uses SYS_ARCH_PROTECT to provide synchronization.  It calls sys_sem_signal() while inside the critical section and this has worked with interrupt enable/disable implementations of SYS_ARCH_PROTECT (and obviously mutex implementations).

Unfortunately this prevents using a spinlock in SMP systems for SYS_ARCH_PROTECT because most spinlock implementations don't support sleeping/task switch while holding the spinlock.

This could be solved by replacing select's use of SYS_ARCH_PROTECT with a mutex

Joel Cunningham <jcunningham>
Group Member

 

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

Attached Files

 

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 jcunningham (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 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-25 jcunningham Percent Complete90% 100%
        Open/ClosedOpen Closed
    2017-07-21 jcunningham Attached File- Added 0001-sockets-Refactor-event_callback.patch, #41283
        Percent Complete100% 90%
        Assigned togoldsimon jcunningham
        Open/ClosedClosed Open
    2017-07-19 jcunningham Percent Complete90% 100%
        Open/ClosedOpen Closed
    2017-07-19 jcunningham Percent Complete100% 90%
        Open/ClosedClosed Open
    2017-04-11 goldsimon StatusNone Done
        Percent Complete0% 100%
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code