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 13:33:36 +0200
Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> > > 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.  
> 
> +1 we sync based on the min() value of those 
> 
> > 
> > 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)?  
> 
> Not sure about this, since it does not break anything apart from some
> performance hit

I don't follow the 'performance hit' comment.  This is checked at setup
time (page_pool_init), thus it doesn't affect runtime.

This is a generic optimization principle that I use a lot. Moving code
checks out of fast-path, and instead do more at setup/load-time, or
even at shutdown-time (like we do for page_pool e.g. check refcnt
invariance).  This principle is also heavily used by BPF, that adjust
BPF-instructions at load-time.  It is core to getting the performance
we need for high-speed networking.

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