patchlwIP - A Lightweight TCP/IP stack - Patches: patch #8882, Vector improvements

 
 

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

patch #8882: Vector improvements

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Thu 21 Jan 2016 09:51:32 PM UTC
   
 
Category:  sockets/netconn Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  jcunningham Open/Closed:  Closed
Planned Release:  2.1.0

Jump to the original submission

Fri 03 Mar 2017 11:29:09 AM UTC, comment #51: 

Just go ahead with the tests. That file is an ugly accumulation of different tests,so yours is very welcome :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 02 Mar 2017 11:58:19 PM UTC, comment #50: 

Actually, I re-read the comment history here and realized that there was no feedback on the sendmsg tests.  I've reverted the commit so I can get some feedback.  I think they are useful, especially for any devs wanting to test out sendmsg functionality

Joel Cunningham <jcunningham>
Group Member
Thu 02 Mar 2017 10:59:32 PM UTC, comment #49: 

Vector patch committed in 2980f7cc58ec4c6f1a16eb74802b213a063b08f3

sendmsg socket example tests committed in 927da5705674368a19bc14f653d537633c6b5fc1

Closing as done for now

Joel Cunningham <jcunningham>
Group Member
Thu 02 Mar 2017 04:21:25 PM UTC, comment #48: 

Yes, I to master. Why don't you just push it there and we can think about the conversion/assert/mpu problems later...
After all it should work u less using an external definition.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 02 Mar 2017 04:15:08 PM UTC, comment #47: 

Simon,

Any follow up thoughts on if adding LWIP_ASSERTS is enough?  I'd like to continue the momentum on getting this committed :)

2.0.2 is coming off of origin/STABLE-2_0_0, so this would go to current master right?

Joel Cunningham <jcunningham>
Group Member
Tue 28 Feb 2017 10:51:06 PM UTC, comment #46: 


> I'd just have to re-think the planned release. Maybe this would be better off in 2.1.0 to give us some time to test it...


I'm fine with 2.1.0.  Technically this is a performance fix as it prevents each vector from becoming its own individual TCP segment, outputted individually

> As to the binary compatibility of an external struct: we don't we just compare size and offsets of the two structs using LWIP_ASSERT? Somewhere in an init function where it's only done once. Like 'lwip_init()'.


We could do that, what would be the recommended resolution if the asserts are failing though?  Have the port maintainer remove the external definition from include tree?

> I can imagine that going wrong, yes. I'm not too keen on fixing this right now though. Letting it go wrong might get us a feedback on if it's really used :-)


I like this approach, we can learn more when there is yelling on the mailing list ;)

Joel Cunningham <jcunningham>
Group Member
Tue 28 Feb 2017 08:36:33 PM UTC, comment #45: 

I'd just have to re-think the planned release. Maybe this would be better off in 2.1.0 to give us some time to test it...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Feb 2017 08:34:43 PM UTC, comment #44: 

As to the binary compatibility of an external struct: we don't we just compare size and offsets of the two structs using LWIP_ASSERT? Somewhere in an init function where it's only done once. Like 'lwip_init()'.

>  do we have an issue with passing pointer to struct netvector in the MPU case?


I can imagine that going wrong, yes. I'm not too keen on fixing this right now though. Letting it go wrong might get us a feedback on if it's really used :-)

But really, I don't like the current MPU approach. What we would need to make it usable would be a simple "copy this pointer, x bytes, to a shared location" (and freeing it again later) instead of adding all those structs and pools.

I guess I'm OK with the rest of the patch, even if it slows down the non-multi send part.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 28 Feb 2017 02:40:24 PM UTC, comment #43: 

Additional thought: do we have an issue with passing pointer to struct netvector in the MPU case?

I understand that we can't pass a pointer to stack variables from the calling thread, but in the case of struct iovec, we don't know whether the caller allocated it on their stack or not

Joel Cunningham <jcunningham>
Group Member
Mon 27 Feb 2017 10:10:21 PM UTC, comment #42: 


> Having a quick look, the patch seems good. The only thing I'm not certain of is if it's OK to cast 'struct iovec' to the new 'struct netvector'. Especially the "msg.w.vector++" code in lwip_netconn_do_writemore() could go wrong if the (externally defined) struct iovec contained more fields than struct netvector (or was differently aligned).


