patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6537, wnd_scale TCP option addition

 
 

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

patch #6537: wnd_scale TCP option addition

Submitter:  Rishi Khan <rishi>
Submitted:  Mon 09 Jun 2008 06:34:33 PM UTC
   
 
Category:  TCP Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Thu 20 Feb 2014 02:44:49 PM UTC, comment #14: 

Closing as done, please reopen if you think differently

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Feb 2014 10:19:00 AM UTC, comment #13: 

Good point. The call to the application's sent callback has to handle this as well, though.

Does this have anything to do with the u16_t/u32_t changes to memp in your patch?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 07 Feb 2014 10:10:28 AM UTC, comment #12: 

Hi all:

After submitting the patch we discovered some problems when large windows are supported with high bandwidth.  In tcp_receive, the pcb->acked was updated and truncated to 16 bits, limiting the effective window (and bandwidth). The original code was:


/* Update the send buffer space. Diff between the two can never exceed 64K? */
pcb->acked = (u16_t)(ackno - pcb->lastack);

To achieve high bandwidths we needed to use 32 bits because this diff can indeed be more than 64KB, and ended up with:

/* Update the send buffer space. Diff between the two can never exceed 64K? */
/* Yes, this might happen with high BW and Jumbo frames. We change the size of the acked to 32bit. Otherwise, ACKs maybe lost*/
     
pcb->acked = (u32_t)(ackno - pcb->lastack);

with acked field of 32 bits.

christian <xtian>
Fri 07 Feb 2014 08:54:12 AM UTC, comment #11: 

Window scaling is now supported (taken from patch #7858), but the OOS code still needs to be adapted.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 05 Oct 2012 10:49:31 AM UTC, comment #10: 

Hi all:

As a request from Igor Druzhinin I created a patch for window scaling based on lwip-1.4.0. The patch number is #7858 (https://savannah.nongnu.org/patch/index.php?7858). It is based on this original patch. Notice that the last comments from Wim Dumon and its last patch were not included in our patch. Please read the comments on the patch and discussion in https://savannah.nongnu.org/bugs/?36167.

Notice that supporting large windows has an impact on memory. To really take advantage of the window scaling option several parameters have to be carefully set in lwipopts.h. You also have to set the scale used for reception window (TCP_RCV_SCALE).

Christian


christian <xtian>
Mon 20 Apr 2009 09:34:25 AM UTC, comment #9: 

The blocker of this patch will be that it converts the tot_len field of struct pbuf from u16_t to u32_t. The silent agreement under lwIP developers (at least in the past 2 years) has been to not modify struct pbuf as it is really neatly aligned on most platforms with the current size. Adding 16 bits will break alignment on most platforms, essentially doubling its size after aligning: for low-memory targets this is often not an option.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Apr 2009 09:32:41 AM UTC, comment #8: 

In the previous comment you mentioned splitting it into multiple pbufs, but the patch stores the extra data in a new field.  Is there a reason why multiple pbufs wouldn't work?

Kieran Mansley <kieranm>
Group Member
Mon 20 Apr 2009 09:31:23 AM UTC, comment #7: 

I added a patch that deals with the overflow in the pbuf tot_len counter that I described in my previous comment.

Wim Dumon <wdumon>
Wed 08 Apr 2009 09:39:39 AM UTC, comment #6: 

I encountered an issue with this patch. I observed data loss and performance degradation when receiving data over TCP connections with large window sizes (e.g. TCP_WND=120KBytes). My shift is set at 7 (but it could be anything else).
This is the problem: suppose window is usually filled for about 70K, caused by the bandwidth*delay principle. Imagine that the link works well, and at a certain moment a single packet is dropped. The OOSEQ code queues packets, but there is a little gap in the sequence number for the first ooseq packet. When that gap is filled by a retransmit of the missing data, all the out-of-sequence queued packets, amounting to 70K of data in our example, is concatenated in one pbuf (it is called recv_data). But the tot_len field of a pbuf is only 16bit.... The result I observe is that there is a 64K gap in the received data stream.

Isn't the intention of this patch to allow TCP_WND to be > 64K? Any hints on how to solve this? I have the feeling that simply changing tot_len to a u32_t will not be sufficient. Using multiple pbufs is probably easier?

Wim Dumon <wdumon>
Fri 03 Apr 2009 11:14:03 AM UTC, comment #5: 

Note that I've done some work recently to improve the handling of tcp options and window updates, so this code probably won't apply cleanly anymore.

Is anyone interested in getting support for window scaling working against the current CVS head?

Kieran Mansley <kieranm>
Group Member
Mon 23 Jun 2008 09:50:31 PM UTC, comment #4: 

@Simon: I had been thinking the same thing as I think Bill was - we could keep the u16_t and apply the scaling factor when used, rather than storing the u32_t with the scaling factor already applied. (And hence my question 1). But yeah, that might be too much hassle overall.

Rishi, if you want to go ahead as you described that's fine by me, thanks.

Jifl

Jonathan Larmour <jifl>
Group Member
Fri 20 Jun 2008 07:52:11 PM UTC, comment #3: 

Bill wrote:

>> So, if we agree that we need to keep rcv_ann_wnd and snd_wnd as u32_t,
>
> Why when tcp_hdr wnd is u16_t?  If ultimately it gets truncated,
> no sense bloating code for u32_t until the final use of it.  8
> bit processors don't do s/u32_t very efficiently and so the use
> of s/u32_t should be minimized.


The window sizes are supposed to be 32 bit values, so at some point we would have to calculate with them as 32 bit values. You can either store them as 32 bit or store them as 16 bit and shift to 32 bit (using scale factor) on load. I think storing as 32 bit is the better alternative.

(BTW: you are welcome to reply to the bugtracker system for patches!)

re Jonathan 1):
I thought that's the whole meaning of window scaling: to have windows bigger than 64KByte!

