|
| From: | Dirk Ziegelmeier |
| Subject: | Re: [lwip-devel] [bug #49218] pbuf_clen() overflow as a result of tcp_write concatenation |
| Date: | Wed, 19 Oct 2016 20:03:33 +0200 |
Hello Ambroz,
Thank you, much appreciated! Functionally speaking, that is exactly what I intended to make. The form of the patch could probably use a bit of cleanup, e.g. it does not apply cleanly to lwip-current and possibly never has. Also, I would agree with Simon that this is not actually resolving a bug, and I fear that filing it as a bugfix may have started off the discussion on the wrong foot. It is still a very worthy feature in my opinion, and I do hope that a slightly amended version of the patch, which I'd be happy to file later on, will be reconsidered for inclusion. However, that is probably best left to a post-2.0.0 discussion. Either way, you've just saved me quite some time and effort here, so again, thank you. :)
Regards,
David
On 10/2/2016 10:33, Ambroz Bizjak wrote:
Hi all,
My patch in bug https://savannah.nongnu.org/bugs/?46290 (rejected by
Simon) should solve this issue. It makes tcp_write detect when newly
passed non-copy (ref) data is contiguous to the previous, and in that
case the previous pbuf will be extended instead of a new pbuf created.
Best regards,
Ambroz
On Fri, Sep 30, 2016 at 1:28 PM, D.C. van Moolenbroek <address@hidden> wrote:
To be honest I'm not entirely sure what scenario you are sketching there, or
what you are comparing it to? To make sure we're talking about the same
thing, allow me to elaborate what I have in mind and then please feel free
to point out flaws in my logic :)
Take the scenario of a 4-byte chunk and then a 256-byte chunk of data being
sent by an application:
application:
- write(fd, "...", 4) // chunk #1
my side:
- allocate a 512-byte RAM pbuf: my_pbuf
- remote-copy-in requested 4 bytes to &my_pbuf->payload[0]
- call tcp_write(&my_pbuf->payload[0], 4, noflags)
lwip side, in tcp_write():
- allocate a REF pbuf
- point REF pbuf to (&my_pbuf->payload[0], 4)
- enqueue REF pbuf as part of unsent segment
application:
- write(fd, "...", 256) // chunk #2
my side:
- remote-copy-in requested 256 bytes to &my_pbuf->payload[4]
- call tcp_write(&my_pbuf->payload[4], 256, noflags)
lwip side, in tcp_write(ptr, len):
- allocate a REF pbuf
- point REF pbuf to (&my_pbuf->payload[4], 256)
- enqueue REF pbuf as part of unsent segment
At this point there are two reference pbufs attached to the same unsent
segment, and those two reference pbufs will end up at the NIC as well. Now,
with the kind of merging I am proposing, sending the second chunk of data
would proceed like this:
application:
- write(fd, "...", 256) // chunk #2
my side:
- remote-copy-in requested 256 bytes to &my_pbuf->payload[4]
- call tcp_write(&my_pbuf->payload[4], 256, noflags)
lwip side, in tcp_write(ptr, len):
- extend the last REF pbuf on the last unsent segment from
(&my_pbuf->payload[0], 4) to (&my_pbuf->payload[0], 260)
And ultimately only that one reference pbuf will end up at the NIC.
If I were to use WRITE_COPY, I would first have to do a remote-copy-in of
the data to.. somewhere temporary, after which I would hand the data to
tcp_write(), which would allocate an oversized RAM pbuf once and fill that
with the two chunks of data. Without WRITE_COPY, I have thus saved copying
260 bytes at the cost of having lwIP allocate a single REF pbuf. Given that
my expectation is that most writes are actually large, this would generally
be a tradeoff between copying chunks of 512 bytes of data and allocating REF
pbufs. I'd expect/hope the latter is faster..
David
On 9/30/2016 12:54, Dirk Ziegelmeier wrote:
I still don't understand why you do that. You need to get a pbuf_ref
from somewhere, initialize it, append it to the pbuf chain, in case of
sending iterate through that chain to create a consecutive buffer for
the MAC (=copy the data), dechain the pbuf and put it back into some
pbuf pool.
This is more effective/faster than just copying around a few bytes and
increasing the pbuf->len/tot_len?
Dirk
On Fri, Sep 30, 2016 at 12:45 PM, David van Moolenbroek
<address@hidden <mailto:address@hiddeng >> wrote:
Follow-up Comment #6, bug #49218 (project lwip):
> And that was my point: I don't expect you have a scatter-gather-MAC
that
supports TX frames with that many chunks. Being like that, you
probably end up
copying all bytes per CPU anyway. Thus, I don't see the advantage of
doing it
with ref pbufs. On the contrary, it's probably less performant.
That is correct, and that is the other side of why I think the ideal
solution
is to merge multiple consecutive buffer-local references. In that
case we do
eliminate an extra memory copy for all data in such circumstances,
while
keeping the number of chunks very low. As such I'll most probably be
adding
support for that locally in any case. If you think that is (in
principle)
something that is worthwhile might also be merged into lwIP itself,
I'd be
happy to make a proper patch out of it and submit it..
_______________________________________________________
Reply to this item at:
<http://savannah.nongnu.org/bugs/?49218
<http://savannah.nongnu.org/bugs/?49218 >>
_______________________________________________
Message sent via/by Savannah
http://savannah.nongnu.org/
_______________________________________________
lwip-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/lwip-devel
_______________________________________________
lwip-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/lwip-devel
_______________________________________________
lwip-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/lwip-devel
| [Prev in Thread] | Current Thread | [Next in Thread] |