add DMA sync capability to page_pool API

Introduce the following parameters in order to add the possibility to sync DMA memory area before putting allocated buffers in the page_pool caches:

- sync: set to 1 if device is non cache-coherent and needs to flush DMA area

- offset: DMA address offset where the DMA engine starts copying rx data

- max_len: maximum DMA memory size page_pool is allowed to flush. This is currently used in __page_pool_alloc_pages_slow routine when pages are allocated from page allocator

These parameters are supposed to be set by device drivers

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool.h | 11 +++++++----
 net/core/page_pool.c    | 39 +++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 10 deletions(-)

include/net/page_pool.h +7 -4

index 2cbcdbdec254..defbfd90ab46 100644
@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */
 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };
 
 struct page_pool {

@@ -150,8 +153,8 @@ static inline void page_pool_destroy(struct page_pool *pool)
 }
 
 /* Never call this directly, use helpers below */
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct);
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct);
 
 static inline void page_pool_put_page(struct page_pool *pool,
 				      struct page *page, bool allow_direct)

@@ -160,14 +163,14 @@ static inline void page_pool_put_page(struct page_pool *pool,
 	 * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
 	 */
 #ifdef CONFIG_PAGE_POOL
-	__page_pool_put_page(pool, page, allow_direct);
+	__page_pool_put_page(pool, page, 0, allow_direct);
 #endif
 }
 /* Very limited use-cases allow recycle direct */
 static inline void page_pool_recycle_direct(struct page_pool *pool,
 					    struct page *page)
 {
-	__page_pool_put_page(pool, page, true);
+	__page_pool_put_page(pool, page, 0, true);
 }
 
 /* API user MUST have disconnected alloc-side (not allowed to call

net/core/page_pool.c +33 -6

index 5bc65587f1c4..af9514c2d15b 100644
@@ -112,6 +112,17 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
 	return page;
 }
 
+/* Used for non-coherent devices */
+static void page_pool_dma_sync_for_device(struct page_pool *pool,
+					  struct page *page,
+					  unsigned int dma_sync_size)
+{
+	dma_sync_size = min(dma_sync_size, pool->p.max_len);
+	dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
+					 pool->p.offset, dma_sync_size,
+					 pool->p.dma_dir);
+}
+
 /* slow path */
 noinline
 static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

@@ -156,6 +167,10 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	}
 	page->dma_addr = dma;
 
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
+
 skip_dma_map:
 	/* Track how many pages are held 'in-flight' */
 	pool->pages_state_hold_cnt++;

@@ -255,7 +270,8 @@ static void __page_pool_return_page(struct page_pool *pool, struct page *page)
 }
 
 static bool __page_pool_recycle_into_ring(struct page_pool *pool,
-				   struct page *page)
+					  struct page *page,
+					  unsigned int dma_sync_size)
 {
 	int ret;
 	/* BH protection not needed if current is serving softirq */

@@ -264,6 +280,10 @@ static bool __page_pool_recycle_into_ring(struct page_pool *pool,
 	else
 		ret = ptr_ring_produce_bh(&pool->ring, page);
 
+	/* non-coherent devices - flush memory */
+	if (ret == 0 && pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
+
 	return (ret == 0) ? true : false;
 }
 

@@ -273,18 +293,23 @@ static bool __page_pool_recycle_into_ring(struct page_pool *pool,
  * Caller must provide appropriate safe context.
  */
 static bool __page_pool_recycle_direct(struct page *page,
-				       struct page_pool *pool)
+				       struct page_pool *pool,
+				       unsigned int dma_sync_size)
 {
 	if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE))
 		return false;
 
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
 	pool->alloc.cache[pool->alloc.count++] = page;
+
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
 	return true;
 }
 
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct)
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct)
 {
 	/* This allocator is optimized for the XDP mode that uses
 	 * one-frame-per-page, but have fallbacks that act like the

@@ -296,10 +321,12 @@ void __page_pool_put_page(struct page_pool *pool,
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (allow_direct && in_serving_softirq())
-			if (__page_pool_recycle_direct(page, pool))
+			if (__page_pool_recycle_direct(page, pool,
+						       dma_sync_size))
 				return;
 
-		if (!__page_pool_recycle_into_ring(pool, page)) {
+		if (!__page_pool_recycle_into_ring(pool, page,
+						   dma_sync_size)) {
 			/* Cache full, fallback to free pages */
 			__page_pool_return_page(pool, page);
 		}

