Re: [PATCH net] net: avoid 32 x truesize under-estimation for tiny skbs

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Optimal GRO packets are when payload is in page fragments.

(I am speaking not only for raw performance, but ability for systems
to cope with network outages and sudden increase of memory usage in
out of order queues)

This has been quite clearly stated in my changelog.

Thanks.

int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
{
        struct skb_shared_info *pinfo, *skbinfo = skb_shinfo(skb);
        unsigned int offset = skb_gro_offset(skb);
        unsigned int headlen = skb_headlen(skb);

    .......

        if (headlen <= offset) {         // virtio-net will fail
        ........ // merge by frags
                goto done;
        } else if (skb->head_frag) {     // skb->head_frag is fail when use kmalloc() for skb->head allocation
        ........ // merge by frags
                goto done;
        }

merge:
    ......

        if (NAPI_GRO_CB(p)->last == p)
                skb_shinfo(p)->frag_list = skb;
        else
                NAPI_GRO_CB(p)->last->next = skb;

    ......
        return 0;
}


test cmd:
 for i in $(seq 1 4)
 do
    redis-benchmark -r 10000000 -n 10000000 -t set -d 1024 -c 8 -P 32 -h  <ip> -p 6379 2>&1 | grep 'per second'  &
 done

Reported-by: su-lifan@linux.alibaba.com

quoted

