On Tue, Feb 02, 2021 at 08:26:19AM -0800, Alexander Duyck wrote:
> On Sun, Jan 31, 2021 at 12:17 AM Kevin Hao <haoke...@gmail.com> wrote:
> >
> > In the current implementation of {netdev,napi}_alloc_frag(), it doesn't
> > have any align guarantee for the returned buffer address, But for some
> > hardwares they do require the DMA buffer to be aligned correctly,
> > so we would have to use some workarounds like below if the buffers
> > allocated by the {netdev,napi}_alloc_frag() are used by these hardwares
> > for DMA.
> > buf = napi_alloc_frag(really_needed_size + align);
> > buf = PTR_ALIGN(buf, align);
> >
> > These codes seems ugly and would waste a lot of memories if the buffers
> > are used in a network driver for the TX/RX. We have added the align
> > support for the page_frag functions, so add the corresponding
> > {netdev,napi}_frag functions.
> >
> > Signed-off-by: Kevin Hao <haoke...@gmail.com>
> > ---
> > v2: Inline {netdev,napi}_alloc_frag().
> >
> > include/linux/skbuff.h | 22 ++++++++++++++++++++--
> > net/core/skbuff.c | 25 +++++++++----------------
> > 2 files changed, 29 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 9313b5aaf45b..7e8beff4ff22 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -2818,7 +2818,19 @@ void skb_queue_purge(struct sk_buff_head *list);
> >
> > unsigned int skb_rbtree_purge(struct rb_root *root);
> >
> > -void *netdev_alloc_frag(unsigned int fragsz);
> > +void *netdev_alloc_frag_align(unsigned int fragsz, int align);
> > +
> > +/**
> > + * netdev_alloc_frag - allocate a page fragment
> > + * @fragsz: fragment size
> > + *
> > + * Allocates a frag from a page for receive buffer.
> > + * Uses GFP_ATOMIC allocations.
> > + */
> > +static inline void *netdev_alloc_frag(unsigned int fragsz)
> > +{
> > + return netdev_alloc_frag_align(fragsz, 0);
> > +}
> >
>
> So one thing we may want to do is actually split this up so that we
> have a __netdev_alloc_frag_align function that is called by one of two
> inline functions. The standard netdev_alloc_frag would be like what
> you have here, however we would be passing ~0 for the mask.
>
> The "align" version would be taking in an unsigned int align value and
> converting it to a mask. The idea is that your mask value is likely a
> constant so converting the constant to a mask would be much easier to
> do in an inline function as the compiler can take care of converting
> the value during compile time.
>
> An added value to that is you could also add tests to the align value
> to guarantee that the value being passed is a power of 2 so that it
> works with the alignment mask generation as expected.
Fair enough. Thanks Alexander.
Kevin
>
> > struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int
> > length,
> > gfp_t gfp_mask);
> > @@ -2877,7 +2889,13 @@ static inline void skb_free_frag(void *addr)
> > page_frag_free(addr);
> > }
> >
> > -void *napi_alloc_frag(unsigned int fragsz);
> > +void *napi_alloc_frag_align(unsigned int fragsz, int align);
> > +
> > +static inline void *napi_alloc_frag(unsigned int fragsz)
> > +{
> > + return napi_alloc_frag_align(fragsz, 0);
> > +}
> > +
> > struct sk_buff *__napi_alloc_skb(struct napi_struct *napi,
> > unsigned int length, gfp_t gfp_mask);
> > static inline struct sk_buff *napi_alloc_skb(struct napi_struct *napi,
>
> Same for the __napi_alloc_frag code. You could probably convert the
> __napi_alloc_frag below into an __napi_alloc_frag_align that you pass
> a mask to. Then you could convert the other two functions to either
> pass ~0 or the align value and add align value validation.
>
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 2af12f7e170c..a35e75f12428 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -374,29 +374,22 @@ struct napi_alloc_cache {
> > static DEFINE_PER_CPU(struct page_frag_cache, netdev_alloc_cache);
> > static DEFINE_PER_CPU(struct napi_alloc_cache, napi_alloc_cache);
> >
> > -static void *__napi_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
> > +static void *__napi_alloc_frag(unsigned int fragsz, gfp_t gfp_mask, int
> > align)
> > {
> > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
> >
> > - return page_frag_alloc(&nc->page, fragsz, gfp_mask);
> > + return page_frag_alloc_align(&nc->page, fragsz, gfp_mask, align);
> > }
> >
> > -void *napi_alloc_frag(unsigned int fragsz)
> > +void *napi_alloc_frag_align(unsigned int fragsz, int align)
> > {
> > fragsz = SKB_DATA_ALIGN(fragsz);
> >
> > - return __napi_alloc_frag(fragsz, GFP_ATOMIC);
> > + return __napi_alloc_frag(fragsz, GFP_ATOMIC, align);
> > }
> > -EXPORT_SYMBOL(napi_alloc_frag);
> > +EXPORT_SYMBOL(napi_alloc_frag_align);
> >
> > -/**
> > - * netdev_alloc_frag - allocate a page fragment
> > - * @fragsz: fragment size
> > - *
> > - * Allocates a frag from a page for receive buffer.
> > - * Uses GFP_ATOMIC allocations.
> > - */
> > -void *netdev_alloc_frag(unsigned int fragsz)
> > +void *netdev_alloc_frag_align(unsigned int fragsz, int align)
> > {
> > struct page_frag_cache *nc;
> > void *data;
> > @@ -404,15 +397,15 @@ void *netdev_alloc_frag(unsigned int fragsz)
> > fragsz = SKB_DATA_ALIGN(fragsz);
> > if (in_irq() || irqs_disabled()) {
> > nc = this_cpu_ptr(&netdev_alloc_cache);
> > - data = page_frag_alloc(nc, fragsz, GFP_ATOMIC);
> > + data = page_frag_alloc_align(nc, fragsz, GFP_ATOMIC, align);
> > } else {
> > local_bh_disable();
> > - data = __napi_alloc_frag(fragsz, GFP_ATOMIC);
> > + data = __napi_alloc_frag(fragsz, GFP_ATOMIC, align);
> > local_bh_enable();
> > }
> > return data;
> > }
> > -EXPORT_SYMBOL(netdev_alloc_frag);
> > +EXPORT_SYMBOL(netdev_alloc_frag_align);
> >
> > /**
> > * __netdev_alloc_skb - allocate an skbuff for rx on a specific device
> > --
> > 2.29.2
> >
signature.asc
Description: PGP signature