Quoted 4 lines · On Sun, 10 Nov 2019 14:09:09 +0200 Lorenzo Bianconi <[email protected]> wrote:

Introduce the following parameters in order to add the possibility to sync DMA memory area before putting allocated buffers in the page_pool caches:

- sync: set to 1 if device is non cache-coherent and needs to flush DMA area

I don't agree that this is only for non cache-coherent devices.

This change is generally for all device drivers. Via setting 'sync' (which I prefer to rename 'dma_sync') driver request that page_pool takes over doing DMA-sync-for-device. (Very important, DMA-sync-for-CPU is still drivers responsibility). Drivers can benefit from removing their calls to dma_sync_single_for_device().

We need to define meaning/semantics of this setting (my definition):
- This means that all pages that driver gets from page_pool, will be DMA-synced-for-device.

Quoted 6 lines

- offset: DMA address offset where the DMA engine starts copying rx data

- max_len: maximum DMA memory size page_pool is allowed to flush. This is currently used in __page_pool_alloc_pages_slow routine when pages are allocated from page allocator

Implementation wise (you did as I suggested offlist), and does the DMA-sync-for-device at return-time page_pool_put_page() time, because we (often) know the length that was/can touched by CPU. This is key to the optimization, that we know this length.

I also think you/we need to explain why this optimization is correct, my attempt:

This optimization reduce the length of the DMA-sync-for-device. The optimization is valid, because page is initially DMA-synced-for-device, as defined via max_len. At driver RX time, the driver will do a DMA-sync-for-CPU on the memory for the packet length. What is important is the memory occupied by packet payload, because this is the memory CPU is allowed to read and modify. If CPU have not written into a cache-line, then we know that CPU will not be flushing this, thus it doesn't need a DMA-sync-for-device. As we don't track cache-lines written into, simply use the full packet length as dma_sync_size, at page_pool recycle time. This also take into account any tail-extend.

Quoted 151 lines

These parameters are supposed to be set by device drivers

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool.h | 11 +++++++----
 net/core/page_pool.c    | 39 +++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 10 deletions(-)

include/net/page_pool.h +7 -4

index 2cbcdbdec254..defbfd90ab46 100644
@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */
 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };
 
 struct page_pool {

@@ -150,8 +153,8 @@ static inline void page_pool_destroy(struct page_pool *pool)
 }
 
 /* Never call this directly, use helpers below */
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct);
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct);
 
 static inline void page_pool_put_page(struct page_pool *pool,
 				      struct page *page, bool allow_direct)

@@ -160,14 +163,14 @@ static inline void page_pool_put_page(struct page_pool *pool,
 	 * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
 	 */
 #ifdef CONFIG_PAGE_POOL
-	__page_pool_put_page(pool, page, allow_direct);
+	__page_pool_put_page(pool, page, 0, allow_direct);
 #endif
 }
 /* Very limited use-cases allow recycle direct */
 static inline void page_pool_recycle_direct(struct page_pool *pool,
 					    struct page *page)
 {
-	__page_pool_put_page(pool, page, true);
+	__page_pool_put_page(pool, page, 0, true);
 }
 
 /* API user MUST have disconnected alloc-side (not allowed to call

net/core/page_pool.c +33 -6

index 5bc65587f1c4..af9514c2d15b 100644
@@ -112,6 +112,17 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
 	return page;
 }
 
+/* Used for non-coherent devices */
+static void page_pool_dma_sync_for_device(struct page_pool *pool,
+					  struct page *page,
+					  unsigned int dma_sync_size)
+{
+	dma_sync_size = min(dma_sync_size, pool->p.max_len);
+	dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
+					 pool->p.offset, dma_sync_size,
+					 pool->p.dma_dir);
+}
+
 /* slow path */
 noinline
 static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

@@ -156,6 +167,10 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	}
 	page->dma_addr = dma;
 
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
+
 skip_dma_map:
 	/* Track how many pages are held 'in-flight' */
 	pool->pages_state_hold_cnt++;