Fixes: fd11a83dd363 ("net: Pull out core bits of __netdev_alloc_skb and add __napi_alloc_skb")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexanderduyck@fb.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Greg Thelen <redacted>
---
 net/core/skbuff.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7626a33cce590e530f36167bd096026916131897..3a8f55a43e6964344df464a27b9b1faa0eb804f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -501,13 +501,17 @@ EXPORT_SYMBOL(__netdev_alloc_skb);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
                               gfp_t gfp_mask)
 {
-     struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
+     struct napi_alloc_cache *nc;
      struct sk_buff *skb;
      void *data;

      len += NET_SKB_PAD + NET_IP_ALIGN;

-     if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
+     /* If requested length is either too small or too big,
+      * we use kmalloc() for skb->head allocation.
+      */
+     if (len <= SKB_WITH_OVERHEAD(1024) ||
+         len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
          (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
              skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
              if (!skb)

@@ -515,6 +519,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
              goto skb_success;
      }

+     nc = this_cpu_ptr(&napi_alloc_cache);
      len += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
      len = SKB_DATA_ALIGN(len);

--

2.30.0.284.gd98b1dd5eaa7-goog

On Mon, Mar 29, 2021 at 11:06:09AM +0200, Eric Dumazet wrote:

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Hmm we do have hdr_len field, but seem to ignore it on RX.
Jason do you see any issues with using it for the head len?

Optimal GRO packets are when payload is in page fragments.

(I am speaking not only for raw performance, but ability for systems
to cope with network outages and sudden increase of memory usage in
out of order queues)

This has been quite clearly stated in my changelog.

Thanks.

quoted

int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
{
        struct skb_shared_info *pinfo, *skbinfo = skb_shinfo(skb);
        unsigned int offset = skb_gro_offset(skb);
        unsigned int headlen = skb_headlen(skb);

    .......

        if (headlen <= offset) {         // virtio-net will fail
        ........ // merge by frags
                goto done;
        } else if (skb->head_frag) {     // skb->head_frag is fail when use kmalloc() for skb->head allocation
        ........ // merge by frags
                goto done;
        }

merge:
    ......

        if (NAPI_GRO_CB(p)->last == p)
                skb_shinfo(p)->frag_list = skb;
        else
                NAPI_GRO_CB(p)->last->next = skb;

    ......
        return 0;
}


test cmd:
 for i in $(seq 1 4)
 do
    redis-benchmark -r 10000000 -n 10000000 -t set -d 1024 -c 8 -P 32 -h  <ip> -p 6379 2>&1 | grep 'per second'  &
 done

Reported-by: su-lifan@linux.alibaba.com

quoted

Fixes: fd11a83dd363 ("net: Pull out core bits of __netdev_alloc_skb and add __napi_alloc_skb")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexanderduyck@fb.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Greg Thelen <redacted>
---
 net/core/skbuff.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7626a33cce590e530f36167bd096026916131897..3a8f55a43e6964344df464a27b9b1faa0eb804f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -501,13 +501,17 @@ EXPORT_SYMBOL(__netdev_alloc_skb);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
                               gfp_t gfp_mask)
 {
-     struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
+     struct napi_alloc_cache *nc;
      struct sk_buff *skb;
      void *data;

      len += NET_SKB_PAD + NET_IP_ALIGN;

-     if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
+     /* If requested length is either too small or too big,
+      * we use kmalloc() for skb->head allocation.
+      */
+     if (len <= SKB_WITH_OVERHEAD(1024) ||
+         len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
          (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
              skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
              if (!skb)

@@ -515,6 +519,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
              goto skb_success;
      }

+     nc = this_cpu_ptr(&napi_alloc_cache);
      len += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
      len = SKB_DATA_ALIGN(len);

--

2.30.0.284.gd98b1dd5eaa7-goog

On Wed, Mar 31, 2021 at 10:11 AM Michael S. Tsirkin [off-list ref] wrote:

On Mon, Mar 29, 2021 at 11:06:09AM +0200, Eric Dumazet wrote:

quoted

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Hmm we do have hdr_len field, but seem to ignore it on RX.
Jason do you see any issues with using it for the head len?

I was looking at this code (page_to_skb())  a few minutes ago ;)

pulling payload would make sense only if can pull of of it (to free the page)
(This is what some drivers implement and call copybreak)

Even if we do not have an accurate knowledge of header sizes,
it would be better to pull only the Ethernet header and let GRO do the
rest during its dissection.

Once fixed, virtio_net will reduce by 2x number of frags per skb,
compared to the situation before "net: avoid 32 x truesize
under-estimation for tiny skbs"

quoted

Optimal GRO packets are when payload is in page fragments.

(I am speaking not only for raw performance, but ability for systems
to cope with network outages and sudden increase of memory usage in
out of order queues)

This has been quite clearly stated in my changelog.

Thanks.

quoted

int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
{
        struct skb_shared_info *pinfo, *skbinfo = skb_shinfo(skb);
        unsigned int offset = skb_gro_offset(skb);
        unsigned int headlen = skb_headlen(skb);

    .......

        if (headlen <= offset) {         // virtio-net will fail
        ........ // merge by frags
                goto done;
        } else if (skb->head_frag) {     // skb->head_frag is fail when use kmalloc() for skb->head allocation
        ........ // merge by frags
                goto done;
        }

merge:
    ......

        if (NAPI_GRO_CB(p)->last == p)
                skb_shinfo(p)->frag_list = skb;
        else
                NAPI_GRO_CB(p)->last->next = skb;

    ......
        return 0;
}


test cmd:
 for i in $(seq 1 4)
 do
    redis-benchmark -r 10000000 -n 10000000 -t set -d 1024 -c 8 -P 32 -h  <ip> -p 6379 2>&1 | grep 'per second'  &
 done

Reported-by: su-lifan@linux.alibaba.com

quoted

Fixes: fd11a83dd363 ("net: Pull out core bits of __netdev_alloc_skb and add __napi_alloc_skb")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexanderduyck@fb.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Greg Thelen <redacted>
---
 net/core/skbuff.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7626a33cce590e530f36167bd096026916131897..3a8f55a43e6964344df464a27b9b1faa0eb804f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -501,13 +501,17 @@ EXPORT_SYMBOL(__netdev_alloc_skb);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
                               gfp_t gfp_mask)
 {
-     struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
+     struct napi_alloc_cache *nc;
      struct sk_buff *skb;
      void *data;

      len += NET_SKB_PAD + NET_IP_ALIGN;

-     if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
+     /* If requested length is either too small or too big,
+      * we use kmalloc() for skb->head allocation.
+      */
+     if (len <= SKB_WITH_OVERHEAD(1024) ||
+         len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
          (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
              skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
              if (!skb)

@@ -515,6 +519,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
              goto skb_success;
      }

+     nc = this_cpu_ptr(&napi_alloc_cache);
      len += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
      len = SKB_DATA_ALIGN(len);

--

2.30.0.284.gd98b1dd5eaa7-goog

On Wed, Mar 31, 2021 at 10:36 AM Eric Dumazet [off-list ref] wrote:

On Wed, Mar 31, 2021 at 10:11 AM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Mar 29, 2021 at 11:06:09AM +0200, Eric Dumazet wrote:

quoted

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Hmm we do have hdr_len field, but seem to ignore it on RX.
Jason do you see any issues with using it for the head len?

I was looking at this code (page_to_skb())  a few minutes ago ;)

pulling payload would make sense only if can pull of of it (to free the page)
(This is what some drivers implement and call copybreak)

Even if we do not have an accurate knowledge of header sizes,
it would be better to pull only the Ethernet header and let GRO do the
rest during its dissection.

Once fixed, virtio_net will reduce by 2x number of frags per skb,
compared to the situation before "net: avoid 32 x truesize
under-estimation for tiny skbs"

Ie I suspect the simple way to fix this would be :

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bb4ea9dbc16bcb19c5969fc8247478aa66c63fce..a5500bf6ac01051be949edf9fead934a90335f4f

100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -409,9 +409,7 @@ 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 = min_t(int, len, ETH_HLEN);
        skb_put_data(skb, p, copy);

        if (metasize) {

On Wed, Mar 31, 2021 at 10:46 AM Eric Dumazet [off-list ref] wrote:

On Wed, Mar 31, 2021 at 10:36 AM Eric Dumazet [off-list ref] wrote:

quoted

A  'copybreak' aware version would be :

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bb4ea9dbc16bcb19c5969fc8247478aa66c63fce..dd58b075ca53643231bc1795c7283fcd8609547b

100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -409,9 +409,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 GRO pull headers as needed.
+        */
+       if (len <= skb_tailroom(skb))
+               copy = len;
+       else
+               copy = min_t(int, len, ETH_HLEN);
        skb_put_data(skb, p, copy);

        if (metasize) {

On Wed, Mar 31, 2021 at 10:49 AM Eric Dumazet [off-list ref] wrote:

Not that we might need to include 'metasize' in the picture.

maybe :

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bb4ea9dbc16bcb19c5969fc8247478aa66c63fce..f5a3cecd18eada32694714ecb85c205af7108aae

100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -409,9 +409,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 GRO pull headers as needed.
+        */
+       if (len <= skb_tailroom(skb))
+               copy = len;
+       else
+               copy = min_t(int, len, ETH_HLEN + metasize);
        skb_put_data(skb, p, copy);

        if (metasize) {

在 2021/3/31 下午4:11, Michael S. Tsirkin 写道:

On Mon, Mar 29, 2021 at 11:06:09AM +0200, Eric Dumazet wrote:

quoted

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Hmm we do have hdr_len field, but seem to ignore it on RX.
Jason do you see any issues with using it for the head len?

This might work only if the device sets a correct hdr_len. I'm not sure 
all of the devices can do this properly. E.g for tap, we use 
skb_headlen() in virtio_net_hdr_from_skb() which depends highly on the 
behaviour of the underlayer layers (device driver or GRO). And we only 
set this hint for GSO packet but virtio-net may tries to do GRO for non 
GSO packets.

Thanks

quoted

Optimal GRO packets are when payload is in page fragments.

(I am speaking not only for raw performance, but ability for systems
to cope with network outages and sudden increase of memory usage in
out of order queues)

This has been quite clearly stated in my changelog.

Thanks.

quoted

int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
{
         struct skb_shared_info *pinfo, *skbinfo = skb_shinfo(skb);
         unsigned int offset = skb_gro_offset(skb);
         unsigned int headlen = skb_headlen(skb);

     .......

         if (headlen <= offset) {         // virtio-net will fail
         ........ // merge by frags
                 goto done;
         } else if (skb->head_frag) {     // skb->head_frag is fail when use kmalloc() for skb->head allocation
         ........ // merge by frags
                 goto done;
         }

merge:
     ......

         if (NAPI_GRO_CB(p)->last == p)
                 skb_shinfo(p)->frag_list = skb;
         else
                 NAPI_GRO_CB(p)->last->next = skb;

     ......
         return 0;
}


test cmd:
  for i in $(seq 1 4)
  do
     redis-benchmark -r 10000000 -n 10000000 -t set -d 1024 -c 8 -P 32 -h  <ip> -p 6379 2>&1 | grep 'per second'  &
  done

Reported-by: su-lifan@linux.alibaba.com

quoted

Fixes: fd11a83dd363 ("net: Pull out core bits of __netdev_alloc_skb and add __napi_alloc_skb")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexanderduyck@fb.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Greg Thelen <redacted>
---
  net/core/skbuff.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7626a33cce590e530f36167bd096026916131897..3a8f55a43e6964344df464a27b9b1faa0eb804f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -501,13 +501,17 @@ EXPORT_SYMBOL(__netdev_alloc_skb);
  struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
                                gfp_t gfp_mask)
  {
-     struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
+     struct napi_alloc_cache *nc;
       struct sk_buff *skb;
       void *data;

       len += NET_SKB_PAD + NET_IP_ALIGN;

-     if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
+     /* If requested length is either too small or too big,
+      * we use kmalloc() for skb->head allocation.
+      */
+     if (len <= SKB_WITH_OVERHEAD(1024) ||
+         len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
           (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
               skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
               if (!skb)

@@ -515,6 +519,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
               goto skb_success;
       }

+     nc = this_cpu_ptr(&napi_alloc_cache);
       len += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
       len = SKB_DATA_ALIGN(len);

--

2.30.0.284.gd98b1dd5eaa7-goog

On Wed, Mar 31, 2021 at 10:36:35AM +0200, Eric Dumazet wrote:

On Wed, Mar 31, 2021 at 10:11 AM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Mon, Mar 29, 2021 at 11:06:09AM +0200, Eric Dumazet wrote:

quoted

On Mon, Mar 29, 2021 at 10:52 AM Xuan Zhuo [off-list ref] wrote:

quoted

On Wed, 13 Jan 2021 08:18:19 -0800, Eric Dumazet [off-list ref] wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

Both virtio net and napi_get_frags() allocate skbs
with a very small skb->head

While using page fragments instead of a kmalloc backed skb->head might give
a small performance improvement in some cases, there is a huge risk of
under estimating memory usage.

For both GOOD_COPY_LEN and GRO_MAX_HEAD, we can fit at least 32 allocations
per page (order-3 page in x86), or even 64 on PowerPC

We have been tracking OOM issues on GKE hosts hitting tcp_mem limits
but consuming far more memory for TCP buffers than instructed in tcp_mem[2]

Even if we force napi_alloc_skb() to only use order-0 pages, the issue
would still be there on arches with PAGE_SIZE >= 32768

This patch makes sure that small skb head are kmalloc backed, so that
other objects in the slab page can be reused instead of being held as long
as skbs are sitting in socket queues.

Note that we might in the future use the sk_buff napi cache,
instead of going through a more expensive __alloc_skb()

Another idea would be to use separate page sizes depending
on the allocated length (to never have more than 4 frags per page)

I would like to thank Greg Thelen for his precious help on this matter,
analysing crash dumps is always a time consuming task.

This patch causes a performance degradation of about 10% in the scenario of
virtio-net + GRO.

For GRO, there is no way to merge skbs based on frags with this patch, only
frag_list can be used to link skbs. The problem that this cause are that compared
to the GRO package merged into the frags way, the current skb needs to call
kfree_skb_list to release each skb, resulting in performance degradation.

virtio-net will store some data onto the linear space after receiving it. In
addition to the header, there are also some payloads, so "headlen <= offset"
fails. And skb->head_frag is failing when use kmalloc() for skb->head allocation.

Thanks for the report.

There is no way we can make things both fast for existing strategies
used by _insert_your_driver
and malicious usages of data that can sit for seconds/minutes in socket queues.

I think that if you want to gain this 10% back, you have to change
virtio_net to meet optimal behavior.

Normal drivers make sure to not pull payload in skb->head, only headers.

Hmm we do have hdr_len field, but seem to ignore it on RX.
Jason do you see any issues with using it for the head len?

I was looking at this code (page_to_skb())  a few minutes ago ;)

pulling payload would make sense only if can pull of of it (to free the page)
(This is what some drivers implement and call copybreak)

right.. I wonder whether it's preferable to always copy as much as fits,
or to use hdr_len if it's there?

Even if we do not have an accurate knowledge of header sizes,
it would be better to pull only the Ethernet header and let GRO do the
rest during its dissection.

So IIUC what you are saying is we should do more or less
     if (hdr_len != 0)
                        copy hdr_len
?

Once fixed, virtio_net will reduce by 2x number of frags per skb,
compared to the situation before "net: avoid 32 x truesize
under-estimation for tiny skbs"

quoted

quoted

Optimal GRO packets are when payload is in page fragments.

(I am speaking not only for raw performance, but ability for systems
to cope with network outages and sudden increase of memory usage in
out of order queues)

This has been quite clearly stated in my changelog.

Thanks.

quoted

int skb_gro_receive(struct sk_buff *p, struct sk_buff *skb)
{
        struct skb_shared_info *pinfo, *skbinfo = skb_shinfo(skb);
        unsigned int offset = skb_gro_offset(skb);
        unsigned int headlen = skb_headlen(skb);

    .......

        if (headlen <= offset) {         // virtio-net will fail
        ........ // merge by frags
                goto done;
        } else if (skb->head_frag) {     // skb->head_frag is fail when use kmalloc() for skb->head allocation
        ........ // merge by frags
                goto done;
        }

merge:
    ......

        if (NAPI_GRO_CB(p)->last == p)
                skb_shinfo(p)->frag_list = skb;
        else
                NAPI_GRO_CB(p)->last->next = skb;

    ......
        return 0;
}


test cmd:
 for i in $(seq 1 4)
 do
    redis-benchmark -r 10000000 -n 10000000 -t set -d 1024 -c 8 -P 32 -h  <ip> -p 6379 2>&1 | grep 'per second'  &
 done

Reported-by: su-lifan@linux.alibaba.com

quoted

Fixes: fd11a83dd363 ("net: Pull out core bits of __netdev_alloc_skb and add __napi_alloc_skb")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexanderduyck@fb.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Greg Thelen <redacted>
---
 net/core/skbuff.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7626a33cce590e530f36167bd096026916131897..3a8f55a43e6964344df464a27b9b1faa0eb804f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -501,13 +501,17 @@ EXPORT_SYMBOL(__netdev_alloc_skb);
 struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
                               gfp_t gfp_mask)
 {
-     struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
+     struct napi_alloc_cache *nc;
      struct sk_buff *skb;
      void *data;

      len += NET_SKB_PAD + NET_IP_ALIGN;

-     if ((len > SKB_WITH_OVERHEAD(PAGE_SIZE)) ||
+     /* If requested length is either too small or too big,
+      * we use kmalloc() for skb->head allocation.
+      */
+     if (len <= SKB_WITH_OVERHEAD(1024) ||
+         len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
          (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
              skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
              if (!skb)

@@ -515,6 +519,7 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
              goto skb_success;
      }

+     nc = this_cpu_ptr(&napi_alloc_cache);
      len += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
      len = SKB_DATA_ALIGN(len);

--

2.30.0.284.gd98b1dd5eaa7-goog

On Thu, Apr 1, 2021 at 3:51 PM Michael S. Tsirkin [off-list ref] wrote:

So IIUC what you are saying is we should do more or less
     if (hdr_len != 0)
                        copy hdr_len
?

This part is not pulling bytes into skb->head, but into
skb_vnet_hdr(skb) (which is basically skb->cb)

I suggest the following patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index bb4ea9dbc16bcb19c5969fc8247478aa66c63fce..f5a3cecd18eada32694714ecb85c205af7108aae

100644

--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c

@@ -409,9 +409,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 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 6b5fcfa1e5553576b0e853ae31a2df655c04204b..2ee8f3ba76a548d54e0b21321a67da958c9984a0

100644

--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h

@@ -63,8 +63,12 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *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;

@@ -100,14 +104,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;
                }
        }