Good catch on the external definition.  I agree there could be problems.  I don't have a good feeling for whether external struct iovec's are being used currently.  The definition I had provided has a rudimentary check which would only catch external definitions provided by a macro:


#if !defined(iovec)
struct iovec {
  void  *iov_base;
  size_t iov_len;
};
#endif


I see a couple of ways to solve this:

1) Add an additional define if the internal definition is being used.  If not defined, we can copy the pointers/lengths from struct iovec into an equivalent sized struct netvector in the sockets layer before calling netconn.  There will be a performance penalty, but it would ensure struct iovec is only in the sockets layer.  If internal definition is used, we can continue to use the cast.  I can also add some sizeof compiler checks to ensure the structures don't have size-mismatch in the internal case
2) Remove struct netvector and use struct iovec in netconn layer.  I'm guessing this option is out given the previous discussion we had where I had suggested moving it to def.h.  But this issue is evidence that keeping struct iovec isolated in the sockets layer does have cost
3) Remove support for external definitions of struct iovec.  Not sure if this is realistic, but the current method isn't that portable anyways.  We probably need something like struct timeval has (#define LWIP_TIMEVAL_PRIVATE)

> I guess for clarity, we might want to omit the 'len' parameter for the SENDPLUS/SENDMINUS calls...


I'll just change the calls in do_writemore to use 0 if there is no better guidance.  Maybe the parameter can be removed in a subsequent cleanup

Joel Cunningham <jcunningham>
Group Member
Mon 27 Feb 2017 09:03:09 PM UTC, comment #41: 

Having a quick look, the patch seems good. The only thing I'm not certain of is if it's OK to cast 'struct iovec' to the new 'struct netvector'. Especially the "msg.w.vector++" code in lwip_netconn_do_writemore() could go wrong if the (externally defined) struct iovec contained more fields than struct netvector (or was differently aligned).
As to the API_EVENT, I'm unsure too what the meaning of the 'len' is. I guess the original meaning was bytes, but it has shown during development that this consumes too many cycles (we only call SENDMINUS when we want to mark a socket as not readable) and we don't always have a 'len' to be consistent (e.g. call SENDPLUS from tcp_poll() to unlock a write that is blocked due to RAM shortage). I haven't monitored lwIP from it's birth though (I have missed the first ~5 years)...
I guess for clarity, we might want to omit the 'len' parameter for the SENDPLUS/SENDMINUS calls...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 27 Feb 2017 05:45:40 PM UTC, comment #40: 

Attached is two patches:

= 02-27-17-Vectorize-netconn_write-for-TCP.patch =

This is an updated version of my previous patch from a year ago.  To recap, it adds a new netconn API that takes a set of vectors and then performs an atomic write via lwip_netconn_do_writemore (which now has support to loop over the set of vectors, calling tcp_write() for each vector, then tcp_output() (a single time)

Notable changes are:

1) struct iovec is no longer used in other layers than sockets.  To provide binary compatibility, I've defined a struct netvector which functions identically except it has the const attribute (improves upon opengroup omission)
2) IOV_MAX is defined.  This is an improvement that bounds checks that the caller didn't provide more vectors than we can support.  Currently we are limited to u16_t for the vector count in the netconn API.  I'm open to reducing this to u8_t (255 vectors) if that is more appropriate for our goal of being light weight.  My applications using sendmsg typically sends up to 10-15 vectors
3) I added support in do_writemore() to also loop around sending again if we are can't finish the current vector due to 16-bit size limitations.  This means if we have a vector greater then 65535 and a send buffer larger than 65535 we can correctly output the vector without having to wait for poll/sent_tcp to be called for blocking or non-blocking ending up with a partial write

I do have one TODO in the patch that I need some guidance on.  When API_EVENT() is called with NETCONN_EVT_SENDMINUS, previously the len parameter was sent.  It's unclear to me what this variable is communicating (the amount that was last successfully sent before filling the buffer?).  It is un-used in the sockets event callback.  I can't easily replace it with the new equivalent (which is the current total amount sent, stored in msg.w.offset) because the parameter is only 16-bits (msg.w.offset is size_t)

= 02-27-17-Added-sendmsg-tests-to-socket-examples.patch =

This patch adds support for testing sendmsg TCP/UDP functionality.  It doesn't rely on any of the changes in the vectorize patch.  I originally developed it with my first sendmsg patch, but we never got around to committing it.  I've updated it for IPv6 support and would like to check it in if you guys can review it

