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

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

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.

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

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