buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28064, pbuf_alloc(PBUF_POOL) is not...

 
 

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

bug #28064: pbuf_alloc(PBUF_POOL) is not thread-safe

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Fri 20 Nov 2009 05:51:30 PM UTC  
 
Category: pbufsSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.1

(Jump to the original submission Jump to the original submission)

Fri 22 Jul 2011 07:22:42 PM UTC, comment #9:

Fixed for NO_SYS==1, too, by calling PBUF_CHECK_FREE_OOSEQ() from sys_check_timeouts(). If you don't use that function, just make sure you call PBUF_CHECK_FREE_OOSEQ() from your main loop (often enough).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 19 May 2010 06:51:57 PM UTC, comment #8:

Forget that last comment: for NO_SYS==0, tcpip_callback is used to free ooseq pbufs, for NO_SYS==1 the pbuf-freeing code is disabled, so there's no bug left, only the negative impact of running out of pbufs.

And for that, we could make sys_timeout (for NO_SYS==1) thread-safe so that the next call to sys_check_timeouts triggers freeing ooseq pbufs.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 18 May 2010 04:12:28 PM UTC, comment #7:

OK, I didn't think of calling pbuf_alloc() from interrupt context with NO_SYS==1.

After all, I think the easiest solution is probably to set a flag and let tcp_tmr free ooseq segments if that flag is set. This would work for both NO_SYS settings and is thread-safe, although it might be a bit late (250ms...). Another solution would be to add a sys_timeout in this case, but that's not thread-safe either (yet?).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 18 May 2010 01:32:12 PM UTC, comment #6:

Adding target of 1.4.1 for remaining work with NO_SYS=1

Would be good to at least update (before 1.4.0) the NO_SYS=1 case so that it doesn't try to reclaim ooseq pbufs: better to run out of pbufs than to corrupt the stack.

Kieran Mansley <kieranm>
Project Administrator
Sun 29 Nov 2009 12:00:12 PM UTC, comment #5:

Fixed for NO_SYS=0 by using tcpip_callback to free ooseq pbufs from tcpip_thread.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 27 Nov 2009 02:50:29 PM UTC, comment #4:

I think that's a good approach for the short term and we can leave this bug open to come up with something better, if necessary, for 1.4.0

Kieran Mansley <kieranm>
Project Administrator
Fri 27 Nov 2009 06:24:53 AM UTC, comment #3:

The code in question has been added on jan 28th, 2009 after a discussion on lwip-users (without a bugtracker entry): Jonathan came up with a calculation of how long it takes to free ooseq pbufs.

Having that calculation in mind, the most simple solution probably is to call tcpip_callback (non-blocking) to free the ooseq pbufs in the correct thread context. Of course, that doesn't work when allocating from an ISR with NO_SYS=1.

The downside is of course that we still loose some packets until the ooseq-freeing code runs in tcpip_thread, but that's the best I can currently think of.

Any comments on this before I make the canges?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 26 Nov 2009 04:59:12 PM UTC, comment #2:

Updated summary and status:

This is a blocker: if the pbuf-pool is empty, pbuf_alloc_pool iterates the tcp_active_pcbs list from every thread. This has been introduced with 1.3.1.

As nice as this feature (reclaiming ooseq pbufs) is, it doesn't work as it is.

If we still want this, we have to implement it in another way so that the ooseq-freeing code runs in the tcpip-thread.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 22 Nov 2009 03:51:29 PM UTC, comment #1:

I think the simplest solution to solve this is to create macros (LWIP_ISR_ENTER(), LWIP_ISR_LEAVE()) that increase a counter and let pbuf_alloc check that counter. Then, these macros would have to be called from ISRs wanting to use lwIP.

I think allocating pbufs from ISRs is a worthy feature to keep netif drivers simple.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 20 Nov 2009 05:51:30 PM UTC, original submission:

Since 1.3.1 (pbuf.c 1.131: reclaim pbufs from TCP out-of-sequence segments if we run out of pool pbufs), pbuf_alloc tries to free ooseq-queued-pbufs.

However, before that, drivers were allowed to allocate PBUF_POOL pbufs from an ISR, which they now can't any more.

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 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 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 22 Jul 2011 07:22:42 PM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Tue 18 May 2010 01:33:08 PM UTCkieranmStatusFixed=>In Progress
    Tue 18 May 2010 01:32:12 PM UTCkieranmPlanned Release=>1.4.1
    Sun 29 Nov 2009 12:00:12 PM UTCgoldsimonStatusIn Progress=>Fixed
      Severity5 - Blocker=>3 - Normal
    Thu 26 Nov 2009 04:59:12 PM UTCgoldsimonStatusNone=>In Progress
      Severity3 - Normal=>5 - Blocker
      Assigned toNone=>goldsimon
      Summarypbuf_alloc(PBUF_POOL) cannot be used from ISR any more=>pbuf_alloc(PBUF_POOL) is not thread-safe

    Back to the top


    Powered by Savane 3.1-cleanup1