On Tue, Feb 02, 2021 at 08:26:19AM -0800, Alexander Duyck wrote: > On Sun, Jan 31, 2021 at 12:17 AM Kevin Hao <[email protected]> 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 <[email protected]> > > --- > > 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
