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 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.

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