Re: [lwip-users] PPP new Stack

[Sylvain Rochet]

Hi Nikolas,


On Tue, Feb 19, 2013 at 02:55:27PM +1100, Nikolas Karakotas wrote:
> Hi Sylvain,
> 
> Ok I think I found the problem. I use PPP_INPROC_OWNTHREAD and once
> I added ppp_delete() to the end it seems to fix the problem.
> I also removed the ppp_delete from the linkStatusCB. Somehow a clean
> up wasn't completed and a pbuf was always allocated but not freed as
> you can here:

Well, this is one of the reason why I consider to remove the 
PPP_INPROC_OWNTHREAD crap in ppp-new, as said in bugs #37278 and #37353.

1. It requires to be modified to match user system, like you did adding the
vTaskDelete(NULL); FreeRTOS call at the end of the function.

This is a tiny-tiny fonction that should be, in my opinion, on the user 
port, like the Ethernet input thread we see in many Ethernet port.

2. It is actually not thread safe.

2.1. pcb->phase IS modified by the lwIP core thread so it should at 
least be set to volatile, otherwise the pcb->phase copy may live 
indefinitely in CPU register. It works because of the sio_read() 
function call which without doubt flush pcb->phase copy from CPU 
register.

2.2. This function assume PCB still exists whatever is happening, which 
is not the case after you called ppp_delete() function outside of this 
thread. If sio_read() is blocking waiting data and pcb destroyed, it is 
going to read a deallocated pcb which luckily should still have 
pcb->phase set to 0 (=PHASE_DEAD) due to preallocated "control block" 
structures of lwIP.

3. I dislike the sys_msleep(1), it means that systems should have at 
least a 11 chr buffer at 115200/10 byte/s, and bigger with higher serial 
speed, for example with 3G/HSDPA modems accessed through SPI, at 20 
Mbits/s this is a ~2000 bytes buffer required to keep incoming data 
during this sleep, I don't see why we require systems to do so, 
sio_read() should obviously be a blocking call.


Of course, I do not want to call ppp_delete() at the end of this thread, 
keeping the PPP PCB live over (re-)connections is necessary to keep the 
netif interface that might be used with udp_sendto(), 
ppp_over_l2tp_open() (for L2TP-VPN over PPPoE/oS), ... and this is why 
the ppp-new branch creates the netif early during ppp_new(), so that you 
can start to use this netif elsewhere in the code whatever the PPP 
session state is. Deleting a PPP PCB is as hard as removing an Ethernet 
netif.


If I were you I should rewrite the ppp_input_thread() on your side with 
your FreeRTOS-related change and with the necessary signaling to kill 
this thread -before- you call ppp_delete() from another thread, this 
will fix the threading issue you might have (but unlikely to happen, 
luckily) and this will help me removing this one so that you can
upgrade to the updated branch without PPP_INPROC_OWNTHREAD ;-)

But, honestly, deleting a PPP session control block using ppp_delete() 
(or pppapi_delete()) requires you to ensure that are not using anymore 
the pcb ptr nor the PPP netif, which requires in my opinion a quite 
difficult signaling all over your threads to tell them to stop using 
those objects.


I am going to check if adding the ppp_free_current_input_packet() in 
ppp_delete() is necessary, it depends if I already do so at the end of 
the connection or not which I don't remember.


Sylvain

signature.asc
Description: Digital signature