(file #39838, file #39839)

Joel Cunningham <jcunningham>
Group Member
Fri 24 Feb 2017 09:55:54 PM UTC, comment #39: 

Below discussed cleanups committed in 36fa1a97d415410c9d8f1ed350702775829d370b

Also made some minor cleanups in netconn_write_partly committed in 36b9a45c3bf5e06563315dd5d55e091d120fc07b:

(from commit message):

This commit changes netconn_write_partly to use msg.w.offset to set
bytes_written for both blocking and non-blocking connections

This is correct because msg.w.offset is the canonical output from
the do_write call and in the case that not all bytes were written,
(a bug?) returning the full size to the caller is dangerous

Lastly, this commit adds an assert for the blocking case to sanity
check that all the bytes we requested were written.  This will help
catch bugs in do_write

Let me know if you have any questions, thanks!

Also, I'm getting back to rebasing my vector patch, hopefully have it posted this weekend or next week

Joel Cunningham <jcunningham>
Group Member
Fri 17 Feb 2017 09:33:38 PM UTC, comment #38: 

You're right, it is unnecessary and could be removed.  I kept it because the previous code was doing it to msg.w.len.

Joel Cunningham <jcunningham>
Group Member
Fri 17 Feb 2017 09:26:35 PM UTC, comment #37: 


> Those changes [..] are in error cases.


Right. "bytes_written" is only fille for ERR_OK, I missed that. In that case, why are we resetting msg.w.offset to 0 in error cases?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 17 Feb 2017 09:20:31 PM UTC, comment #36: 

Simon,

Those changes from msg.w.len = 0 to msg.w.offset = 0 are in error cases.

If a non-blocking write does a partial write due to filling send buffer, the write will be limited to the size of the send buffer and then the below case will enter write_finished (assuming tcp_write returns ERR_OK)


if (err == ERR_OK) {
  err_t out_err;
  conn->current_msg->msg.w.offset += len;
  if ((conn->current_msg->msg.w.offset == conn->current_msg->msg.w.len) || dontblock) {
    /* return sent length (caller reads length from msg.w.offset) */
    /* everything was written */
    write_finished = 1;
   }


Maybe we should remove the "everything was written" comment because it's not correct.

If a non-blocking write encounters ERR_MEM from tcp_write(), it will return ERR_WOULDBLOCK and 0 for bytes written because tcp_write did not accept the buffer


Joel Cunningham <jcunningham>
Group Member
Fri 17 Feb 2017 09:07:24 PM UTC, comment #35: 

Joel,

looking at the diff, can you please explain the changes like
-        conn->current_msg->msg.w.len = 0;
+        conn->current_msg->msg.w.offset = 0;

To my knowledge, msg.w.offset is used as the "bytes_written": e.g. nonblocking write cannot write everything without blocking, just a few bytes --> doesn't that always get zero now?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 17 Feb 2017 07:41:17 PM UTC, comment #34: 

Simon,

I went ahead and made the change in 4c76fd500c59f0fb38027f6c602641b1b79ae505.

I think you'll be pleased at the elimination of extra work in checking/clearing write_offset.  Also I found that once offset was in msg.w, there was no need to marshal the result to msg.w.len for output and that msg.w.offset could be used directly as output, leaving msg.w.len as purely the input length of dataptr.

Please let me know if you have any questions on the commit

Joel Cunningham <jcunningham>
Group Member
Fri 17 Feb 2017 05:35:16 AM UTC, comment #33: 

Without reading the code, I guess it's OK to move the member.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 17 Feb 2017 12:51:18 AM UTC, comment #32: 

As I was working on updating my old patch, I was studying lwip_netconn_do_writemore() and one thing that is curious is why write_offset is a member of struct netconn and not struct api_msg (under the "w" union).

write_offset is only used by lwip_netconn_do_writemore() to store offset into the dataptr.  When a write is not in progress, the variable seems to have no use.  I'm wonder if write_offset could be relocated to struct api_msg and reduce the size of struct netconn.

Digging through git, it looks like write_offset was added with 3 others way back in 9f05cabf872919828eda2138170e6eaded510fdf. 

My vector work is looking at having to add an offset into current vector in addition to write_offset and it might be better to put that in struct api_msg

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 03:15:21 PM UTC, comment #31: 

I'll work on an updated patchset with this approach and we can examine the overhead

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 07:12:10 AM UTC, comment #30: 

Good that we waited so long. By now I'm with you that the two code paths should be the same ;-) But let's watch out the overhead stays small enough.

I still have code size measurements on my private todo-list, but that could well take a while...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 08 Feb 2017 10:12:17 PM UTC, comment #29: 

Sounds good!  From where we left off, there wasn't a conclusion on the way to represent a set of vectors as they move down the stack (sockets -> netconn -> TCP).

We had explored the idea of creating a pbuf chain in the sockets layer (to avoid introducing a vector type) but then had discussed how a chain of PBUF_REFs would end up being copied; 1) because tcp_write() doesn't take pbuf as input and
2) even if we had a tcp_write() that took pbufs, we need a copy since TCP holds onto the data.  This makes creating the pbuf chain in sockets a wasted effort.

