buglwIP - A Lightweight TCP/IP stack - Bugs: bug #21433, Calling mem_free/pbuf_free from...

 
 

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

bug #21433: Calling mem_free/pbuf_free from interrupt context isn't safe

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Thu 25 Oct 2007 07:14:47 PM UTC  
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

( Jump to the original submission)

Thu 10 Jul 2008 07:03:37 PM UTC, comment #26: 

done.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 30 Jun 2008 04:05:49 PM UTC, comment #25: 

I must say I really don't care whether this is an option or not: I know what it does and wouldn't use mem_free/pbuf_free from interrupt context, anyway.

I think it is probably still better to keep the option (default off) since that is what most people will expect, I guess (that the heap cannot be used from interrupt level, I mean).

I'll check it in.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 30 Jun 2008 03:40:05 PM UTC, comment #24: 

I haven't tried out the code, but it looks great to me. Since you've tested it, I think this is fine to go in. Presumably there's a tweak to opt.h of course, and the description around that can probably be the documentation (since there's no other place to doc it, I think?).

It's tempting to simplify it by eliminating the option, but that might mean lots of calls to enable/disable interrupts or scheduling. I think I'd slightly prefer to retain the option because of that (rather than the extra code/data of the changes themselves), but don't mind particularly if others disagree.

Jonathan Larmour <jifl>
Project Member
Fri 27 Jun 2008 09:00:13 PM UTC, comment #23: 

Jonathan: I've changed mem_free/mem_malloc according to your idea from comment #10
(with the little change that I only start re-searching from the beginning if mem_free interrupted mem_malloc and it runs to the end without finding anything: longer but the re-search should occur less often)

I'll attach a patch file. Since I tested it, I'm pretty sure it works... We even could make this the standard way and remove the LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT define.

BTW: mem_realloc behaves like mem_free: even if it is not used from interrupt context, it is faster that way!