@@ -255,7 +270,8 @@ static void __page_pool_return_page(struct page_pool *pool, struct page *page)
 }
 
 static bool __page_pool_recycle_into_ring(struct page_pool *pool,
-				   struct page *page)
+					  struct page *page,
+					  unsigned int dma_sync_size)
 {
 	int ret;
 	/* BH protection not needed if current is serving softirq */

@@ -264,6 +280,10 @@ static bool __page_pool_recycle_into_ring(struct page_pool *pool,
 	else
 		ret = ptr_ring_produce_bh(&pool->ring, page);
 
+	/* non-coherent devices - flush memory */
+	if (ret == 0 && pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
+
 	return (ret == 0) ? true : false;
 }
 

@@ -273,18 +293,23 @@ static bool __page_pool_recycle_into_ring(struct page_pool *pool,
  * Caller must provide appropriate safe context.
  */
 static bool __page_pool_recycle_direct(struct page *page,
-				       struct page_pool *pool)
+				       struct page_pool *pool,
+				       unsigned int dma_sync_size)
 {
 	if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE))
 		return false;
 
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
 	pool->alloc.cache[pool->alloc.count++] = page;
+
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
 	return true;
 }
 
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct)
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct)
 {
 	/* This allocator is optimized for the XDP mode that uses
 	 * one-frame-per-page, but have fallbacks that act like the

@@ -296,10 +321,12 @@ void __page_pool_put_page(struct page_pool *pool,
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (allow_direct && in_serving_softirq())
-			if (__page_pool_recycle_direct(page, pool))
+			if (__page_pool_recycle_direct(page, pool,
+						       dma_sync_size))
 				return;
 
-		if (!__page_pool_recycle_into_ring(pool, page)) {
+		if (!__page_pool_recycle_into_ring(pool, page,
+						   dma_sync_size)) {
 			/* Cache full, fallback to free pages */
 			__page_pool_return_page(pool, page);
 		}

Signature · 4 lines

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

include/net/page_pool.h +0 -0

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +2 -3

@@ -150,8 +153,8 @@ static inline void page_pool_destroy(struct page_pool *pool)
 }
 
 /* Never call this directly, use helpers below */
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct);
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct);
 
 static inline void page_pool_put_page(struct page_pool *pool,
 				      struct page *page, bool allow_direct)

@@ -160,14 +163,14 @@ static inline void page_pool_put_page(struct page_pool *pool,
 	 * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
 	 */
 #ifdef CONFIG_PAGE_POOL
-	__page_pool_put_page(pool, page, allow_direct);

include/net/page_pool.h +0 -0

 /* API user MUST have disconnected alloc-side (not allowed to call

net/core/page_pool.c +10 -0

index 5bc65587f1c4..af9514c2d15b 100644
@@ -112,6 +112,17 @@ static struct page
 *__page_pool_get_cached(struct page_pool *pool) return page;
 }
 
+/* Used for non-coherent devices */
+static void page_pool_dma_sync_for_device(struct page_pool *pool,
+					  struct page *page,
+					  unsigned int dma_sync_size)
+{
+	dma_sync_size = min(dma_sync_size, pool->p.max_len);
+	dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
+					 pool->p.offset, dma_sync_size,
+					 pool->p.dma_dir);
+}

Introduce the following parameters in order to add the possibility to sync DMA memory area before putting allocated buffers in the page_pool caches:
- sync: set to 1 if device is non cache-coherent and needs to flush DMA
area
- offset: DMA address offset where the DMA engine starts copying rx data
- max_len: maximum DMA memory size page_pool is allowed to flush. This is currently used in __page_pool_alloc_pages_slow routine when pages are allocated from page allocator
These parameters are supposed to be set by device drivers

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/page_pool.h | 11 +++++++----
 net/core/page_pool.c    | 39 +++++++++++++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 10 deletions(-)

include/net/page_pool.h +0 -0

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +2 -2

 struct page_pool {

@@ -150,8 +153,8 @@ static inline void page_pool_destroy(struct 
 page_pool *pool)
 }
 
 /* Never call this directly, use helpers below */
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct);
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct);
 
 static inline void page_pool_put_page(struct page_pool *pool,

include/net/page_pool.h +2 -2

 				      struct page *page, bool allow_direct)

