From: Eric Dumazet <[email protected]>
[ Upstream commit 0f6925b3e8da0dbbb52447ca8a8b42b371aac7db ]
Xuan Zhuo reported that commit 3226b158e67c ("net: avoid 32 x truesize
under-estimation for tiny skbs") brought a ~10% performance drop.
The reason for the performance drop was that GRO was forced
to chain sk_buff (using skb_shinfo(skb)->frag_list), which
uses more memory but also cause packet consumers to go over
a lot of overhead handling all the tiny skbs.
It turns out that virtio_net page_to_skb() has a wrong strategy :
It allocates skbs with GOOD_COPY_LEN (128) bytes in skb->head, then
copies 128 bytes from the page, before feeding the packet to GRO stack.
This was suboptimal before commit 3226b158e67c ("net: avoid 32 x truesize
under-estimation for tiny skbs") because GRO was using 2 frags per MSS,
meaning we were not packing MSS with 100% efficiency.
Fix is to pull only the ethernet header in page_to_skb()
Then, we change virtio_net_hdr_to_skb() to pull the missing
headers, instead of assuming they were already pulled by callers.
This fixes the performance regression, but could also allow virtio_net
to accept packets with more than 128bytes of headers.
Many thanks to Xuan Zhuo for his report, and his tests/help.
Fixes: 3226b158e67c ("net: avoid 32 x truesize under-estimation for tiny skbs")
Reported-by: Xuan Zhuo <[email protected]>
Link: https://www.spinics.net/lists/netdev/msg731397.html
Co-Developed-by: Xuan Zhuo <[email protected]>
Signed-off-by: Xuan Zhuo <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Cc: [email protected]
Acked-by: Jason Wang <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
drivers/net/virtio_net.c | 10 +++++++---
include/linux/virtio_net.h | 14 +++++++++-----
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b67460864b3c..d8ee001d8e8e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -406,9 +406,13 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
offset += hdr_padded_len;
p += hdr_padded_len;
- copy = len;
- if (copy > skb_tailroom(skb))
- copy = skb_tailroom(skb);
+ /* Copy all frame if it fits skb->head, otherwise
+ * we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
+ */
+ if (len <= skb_tailroom(skb))
+ copy = len;
+ else
+ copy = ETH_HLEN + metasize;
skb_put_data(skb, p, copy);
if (metasize) {
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 98775d7fa696..b465f8f3e554 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -65,14 +65,18 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
skb_reset_mac_header(skb);
if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
- u16 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
- u16 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
+ u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
+ u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
+ u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
+
+ if (!pskb_may_pull(skb, needed))
+ return -EINVAL;
if (!skb_partial_csum_set(skb, start, off))
return -EINVAL;
p_off = skb_transport_offset(skb) + thlen;
- if (p_off > skb_headlen(skb))
+ if (!pskb_may_pull(skb, p_off))
return -EINVAL;
} else {
/* gso packets without NEEDS_CSUM do not set transport_offset.
@@ -102,14 +106,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
}
p_off = keys.control.thoff + thlen;
- if (p_off > skb_headlen(skb) ||
+ if (!pskb_may_pull(skb, p_off) ||
keys.basic.ip_proto != ip_proto)
return -EINVAL;
skb_set_transport_header(skb, keys.control.thoff);
} else if (gso_type) {
p_off = thlen;
- if (p_off > skb_headlen(skb))
+ if (!pskb_may_pull(skb, p_off))
return -EINVAL;
}
}
--
2.30.2
On Mon, Apr 12, 2021 at 10:40:20AM +0200, Greg Kroah-Hartman wrote:
> From: Eric Dumazet <[email protected]>
>
> [ Upstream commit 0f6925b3e8da0dbbb52447ca8a8b42b371aac7db ]
>
> Xuan Zhuo reported that commit 3226b158e67c ("net: avoid 32 x truesize
> under-estimation for tiny skbs") brought a ~10% performance drop.
>
> The reason for the performance drop was that GRO was forced
> to chain sk_buff (using skb_shinfo(skb)->frag_list), which
> uses more memory but also cause packet consumers to go over
> a lot of overhead handling all the tiny skbs.
>
> It turns out that virtio_net page_to_skb() has a wrong strategy :
> It allocates skbs with GOOD_COPY_LEN (128) bytes in skb->head, then
> copies 128 bytes from the page, before feeding the packet to GRO stack.
>
> This was suboptimal before commit 3226b158e67c ("net: avoid 32 x truesize
> under-estimation for tiny skbs") because GRO was using 2 frags per MSS,
> meaning we were not packing MSS with 100% efficiency.
>
> Fix is to pull only the ethernet header in page_to_skb()
>
> Then, we change virtio_net_hdr_to_skb() to pull the missing
> headers, instead of assuming they were already pulled by callers.
>
> This fixes the performance regression, but could also allow virtio_net
> to accept packets with more than 128bytes of headers.
>
> Many thanks to Xuan Zhuo for his report, and his tests/help.
>
> Fixes: 3226b158e67c ("net: avoid 32 x truesize under-estimation for tiny skbs")
> Reported-by: Xuan Zhuo <[email protected]>
> Link: https://www.spinics.net/lists/netdev/msg731397.html
> Co-Developed-by: Xuan Zhuo <[email protected]>
> Signed-off-by: Xuan Zhuo <[email protected]>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Cc: Jason Wang <[email protected]>
> Cc: [email protected]
> Acked-by: Jason Wang <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
Note that an issue related to this patch was recently reported.
It's quite possible that the root cause is a bug elsewhere
in the kernel, but it probably makes sense to defer the backport
until we know more ...
> ---
> drivers/net/virtio_net.c | 10 +++++++---
> include/linux/virtio_net.h | 14 +++++++++-----
> 2 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b67460864b3c..d8ee001d8e8e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -406,9 +406,13 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> offset += hdr_padded_len;
> p += hdr_padded_len;
>
> - copy = len;
> - if (copy > skb_tailroom(skb))
> - copy = skb_tailroom(skb);
> + /* Copy all frame if it fits skb->head, otherwise
> + * we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
> + */
> + if (len <= skb_tailroom(skb))
> + copy = len;
> + else
> + copy = ETH_HLEN + metasize;
> skb_put_data(skb, p, copy);
>
> if (metasize) {
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 98775d7fa696..b465f8f3e554 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -65,14 +65,18 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> skb_reset_mac_header(skb);
>
> if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> - u16 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
> - u16 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> + u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
> + u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> + u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
> +
> + if (!pskb_may_pull(skb, needed))
> + return -EINVAL;
>
> if (!skb_partial_csum_set(skb, start, off))
> return -EINVAL;
>
> p_off = skb_transport_offset(skb) + thlen;
> - if (p_off > skb_headlen(skb))
> + if (!pskb_may_pull(skb, p_off))
> return -EINVAL;
> } else {
> /* gso packets without NEEDS_CSUM do not set transport_offset.
> @@ -102,14 +106,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> }
>
> p_off = keys.control.thoff + thlen;
> - if (p_off > skb_headlen(skb) ||
> + if (!pskb_may_pull(skb, p_off) ||
> keys.basic.ip_proto != ip_proto)
> return -EINVAL;
>
> skb_set_transport_header(skb, keys.control.thoff);
> } else if (gso_type) {
> p_off = thlen;
> - if (p_off > skb_headlen(skb))
> + if (!pskb_may_pull(skb, p_off))
> return -EINVAL;
> }
> }
> --
> 2.30.2
>
>
On Mon, Apr 12, 2021 at 11:12 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Apr 12, 2021 at 10:40:20AM +0200, Greg Kroah-Hartman wrote:
> > From: Eric Dumazet <[email protected]>
> >
> > [ Upstream commit 0f6925b3e8da0dbbb52447ca8a8b42b371aac7db ]
> >
> > Xuan Zhuo reported that commit 3226b158e67c ("net: avoid 32 x truesize
> > under-estimation for tiny skbs") brought a ~10% performance drop.
> >
> > The reason for the performance drop was that GRO was forced
> > to chain sk_buff (using skb_shinfo(skb)->frag_list), which
> > uses more memory but also cause packet consumers to go over
> > a lot of overhead handling all the tiny skbs.
> >
> > It turns out that virtio_net page_to_skb() has a wrong strategy :
> > It allocates skbs with GOOD_COPY_LEN (128) bytes in skb->head, then
> > copies 128 bytes from the page, before feeding the packet to GRO stack.
> >
> > This was suboptimal before commit 3226b158e67c ("net: avoid 32 x truesize
> > under-estimation for tiny skbs") because GRO was using 2 frags per MSS,
> > meaning we were not packing MSS with 100% efficiency.
> >
> > Fix is to pull only the ethernet header in page_to_skb()
> >
> > Then, we change virtio_net_hdr_to_skb() to pull the missing
> > headers, instead of assuming they were already pulled by callers.
> >
> > This fixes the performance regression, but could also allow virtio_net
> > to accept packets with more than 128bytes of headers.
> >
> > Many thanks to Xuan Zhuo for his report, and his tests/help.
> >
> > Fixes: 3226b158e67c ("net: avoid 32 x truesize under-estimation for tiny skbs")
> > Reported-by: Xuan Zhuo <[email protected]>
> > Link: https://www.spinics.net/lists/netdev/msg731397.html
> > Co-Developed-by: Xuan Zhuo <[email protected]>
> > Signed-off-by: Xuan Zhuo <[email protected]>
> > Signed-off-by: Eric Dumazet <[email protected]>
> > Cc: "Michael S. Tsirkin" <[email protected]>
> > Cc: Jason Wang <[email protected]>
> > Cc: [email protected]
> > Acked-by: Jason Wang <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> > Signed-off-by: Sasha Levin <[email protected]>
>
>
> Note that an issue related to this patch was recently reported.
> It's quite possible that the root cause is a bug elsewhere
> in the kernel, but it probably makes sense to defer the backport
> until we know more ...
I think the patch should be backported now, all issues have been sorted out ?
>
>
> > ---
> > drivers/net/virtio_net.c | 10 +++++++---
> > include/linux/virtio_net.h | 14 +++++++++-----
> > 2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b67460864b3c..d8ee001d8e8e 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -406,9 +406,13 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> > offset += hdr_padded_len;
> > p += hdr_padded_len;
> >
> > - copy = len;
> > - if (copy > skb_tailroom(skb))
> > - copy = skb_tailroom(skb);
> > + /* Copy all frame if it fits skb->head, otherwise
> > + * we let virtio_net_hdr_to_skb() and GRO pull headers as needed.
> > + */
> > + if (len <= skb_tailroom(skb))
> > + copy = len;
> > + else
> > + copy = ETH_HLEN + metasize;
> > skb_put_data(skb, p, copy);
> >
> > if (metasize) {
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index 98775d7fa696..b465f8f3e554 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -65,14 +65,18 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > skb_reset_mac_header(skb);
> >
> > if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > - u16 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
> > - u16 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> > + u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
> > + u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
> > + u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
> > +
> > + if (!pskb_may_pull(skb, needed))
> > + return -EINVAL;
> >
> > if (!skb_partial_csum_set(skb, start, off))
> > return -EINVAL;
> >
> > p_off = skb_transport_offset(skb) + thlen;
> > - if (p_off > skb_headlen(skb))
> > + if (!pskb_may_pull(skb, p_off))
> > return -EINVAL;
> > } else {
> > /* gso packets without NEEDS_CSUM do not set transport_offset.
> > @@ -102,14 +106,14 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > }
> >
> > p_off = keys.control.thoff + thlen;
> > - if (p_off > skb_headlen(skb) ||
> > + if (!pskb_may_pull(skb, p_off) ||
> > keys.basic.ip_proto != ip_proto)
> > return -EINVAL;
> >
> > skb_set_transport_header(skb, keys.control.thoff);
> > } else if (gso_type) {
> > p_off = thlen;
> > - if (p_off > skb_headlen(skb))
> > + if (!pskb_may_pull(skb, p_off))
> > return -EINVAL;
> > }
> > }
> > --
> > 2.30.2
> >
> >
>
On Tue, May 18, 2021 at 09:35:26PM +0200, Eric Dumazet wrote:
>On Mon, Apr 12, 2021 at 11:12 AM Michael S. Tsirkin <[email protected]> wrote:
>> Note that an issue related to this patch was recently reported.
>> It's quite possible that the root cause is a bug elsewhere
>> in the kernel, but it probably makes sense to defer the backport
>> until we know more ...
>
>I think the patch should be backported now, all issues have been sorted out ?
This is just 38ec4944b593 ("gro: ensure frag0 meets IP header
alignment"), right? I can queue both for the next round of releases.
--
Thanks,
Sasha