(file #15950)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Mar 2008 02:18:01 PM UTC, comment #22: 

Oh I see what you're getting at. What I was envisaging was that the existing semaphore remained, but mem_free wouldn't use it.

Jonathan Larmour <jifl>
Project Member
Fri 28 Mar 2008 02:04:11 PM UTC, comment #21: 

> Well, the point was to allow mem_free from other contexts such as
> interrupts. Allowing mem_alloc as well would indeed be a different kettle of fish.


Of course. Maybe I don't fully understand your approach. I try to summarize the changes:

- The change is only valid if the heap is protected by SYS_ARCH_PROTECT, not if it is protected using a semaphore
- In mem_malloc, in the big for loop, we include the SYS_ARCH_UNPROTECT/SYS_ARCH_PROTECT/check like you stated in comment #10 to allow mem_free to run in that case and keep the max time interrupts are disabled low (short: "blip phase" :)

But in this case, how do you keep mem_malloc from running again while another thread is in the "blip phase"?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Mar 2008 12:50:51 PM UTC, comment #20: 

Well, the point was to allow mem_free from other contexts such as interrupts. Allowing mem_alloc as well would indeed be a different kettle of fish.

Jonathan Larmour <jifl>
Project Member
Fri 28 Mar 2008 11:39:42 AM UTC, comment #19: 

Actually, thinking more about your idea, I must say I like it!

Although care has to be taken that no second mem_malloc runs during your "blip interrupt protection" phase.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Mar 2008 09:21:29 AM UTC, comment #18: 

Just to be clear, I don't mind if you don't like the idea, as long as you don't like it only after we properly understand its merits and any drawbacks :-).

Jonathan Larmour <jifl>
Project Member
Fri 28 Mar 2008 09:19:03 AM UTC, comment #17: 

>> I'm slightly unsure about referring to disabling interrupts all the time
>
> You're right on that.


Ok, instead of disabling "interrupts" maybe we could just refer to "other context", so e.g. the option would be LWIP_USE_HEAP_FROM_OTHER_CONTEXT. Only a suggestion mind.

> I created pbuf_free_callback() and mem_free_callback()


Yes I like the callback functions.

>> [comment #10]...
>
> Honestly, I'm not sure I completely understand your idea: You
> want to unprotect mem_MALLOC (your comment said mem_free)


Sorry yes my mistake (although mem_free changes too).

> during the search for a free block and restart the search if a
> free comes between that? That would make mem_malloc slower
> every time only because few times, mem_free could be called!
> Personally, I think that would be bad design (making the common
> case slower to be able to cope with an uncommon case)...


For the case of what you're trying to solve (LWIP_USE_HEAP_FROM_INTERRUPT/OTHER_CONTEXT==1), it would greatly reduce the interrupt latency (or the time the kernel task-switching is locked). In the code at present, the latency increase is a far worse problem than the overall run time in this case.

For the common case of LWIP_USE_HEAP_FROM_INTERRUPT/OTHER_CONTEXT==0 we can still make this modified behaviour be controlled by a #if on this option, so the previous (1.3.0 and older) behaviour is unmodified.

Jonathan Larmour <jifl>
Project Member
Fri 28 Mar 2008 08:04:10 AM UTC, comment #16: 

> I'm slightly unsure about referring to disabling interrupts all the time


You're right on that.

> I think the opt.h comment could clarify that pbuf_free_int (and
> mem_free) are functions which already exist and don't need
> writing. Perhaps this should also be documented somewhere else? Wiki?


I changed that a little: I created pbuf_free_callback() and mem_free_callback() (in tcpip.c, since they only can be used with the tcpip_thread). Of course this whole problem must be documented somewhere, opt.h can only be a quick hint: writing more would bloat the file...

> [comment #10]...


Honestly, I'm not sure I completely understand your idea: You want to unprotect mem_MALLOC (your comment said mem_free) during the search for a free block and restart the search if a free comes between that? That would make mem_malloc slower every time only because few times, mem_free could be called! Personally, I think that would be bad design (making the common case slower to be able to cope with an uncommon case)...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 27 Mar 2008 10:11:37 PM UTC, comment #15: 

I'm slightly unsure about referring to disabling interrupts all the time since on many embedded OSes this would actually disable tasking, which is slightly different.

I think the opt.h comment could clarify that pbuf_free_int (and mem_free) are functions which already exist and don't need writing. Perhaps this should also be documented somewhere else? Wiki?

I think the implementation could be improved by my suggestion in comment #10. But then I'm not the one doing it, so it's up to you :-).

Jonathan Larmour <jifl>
Project Member
Thu 27 Mar 2008 07:29:50 PM UTC, comment #14: 

I included a new option LWIP_USE_HEAP_FROM_INTERRUPT in opt.h (default = 0) that lets mem.c disable interrupts instead of using a semaphore (well documented in opt.h).

Additionally, I included a define tcpip_callback_nonblocking() and a function pbuf_free_int(struct pbuf) in tcpip.h, which lets you call

- tcpip_callback_nonblocking(pbuf_free_int, p);
- tcpip_callback_nonblocking(mem_free, m);

from interrupt level to free pbufs or heap pointers.

Anything missing?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 01 Nov 2007 01:04:42 PM UTC, comment #13: 

OK, that sounds like a plan, and I agree with your a) b) c) requirements.

Kieran Mansley <kieranm>
Project Member
Thu 01 Nov 2007 01:01:21 PM UTC, comment #12: 

> plug_holes isn't a problem


Of course it's not, since there is no loop in it!

[Kieran wrote:]

> I have to admit I'm a bit uneasy about increasing the amount of > stuff that we support from interrupt [->bad code]


[Jonathan wrote:]

> I would have guessed (possibly wrongly) that most ethernet
> driver systems do operate at thread level because the amount
> of processing


For RX, I agree; for TX enqueuing also. But freeing ref'ed pbufs when they are actually sent can be done quite fast in interrupt context, I think (nothing to be copied), and I don't see bad coding here.

> [if] SYS_ARCH_PROTECT is OK then that would be a relatively
> simple change I think.