@@ -160,14 +163,14 @@ static inline void page_pool_put_page(struct 
 page_pool *pool,
 	 * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
 	 */
 #ifdef CONFIG_PAGE_POOL
-	__page_pool_put_page(pool, page, allow_direct);
+	__page_pool_put_page(pool, page, 0, allow_direct);
 #endif
 }
 /* Very limited use-cases allow recycle direct */
 static inline void page_pool_recycle_direct(struct page_pool *pool,
 					    struct page *page)
 {
-	__page_pool_put_page(pool, page, true);
+	__page_pool_put_page(pool, page, 0, true);
 }
 

/* API user MUST have disconnected alloc-side (not allowed to call

net/core/page_pool.c +11 -0

index 5bc65587f1c4..af9514c2d15b 100644
@@ -112,6 +112,17 @@ static struct page *__page_pool_get_cached(struct 
 page_pool *pool)
 	return page;
 }
 
+/* Used for non-coherent devices */
+static void page_pool_dma_sync_for_device(struct page_pool *pool,
+					  struct page *page,
+					  unsigned int dma_sync_size)
+{
+	dma_sync_size = min(dma_sync_size, pool->p.max_len);
+	dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
+					 pool->p.offset, dma_sync_size,
+					 pool->p.dma_dir);
+}
+
 /* slow path */
 noinline

static struct page *__page_pool_alloc_pages_slow(struct page_pool
*pool,

net/core/page_pool.c +4 -0

@@ -156,6 +167,10 @@ static struct page 
 *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	}
 	page->dma_addr = dma;
 
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
+
 skip_dma_map:
 	/* Track how many pages are held 'in-flight' */

net/core/page_pool.c +2 -1

 	pool->pages_state_hold_cnt++;

