[lwip-devel] Re: [patch #6537] wnd_scale TCP option addition

[Wim Dumon]

Kieran,

I started doing it that way, but felt that I would end up adding a lot
of complexity for no good reason. pbuf is already a chained structure,
and chaining these chains again would need extra allocated structures
& logic. I'm not too familiar with lwip, maybe there is another
list-like structures that I could use? Or do you think it's a good
idea to implement recursive pbufs (pbufs with pbufs in their payload)?
And then there's the question of where to store these list-related
structs: do we need a new pool just for this?

Note: you're talking about a new field, but I did not add a new field.
refused_data was there already, the big thing is that tot_len changed
from 16 bit to 32 bit.

So why do I think the 32 bit tot_len is an acceptable approach?
Systems that care to support >64K rcv windows probably have enough
memory to not care about the extra few bytes in the pbuf struct.
Secondly, because the solution is simple. Thridly, because the 32bit
length field can easily be made a compile-time configurable option, in
which case the situation would be identical to what it is now for
people that don't care about >64K windows.

With respect to Simon's remarks:  aren't you exaggerating a bit when
we're talking about doubling the size of pbuf? On a 32 bit platform,
the sizes in this struct were 4, 4, 2, 2, 1, 1, 2 bytes. Now it's 4,
4, 4, 2, 1, 1, 2. Adding alignment overhead, I would expect this
structure to be padded with 2 bytes, so we're going from 16 to 20
bytes. On 16 bit platforms, the sizes were 2, 2, 2, 2, 1, 1, 2. Now
it's 2, 2, 4, 2, 1, 1, 2. Assuming that a 16 bit processor only needs
16 bit alignment, this does not require any padding. So we grow from
12 to 14 bytes. (but your mileage may vary)

As a side remark, I propose to also add an assert in pbuf_cat to check
for pbuf tot_len overflow.

Regards,
Wim.

On Mon, Apr 20, 2009 at 11:32 AM, Kieran Mansley
<address@hidden> wrote:
>
> Follow-up Comment #8, patch #6537 (project lwip):
>
> 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?
>
>    _______________________________________________________
>
> Reply to this item at:
>
>  <http://savannah.nongnu.org/patch/?6537>
>
> _______________________________________________
>  Message sent via/by Savannah
>  http://savannah.nongnu.org/
>
>