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

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]

> On Mon, 18 Nov 2019 15:33:45 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 1121faa99c12..6f684c3a3434 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -34,8 +34,15 @@
> >  #include <linux/ptr_ring.h>
> >  #include <linux/dma-direction.h>
> >  
> > -#define PP_FLAG_DMA_MAP 1 /* Should page_pool do the DMA map/unmap */
> > -#define PP_FLAG_ALL	PP_FLAG_DMA_MAP
> > +#define PP_FLAG_DMA_MAP		1 /* Should page_pool do the DMA map/unmap */
> > +#define PP_FLAG_DMA_SYNC_DEV	2 /* if set 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
> > +				   */
> > +#define PP_FLAG_ALL		(PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV)
> >  
> [...]
> 
> Can you please change this to use the BIT(X) api.
> 
> #include <linux/bits.h>
> 
> #define PP_FLAG_DMA_MAP		BIT(0)
> #define PP_FLAG_DMA_SYNC_DEV	BIT(1)

Hi Jesper,

sure, will do in v5

> 
> 
> 
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index dfc2501c35d9..4f9aed7bce5a 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -47,6 +47,13 @@ static int page_pool_init(struct page_pool *pool,
> >  	    (pool->p.dma_dir != DMA_BIDIRECTIONAL))
> >  		return -EINVAL;
> >  
> > +	/* In order to request DMA-sync-for-device the page needs to
> > +	 * be mapped
> > +	 */
> > +	if ((pool->p.flags & PP_FLAG_DMA_SYNC_DEV) &&
> > +	    !(pool->p.flags & PP_FLAG_DMA_MAP))
> > +		return -EINVAL;
> > +
> 
> I like that you have moved this check to setup time.
> 
> There are two other parameters the DMA_SYNC_DEV depend on:
> 
>  	struct page_pool_params pp_params = {
>  		.order = 0,
> -		.flags = PP_FLAG_DMA_MAP,
> +		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
>  		.pool_size = size,
>  		.nid = cpu_to_node(0),
>  		.dev = pp->dev->dev.parent,
>  		.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE,
> +		.offset = pp->rx_offset_correction,
> +		.max_len = MVNETA_MAX_RX_BUF_SIZE,
>  	};
> 
> Can you add a check, that .max_len must not be zero.  The reason is
> that I can easily see people misconfiguring this.  And the effect is
> that the DMA-sync-for-device is essentially disabled, without user
> realizing this. The not-realizing part is really bad, especially
> because bugs that can occur from this are very rare and hard to catch.

I guess we need to check it just if we provide PP_FLAG_DMA_SYNC_DEV.
Something like:

	if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV) {
		if (!(pool->p.flags & PP_FLAG_DMA_MAP))
			return -EINVAL;

		if (!pool->p.max_len)
			return -EINVAL;
	}

> 
> I'm up for discussing if there should be a similar check for .offset.
> IMHO we should also check .offset is configured, and then be open to
> remove this check once a driver user want to use offset=0.  Does the
> mvneta driver already have a use-case for this (in non-XDP mode)?

With 'non-XDP mode' do you mean not loading a BPF program? If so yes, it used
in __page_pool_alloc_pages_slow getting pages from page allocator.
What would be a right min value for it? Just 0 or
XDP_PACKET_HEADROOM/NET_SKB_PAD? I guess here it matters if a BPF program is
loaded or not.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]