If we refresh on my original patch, it was adding support to lwip_netconn_do_writemore() to operate on a set of data pointers, calling tcp_write for each pointer before calling tcp_output (making the send of all vectors atomic).  There is no in-efficiency here for the vector case, but there was concern about the added checks for the normal TCP send case (which becomes a single vector case) because after calling tcp_write(), we do a couple of small checks to see if there are any more vectors before exiting do_writemore().  Maybe this is a helpful data point, but other TCP/IP stacks that I've studied implement send() and sendmsg() in the same pathway, with send() using a single vector.

Joel Cunningham <jcunningham>
Group Member
Wed 08 Feb 2017 09:22:03 PM UTC, comment #28: 

Now that 2.0.1 is out (for some time), let's work on this again.

If I'm not mistaken, the first thing we need to really get a performance improvement is a decent implementation of adding preallocated (and prefilled) pbufs to a tcp pcb?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Dec 2016 11:14:01 PM UTC, comment #27: 

I'm fine with delaying until a 2.0.2, thanks for checking!

Joel Cunningham <jcunningham>
Group Member
Thu 15 Dec 2016 11:59:12 AM UTC, comment #26: 

Joel, can we delay this further? I would want to release 2.0.1 with the bugs fixed up to now before christmas, and I think we need a little more time on this, of only for testing...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 29 Nov 2016 03:25:55 PM UTC, comment #25: 

I think in order to decide on the type to represent our vectors (either a netbuf, pbuf, or struct iovec (original proposal)) we need some longer term direction on how we envision the TCP APIs accepting data.

If the vision is that TCP APIs (tcp_write) will always be responsible for creating TCP segment payload pbufs, then having the socket/netconn layers allocate and free netbufs/pbufs is wasteful and the only gain would be storing our vectors in a pre-existing type (even though it is inefficient).

If there is a vision for netconn API that would take netbuf directly and eventually tcp_write would have support for accepting data in a pbuf, then I could see value in using netbuf/pbuf to represent out set of vectors

If we are looking for the most efficient structure for storing vectors, then something like struct iovec would be our choice.  Since we are restricting open group types to the sockets layer, we could introduce a netconn vector set type

Joel Cunningham <jcunningham>
Group Member
Tue 22 Nov 2016 09:09:31 PM UTC, comment #24: 

I'm really sorry I keep forgetting to answer. I'm just very busy with other (non-lwIP) things lately... :-(

Short answer to the last question first: it might be better to not expose pbuf in netconn API since we have netbuf for that. I don't know why, though, as I'm not sure what we gain from that indirection?

And an explanation to all my hesistant answers: we have 2 goals here:
a) make vector sending efficient. This is more important for high performance environments.
b) keep code size and execution overhead small. This is more important for small, low performance systems.

We have to find a way to fit both requirements without having too much code duplication...

Creating a netbuf (or pbuf) that is of REF type just to throw it away in tcp_write() is not the best thing for b), but I hope we get is sorted out soon to find the best for a) AND b)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Nov 2016 03:26:26 PM UTC, comment #23: 

So the last patch that I posted is file #37785 which converts lwip_netconn_do_write() to operate on a netbuf containing a chained pbuf rather than just a data pointer.  I think the previous patches were either already accepted (UDP optimization) or were rejected.

Our last brainstorming/approach was that if we could get the vectors for a TCP send into a netbuf, we could also add TCP support to netconn_send() and have a common netbuf API for UDP/TCP sending.

