Re: svn commit: r323942 - head/sys/net
On 25 Sep 2017, at 17:04, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 23:46, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 6:32, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 1:35, Stephen Hurd wrote: Author: shurd Date: Sat Sep 23 01:35:14 2017 New Revision: 323942 URL: https://svnweb.freebsd.org/changeset/base/323942 Log: Chain mbufs before passing to if_input() Build a list of mbufs to pass to if_input() after LRO. Results in 12% small packet forwarding rate improvement. I not saying anything against the change, I am just saying the commit message doesn’t describe what it does. Can you explain what was confusing about it or propose other wording? I'm not sure what was confusing, and I'd like to avoid similarly confusing messages in the future. I think it’s because I read “after LRO” as “after LRO processing happened” which is exactly not what is happening in that case; I know logically in the code order it’s “after LRO”. If I understand the change correctly (and I think jtl summarised it quite well already as well): “In cases when LRO is disabled or LRO is not consuming the packet, try to build an mbuf chain and pass the chain to if_input() thus lowering the per-packet overheads (*). For a packet forwarding case we have seen a 12% rate improvement for small packets.” (*) would be nice to describe them at this point so people understand where 12% come from (e.g., function call overhead, locking overhead, whatever ..) because that’s the reason you are doing the change. Also I am pretty sure this works with ether_input but not so much with fddi_input, iso88025_input, and ifdead_input is probably going to leak as well. Thanks for the heads up. They all seem to use m_freem(), so they shouldn't leak. Right. My bad. It doesn't look like they would actually work though (except ifdead_input of course). Well, they’d work with a bit of packet loss I guess ;-) /bz ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
Bjoern A. Zeeb wrote: On 23 Sep 2017, at 23:46, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 6:32, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 1:35, Stephen Hurd wrote: Author: shurd Date: Sat Sep 23 01:35:14 2017 New Revision: 323942 URL: https://svnweb.freebsd.org/changeset/base/323942 Log: Chain mbufs before passing to if_input() Build a list of mbufs to pass to if_input() after LRO. Results in 12% small packet forwarding rate improvement. forwarding seems a confusing word here.. The test was small (64 byte frames) received on one interface, then sent out on a different one using the net.inet.ip.forwarding sysctl (controlled via the gateway_enable setting in rc.conf). Then this makes no sense as we don’t do LRO if forwarding is enabled on the machine; https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?annotate=317390#l645 Basically, it changed from this: .. To this: … So while before it called if_input() for each separate mbuf that was not LROed, it now builds a chain of mbufs that were not LROed, and makes a single call to if_input() with the whole chain. For cases like packet forwarding where no packets are LROed, performance is better. Got it, so the “after LRO” in the original commit message is as confusing as forwarding. I not saying anything against the change, I am just saying the commit message doesn’t describe what it does. Can you explain what was confusing about it or propose other wording? I'm not sure what was confusing, and I'd like to avoid similarly confusing messages in the future. Also I am pretty sure this works with ether_input but not so much with fddi_input, iso88025_input, and ifdead_input is probably going to leak as well. Thanks for the heads up. They all seem to use m_freem(), so they shouldn't leak. It doesn't look like they would actually work though (except ifdead_input of course). ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On 23 Sep 2017, at 23:46, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 6:32, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 1:35, Stephen Hurd wrote: Author: shurd Date: Sat Sep 23 01:35:14 2017 New Revision: 323942 URL: https://svnweb.freebsd.org/changeset/base/323942 Log: Chain mbufs before passing to if_input() Build a list of mbufs to pass to if_input() after LRO. Results in 12% small packet forwarding rate improvement. forwarding seems a confusing word here.. The test was small (64 byte frames) received on one interface, then sent out on a different one using the net.inet.ip.forwarding sysctl (controlled via the gateway_enable setting in rc.conf). Then this makes no sense as we don’t do LRO if forwarding is enabled on the machine; https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?annotate=317390#l645 Basically, it changed from this: .. To this: … So while before it called if_input() for each separate mbuf that was not LROed, it now builds a chain of mbufs that were not LROed, and makes a single call to if_input() with the whole chain. For cases like packet forwarding where no packets are LROed, performance is better. Got it, so the “after LRO” in the original commit message is as confusing as forwarding. I not saying anything against the change, I am just saying the commit message doesn’t describe what it does. Also I am pretty sure this works with ether_input but not so much with fddi_input, iso88025_input, and ifdead_input is probably going to leak as well. /bz ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On 09/25/17 00:12, Stephen Hurd wrote: I've done an initial pass here: https://reviews.freebsd.org/D12487 Feel free to test it out and report findings in the review. I see some bugs. I'll send you a patch off-list. --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
Stephen Hurd wrote:
Hans Petter Selasky wrote:
On 09/24/17 01:46, Stephen Hurd wrote:
Basically, it changed from this:
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if_input(mbuf)
}
To this:
prev_mbuf = first_mbuf = NULL;
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if (prev_mbuf) {
prev_mbuf->m_nextpkt = mbuf;
prev_mbuf = mbuf;
}
else {
first_mbuf = prev_mbuf = mbuf;
}
}
if (first_mbuf)
if_input(first_mbuf);
So while before it called if_input() for each separate mbuf that was
not LROed, it now builds a chain of mbufs that were not LROed, and
makes a single call to if_input() with the whole chain. For cases
like packet forwarding where no packets are LROed, performance is
better.
Can such a similar logic be applied inside TCP LRO aswell?
It looks like it would be more complex to do a similar thing in
tcp_lro.c, and I'm not certain it would help much except in cases with
a large number of streams that mostly end up not being coalesced.
Taking a quick look, tcp_lro_flush() would need to be modified to
return an mbuf head and tail, then the caller would need to be
responsible for combining them into a single mbuf chain and calling
if_input().
Either that, or an mbuf tail could be passed into tcp_lro_flush(), the
tail modified in there, and an mbuf head returned... that way it would
work something like this:
The caller would be something like this:
m_head = m_tail = NULL;
LIST_FOREACH(le, bucket, hash_next) {
head = tcp_lro_flush(lc, le, _tail);
if (m_head == NULL)
m_head = head;
}
if (m_head)
if_input(m_head);
And tcp_lro_flush() would be something like this:
struct mbuf *tcp_lro_flush(struct lro_ctrl *lc, struct lro_entry *le,
struct mbuf **tail)
{
...
if (*tail)
*tail->m_next = le->m_head;
*tail = le->m_tail;
...
return le->m_head;
}
Hrm, maybe it wouldn't be all that difficult after all. :-)
I'll be driving across the country later this week, so I don't want to
start poking into LRO then disappear, so if nobody else tries it out
before then, I should take a look in a couple weeks.
I've done an initial pass here: https://reviews.freebsd.org/D12487
Feel free to test it out and report findings in the review.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
Hans Petter Selasky wrote:
On 09/24/17 01:46, Stephen Hurd wrote:
Basically, it changed from this:
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if_input(mbuf)
}
To this:
prev_mbuf = first_mbuf = NULL;
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if (prev_mbuf) {
prev_mbuf->m_nextpkt = mbuf;
prev_mbuf = mbuf;
}
else {
first_mbuf = prev_mbuf = mbuf;
}
}
if (first_mbuf)
if_input(first_mbuf);
So while before it called if_input() for each separate mbuf that was
not LROed, it now builds a chain of mbufs that were not LROed, and
makes a single call to if_input() with the whole chain. For cases
like packet forwarding where no packets are LROed, performance is
better.
Can such a similar logic be applied inside TCP LRO aswell?
It looks like it would be more complex to do a similar thing in
tcp_lro.c, and I'm not certain it would help much except in cases with a
large number of streams that mostly end up not being coalesced. Taking
a quick look, tcp_lro_flush() would need to be modified to return an
mbuf head and tail, then the caller would need to be responsible for
combining them into a single mbuf chain and calling if_input().
Either that, or an mbuf tail could be passed into tcp_lro_flush(), the
tail modified in there, and an mbuf head returned... that way it would
work something like this:
The caller would be something like this:
m_head = m_tail = NULL;
LIST_FOREACH(le, bucket, hash_next) {
head = tcp_lro_flush(lc, le, _tail);
if (m_head == NULL)
m_head = head;
}
if (m_head)
if_input(m_head);
And tcp_lro_flush() would be something like this:
struct mbuf *tcp_lro_flush(struct lro_ctrl *lc, struct lro_entry *le,
struct mbuf **tail)
{
...
if (*tail)
*tail->m_next = le->m_head;
*tail = le->m_tail;
...
return le->m_head;
}
Hrm, maybe it wouldn't be all that difficult after all. :-)
I'll be driving across the country later this week, so I don't want to
start poking into LRO then disappear, so if nobody else tries it out
before then, I should take a look in a couple weeks.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On 09/24/17 01:46, Stephen Hurd wrote:
Bjoern A. Zeeb wrote:
On 23 Sep 2017, at 6:32, Stephen Hurd wrote:
Bjoern A. Zeeb wrote:
On 23 Sep 2017, at 1:35, Stephen Hurd wrote:
Author: shurd
Date: Sat Sep 23 01:35:14 2017
New Revision: 323942
URL: https://svnweb.freebsd.org/changeset/base/323942
Log:
Chain mbufs before passing to if_input()
Build a list of mbufs to pass to if_input() after LRO. Results in
12% small packet forwarding rate improvement.
forwarding seems a confusing word here..
The test was small (64 byte frames) received on one interface, then
sent out on a different one using the net.inet.ip.forwarding sysctl
(controlled via the gateway_enable setting in rc.conf).
Then this makes no sense as we don’t do LRO if forwarding is enabled
on the machine;
https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?annotate=317390#l645
Basically, it changed from this:
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if_input(mbuf)
}
To this:
prev_mbuf = first_mbuf = NULL;
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if (prev_mbuf) {
prev_mbuf->m_nextpkt = mbuf;
prev_mbuf = mbuf;
}
else {
first_mbuf = prev_mbuf = mbuf;
}
}
if (first_mbuf)
if_input(first_mbuf);
So while before it called if_input() for each separate mbuf that was not
LROed, it now builds a chain of mbufs that were not LROed, and makes a
single call to if_input() with the whole chain. For cases like packet
forwarding where no packets are LROed, performance is better.
Can such a similar logic be applied inside TCP LRO aswell?
--HPS
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
Bjoern A. Zeeb wrote:
On 23 Sep 2017, at 6:32, Stephen Hurd wrote:
Bjoern A. Zeeb wrote:
On 23 Sep 2017, at 1:35, Stephen Hurd wrote:
Author: shurd
Date: Sat Sep 23 01:35:14 2017
New Revision: 323942
URL: https://svnweb.freebsd.org/changeset/base/323942
Log:
Chain mbufs before passing to if_input()
Build a list of mbufs to pass to if_input() after LRO. Results in
12% small packet forwarding rate improvement.
forwarding seems a confusing word here..
The test was small (64 byte frames) received on one interface, then
sent out on a different one using the net.inet.ip.forwarding sysctl
(controlled via the gateway_enable setting in rc.conf).
Then this makes no sense as we don’t do LRO if forwarding is enabled
on the machine;
https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?annotate=317390#l645
Basically, it changed from this:
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if_input(mbuf)
}
To this:
prev_mbuf = first_mbuf = NULL;
foreach (mbuf in rx) {
if (lro && tcp_lro_rx(mbuf) == 0)
continue;
if (prev_mbuf) {
prev_mbuf->m_nextpkt = mbuf;
prev_mbuf = mbuf;
}
else {
first_mbuf = prev_mbuf = mbuf;
}
}
if (first_mbuf)
if_input(first_mbuf);
So while before it called if_input() for each separate mbuf that was not
LROed, it now builds a chain of mbufs that were not LROed, and makes a
single call to if_input() with the whole chain. For cases like packet
forwarding where no packets are LROed, performance is better.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On Sat, Sep 23, 2017 at 4:37 AM, Bjoern A. Zeeb < bzeeb-li...@lists.zabbadoz.net> wrote: > > Then this makes no sense as we don’t do LRO if forwarding is enabled on > the machine; > https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c? > annotate=317390#l645 Yes, that is true. However, this change still makes a difference. Previously, if LRO was not enabled or the packet was not eligible for LRO, the iflib code would call ifp->if_input() once for each packet. Now, the iflib code will build a chain of packets for which it couldn't do LRO and call ifp->if_input() once for the entire chain. (I agree that was not obvious from the rather short commit message and the diff in the email. The lack of comments or meaningful variable names did not help to alleviate the confusion. Nonetheless, when I looked at the diff with enough surrounding context, it became clear.) Jonathan ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On 23 Sep 2017, at 6:32, Stephen Hurd wrote: Bjoern A. Zeeb wrote: On 23 Sep 2017, at 1:35, Stephen Hurd wrote: Author: shurd Date: Sat Sep 23 01:35:14 2017 New Revision: 323942 URL: https://svnweb.freebsd.org/changeset/base/323942 Log: Chain mbufs before passing to if_input() Build a list of mbufs to pass to if_input() after LRO. Results in 12% small packet forwarding rate improvement. forwarding seems a confusing word here.. The test was small (64 byte frames) received on one interface, then sent out on a different one using the net.inet.ip.forwarding sysctl (controlled via the gateway_enable setting in rc.conf). Then this makes no sense as we don’t do LRO if forwarding is enabled on the machine; https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?annotate=317390#l645 ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
Bjoern A. Zeeb wrote: On 23 Sep 2017, at 1:35, Stephen Hurd wrote: Author: shurd Date: Sat Sep 23 01:35:14 2017 New Revision: 323942 URL: https://svnweb.freebsd.org/changeset/base/323942 Log: Chain mbufs before passing to if_input() Build a list of mbufs to pass to if_input() after LRO. Results in 12% small packet forwarding rate improvement. forwarding seems a confusing word here.. The test was small (64 byte frames) received on one interface, then sent out on a different one using the net.inet.ip.forwarding sysctl (controlled via the gateway_enable setting in rc.conf). ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r323942 - head/sys/net
On 23 Sep 2017, at 1:35, Stephen Hurd wrote: > Author: shurd > Date: Sat Sep 23 01:35:14 2017 > New Revision: 323942 > URL: https://svnweb.freebsd.org/changeset/base/323942 > > Log: > Chain mbufs before passing to if_input() > > Build a list of mbufs to pass to if_input() after LRO. Results in > 12% small packet forwarding rate improvement. forwarding seems a confusing word here.. > Reviewed by:sbruno > Approved by:sbruno (mentor) > Sponsored by: Limelight Networks > Differential Revision: https://reviews.freebsd.org/D12444 > > Modified: > head/sys/net/iflib.c ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r323942 - head/sys/net
Author: shurd
Date: Sat Sep 23 01:35:14 2017
New Revision: 323942
URL: https://svnweb.freebsd.org/changeset/base/323942
Log:
Chain mbufs before passing to if_input()
Build a list of mbufs to pass to if_input() after LRO. Results in
12% small packet forwarding rate improvement.
Reviewed by: sbruno
Approved by: sbruno (mentor)
Sponsored by: Limelight Networks
Differential Revision:https://reviews.freebsd.org/D12444
Modified:
head/sys/net/iflib.c
Modified: head/sys/net/iflib.c
==
--- head/sys/net/iflib.cSat Sep 23 01:33:20 2017(r323941)
+++ head/sys/net/iflib.cSat Sep 23 01:35:14 2017(r323942)
@@ -2470,7 +2470,7 @@ iflib_rxeof(iflib_rxq_t rxq, qidx_t budget)
* XXX early demux data packets so that if_input processing only handles
* acks in interrupt context
*/
- struct mbuf *m, *mh, *mt;
+ struct mbuf *m, *mh, *mt, *mf;
ifp = ctx->ifc_ifp;
mh = mt = NULL;
@@ -2541,8 +2541,11 @@ iflib_rxeof(iflib_rxq_t rxq, qidx_t budget)
__iflib_fl_refill_lt(ctx, fl, budget + 8);
lro_enabled = (if_getcapenable(ifp) & IFCAP_LRO);
+ mt = mf = NULL;
while (mh != NULL) {
m = mh;
+ if (mf == NULL)
+ mf = m;
mh = mh->m_nextpkt;
m->m_nextpkt = NULL;
#ifndef __NO_STRICT_ALIGNMENT
@@ -2552,11 +2555,19 @@ iflib_rxeof(iflib_rxq_t rxq, qidx_t budget)
rx_bytes += m->m_pkthdr.len;
rx_pkts++;
#if defined(INET6) || defined(INET)
- if (lro_enabled && tcp_lro_rx(>ifr_lc, m, 0) == 0)
+ if (lro_enabled && tcp_lro_rx(>ifr_lc, m, 0) == 0) {
+ if (mf == m)
+ mf = NULL;
continue;
+ }
#endif
+ if (mt != NULL)
+ mt->m_nextpkt = m;
+ mt = m;
+ }
+ if (mf != NULL) {
+ ifp->if_input(ifp, mf);
DBG_COUNTER_INC(rx_if_input);
- ifp->if_input(ifp, m);
}
if_inc_counter(ifp, IFCOUNTER_IBYTES, rx_bytes);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
