Re: [lwip-users] Bug in pbuf.c regarding PBUF_POOL

[Jonathan Larmour]

Leon Woestenberg wrote:
> > that it looks to me that SYS_LIGHTWEIGHT_PROT is essentially mandatory 
> > because packets arrive asynchronously and so pbuf allocation needs to have 
> > some protection no matter what since the allocation will take place from a 
> > device driver. That would be true whatever the context, or whether single 
> > or multi-threaded.
> In a fully pre-emptive environment (like full-blown eCos), this
> light-weight locking mechanism must then disable the Ethernet chip
> receive interrupt, to prevent concurrent access to the pbufs, during
> LIGHTWEIGHT_PROT locks from non-interrupt context.

That would be sufficient if the only access to the PBUF_POOL buffers was 
from the ethernet interrupt driver. Unfortunately as I described in my 
previous mail, it is not. In the current trunk, there are pool allocations 
from ip_frag.c, snmp/msg_out.c, ppp.c, vj.c, slipif.c, ethernetif.c, and 
via pbuf_take, etharp.c.

Most of these can happen in different contexts, either TCPIP thread, eth 
processing thread or PPP or SLIP threads.

As I described in my earlier thread, the only way the pbuf 
!SYS_LIGHTWEIGHT_PROT support could theoretically work as it stands is if 
all pbuf_alloc (... PBUF_POOL) calls were guaranteed to only be called from 
the same context.

> I would certainly design a pre-emptive system by NOT entering the pbuf
> layer from interrupt context, but instead schedule a high-priority
> real-time task to move the packets of the chip from the interrupt
> handler, and nothing more.

There is no difference in practice with a pre-emptive system - if a lower 
priority thread could call pbuf_alloc and be interrupted by a higher 
priority thread which does the same, there's still a problem.


> We have a proven real-time pre-emptive design (even with nested
> pre-emptable interrupts) that does not use SYS_LIGHTWEIGHT_PROT but uses
> the "bottom-half" approach.

I would be surprised if it were truly thread-safe given my understanding of 
the current code. Maybe it used to be with different earlier code. But I 
haven't seen the code for that port.

> > The !SYS_LIGHTWEIGHT_PROT code simply doesn't give any thread or interrupt 
> > safety at all. So in fact the !SYS_LIGHTWEIGHT_PROT code does not appear 
> > to solve the problem it seems intended to solve. Hence me saying that that 
> > code should be removed entirely since as far as I can tell it doesn't 
> > achieve anything. If protection is needed, something using 
> > SYS_LIGHTWEIGHT_PROT is the solution.
>
> I am NOT sure if SYS_LIGHTWEIGHT_PROT protects against all concurrent
> acccess possible, I thought this was exactly the reason why it was
> called lightweight when introduced.

I thought it just meant that it was a slightly sledgehammer approach 
(blocking all tasking or maybe even all interrupts) and only to be used for 
guaranteed brief periods.

Anyway, my beef is with the !SYS_LIGHTWEIGHT_PROT support which appears to 
me to be not merely non-functional (e.g. no incr of pbuf_pool_free_lock), 
but incapable of providing protection, in which case why have it at all if 
you are _guaranteed_ only one context.

> Pushing a packet into the stack will have a chain reaction where all
> higher-level layers are called into. I would not run this from interrupt
> context.

Out of interest, the approach I have taken with my own improvements to the 
eCos port is to remove the separate ethernet input processing thread 
entirely, and instead make the tcpip thread suck the packets in (courtesy 
of mods to api/tcpip.c). This also removes nasty race conditions affecting 
the etharp data structures. The etharp code (in the old eCos port) could 
also otherwise be called from two different thread contexts, therefore 
potentially unsafely. There are other serious problems with the port I 
won't go into here.

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine