[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [lwip-users] PPP new Stack
From: |
Sylvain Rochet |
Subject: |
Re: [lwip-users] PPP new Stack |
Date: |
Tue, 19 Feb 2013 21:05:28 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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