re Jonathan 2):
This is indeed true. Having more than 64K pbufs queued on a single PCB seems highly unlikely!


Simon Goldschmidt <goldsimon>
Group administrator
Fri 20 Jun 2008 03:26:50 PM UTC, comment #2: 

Answers to questions:
1) why are the various window sizes in tcp.h changed to u32_t - I thought they were still limited to 65536?
rcv_ann_wnd, and snd_wnd are u32_t because of:
rcv_ann_wnd:
tcphdr->wnd = htons(pcb->rcv_ann_wnd >> pcb->rcv_scale);
snd_wnd:
pcb->snd_wnd = tcphdr->wnd << pcb->snd_scale

I think you are right about rcv_wnd.

2) why is snd_queuelen increased to u32_t - I can't foresee having more than 65536 segs for a single PCB!

In lwipopts I see this:
/* TCP sender buffer space (pbufs). This must be at least = 2 *
   TCP_SND_BUF/TCP_MSS for things to work. */
#define TCP_SND_QUEUELEN        (4 * TCP_SND_BUF/TCP_MSS)

Therefore, if the snd_buf needs to be tcphdr->wnd << pcb->snd_scale, so if the snd_scale is greater than log2(TCP_MSS), then there is a problem. I've never seen a snd_scale greater than 7, so this is highly unlikely and snd_queuelen is probably fine as u16_t.

I agree with your readability changes for optdata.

However, I don't think the RCV_WND_SCALE #define will work because you need to do the htons AFTER the shift, not before. So, for this one, how about:
#define RCV_WND_SCALE(pcb, wnd) (htons((wnd) >> (pcb)->rcv_scale))
and seg->tcphdr->wnd = RCV_WND_SCALE(pcb, pcb->rcv_ann_wnd);

So, if we agree that we need to keep rcv_ann_wnd and snd_wnd as u32_t, and change RCV_WND_SCALE I will make the other changes and a new patch.

Rishi Khan <rishi>
Fri 20 Jun 2008 02:46:33 PM UTC, comment #1: 

I think this patch looks good, although would suggest a few tweaks, and I have 2 questions.

First the questions:
1) why are the various window sizes in tcp.h changed to u32_t - I thought they were still limited to 65536?

2) why is snd_queuelen increased to u32_t - I can't foresee having more than 65536 segs for a single PCB!

As for some suggested tweaks, rather than trying to bolt-on optdata2, and do a memcpy, I think we should just rationalise the option addition into a single new function in tcp.c (which tcp_in.c could call), which allows for some future additions.

Something like
#if TCP_RCVSCALE
typedef u32_t tcp_option_data[2];
#else
typedef u32_t tcp_option_data;
#endif

void tcp_build_options( struct tcp_pcb *pcb, tcp_option_data *optdata, u8_t *optlen );

And then the tcp_enqueue stuff becomes something like:
tcp_option_data optdata;
u16_t optlen;
tcp_build_options(npcb, &optdata, &optdatasize)'
tcp_enqueue(npcb, NULL, 0, TCP_SYN | TCP_ACK, 0, (u8_t *)&optdata, optlen);

To make the code more readable, it may be nice to have and use the following macros in tcp.h:
#if TCP_RCVSCALE
#define RCV_WND_SCALE(pcb, wnd) ((wnd) >> (pcb)->rcv_scale)
#define SND_WND_SCALE(pcb, wnd) ((wnd) << (pcb)->snd_scale)
#else
#define RCV_WND_SCALE(pcb, wnd) (wnd)
#define SND_WND_SCALE(pcb, wnd) (wnd)
#endif

So that elsewhere we could have just something like e.g.
seg->tcphdr->wnd = RCV_WND_SCALE(pcb, htons(pcb->rcv_ann_wnd));

Would you be able to look at doing it that way? I hope it should be an easy change and I think makes the patch a bit nicer.

Thanks for contributing back!

Jifl


Jonathan Larmour <jifl>
Group Member
Mon 09 Jun 2008 06:34:33 PM UTC, original submission:  

I needed TCP window scaling for my app. I added it and here is the patch. I have tested it and it works properly. It is activated when the parameter TCP_RCVSCALE is set in lwipopts.h. I don't know how well it will patch against the CVS because I diff'd it off of my latest version and not off of 1.3.0. Please look at it and see if you like it. If it doesn't patch well to the CVS and you think it's a good addition, I'll re-diff it.

Rishi Khan <rishi>

 

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

Attached Files
file #17971:  large-pbuf.patch added by wdumon (5KiB - application/octet-stream - deal with large pbufs (>64K) for situations with rcv windows > 64K)
file #15825:  wnd_scale.patch added by rishi (8KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-02-20 goldsimon StatusIn Progress Done
        Open/ClosedOpen Closed
    2014-02-07 goldsimon StatusNeed Info In Progress
        Assigned tojifl goldsimon
    2013-01-15 goldsimon CategoryNone TCP
    2009-04-20 wdumon Attached File- Added large-pbuf.patch, #17971
    2008-06-20 jifl StatusNone Need Info
        Assigned toNone jifl
    2008-06-09 rishi Attached File- Added wnd_scale.patch, #15825
        Carbon-Copy- Added rishi

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code