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



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

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