@@ -255,7 +270,8 @@ static void __page_pool_return_page(struct 
 page_pool *pool, struct page *page)
 }
 
 static bool __page_pool_recycle_into_ring(struct page_pool *pool,
-				   struct page *page)
+					  struct page *page,
+					  unsigned int dma_sync_size)
 {
 	int ret;

net/core/page_pool.c +12 -3

 	/* BH protection not needed if current is serving softirq */

@@ -264,6 +280,10 @@ static bool __page_pool_recycle_into_ring(struct 
 page_pool *pool,
 	else
 		ret = ptr_ring_produce_bh(&pool->ring, page);
 
+	/* non-coherent devices - flush memory */
+	if (ret == 0 && pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
+
 	return (ret == 0) ? true : false;
 }

@@ -273,18 +293,23 @@ static bool __page_pool_recycle_into_ring(struct 
 page_pool *pool,
  * Caller must provide appropriate safe context.
  */
 static bool __page_pool_recycle_direct(struct page *page,
-				       struct page_pool *pool)
+				       struct page_pool *pool,
+				       unsigned int dma_sync_size)
 {
 	if (unlikely(pool->alloc.count == PP_ALLOC_CACHE_SIZE))
 		return false;
 
 	/* Caller MUST have verified/know (page_ref_count(page) == 1) */
 	pool->alloc.cache[pool->alloc.count++] = page;
+
+	/* non-coherent devices - flush memory */
+	if (pool->p.sync)
+		page_pool_dma_sync_for_device(pool, page, dma_sync_size);
 	return true;
 }
 
-void __page_pool_put_page(struct page_pool *pool,
-			  struct page *page, bool allow_direct)
+void __page_pool_put_page(struct page_pool *pool, struct page *page,
+			  unsigned int dma_sync_size, bool allow_direct)
 {
 	/* This allocator is optimized for the XDP mode that uses

net/core/page_pool.c +4 -2

 	 * one-frame-per-page, but have fallbacks that act like the

@@ -296,10 +321,12 @@ void __page_pool_put_page(struct page_pool 
 *pool,
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (allow_direct && in_serving_softirq())
-			if (__page_pool_recycle_direct(page, pool))
+			if (__page_pool_recycle_direct(page, pool,
+						       dma_sync_size))
 				return;
 
-		if (!__page_pool_recycle_into_ring(pool, page)) {
+		if (!__page_pool_recycle_into_ring(pool, page,
+						   dma_sync_size)) {
 			/* Cache full, fallback to free pages */
 			__page_pool_return_page(pool, page);

}

Signature · 1 line

2.21.0

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.
As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.
The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 16 lines · Ilias Apalodimas

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

Quoted 12 lines

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to
device.
As the API tries to be generic a u8 was choosen instead of a flag to cover these
use cases. So in time we'll change the semantics of this to 'always sync', 'dont
sync if it's an skb-only queue' etc.
The first case Lorenzo covered is sync the required len only instead of the full
buffer

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

Since .sync alone doesn't make sense if the page pool isn't performing any DMA mapping, right?

Then existing drivers, if they're converted, can just add the SYNC flag.

I did see the initial case where only the RX_BUF_SIZE (1536) is sync'd instead of the full page.

Could you expand on your 'skb-only queue' comment? I'm currently running a variant of your patch where iommu mapped pages are attached to skb's and sent up the stack, then reclaimed on release. I imagine that with this change, they would have the full RX_BUF_SIZE sync'd before returning to the driver, since the upper layers could basically do anything with the buffer area.

Signature · 1 line

Jonathan

Hi Jonathan,

Quoted 36 lines · Jonathan Lemon

Quoted 16 lines · Ilias Apalodimas

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

Quoted 12 lines

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to
device.
As the API tries to be generic a u8 was choosen instead of a flag to cover these
use cases. So in time we'll change the semantics of this to 'always sync', 'dont
sync if it's an skb-only queue' etc.
The first case Lorenzo covered is sync the required len only instead of the full
buffer

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Quoted 5 lines

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

Since .sync alone doesn't make sense if the page pool isn't performing any DMA mapping, right?

Correct. If the sync happens regardless of the page pool mapping capabilities, this will affect performance negatively as well (on non-coherent architectures)

Quoted 6 lines

Then existing drivers, if they're converted, can just add the SYNC flag.

I did see the initial case where only the RX_BUF_SIZE (1536) is sync'd instead of the full page.

Quoted 6 lines

Could you expand on your 'skb-only queue' comment? I'm currently running a variant of your patch where iommu mapped pages are attached to skb's and sent up the stack, then reclaimed on release. I imagine that with this change, they would have the full RX_BUF_SIZE sync'd before returning to the driver, since the upper layers could basically do anything with the buffer area.

The idea was that page_pool lives per device queue. Usually some queues are reserved for XDP only. Since eBPF progs can change the packet we have to sync for the device, before we fill in the device descriptors.

For the skb reserved queues, this depends on the 'anything'. If the rest of the layers touch (or rather write) into that area, then we'll again gave to sync. If we know that the data has not been altered though, we can hand them back to the device skipping that sync right?

Quoted 34 lines · Ilias Apalodimas

Hi Jonathan,

Quoted 31 lines · Jonathan Lemon

Quoted 26 lines · Ilias Apalodimas

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };  

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.

As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.

The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 6 lines

Quoted 6 lines

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

Quoted 36 lines · On Thu, 14 Nov 2019 13:04:26 -0800 "Jonathan Lemon" <[email protected]>…

Quoted 34 lines · Ilias Apalodimas

Hi Jonathan,

Quoted 31 lines · Jonathan Lemon

Quoted 26 lines · Ilias Apalodimas

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };  

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.

As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.

The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

I actually agree and think we could use a flag. I suggest PP_FLAG_DMA_SYNC_DEV to indicate that this DMA-sync-for-device.

Ilias notice that the change I requested to Lorenzo, that dma_sync_size default value is 0xFFFFFFFF (-1). That makes dma_sync_size==0 a valid value, which you can use in the cases, where you know that nobody have written into the data-area. This allow us to selectively choose it for these cases.

Signature · 5 lines

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

Quoted 49 lines

Quoted 36 lines · On Thu, 14 Nov 2019 13:04:26 -0800 "Jonathan Lemon" <[email protected]>…

Quoted 34 lines · Ilias Apalodimas

Hi Jonathan,

Quoted 31 lines · Jonathan Lemon

Quoted 26 lines · Ilias Apalodimas

[...]

Quoted 6 lines

Quoted 6 lines

index 2cbcdbdec254..defbfd90ab46 100644

include/net/page_pool.h +0 -0

