Re: [PATCH net] Revert "virtio_net: replace netdev_alloc_skb_ip_align() with napi_alloc_skb()"

[Eric Dumazet <edumazet@google.com>]

On Wed, Jan 13, 2021 at 4:44 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2021-01-12 at 21:12 -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > This reverts commit c67f5db82027ba6d2ea4ac9176bc45996a03ae6a.
> >
> > 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
> > memory use under estimation.
> >
> > GOOD_COPY_LEN is 128 bytes. This means that we need a small amount
> > of memory to hold the headers and struct skb_shared_info
> >
> > Yet, napi_alloc_skb() might use a whole 32KB page (or 64KB on PowerPc)
> > for long lived incoming TCP packets.
> >
> > 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
> >
> > Using alloc_skb() and thus standard kmallloc() for skb->head allocations
> > will get the benefit of letting other objects in each page being independently
> > used by other skbs, regardless of the lifetime.
> >
> > Note that a similar problem exists for skbs allocated from napi_get_frags(),
> > this is handled in a separate patch.
> >
> > I would like to thank Greg Thelen for his precious help on this matter,
> > analysing crash dumps is always a time consuming task.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Greg Thelen <gthelen@google.com>
> > ---
> >  drivers/net/virtio_net.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 508408fbe78fbd8658dc226834b5b1b334b8b011..5886504c1acacf3f6148127b5c1cc7f6a906b827 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -386,7 +386,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> >       p = page_address(page) + offset;
> >
> >       /* copy small packet so we can reuse these pages for small data */
> > -     skb = napi_alloc_skb(&rq->napi, GOOD_COPY_LEN);
> > +     skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> >       if (unlikely(!skb))
> >               return NULL;
>
> I'm ok with the revert. The gain given by the original change in my
> tests was measurable, but small.
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>

To be clear, I now think the revert is not needed.

I will post instead a patch that should take care of the problem both
for virtio and napi_get_frags() tiny skbs

Something like :

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);