Only chain non-LRO mbufs when LRO is not possible
ClosedPublic

Authored by shurd on Oct 31 2017, 5:35 PM.

Details

Summary

Preserve packet order between tcp_lro_rx() and if_input() to avoid
creating extra corner cases. If no packets can be LROed, combine them
into one chain for submission via if_input(). If any packet can
potentially be LROed however, retain old behaviour and call if_input()
for each packet.

This should keep the 12% improvement for small packet forwarding intact,
but mostly avoids impacting the LRO case.

Test Plan

Ensure small packet forwarding case is still improved, check
that LRO performance isn't degraded.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 12406
Build 12682: arc lint + arc unit
shurd created this revision.Oct 31 2017, 5:35 PM
shurd edited the summary of this revision. (Show Details)Oct 31 2017, 5:38 PM
sbruno accepted this revision.Oct 31 2017, 5:39 PM

Found while testing the iflib'd version of ixgbe(4).

This revision is now accepted and ready to land.Oct 31 2017, 5:39 PM
cem accepted this revision.Oct 31 2017, 5:45 PM

If no packets can be LROed, combine them into one change

Probably "chain"

sys/net/iflib.c
2476

It seems like a more accurate name might be check_ipforwarding_disabled. No?

2615–2616

Could combine these to drop a level of indentation

shurd updated this revision to Diff 34552.Oct 31 2017, 6:07 PM
shurd marked an inline comment as done.

Incorporate feedback.

This revision now requires review to proceed.Oct 31 2017, 6:07 PM
shurd added inline comments.Oct 31 2017, 6:08 PM
sys/net/iflib.c
2476

That's what it currently does, but the intent is to indicate if it's possible the packet could be LROed. I'm considering copying the TCP header checks into the function as well, but can't make a strong case for them at this point.

Added a comment to clarify.

cem added inline comments.Oct 31 2017, 6:11 PM
sys/net/iflib.c
2613

nit: parens aren't needed for this line

2614–2615

I was suggesting to combine the two lines above, not move the assignment into an if(). Like:

if (lro_possible && mf != NULL) {
    ifp->if_input(ifp, mf);
    ...
}
shurd updated this revision to Diff 34558.Oct 31 2017, 6:57 PM

Much better way of dropping a level of indentation. :-)

shurd marked an inline comment as done.Oct 31 2017, 6:57 PM
shurd marked an inline comment as done.
cem accepted this revision.Oct 31 2017, 7:06 PM
This revision is now accepted and ready to land.Oct 31 2017, 7:06 PM
shurd updated this revision to Diff 34689.Nov 2 2017, 6:33 PM

Fix typo

This revision now requires review to proceed.Nov 2 2017, 6:33 PM
sbruno accepted this revision.Nov 6 2017, 3:53 PM
This revision is now accepted and ready to land.Nov 6 2017, 3:53 PM
shurd closed this revision.Nov 6 2017, 9:30 PM

committed as r325487