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: 4721 bytes --]

> On Tue, 19 Nov 2019 14:14:30 +0200
> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
> 
> > > 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;
> >	}
> 
> Yes, exactly.
> 

ack, I will add it to v5

> > > 
> > > 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.
> 
> I think you are saying, that we need to allow .offset==0, because it is
> used by mvneta.  Did I understand that correctly?

I was just wondering what is the right value for the min offset, but rethinking
about it yes, there is a condition where  mvneta is using offset set 0 (it is the
regression reported by Andrew, when mvneta is running on a hw bm device  but bm
code is not compiled). Do you think we can skip this check for the moment until we fix
XDP on that particular board?

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