If it's configurable it would be OK with me but in general it leads to very long periods of time where the interrupts are disabled (which is bad code to me; after all it's nearly the same as a very long running ISR). But since this seems the only nice solution for both NO_SYS settings, I'm OK with it as long as

a) using semaphore is the default
b) saying 'pbuf_free is not supported from interrupt level' is our default and
c) pointing users to documentation that explicitly tells them to enable the SYS_ARCH in mem.c so that they can use pbuf_free from interrupt context

> Either way, I think this is best left till after 1.3.0
> otherwise we'll never get it out.


But then again, maybe using tcpip_callback isn't so bad after all? :-)

But honestly: we can just introduce an option to use SYS_ARCH_PROTECT, that's easily done before 1.3.0!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 01 Nov 2007 12:49:22 PM UTC, comment #11: 

I have to admit I'm a bit uneasy about increasing the amount of stuff that we support from interrupt context because in many cases doing this sort of thing from interrupt context is a bad idea.  There are cases (perhaps like the one that prompted this) where it helps, but by adding support we make it tempting for people to write bad code.

I pretty much agree with Simon in comment #5.  i.e. all of the suggestions have their merits but none is ideal.  If Jonathan can persuade Simon that SYS_ARCH_PROTECT is OK then that would be a relatively simple change I think. 

Simon - can you clarify your objection?  I like the list-postponement as that avoids doing it at interrupt time, but the extra code and overhead to support this might be too much for a rarely necessary feature. With it not working for all APIs and NO_SYS we'd need something else as well.

Either way, I think this is best left till after 1.3.0 otherwise we'll never get it out. 

Kieran Mansley <kieranm>
Project Member
Tue 30 Oct 2007 04:49:47 PM UTC, comment #10: 

plug_holes isn't a problem - for sure it would not be the largest part in a system that is run with interrupts off (certainly not if you have ISRs is doing things like pbuf_free!).

For the connection with heap size, I think I see what you're getting at - some interrupt-driven drivers want to call mem_free, not mem_alloc; but mem_free/mem_alloc need mutual exclusion from each other.

That's a slightly different problem with less intrusive possible solutions.

For example, we can assume the chances of a driver mem_free() occuring in the middle of a mem_alloc() are very small. Small enough we can do something relatively inefficient in the unlikely case it occurs, such as changing mem_free to something like:

static volatile u8_t mem_free_count;

  SYS_ARCH_PROTECT(level);
  ptr = (u8_t *)lfree - ram;
  do {
       mem = (struct mem *)&ram[ptr];
       mem_free_count=0;
       SYS_ARCH_UNPROTECT(level);
       /* Blip interrupt-protection to avoid high
        * interrupt latency
        */
       SYS_ARCH_PROTECT(level);
       if ( mem_free_count != 0 ) {
         /* An interrupt did a free. Restart search. */
         ptr = (u8_t *)lfree - ram;
         continue;
       }

       if ((!mem->used) &&
           (mem->next - (ptr + SIZEOF_STRUCT_MEM)) >= size) {
[etc. as before]
       }
       ptr = ((struct mem *)&ram[ptr])->next) {
   while ( ptr < MEM_SIZE_ALIGNED - size );

and having mem_free() set mem_free_count;

But whatever the solution, personally I think that this is going to affect few drivers. I would have guessed (possibly wrongly) that most ethernet driver systems do operate at thread level because the amount of processing, especially copying, that would otherwise happen at interrupt level would make interrupt latency dreadful. At best it should be an option. Just my opinion though.

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 04:07:43 PM UTC, comment #9: 

>Hold on here, Simon is saying that using SYS_ARCH_PROTECT in mem_free is not good because it takes too long to walk the heap. I'm failing to see where the size of the heap (or number of allocations) is a factor. Maybe I'm just blind. Simon, can you tell me where this dependency is?


I believe he's talking about walking the heap finding a memory segment that is big enough to allocate. The more we allocate, the more we must walk the heap. I think plug holes can also be consuming.

Fred

EVS Hardware Dpt <vse>
Tue 30 Oct 2007 03:15:04 PM UTC, comment #8: 

Hold on here, Simon is saying that using SYS_ARCH_PROTECT in mem_free is not good because it takes too long to walk the heap. I'm failing to see where the size of the heap (or number of allocations) is a factor. Maybe I'm just blind. Simon, can you tell me where this dependency is?

Jonathan Larmour <jifl>
Project Member
Tue 30 Oct 2007 09:03:41 AM UTC, comment #7: 

Hello,

    >SYS_ARCH_LOCK_RESOURCE

    It's a idea which can be merged with "task #7212 : Add Mutex concept in
    sys_arch"

Yes and No, a mutex should block until it's free again. We do not have an OS, and so no primitive to block on something. You may think "OK, no OS, no thread, no need to block", but it's the very problem we have. How to lock access to a resource without the need to disable Interrupts and without OS primitives to wait until resource is free.

The proposed architecture could solve the problem although limited to mem.c module. You don't execute some parts of code because they are locked (in our example, we are currently at ISR level trying to free memory allocated by a PBUF, ISR interrupted the program while allocating some memory. All was protected by semaphores, but implemented as nothing because NO_SYS=1) but remember they weren't executed. Then, when possible execute them.

Fred.

EVS Hardware Dpt <vse>
Sun 28 Oct 2007 09:30:49 PM UTC, comment #6: 

>Personally, I'm not sure I agree. This adds extra data structures, list processing code, and does so whether the driver model requires it or not, and I suspect many do not (mine doesn't, for example).