Once I converted lwip_netconn_do_writemore() to take a netbuf, then we could hook up sendmsg to netconn_send, but the problem is that the function signature of netconn_send() doesn't really support partial writes as there is no way to output the size sent.  This left me thinking that the netconn_send approach may not make sense for TCP and we need a new function

Coming back to the issue after some time, I'm not sure why we thinking of using a netbuf because lwip_netconn_do_writemore() is only operating on the chained pbuf.  We could have sendmsg assemble the pbuf, then have a netconn API that takes the pbuf and sends it through to lwip_netconn_do_writemore().  My only question is, is it appropriate to have a pbuf exposed in the netconn API?

Joel Cunningham <jcunningham>
Group Member
Mon 14 Nov 2016 10:43:51 AM UTC, comment #22: 

Coming back on this, I think I'll have to read up a bit of the last posts...

To sum up my thoughts:
- I don't think we should dump netconn_write(): people are using this and currently, there's no use in passing netbufs for TCP as data is copied in tcp_write, anyway
- a consistent send function for use by sendmsg is still a good idea
- passing pbufs to enqueue in TCP would make sense with sendmsg (I'm not up to date on what the corresponding patch does here, but we'd need this for zero-copy from app-threads)

I hope to find the time in the next days/weeks to get a closer look.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 19 Aug 2016 03:47:43 PM UTC, comment #21: 

Simon,

Ok thanks, for letting me know.  I agree the 2.0 release is top priority :)

Joel Cunningham <jcunningham>
Group Member
Thu 18 Aug 2016 07:54:56 PM UTC, comment #20: 

Joel, I'm sorry I don't have time for this at the moment. I'm struggeling to find time for lwIP and discussing things with Dirk to get the release done. We'll have to delay discussion on this.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 15 Aug 2016 09:27:03 PM UTC, comment #19: 

Simon,

Any thoughts on my last comment?  Thinking about it more over the last week, maybe a TCP specific netconn send that takes a netbuf isn't that bad.

Something like netconn_send_tcp() and then leave netconn_send() as it is now (only works for UDP/RAW)

Once we have this support, we could deprecate netconn_write_partly().  It's been strange that the netconn APIs for sending data over TCP and UDP are really different.  UDP expects the caller to assemble a netbuf, but not TCP

Joel Cunningham <jcunningham>
Group Member
Sat 30 Jul 2016 12:42:58 AM UTC, comment #18: 

So I started working on part 2 (adding TCP support to netconn_send).  I came across two issues with trying to use this API for TCP

  1. No way to pass apiflags - I was able to work around this by reusing the flags variable in struct netbuf.  This worked out nicely, because it allowed me to remove the apiflags variable from struct api_msg.w.  So a net-zero for adding/removing variables :)
  2. No way to output bytes sent - I'm stuck here on this one.  netconn_send() returns err_t, which isn't big enough to return the number of bytes sent


Other than the bytes sent, things are hooked up working.  Any ideas on how to move forward with netconn_send or do we need another API that is TCP only (wich takes a netbuf) and leave netconn_send as UDP/RAW?

I also had to introduce a static function which allows reuse of the logic in netconn_write_partly() for setting up the message and invoking lwip_netconn_do_write.  This leaves netconn_write_partly() simply translating between data pointer and size into a netbuf and then calling the common function

Joel Cunningham <jcunningham>
Group Member
Sat 09 Jul 2016 05:55:11 PM UTC, comment #17: 

I was able to get started on this during the weekend.  I've split the work up into a couple of chunks.  First is converting lwip_netconn_do_write() to operate in terms of a struct netbuf rather than void * and size.  See attached patch.  Details of the change are contained in the commit message.

Next I'll work on another patch adding support in netconn_send for TCP and then a third patch will convert the sendmsg TCP path to use netconn_send and combine the logic for UDP/TCP to create a netbuf out of the IO vectors

Feel free to look over the patches as I post them and give feedback

Thanks!

(file #37785)

Joel Cunningham <jcunningham>
Group Member
Thu 07 Jul 2016 10:24:25 PM UTC, comment #16: 

Going with the netbuf approach + lwip_netconn_do_writemore() and targeting for 2.0.1 sounds good to me :)

I can start working on an updated patch.  Hopefully I can get something together in the next couple of weeks.  I've been busy with non-lwip projects

Joel Cunningham <jcunningham>
Group Member
Thu 07 Jul 2016 01:39:52 PM UTC, comment #15: 


