patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6330, Limit input packets on tcpip mbox

 
 

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

patch #6330: Limit input packets on tcpip mbox

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Thu 06 Dec 2007 08:55:23 PM UTC
   
 
Category:  None Priority:  1 - Later
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 08 Jan 2008 07:34:48 PM UTC, comment #12: 


> I also have issues with some of the specific patch details since in fact it does increase code for those who don't care about this feature.



The patch was designed to be an option. However it does also work the way it is now, so I close this item.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 06 Jan 2008 08:20:21 PM UTC, comment #11: 

I think Simon would agree that it is not "necessary". In my opinion, at best it's a workaround since it doesn't really prevent the condition it is ostensibly aimed at (preventing the application threads blocking) - it just tries to reduce the chances or frequency of it happening. I also have issues with some of the specific patch details since in fact it does increase code for those who don't care about this feature.

But we were waiting for other opinions, so if you want to give one, please do.

Jonathan Larmour <jifl>
Group Member
Sun 06 Jan 2008 07:51:31 PM UTC, comment #10: 

Is this patch always necessary now? Even if task #7490 is not completed, this part if check in...

Frédéric Bernon <fbernon>
Group Member
Tue 11 Dec 2007 07:39:13 PM UTC, comment #9: 

Let's leave this patch open as a patch that can be applied by users that want to. Once task #7490 is done we can have another talk about the necessity of this patch.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 10 Dec 2007 07:18:10 PM UTC, comment #8: 


> The #ifndef should be around the definition of the 3 defines (max
> packets, refused/allowed macro.


Oh I had thought we wouldn't increase code size for those who wouldn't use it - so if it isn't defined at all, the code disappears.

Anyway, I'm still unconvinced about a general need to stop the application temporarily being blocked (once the task #7490 changes are made - it's true that until then, this would have more problematic symptoms).

But as I said, let's hear from others. I'm just one opinion.

Jonathan Larmour <jifl>
Group Member
Mon 10 Dec 2007 06:42:03 PM UTC, comment #7: 


> BTW you would also need to remove the "MySleep" call from that patch.


oops... that was the only way I could receive frames faster than tcpip_thread can process them under windows :)

> something like #ifndef TCPIP_MAX_INPKTS_QUEUED around every addition in the patch?


The #ifndef should be around the definition of the 3 defines (max packets, refused/allowed macro.

> The application can send, but if TCP ACKs can't get returned, what's the point.


Of course that's true for TCP connections. In a scenario where sending UDP frames out is the most important thing (it is for us...), you sometimes don't mind a few dropped input packets.

Anyway many of the currently used small embedded systems are not capable to process 100 mbit at full wire-speed, so you will always drop some packets when e.g. a PC sends at full wire-speed. The only question is what to do in that situation!

I don't have a real problem with this not going in (I always can add it to my local copy only), I just think sys_mbox_trypost might not be enough: what would you suggest for tcpip_callback, for example? Block or don't block? If the queue can be full, tcpip_callback would be unsafe to call from interrupt context!

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Dec 2007 04:56:05 PM UTC, comment #6: 

That's true, but this implies your cpu is already overloaded with network processing, and dropping input packets is not going to help this. The application can send, but if TCP ACKs can't get returned, what's the point. An application thread that interfaces with the network stack should already expect that it may block (if the remote end doesn't consume and ack as quickly as you may like, before send buffers fill up). If you don't want that, you need to move those operations to a different thread.

I may be being overly averse to changes. So perhaps someone else could comment?

BTW you would also need to remove the "MySleep" call from that patch. And to check, in comment #1, you meant to have something like #ifndef TCPIP_MAX_INPKTS_QUEUED around every addition in the patch?

Jonathan Larmour <jifl>
Group Member
Fri 07 Dec 2007 04:02:32 PM UTC, comment #5: 

Yes, but with sys_arch_mox_trypost as only limitation, input packets can still fill the whole mbox, leaving no place for application task to communicate with the tcpip_task...

My patch has the ability, to prevent a Denail-of-Service by preventing a storm of packets to lead to a problem in the application.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Dec 2007 02:49:29 PM UTC, comment #4: 

After task #7490 is complete, there will be a means for tcpip_input() to handle that. tcpip_input() should never block, in any context (as per your comment #9 in task #7490 :-)).

Jonathan Larmour <jifl>
Group Member
Fri 07 Dec 2007 02:05:05 PM UTC, comment #3: 

But when you feed packets into tcpip_input() from interrupt context, you have no chance to block (and don't want to, anyway).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Dec 2007 01:50:15 PM UTC, comment #2: 

I'm not sure there's a real problem here. As discussed in task #7490, it's ok for the application thread to block waiting for the tcpip thread to service their request. In fact, depending on the underlying synchronisation primitives, for systems with priority inheritance, this would be a good thing as the tcpip thread will have its priority boosted.

Of course task #7490 isn't done yet, but I think that's where the effort is needed. In fact on rereading the thread in it I have a comment to make on it, which I'll do.

Jonathan Larmour <jifl>
Group Member
Thu 06 Dec 2007 08:56:57 PM UTC, comment #1: 

Wrong file attached :-(

imagine #ifndef around the defines TCPIP_MAX_INPKTS_QUEUED/TCPIP_INPKTS_REFUSED/TCPIP_INPKTS_ALLOWED. Also, they should be moved to opt.h when included in CVS

Simon Goldschmidt <goldsimon>
Group administrator
Thu 06 Dec 2007 08:55:23 PM UTC, original submission:  

To prevent the input mbox overflowing, I suggest to define an upper limit of input packets on the input queue (the mbox in tcpip.c).

The reason for this is even a return value for sys_mbox_post is no good here: input packets might fill the queue and an application task can't execute a send call -> unwanted behaviour.

The patch is a simple counting variable: counted up in tcpip_input when enqueueing a packet, counted down in tcpip_thread when dequeueing a packet. If an upper limit is reached, tcpip_input returns ERR_MEM and the caller can delete the packet.

In addition to that, I included 2 defines that are called when the upper level is reached and left. Normally define to nothing, a user can override them to get informed. Use case for this would be to turn off TX interrupt while the queue is full. A hysteresis to block/unblock would help further.

Please see attached patch for the implementation.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14578:  tcpic.c_limitinput.patch added by goldsimon (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-01-08 goldsimon StatusPostponed Wont Do
        Open/ClosedOpen Closed
    2007-12-11 goldsimon Priority5 - Normal 1 - Later
        StatusNone Postponed
    2007-12-06 goldsimon Attached File- Added tcpic.c_limitinput.patch, #14578

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code