I share this point of view with Jonathan.

>SYS_ARCH_LOCK_RESOURCE


It's a idea which can be merged with "task #7212 : Add Mutex concept in sys_arch"

Frédéric Bernon <fbernon>
Project Member
Fri 26 Oct 2007 03:51:31 PM UTC, comment #5: 

Personally, I don't know what to prefer here.

- Using SYS_ARCH_PROTECT to walk the heap seems like a bad idea to me, since my heap is pretty big, this locks interrupts for too long.

- Using the chained list we have to implement many new functions (lock/unlock a resource for NO_SYS=1, sys_sem_tryget for NO_SYS=0), the code is rather slower (but the mem_* functions are slow, anyway) and it adds unused code for people that don't need it.

- tcpip_callback makes the stack dependent on the API part and we need to malloca a memp-message when we want to free memory (seems a little big awkward to me).

It does work with NO_SYS=1, though: you only have to implement a function called tcpip_callback that puts function calls and their arguments into a linked list and process that list when back in normal context (poll it like the rest of the stack). Of course, that would also be code not needed by most of the people.

The reason I prefer the list (like in my patch) is it just works (provided we have the necessary functions and the port is correct) independent of the context mem_free() is called from, which makes lwIP easier to implement.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 26 Oct 2007 08:47:52 AM UTC, comment #4: 

Hello,

Here is my proposal for an alternative to sys_sem_tryget with NO_SYS = 1.

Your comment are welcome.

Fred.

(file #14216)

EVS Hardware Dpt <vse>
Fri 26 Oct 2007 08:28:43 AM UTC, comment #3: 

I agree with Simon, the proposed patch is a good generic solution.

Jonathan, I've looked in the stack, but when using Raw API, we don't have such callback facilities you suggest. So the only generic way to do it so is to check if we can free memory at a given moment, and if not, simply postpone it when trying to allocate or to free mem.

And it does correct the problem with NO_SYS = 1 (and SYS_LIGHTWEIGHT_PROT = 1 btw), provided we have a mean to simulate Simon's function with a single variable that is 0 or 1 to indicate the resource is locked or not. I currently work on some simple function that could simulate that functionality.

It would be something like

typedef unsigned int lock_t;
lock_t lock_mem = 0;

#define SYS_ARCH_LOCK_RESOURCE(locked_resource)
#define SYS_ARCH_UNLOCK_RESOURCE(locked_resource)
#define SYS_ARCH_TRY_LOCKED_RESOURCE(locked_resource)

SYS_ARCH_LOCK_RESOURCE would lock a resource
SYS_ARCH_UNLOCK_RESOURCE would unlock a previously locked resource (must verify if we must not take care of previous state for nested call support)
SYS_ARCH_TRY_LOCKED_RESOURCE would return 0 if we can access resource, or SYS_ARCH_TIMEOUT if resource is locked.

They could be implemented as semaphore with NO_SYS = 0, and use the locked = 1/non_locked = 0 proposal.

Please tell me if it's a correct solution.

Fred.

EVS Hardware Dpt <vse>
Thu 25 Oct 2007 08:25:46 PM UTC, comment #2: 

Personally, I'm not sure I agree. This adds extra data structures, list processing code, and does so whether the driver model requires it or not, and I suspect many do not (mine doesn't, for example). Most importantly it does not help raw API users with NO_SYS==1.