> I think adding support for TCP sending to
> lwip_netconn_do_send() is going to be complicated if we don't
> use/re-use logic from lwip_netconn_do_writemore()


That's true, didn't think of that :-/

> What do you think about modifying lwip_netconn_do_writemore()
> to use a netbuf pointer instead of a void *?


That's a good iead. However, I wouldn't do this for 2.0.0 but for 2.0.1 rather.

> Since the netbuf would be on the callers stack


That won't work for LWIP_MPU_COMPATIBLE, but it can be done through PP-magic like for the rest of the MPU-compatible code...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 07 Jul 2016 01:29:08 PM UTC, comment #14: 

Simon,

I like the idea of treating the set of vectors the same for both UDP and TCP but I think adding support for TCP sending to lwip_netconn_do_send() is going to be complicated if we don't use/re-use logic from lwip_netconn_do_writemore()

do_writemore() has all the infrastructure to block the caller during a partial write and then callback during poll_tcp() and sent_tcp() to continue sending.  Non-blocking would be easy, as we just return the partial number of bytes, it's the blocking that is challenging.

What do you think about modifying lwip_netconn_do_writemore() to use a netbuf pointer instead of a void *?  We currently have the following state in struct api_msg:


/** used for lwip_netconn_do_write */
    struct {
      const void *dataptr;
      size_t len;
      u8_t apiflags;
#if LWIP_SO_SNDTIMEO
      u32_t time_started;
#endif /* LWIP_SO_SNDTIMEO */
    } w;


Since the netbuf would be on the callers stack, we wouldn't need to increase the size of struct api_msg.  We could then modify lwip_netconn_do_writemore() to operate on a netbuf containing one or more pbufs.  I think the only thing that would change in the normal send case is that netconn_write_partly() would need to assign dataptr to a netbuf via netbuf_ref() (like is currently done for UDP sendto)

This work would also start laying the ground work for having tcp_write() accept pbufs directly as we'd already have the top end working with pbufs, then later tcp_write could be improved to also support pbufs.

Joel Cunningham <jcunningham>
Group Member
Thu 07 Jul 2016 12:11:49 PM UTC, comment #13: 

Sorry for taking so long again.

Could we combine the TCP path of lwip_sendmsg() with the UDP path (allocate a netbuf + PBUF_REF for each iovec)? Then we would have to change lwip_netconn_do_send() to handle the netbuf for TCP.

(Although lwip_netconn_do_send() doesn't return the number of written bytes, but I'm not sure what would be correct here: retry or return less than requested).

Either way, since sendmsg() is new to 2.0.0, let's sort this out before the release.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Jun 2016 02:44:49 PM UTC, comment #12: 

Simon,

Have you had a chance to think about this issue anymore?

Just to re-cap since this ticket has been used for a couple of vectored I/O improvements, the remaining issue is that TCP can't atomically accept a set of vectors because lwip_netconn_do_writemore() only takes a single data pointer.  The current code ends up calling tcp_write() and tcp_output() per vector which can result in multiple small packets (one per vector) when TCP_NODELAY is enabled

I've been running with the patch in my port than modifies lwip_netconn_do_writemore() to take a set of vectors as input and call tcp_write() in a loop until finished and then a single tcp_output() call

I could see this either going in 2.0.0 since it addresses a problem with an existing feature (added since 1.4.1 release) or going in 2.0.1 depending on how much development changes the final solution requires

Joel Cunningham <jcunningham>
Group Member
Mon 14 Mar 2016 01:58:33 PM UTC, comment #11: 

Sylvain,

Thanks for the update, I'm not very familiar with the PPP code and didn't realize it originated as 3rd party code (as indicated by Simon).

I'm on board with constraining the open group/posix types to the sockets layer and the current discussion remains on how to implement the feature in a way that doesn't duplicate complex code but also doesn't cause worse performance in the single vector (data pointer) write case.

Joel Cunningham <jcunningham>
Group Member
Sat 12 Mar 2016 08:37:10 PM UTC, comment #10: 

I'm a bit late here, but PPP does not use struct timeval, struct timeval declarations are only present inside features that are not supported :-)

Sylvain

Sylvain Rochet <gradator>
Group Member
Tue 08 Mar 2016 09:12:49 PM UTC, comment #9: 

