Re: [PATCH v5 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 20 Nov 2019 10:42:47 -0800
"Jonathan Lemon" <jonathan.lemon@gmail.com> wrote:

> On 20 Nov 2019, at 9:49, Jesper Dangaard Brouer wrote:
> 
> > On Wed, 20 Nov 2019 16:54:18 +0200
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >  
> >> Introduce the following parameters in order to add the possibility to 
> >> sync
> >> DMA memory for device before putting allocated pages in the page_pool
> >> caches:
> >> - PP_FLAG_DMA_SYNC_DEV: if set in page_pool_params flags, all pages 
> >> that
> >>   the driver gets from page_pool will be DMA-synced-for-device 
> >> according
> >>   to the length provided by the device driver. Please note 
> >> DMA-sync-for-CPU
> >>   is still device driver responsibility
> >> - offset: DMA address offset where the DMA engine starts copying rx 
> >> data
> >> - max_len: maximum DMA memory size page_pool is allowed to flush. 
> >> This
> >>   is currently used in __page_pool_alloc_pages_slow routine when 
> >> pages
> >>   are allocated from page allocator
> >> These parameters are supposed to be set by device drivers.
> >>
> >> This optimization reduces the length of the DMA-sync-for-device.
> >> The optimization is valid because pages are initially
> >> DMA-synced-for-device as defined via max_len. At RX time, the driver
> >> will perform a DMA-sync-for-CPU on the memory for the packet length.
> >> What is important is the memory occupied by packet payload, because
> >> this is the area CPU is allowed to read and modify. As we don't track
> >> cache-lines written into by the CPU, simply use the packet payload 
> >> length
> >> as dma_sync_size at page_pool recycle time. This also take into 
> >> account
> >> any tail-extend.
> >>
> >> Tested-by: Matteo Croce <mcroce@redhat.com>
> >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> >> ---  
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> >
> > [...]  
> >> @@ -281,8 +309,8 @@ static bool __page_pool_recycle_direct(struct 
> >> page *page,
> >>  	return true;
> >>  }
> >>
> >> -void __page_pool_put_page(struct page_pool *pool,
> >> -			  struct page *page, bool allow_direct)
> >> +void __page_pool_put_page(struct page_pool *pool, struct page *page,
> >> +			  unsigned int dma_sync_size, bool allow_direct)
> >>  {
> >>  	/* This allocator is optimized for the XDP mode that uses
> >>  	 * one-frame-per-page, but have fallbacks that act like the
> >> @@ -293,6 +321,10 @@ void __page_pool_put_page(struct page_pool 
> >> *pool,
> >>  	if (likely(page_ref_count(page) == 1)) {
> >>  		/* Read barrier done in page_ref_count / READ_ONCE */
> >>
> >> +		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> >> +			page_pool_dma_sync_for_device(pool, page,
> >> +						      dma_sync_size);
> >> +
> >>  		if (allow_direct && in_serving_softirq())
> >>  			if (__page_pool_recycle_direct(page, pool))
> >>  				return;  
> >
> > I am slightly concerned this touch the fast-path code. But at-least on
> > Intel, I don't think this is measurable.  And for the ARM64 board it
> > was a huge win... thus I'll accept this.  
> 
> For the next series:
> 
> The "in_serving_softirq()" check shows up on profiling.  I'd
> like to remove this and just have a "direct" flag, where the
> caller takes the responsibility of the correct context.

As far as I can remember, this was added due to a bug in mlx5 shutdown
path... that needs to be fixed first.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer