Re: [lwip-devel] [lwip-commits] [SCM] lwIP - A Lightweight TCPIP stack branch, master, updated. ea58a8103ceb70d20b88d37bfdbbe8ce8e9c6e71

[Sylvain Rochet]

Hello,

On Mon, Mar 09, 2015 at 11:07:53AM +0100, Sylvain Rochet wrote:
> 
> But I still want to fix the issue, even if nobody complained, that's the 
> problem with threading issues which might only happens in very very very 
> rare corner cases ;-)
> 
> What do you think about using pbuf_ref() in pppos_input() after 
> pbuf_alloc() so pppos_free_current_input_packet() doesn't free the pbuf 
> if it is still used by pppos_input() ?
> 
> There is still a problem, because pbuf_alloc() + pbuf_ref() must be 
> called atomically in pppos_input() as well as if (pbuf_free(in) == 0) { 
> in = NULL; } in pppos_free_current_input_packet(). What do you think 
> about making those atomic using SYS_ARCH_PROTECT/SYS_ARCH_UNPROTECT ?

That was a very stupid idea. I ended up with pbuf_ref()/pbuf_free() 
everywhere along the pppos_input() function to handle packet drop, 
function enter, function exit.

And it didn't fix the issue at the end, protecting the buffers are 
actually not enough because we also need to protect the current state 
machine because buffers management depends on in_state == PDDATA.

There is no way to fix that without spinlock with an irqsave/irqrestore 
helper for IRQ contexts. The other way is to move out PPPoS RX state 
machine and all inputs function from PPPoS PCB and ask the user to 
manage the free()ing of pbuf left on its side when PPPoS is closed, but 
I am sure it will end-up with the same threading issues ;-)


Anyway, I did my best at improving it without using the holy mutex and I 
added a new input path for data input of point to point protocols so 
users (both NO_SYS and !NO_SYS) running pppos_input() in the context of 
the lwIP thread are safe !  (and they even get an improved performance 
because I removed useless locks for them).


Sylvain

signature.asc
Description: Digital signature