I agree the trade off is having lwip_netconn_do_writemore() perform a couple of extra checks if there are additional vectors in the set and loop around to send the additional vector.  In netconn_write_partly, we would do a small amount of extra work to setup a vector of 1.

In order to support an atomic write of vectors, the only other approach I see is having a second implementation of lwip_netconn_do_writemore, handling the vectors, unblocking of the caller, trying to send again during sent_tcp and poll_tcp. Since the netconn is only setup to have a single set of callbacks, which reference lwip_netconn_do_writemore(), we'd need to figure out how to handle that.

In general, lwip_netconn_do_writemore() is really complex.  I probably spent 3-4 hours going through all the execution paths in lwip_netconn_do_writemore() making sure my change maintained the existing behavior for a single vector versus a set of vectors.  I'd like to avoid duplicating this level of complex code if possible




Joel Cunningham <jcunningham>
Group Member
Mon 07 Mar 2016 06:54:08 PM UTC, comment #8: 


> Were you suggesting re-writing lwip_netconn_do_write() to use a pbuf instead of a pointer and size?


No, not now :-)
I'm not convinced yet, that this would be a good idea as it makes the standard path of a single write more complicated.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 07 Mar 2016 03:56:05 PM UTC, comment #7: 

0002-Optimize-sendmsg-UDP-pbuf-chain-creation.patch submitted in 80cc988f2f27479c6e75cfa986e706382dd47c48

Simon,

I agree that a pbuf chain would be a way to represent a set of vectors in the TCP case as well, but unfortunately the TCP write path in the netconn layer can't accept a pbuf.

Were you suggesting re-writing lwip_netconn_do_write() to use a pbuf instead of a pointer and size?

We could do this, though since tcp_write() also doesn't take a pbuf, we'd end up with the change that effectively I created, where lwip_netconn_do_writemore() is managing a set of vectors and is iterating through them as the TCP buffer/window allows more sending, but instead we are using a pbuf chain for our type rather than struct iovec.

This eliminates the type issue you brought up and I'd be fine to implement it this way if you're ok with lwip_netconn_do_writemore() iterating through pbufs rather than a more traditional set of vectors (or a single pointer as is now).  This could even be an iterative step towards having a version of tcp_write that handles pbufs.

Joel Cunningham <jcunningham>
Group Member
Mon 07 Mar 2016 09:18:53 AM UTC, comment #6: 

Looks good. Too bad TCP doesn't accept pbufs. Maybe a way to pass netbufs (ref) into the tcpip_thread/api_msg.c might be a way to do the TCP part...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 02 Mar 2016 10:31:18 PM UTC, comment #5: 

0002-Optimize-sendmsg-UDP-pbuf-chain-creation.patch contains the UDP pbuf chain building all within the sockets layer (lwip_sendmsg call).  It actually wasn't as messy as I was thinking, thanks for the suggestion.