@@ -65,6 +65,9 @@ struct page_pool_params {
 	int		nid;  /* Numa node id to allocate from pages from */
 	struct device	*dev; /* device, for DMA pre-mapping purposes */

Quoted 8 lines

include/net/page_pool.h struct page_pool_params { · line 71

Quoted 5 lines

include/net/page_pool.h +3 -0

 	enum dma_data_direction dma_dir; /* DMA mapping direction */
+	unsigned int	max_len; /* max DMA sync memory size */
+	unsigned int	offset;  /* DMA addr offset */
+	u8 sync;
 };  

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.

As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.

The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

I actually agree and think we could use a flag. I suggest PP_FLAG_DMA_SYNC_DEV to indicate that this DMA-sync-for-device.

Quoted 5 lines

Ilias notice that the change I requested to Lorenzo, that dma_sync_size default value is 0xFFFFFFFF (-1). That makes dma_sync_size==0 a valid value, which you can use in the cases, where you know that nobody have written into the data-area. This allow us to selectively choose it for these cases.

Okay, then i guess the flag is a better fit for this.
The only difference would be that the sync semantics will be done on 'per packet' basis, instead of 'per pool', but that should be fine for our cases.

Cheers
/Ilias

Quoted 7 lines

Signature · 5 lines

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

Quoted 30 lines

Quoted 24 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 2 lines

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.

As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.

The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

I actually agree and think we could use a flag. I suggest PP_FLAG_DMA_SYNC_DEV to indicate that this DMA-sync-for-device.

Quoted 5 lines

Ilias notice that the change I requested to Lorenzo, that dma_sync_size default value is 0xFFFFFFFF (-1). That makes dma_sync_size==0 a valid value, which you can use in the cases, where you know that nobody have written into the data-area. This allow us to selectively choose it for these cases.

Quoted 3 lines

Okay, then i guess the flag is a better fit for this.
The only difference would be that the sync semantics will be done on 'per packet' basis, instead of 'per pool', but that should be fine for our cases.

Ack, fine for me.
Do you think when checking for PP_FLAG_DMA_SYNC_DEV we should even verify PP_FLAG_DMA_MAP? Something like:

if ((pool->p.flags & (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV)) ==

    (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV))
	page_pool_dma_sync_for_device();

Regards,
Lorenzo

Quoted 12 lines

Cheers
/Ilias

Quoted 7 lines

Signature · 5 lines

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

Hi Lorenzo,

Quoted 42 lines

Quoted 30 lines

Quoted 24 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 2 lines

How about using PP_FLAG_DMA_SYNC instead of another flag word? (then it can also be gated on having DMA_MAP enabled)

You mean instead of the u8?
As you pointed out on your V2 comment of the mail, some cards don't sync back to device.

As the API tries to be generic a u8 was choosen instead of a flag to cover these use cases. So in time we'll change the semantics of this to 'always sync', 'dont sync if it's an skb-only queue' etc.

The first case Lorenzo covered is sync the required len only instead of the full buffer

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Yes, I meant instead of:

(quoted diff) +1 -0

+		.sync = 1,

Something like:
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC

I actually agree and think we could use a flag. I suggest PP_FLAG_DMA_SYNC_DEV to indicate that this DMA-sync-for-device.

Quoted 5 lines

Ilias notice that the change I requested to Lorenzo, that dma_sync_size default value is 0xFFFFFFFF (-1). That makes dma_sync_size==0 a valid value, which you can use in the cases, where you know that nobody have written into the data-area. This allow us to selectively choose it for these cases.

Quoted 3 lines

Okay, then i guess the flag is a better fit for this.
The only difference would be that the sync semantics will be done on 'per packet' basis, instead of 'per pool', but that should be fine for our cases.

Ack, fine for me.
Do you think when checking for PP_FLAG_DMA_SYNC_DEV we should even verify PP_FLAG_DMA_MAP? Something like:

if ((pool->p.flags & (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV)) ==

    (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV))
	page_pool_dma_sync_for_device();

Quoted 2 lines

Regards,
Lorenzo

I think it's better to do the check once on the pool registration and maybe refuse to allocate the pool? Syncing without mapping doesn't really make sense

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

Okay, then i guess the flag is a better fit for this. The only difference would be that the sync semantics will be done on 'per
packet' basis, instead of 'per pool', but that should be fine for our cases.

Ack, fine for me.
Do you think when checking for PP_FLAG_DMA_SYNC_DEV we should even verify
PP_FLAG_DMA_MAP? Something like:

if ((pool->p.flags & (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV)) ==

    (PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV))
	page_pool_dma_sync_for_device();

Regards,
Lorenzo

Quoted 4 lines

I think it's better to do the check once on the pool registration and maybe
refuse to allocate the pool? Syncing without mapping doesn't really make sense

+1.

Signature · 1 line

Jonathan

Quoted 7 lines

For the skb reserved queues, this depends on the 'anything'. If the rest of the
layers touch (or rather write) into that area, then we'll again gave to sync.
If we know that the data has not been altered though, we can hand them back to
the device skipping that sync right?

Sure, but this is also true for eBPF programs. How would the driver know that
the data has not been altered / compacted by the upper layers?

Signature · 1 line

Jonathan

Get rid of costly dma_sync_single_for_device in mvneta_rx_refill since now the driver can let page_pool API to manage needed DMA sync with a proper size.

- XDP_DROP DMA sync managed by mvneta driver: ~420Kpps
- XDP_DROP DMA sync managed by page_pool API: ~595Kpps

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -4

index ed93eecb7485..591d580c68b4 100644
@@ -1846,7 +1846,6 @@ static int mvneta_rx_refill(struct mvneta_port 
 *pp,
 			    struct mvneta_rx_queue *rxq,
 			    gfp_t gfp_mask)
 {
-	enum dma_data_direction dma_dir;
 	dma_addr_t phys_addr;
 	struct page *page;

@@ -1856,9 +1855,6 @@ static int mvneta_rx_refill(struct mvneta_port 
 *pp,
 		return -ENOMEM;
 
 	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
-	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
-	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
-				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
 	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
 

drivers/net/ethernet/marvell/mvneta.c +0 -0

 	return 0;

Quoted 39 lines · Lorenzo Bianconi

Get rid of costly dma_sync_single_for_device in mvneta_rx_refill since now the driver can let page_pool API to manage needed DMA sync with a proper size.

- XDP_DROP DMA sync managed by mvneta driver: ~420Kpps
- XDP_DROP DMA sync managed by page_pool API: ~595Kpps

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -4

index ed93eecb7485..591d580c68b4 100644
@@ -1846,7 +1846,6 @@ static int mvneta_rx_refill(struct mvneta_port
 *pp,
 			    struct mvneta_rx_queue *rxq,
 			    gfp_t gfp_mask)
 {
-	enum dma_data_direction dma_dir;
 	dma_addr_t phys_addr;
 	struct page *page;

@@ -1856,9 +1855,6 @@ static int mvneta_rx_refill(struct mvneta_port
 *pp,
 		return -ENOMEM;
 
 	phys_addr = page_pool_get_dma_addr(page) + pp->rx_offset_correction;
-	dma_dir = page_pool_get_dma_dir(rxq->page_pool);
-	dma_sync_single_for_device(pp->dev->dev.parent, phys_addr,
-				   MVNETA_MAX_RX_BUF_SIZE, dma_dir);
 	mvneta_rx_desc_fill(rx_desc, phys_addr, page, rxq);
 

drivers/net/ethernet/marvell/mvneta.c +0 -0

 	return 0;

drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct · line 2100

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +4 -2

@@ -2097,8 +2093,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct
 mvneta_rx_queue *rxq,
 		err = xdp_do_redirect(pp->dev, xdp, prog);
 		if (err) {
 			ret = MVNETA_XDP_DROPPED;
-			page_pool_recycle_direct(rxq->page_pool,
-						 virt_to_head_page(xdp->data));
+			__page_pool_put_page(rxq->page_pool,
+					virt_to_head_page(xdp->data),
+					xdp->data_end - xdp->data_hard_start,
+					true);

I just have a clarifying question. Here, the RX buffer was received and then
dma_sync'd to the CPU. Now, it is going to be recycled for RX again; does it
actually need to be sync'd back to the device?

I'm asking since several of the other network drivers (mellanox, for example)
don't resync the buffer back to the device when recycling it for reuse.

Signature · 1 line

Jonathan