From: Alexander Lobakin <hidden> Date: 2019-11-12 12:30:09
Commit 78d3fd0b7de8 ("gro: Only use skb_gro_header for completely
non-linear packets") back in May'09 (2.6.31-rc1) has changed the
original condition '!skb_headlen(skb)' to the current
'skb_mac_header(skb) == skb_tail_pointer(skb)' in gro_reset_offset()
saying: "Since the drivers that need this optimisation all provide
completely non-linear packets".
For now, we have the following rough statistics for 5.4-rc7:
1) napi_gro_frags: 14
2) napi_gro_receive with skb->head containing (most of) payload: 83
3) napi_gro_receive with skb->head containing all the headers: 20
4) napi_gro_receive with skb->head containing only Ethernet header: 2
With the current condition, fast GRO with the usage of
NAPI_GRO_CB(skb)->frag0 is available only in the [1] case.
Packets pushed by [2] and [3] go through the 'slow' path, but
it's not a problem for them as they already contains all the needed
headers in skb->head, so pskb_may_pull() only moves skb->data.
The layout of skbs in the fourth [4] case at the moment of
dev_gro_receive() is identical to skbs that have come through [1],
as napi_frags_skb() pulls Ethernet header to skb->head. The only
difference is that the mentioned condition is always false for them,
because skb_put() and friends irreversibly alter the tail pointer.
They also go though the 'slow' path, but now every single
pskb_may_pull() in every single .gro_receive() will call the *really*
slow __pskb_pull_tail() to pull headers to head. This significantly
decreases the overall performance for no visible reasons.
The only two users of method [4] is:
* drivers/staging/qlge
* drivers/net/wireless/iwlwifi (all three variants: dvm, mvm, mvm-mq)
Note that in case with wireless drivers we can't use [1]
(napi_gro_frags()) at least for now and mac80211 stack always
performs pushes and pulls anyways, so performance hit is inavoidable.
We can simply change the condition in gro_reset_offset() to allow
skbs from [4] go through 'fast' path just like in case [1].
This was tested on a custom driver and this patch gave boosts up to
40 Mbps to method [4] in both directions comparing to net-next, which
made overall performance relatively close to [1] (without it, [4] is
the slowest).
Signed-off-by: Alexander Lobakin <redacted>
---
net/core/dev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: David Miller <davem@davemloft.net> Date: 2019-11-15 01:25:12
From: Alexander Lobakin <redacted>
Date: Tue, 12 Nov 2019 15:28:43 +0300
Commit 78d3fd0b7de8 ("gro: Only use skb_gro_header for completely
non-linear packets") back in May'09 (2.6.31-rc1) has changed the
original condition '!skb_headlen(skb)' to the current
'skb_mac_header(skb) == skb_tail_pointer(skb)' in gro_reset_offset()
saying: "Since the drivers that need this optimisation all provide
completely non-linear packets".
Please reference the appropriate SHA1-ID both here in this paragraph and
also in an appropriate Fixes: tag.
If this goes so far back that it is before GIT, then you need to provide
a reference to the patch posting via lore.kernel.org or similar because
it is absolutely essentialy for people reviewing this patch to be able
to do some digging into why the condition is code the way that it is
currently.
Thank you.
From: Alexander Lobakin <hidden> Date: 2019-11-15 07:36:26
Hi Dave,
David Miller wrote 15.11.2019 04:25:
From: Alexander Lobakin <redacted>
Date: Tue, 12 Nov 2019 15:28:43 +0300
quoted
Commit 78d3fd0b7de8 ("gro: Only use skb_gro_header for completely
non-linear packets") back in May'09 (2.6.31-rc1) has changed the
original condition '!skb_headlen(skb)' to the current
'skb_mac_header(skb) == skb_tail_pointer(skb)' in gro_reset_offset()
saying: "Since the drivers that need this optimisation all provide
completely non-linear packets".
Please reference the appropriate SHA1-ID both here in this paragraph
and
also in an appropriate Fixes: tag.
Sorry for confusing. The SHA1-ID from commit message is correct
actually. At the moment of 2.6.31 we used skb->mac_header and skb->tail
pointers directly, so the original condition was
'skb->mac_header == skb->tail'.
Commit ced14f6804a9 ("net: Correct comparisons and calculations using
skb->tail and skb-transport_header") has changed this condition to
the referred 'skb_mac_header(skb) == skb_tail_pointer(skb)' without
any functional changes.
I didn't add the "Fixes:" tag because at the moment of 2.6.31 it was
a needed change, but it became obsolete later, so now we can revert
it back to speed up skbs with only Ethernet header in head.
Please let me know if I must send v2 of this patch with corrected
description before getting any further reviews.
Thanks.
If this goes so far back that it is before GIT, then you need to
provide
a reference to the patch posting via lore.kernel.org or similar because
it is absolutely essentialy for people reviewing this patch to be able
to do some digging into why the condition is code the way that it is
currently.
Thank you.