(file #36530)

Joel Cunningham <jcunningham>
Group Member
Mon 29 Feb 2016 03:23:05 PM UTC, comment #4: 

Simon,

I agree creating the chain of pbufs could also happen in the sendmsg() call itself.  I initially placed it in netbuf because it seemed like netbuf is an abstraction that hides dealing with pbufs directly and it would be better (?) to continue to use the abstraction if possible.

I've spotted other places in sockets.c where pbufs are accessed/manipulated directly, so this wouldn't be establishing a new precedent.

Keeping it in the sockets layer would also avoid having to pass the array of vectors to another layer and thus we no longer need the struct iovec change for getting the UDP change in.

I'll work on another patch

Joel Cunningham <jcunningham>
Group Member
Thu 25 Feb 2016 08:05:23 PM UTC, comment #3: 

Well, PPP is somewhat a special case in lwIP as it is very much just copied from pppd and then adpted to lwIP. The core lwIP code tries to not use structures define by the (network-) standards.

This is probably to prevent confusion when the types are already defined. E.g. when compiling on windows, you're bound to get compiler errors when including winsock.h to get struct timeval defined, so you end up with 2 different types with the same name. That's OK for sockets.h, but not for netconns (at least that's the way I see it, currently).

Can't we convert from struct iovec to pbuf in sockets.c?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 19 Feb 2016 03:33:21 PM UTC, comment #2: 

Simon,

I've got 3 changes here in this patch, let's see if we can make some progress on some of them

struct iovec - my first thought on this was more in line with your thinking where we didn't want open group types in netconn layer, but then I was grepping and found that struct timeval is used in other files than sockets.c (some of the PPP code).  So it wasn't as clear to me.

My other thoughts is that we can make a identical type for use in netconn/netbuf and then just cast struct iovec when passed in from sockets.c calls.  Netconn and netbuf code don't seem to share a "net" level header so we'd need the type in a file both can include to avoid two net level structures

UDP optimization - this patch only changes the UDP path of sendmsg to be more efficient by not having to allocate and destroy netbufs in order to create a pbuf chain.  Did you get a chance to look at this?

TCP optimization - let's leave this aside for now, and keep the discussion to the first two things

Joel Cunningham <jcunningham>
Group Member
Thu 18 Feb 2016 07:24:29 PM UTC, comment #1: 

I'm not sure how to solve it differently, but I don't really like moving struct iovec to def.h. The core of lwIP should be compilable  without opengroup things. Setting LWIP_IOVEC_PRIVATE to 1 by default makes it worse by having conflicting typedefs on some systems.

Another backdraw is that your change would make non-vectored writes slower as they are converted to a single-vector write, even ifa simple write is the more common case (in my opinion at least).

I dont' really have an idea of how to improve this, though. Bypassing the netconn layer, it would be much easier to implement ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jan 2016 09:51:32 PM UTC, original submission:  

Continuing my work with sendmsg(), I've got a couple more improvements:

1) Move struct iovec to def.h - This patch moves the LwIP provided definition of struct iovec to def.h from sockets.h. This was done so that netconn code can reference the type without having the socket layer enabled

This also switches to using a compiler guard of #if LWIP_IOVEC_PRIVATE because #if !defined() on a structure doesn't actually work.  This also allows a port to include <sys/uio.h> instead for struct iovec

2) Optimize sendmsg UDP pbuf chain creation - This patch optimizes the sendmsg UDP pbuf chain creation by not allocating/freeing a netbuf per IO vector.  This adds a new API (netbuf_ref_vectors()) which takes an array of struct iovecs and creates a chain of PBUF_REFs in the provided netbuf. The logic is a combination of netbuf_ref() and netbuf_chain()

3) Vectorize netconn_write for TCP - This patch adds support to the netconn write APIs to take an input of vectors instead of a single data pointer.  This allows vectors sent on a TCP connection via sendmsg to be treated atomically.  The set of vectors is segmented into as much data as can fit into the send buffer and then the TCP output function is called

Previously, each vector was passed to netconn_write_partly and tcp_write segmented it into its own packet, which was then it was sent via tcp_output (if not Nagleing)

This patch adds vector support to lwip_netconn_do_writemore() which is the meat of the TCP write functionality from netconn/sockets layer. A new netconn API netconn_write_vectors_partly() takes a set of vectors as input and hooks up to do_writemore()

The patches have the follow dependency order:

0001-Move-struct-iovec-to-def.h.patch
0001-Optimzie-sendmsg-UDP-pbuf-chain-creation.patch
0001-Vectorize-netconn_write-for-TCP.patch

Joel Cunningham <jcunningham>
Group Member

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-02 jcunningham StatusNone Done
        Open/ClosedOpen Closed
    2017-03-01 goldsimon Planned Release2.0.2 2.1.0
    2017-02-27 jcunningham Attached File- Added 02-27-17-Vectorize-netconn_write-for-TCP.patch, #39838
        Attached File- Added 02-27-17-Added-sendmsg-tests-to-socket-examples.patch, #39839
    2016-12-15 goldsimon Planned Release2.0.1 2.0.2
    2016-07-09 jcunningham Attached File- Added 0001-Convert-lwip_netconn_do_write-to-use-a-netbuf.patch, #37785
    2016-07-07 goldsimon Planned Release2.0.0 Beta2 2.0.1
    2016-07-07 goldsimon Planned ReleaseNone 2.0.0 Beta2
    2016-03-02 jcunningham Attached File- Added 0002-Optimize-sendmsg-UDP-pbuf-chain-creation.patch, #36530
    2016-01-21 jcunningham Attached File- Added 0001-Move-struct-iovec-to-def.h.patch, #36123
        Attached File- Added 0001-Vectorize-netconn_write-for-TCP.patch, #36124
        Attached File- Added 0001-Optimzie-sendmsg-UDP-pbuf-chain-creation.patch, #36125

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code