Otherwise if we don't care about NO_SYS==1, we can take the much simpler approach I suggested based around tcpip_callback(), whether called directly or indirectly through something like pbuf_free_safe.

Or perhaps, for NO_SYS==1, we just have to tell people that they have to sort out the drivers themselves - perhaps add it to their own list which they maintain and protect themselves. Perhaps a helper function so that drivers can remain abstracted:

#if NO_SYS==1
#define PBUF_FREE_INT_SAFE(_p) \
    lwip_callback_pbuf_free_int_safe(_p)
#else
#define PBUF_FREE_INT_SAFE(_p) \
    tcpip_callback(pbuf_free_wrapper, (_p))
#endif

With it left to raw API users to provide a lwip_callback_pbuf_free_int_safe function.

An alternative would be to just go ahead and add SYS_ARCH_PROTECT protection to mem_free. Looking at the code, this doesn't seem that bad to me. There are no non-deterministic loops, and most ISRs are likely to be longer. Or perhaps if concerned, you could maybe release and immediately relock the protection before the call to plug_holes(mem), then that gives any pending interrupts time to run, thus reducing the maximum latency. But I don't think that's needed.

Jonathan Larmour <jifl>
Project Member
Thu 25 Oct 2007 08:03:24 PM UTC, comment #1: 

My favourite solution for this is to create a new semaphore function 'sys_sem_tryget(sem)' which behaves much like 'sys_arch_mbox_tryfetch()' (check if the semaphore is available, return SYS_ARCH_TIMEOUT if not; don't block -> usable from interrupt context).

Then, if the semaphore is not available, put the struct mem on a list of memory to free. This list is then freed both at the end of mem_free (from application context) and at the start of mem_malloc (to make sure everything is freed before allocating again), all the time protected by mem_sem.

Attached a patch, please comment!

The reason I'd change this fast is we could include sys_sem_tryget in 1.3.0 so most port-breaks are done now.

(file #14212)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 25 Oct 2007 07:14:47 PM UTC, original submission:  

As discussed on lwip-users on the 25th of october, PBUF_RAM pbufs may not be freed from interrupt context since mem_free is only protected by a semaphore (or not at all if NO_SYS==1) see http://lists.nongnu.org/archive/html/lwip-users/2007-10/msg00116.html and replies.

Question is: do we want to allow pbuf_free from interrupt context? If not, we should explicitly document this implementation requirement!

(If someone asked me, I'd vote for allowing to free a pbuf from interrupt context :)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

Attached Files
file #15950:  mem.c_21433.patch added by goldsimon (7KiB - text/x-patch)
file #14216:  LockedAccess.c added by vse (1KiB - text/plain - Locked Access routine proposal for NO_SYS = 1)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by vse (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-07-10 goldsimon StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2008-06-27 goldsimon Attached File- => Added mem.c_21433.patch, #15950
    2008-03-27 goldsimon StatusNone => Ready For Test
        Assigned toNone => goldsimon
    2008-03-04 kieranm Planned Release => 1.3.1
    2007-10-26 vse Attached File- => Added LockedAccess.c, #14216
    2007-10-25 goldsimon Attached File- => Added mem_free_from_interrupt.patch, #14212

    Back to the top


    Powered by Savane 3.5