Re: [PATCH net-next 1/4] net: allow alloc_skb_with_frags() to allocate bigger packets

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Eric Dumazet wrote:
> On Tue, Aug 1, 2023 at 5:44 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
> >
> > Eric Dumazet wrote:
> > > Refactor alloc_skb_with_frags() to allow bigger packets allocations.
> > >
> > > Instead of assuming that only order-0 allocations will be attempted,
> > > use the caller supplied max order.
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Cc: Tahsin Erdogan <trdgn@amazon.com>
> > > ---
> > >  net/core/skbuff.c | 56 +++++++++++++++++++++--------------------------
> > >  1 file changed, 25 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > > index a298992060e6efdecb87c7ffc8290eafe330583f..0ac70a0144a7c1f4e7824ddc19980aee73e4c121 100644
> > > --- a/net/core/skbuff.c
> > > +++ b/net/core/skbuff.c
> > > @@ -6204,7 +6204,7 @@ EXPORT_SYMBOL_GPL(skb_mpls_dec_ttl);
> > >   *
> > >   * @header_len: size of linear part
> > >   * @data_len: needed length in frags
> > > - * @max_page_order: max page order desired.
> > > + * @order: max page order desired.
> > >   * @errcode: pointer to error code if any
> > >   * @gfp_mask: allocation mask
> > >   *
> > > @@ -6212,21 +6212,17 @@ EXPORT_SYMBOL_GPL(skb_mpls_dec_ttl);
> > >   */
> > >  struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
> > >                                    unsigned long data_len,
> > > -                                  int max_page_order,
> > > +                                  int order,
> > >                                    int *errcode,
> > >                                    gfp_t gfp_mask)
> > >  {
> > > -     int npages = (data_len + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> > >       unsigned long chunk;
> > >       struct sk_buff *skb;
> > >       struct page *page;
> > > -     int i;
> > > +     int nr_frags = 0;
> > >
> > >       *errcode = -EMSGSIZE;
> > > -     /* Note this test could be relaxed, if we succeed to allocate
> > > -      * high order pages...
> > > -      */
> > > -     if (npages > MAX_SKB_FRAGS)
> > > +     if (unlikely(data_len > MAX_SKB_FRAGS * (PAGE_SIZE << order)))
> > >               return NULL;
> > >
> > >       *errcode = -ENOBUFS;
> > > @@ -6234,34 +6230,32 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
> > >       if (!skb)
> > >               return NULL;
> > >
> > > -     skb->truesize += npages << PAGE_SHIFT;
> > > -
> > > -     for (i = 0; npages > 0; i++) {
> > > -             int order = max_page_order;
> > > -
> > > -             while (order) {
> > > -                     if (npages >= 1 << order) {
> > > -                             page = alloc_pages((gfp_mask & ~__GFP_DIRECT_RECLAIM) |
> > > -                                                __GFP_COMP |
> > > -                                                __GFP_NOWARN,
> > > -                                                order);
> > > -                             if (page)
> > > -                                     goto fill_page;
> > > -                             /* Do not retry other high order allocations */
> >
> > Is this heuristic to only try one type of compound pages and else
> > fall back onto regular pages still relevant? I don't know the story
> > behind it.
> 
> I keep doing high-order attempts without direct reclaim,
> they should be fine and we eventually fallback to order-2 pages
> if we have plenty of them.
> 
> Immediate fallback to order-0 seems pessimistic.
> 
> >
> > > -                             order = 1;
> > > -                             max_page_order = 0;
> > > -                     }
> > > +     while (data_len) {
> > > +             if (nr_frags == MAX_SKB_FRAGS - 1)
> > > +                     goto failure;
> > > +             while (order && data_len < (PAGE_SIZE << order))
> > >                       order--;
> >
> > Why decrement order on every iteration through the loop, not just when
> > alloc_pages fails?
> 
> Say we enter the function with initial @data_len == 4000, and @order==3
> 
> We do not want to allocate/waste an order-3 page (32768 bytes on x86)
> while an order-0 one should be good enough to fit the expected
> payload.
> 
> Same story if initial data_len = 33000:
> - We should allocate one order-3 page, and one order-0 one, instead of
> two order-3 pages.

Thanks for the explanation. For @data_len == 5000, you would want to
allocate an order-1?