10.12.2015 09:20, Michael Tokarev wrote:> 10.12.2015 03:35, Samuel Thibault wrote: > [] > > I suggest reducing ifdeffery in handle_incoming_vpn_data(), especially > the error checking code. > > The function isn't that large now, it might be much better to have > two different implementations. Like this (untested, patch attached): > > void handle_incoming_vpn_data(int sock) { > > #ifdef HAVE_RECVMMSG > #define MAX_MSG 256 > > vpn_packet_t pkt[MAX_MSG]; > sockaddr_t from[MAX_MSG]; > struct mmsghdr msg[MAX_MSG]; > struct iovec iov[MAX_MSG]; > int num = 1, i; > > for(i = 0; i < MAX_MSG; i++) > { > msg[i].msg_hdr.msg_name = &from[i].sa; > msg[i].msg_hdr.msg_namelen = sizeof(from[i]); > iov[i].iov_base = &pkt[i].seqno; > iov[i].iov_len = MAXSIZE; > msg[i].msg_hdr.msg_iov = &iov[i]; > msg[i].msg_hdr.msg_iovlen = 1; > msg[i].msg_hdr.msg_control = NULL; > msg[i].msg_hdr.msg_controllen = 0; > }Also, this is interesting. We should either reduce MAX_MSG to, say, 64, or even 16, and/or initialize this array statically (except of iov.iov_len). There's just too much work doing on each step, and it is highly unlikely we'll have 256 packets in queue to process. Thanks, /mjt
On Thu, Dec 10, 2015 at 09:34:45AM +0300, Michael Tokarev wrote:> > 10.12.2015 03:35, Samuel Thibault wrote: > > Here it is.Thanks!> 10.12.2015 09:20, Michael Tokarev wrote: > > > > I suggest reducing ifdeffery in handle_incoming_vpn_data(), especially > > the error checking code.That does seem to make it even more readable. I can merge both your changes :)> Also, this is interesting. We should either reduce MAX_MSG to, say, 64, > or even 16, and/or initialize this array statically (except of iov.iov_len).Reducing MAX_MSG might make sense. I think some real-world testing should tell us how many packets are queued on average under high load, and size the arrays accordingly. As for static initialization, I don't see that being helpful. It would be much more verbose to write out the initialization of a whole array, and the compiler would generate more or less identical code to the for-loop in Samuel's patch. The only improvement I can think of is this: for(i = 0; i < MAX_MSG; i++) { iov[i] = (struct iovec){ .iov_base = &pkt[i].seqno; .iov_len = MAXSIZE; }; msg[i].msg_hdr = (struct msg_hdr){ .msg_name = &from[i].sa, .msg_namelen = sizeof from[i], .msg_iov = &iov[i], msg_iovlen = 1, }; } Or am I missing something? -- Met vriendelijke groet / with kind regards, Guus Sliepen <guus at tinc-vpn.org> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20151210/4f9c0fc5/attachment.sig>
Michael Tokarev, on Thu 10 Dec 2015 09:34:45 +0300, wrote:> it is highly unlikely we'll have 256 packets in queue to process.I did get a bit more than a hundred packets in the queue at 800Mbps. But we can reduce to 64, yes. I wouldn't recommend using a static buffer, since we'd want to go threaded at some point. Allocating an array on the stack is very cheap anyway. Samuel
On Thu, Dec 10, 2015 at 10:15:19AM +0100, Samuel Thibault wrote:> I did get a bit more than a hundred packets in the queue at 800Mbps. But > we can reduce to 64, yes. I wouldn't recommend using a static buffer, > since we'd want to go threaded at some point. Allocating an array on the > stack is very cheap anyway.I assumed that one would only get more than 1 packet at a time under heavy load, but apparently traffic is bursty enough that it even triggers in a simple SSH session. But it rarely is over 64, so I kept it at that. Your patch was apparently against 1.0.26, not 1.1pre11 as you said earlier, so I had to port it to the 1.1 branch. It's committed now, with Michael's de-ifdeffication incorporated as well. I also made the buffers static, as suggested by Michael. If we go the multi-threaded way, they can be made thread-local. -- Met vriendelijke groet / with kind regards, Guus Sliepen <guus at tinc-vpn.org> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20151210/c6f755b2/attachment.sig>