add DMA-sync-for-device capability to page_pool API
Subsystems (4)
v4 cover add DMA-sync-for-device capability to page_pool API
Introduce the possibility to sync DMA memory for device in the page_pool API. This feature allows to sync proper DMA size and not always full buffer (dma_sync_single_for_device can be very costly).
Please note DMA-sync-for-CPU is still device driver responsibility. Relying on page_pool DMA sync mvneta driver improves XDP_DROP pps of about 170Kpps:
- XDP_DROP DMA sync managed by mvneta driver: ~420Kpps
- XDP_DROP DMA sync managed by page_pool API: ~585Kpps
Changes since v3:
- move dma_sync_for_device before putting the page in ptr_ring in __page_pool_recycle_into_ring since ptr_ring can be consumed concurrently. Simplify the code moving dma_sync_for_device before running __page_pool_recycle_direct/__page_pool_recycle_into_ring
Changes since v2:
- rely on PP_FLAG_DMA_SYNC_DEV flag instead of dma_sync
Changes since v1:
- rename sync in dma_sync
- set dma_sync_size to 0xFFFFFFFF in page_pool_recycle_direct and page_pool_put_page routines
- Improve documentation
Lorenzo Bianconi (3):
net: mvneta: rely on page_pool_recycle_direct in mvneta_run_xdp
net: page_pool: add the possibility to sync DMA memory for device
net: mvneta: get rid of huge dma sync in mvneta_rx_refill
drivers/net/ethernet/marvell/mvneta.c | 24 ++++++++++++++--------- include/net/page_pool.h | 21 ++++++++++++++------ net/core/page_pool.c | 28 +++++++++++++++++++++++++-- 3 files changed, 56 insertions(+), 17 deletions(-)
Signature · 1 line
2.21.0
PATCH v4 1/3 net: mvneta: rely on page_pool_recycle_direct in mvneta_run_xdp
Rely on page_pool_recycle_direct and not on xdp_return_buff in mvneta_run_xdp. This is a preliminary patch to limit the dma sync len to the one strictly necessary
--- drivers/net/ethernet/marvell/mvneta.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +4 -2
@@ -2097,7 +2097,8 @@ 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 ; -xdp_return_buff (xdp); +page_pool_recycle_direct (rxq->page_pool, +virt_to_head_page (xdp->data)); }else { ret =MVNETA_XDP_REDIR ; }
@@ -2106,7 +2107,8 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,case XDP_TX : ret =mvneta_xdp_xmit_back (pp, xdp);if (ret !=MVNETA_XDP_TX ) -xdp_return_buff (xdp); +page_pool_recycle_direct (rxq->page_pool, +virt_to_head_page (xdp->data));break ;default :bpf_warn_invalid_xdp_action (act);
Signature · 1 line
2.21.0
PATCH v4 2/3 net: page_pool: add the possibility to sync DMA memory for device
In reply to Lorenzo BianconiIntroduce the following parameters in order to add the possibility to sync DMA memory for device before putting allocated pages in the page_pool caches:
- PP_FLAG_DMA_SYNC_DEV: if set in page_pool_params flags, 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
- 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.
This optimization reduces the length of the DMA-sync-for-device. The optimization is valid because pages are initially DMA-synced-for-device as defined via max_len. At RX time, the driver will perform 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 area CPU is allowed to read and modify. As we don't track cache-lines written into by the CPU, simply use the packet payload length as dma_sync_size at page_pool recycle time. This also take into account any tail-extend.
--- include/net/page_pool.h | 21 +++++++++++++++------ net/core/page_pool.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 8 deletions(-)
include/net/page_pool.h +15 -6
@@ -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)/* * Fast allocation side cache array/stack
@@ -65,6 +72,8 @@ 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 */ };struct page_pool {
@@ -149,8 +158,8 @@ static inline void page_pool_use_xdp_mem(struct page_pool *pool,#endif /* 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)
@@ -159,14 +168,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,-1 , 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,-1 , true); }/* Disconnects a page (from a page_pool). API users can have a need
net/core/page_pool.c +26 -2
@@ -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 ; +if (ptr_ring_init (&pool->ring, ring_qsize,GFP_KERNEL ) <0 )return -ENOMEM ;
@@ -115,6 +122,16 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)return page; } +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,
@@ -159,6 +176,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool, } page->dma_addr = dma; +if (pool->p.flags &PP_FLAG_DMA_SYNC_DEV ) +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++;
@@ -281,8 +301,8 @@ static bool __page_pool_recycle_direct(struct page *page,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
@@ -293,6 +313,10 @@ void __page_pool_put_page(struct page_pool *pool,if (likely (page_ref_count (page) ==1 )) {/* Read barrier done in page_ref_count / READ_ONCE */ +if (pool->p.flags &PP_FLAG_DMA_SYNC_DEV ) +page_pool_dma_sync_for_device (pool, page, + dma_sync_size); +if (allow_direct &&in_serving_softirq ())if (__page_pool_recycle_direct (page, pool))return ;
Signature · 1 line
2.21.0
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 10 lines · On Mon, 18 Nov 2019 15:33:45 +0200 Lorenzo Bianconi <[email protected]> wrote:
include/net/page_pool.h +0
-2@@ -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
include/net/page_pool.h line 46
Quoted 10 lines
include/net/page_pool.h +9
-0+#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)
Quoted 5 lines
net/core/page_pool.c +0
-0
net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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)?
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Ilias Apalodimas
maintainerRe: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 32 lines
Quoted 5 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };
Quoted 5 lines
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
Quoted 5 lines
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
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
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 41 lines · On Tue, 19 Nov 2019 13:33:36 +0200 Ilias Apalodimas…
Quoted 32 lines
Quoted 5 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };Quoted 5 lines
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 thoseQuoted 1 line
Quoted 7 lines
Quoted 4 lines
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.
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Ilias Apalodimas
maintainerRe: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 49 lines · Jesper Dangaard Brouer
Quoted 46 lines · On Tue, 19 Nov 2019 13:33:36 +0200 Ilias Apalodimas…
Quoted 32 lines
Quoted 5 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };Quoted 5 lines
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 thoseQuoted 5 lines
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)?
Quoted 5 lines
Quoted 2 lines
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.
If the offset is 0, you'll end up syncing a couple of uneeded bytes (whatever headers the buffer has which doesn't need syncing).
Quoted 7 lines
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_poole.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.
The offset will affect the fast path running code.
What i am worried about is that XDP and SKB pool will have different needs for offsets. In the netsec driver i am dealing with this with reserving the same header whether the packet is an SKB or XDP buffer. If we check the offset we are practically forcing people to do something similar
Thanks
/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
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
In reply to Jesper Dangaard BrouerQuoted 25 lines
Quoted 20 lines · On Mon, 18 Nov 2019 15:33:45 +0200 Lorenzo Bianconi <[email protected]> wrote:
include/net/page_pool.h +9
-2@@ -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)[...]
Quoted 6 lines
Can you please change this to use the BIT(X) api.
#include <
linux/bits.h>#define
PP_FLAG_DMA_MAPBIT(0)
#definePP_FLAG_DMA_SYNC_DEVBIT(1)
Hi Jesper,
sure, will do in v5
Quoted 35 lines
Quoted 8 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };
Quoted 5 lines
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;
}Quoted 5 lines
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.
Regards,
Lorenzo
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
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 80 lines · On Tue, 19 Nov 2019 14:14:30 +0200 Lorenzo Bianconi…
Quoted 25 lines
Quoted 20 lines · On Mon, 18 Nov 2019 15:33:45 +0200 Lorenzo Bianconi <[email protected]> wrote:
include/net/page_pool.h +9
-2@@ -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)[...]
Quoted 6 lines
Can you please change this to use the BIT(X) api.
#include <
linux/bits.h>#define
PP_FLAG_DMA_MAPBIT(0)
#definePP_FLAG_DMA_SYNC_DEVBIT(1)Hi Jesper,
sure, will do in v5
Quoted 35 lines
Quoted 8 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };Quoted 5 lines
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:
Quoted 7 lines
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.
Quoted 11 lines
Quoted 5 lines
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_slowgetting 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?
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 87 lines
Quoted 84 lines · On Tue, 19 Nov 2019 14:14:30 +0200 Lorenzo Bianconi…
Quoted 25 lines
Quoted 20 lines · On Mon, 18 Nov 2019 15:33:45 +0200 Lorenzo Bianconi <[email protected]> wrote:
include/net/page_pool.h +9
-2@@ -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)[...]
Quoted 6 lines
Can you please change this to use the BIT(X) api.
#include <
linux/bits.h>#define
PP_FLAG_DMA_MAPBIT(0)
#definePP_FLAG_DMA_SYNC_DEVBIT(1)Hi Jesper,
sure, will do in v5
Quoted 35 lines
Quoted 8 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };Quoted 5 lines
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;
Quoted 6 lines
Quoted 3 lines
if (!pool->p.max_len) return -EINVAL; }Yes, exactly.
ack, I will add it to v5
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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)?
Quoted 8 lines
Quoted 5 lines
With 'non-XDP mode' do you mean not loading a BPF program? If so yes, it used in
__page_pool_alloc_pages_slowgetting 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
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
Re: [PATCH v4 net-next 2/3] net: page_pool: add the possibility to sync DMA memory for device
Quoted 111 lines · On Tue, 19 Nov 2019 17:25:43 +0200 Lorenzo Bianconi…
Quoted 87 lines
Quoted 84 lines · On Tue, 19 Nov 2019 14:14:30 +0200 Lorenzo Bianconi…
Quoted 25 lines
Quoted 20 lines · On Mon, 18 Nov 2019 15:33:45 +0200 Lorenzo Bianconi <[email protected]> wrote:
include/net/page_pool.h +9
-2@@ -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)[...]
Quoted 6 lines
Can you please change this to use the BIT(X) api.
#include <
linux/bits.h>#define
PP_FLAG_DMA_MAPBIT(0)
#definePP_FLAG_DMA_SYNC_DEVBIT(1)Hi Jesper,
sure, will do in v5
Quoted 35 lines
Quoted 8 lines
net/core/page_pool.c +0
-0net/core/page_pool.c static int page_pool_init(struct page_pool *pool, · line 56
Quoted 10 lines
net/core/page_pool.c +7
-0@@ -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_DEVdepend on:(quoted diff) +3
-1struct 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, };Quoted 5 lines
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;Quoted 6 lines
Quoted 3 lines
if (!pool->p.max_len) return -EINVAL; }Yes, exactly.
ack, I will add it to v5
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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)?
Quoted 8 lines
Quoted 5 lines
With 'non-XDP mode' do you mean not loading a BPF program? If so yes, it used in
__page_pool_alloc_pages_slowgetting 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?
Quoted 6 lines
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?
Yes. I guess we just accept .offset can be zero. It is an artificial limitation.
The check is not important if API is used correctly. It comes from my API design philosophy for page_pool, which is "Easy to use, and hard to misuse". This is a case of catching "misuse" and signaling that this was a wrong config. The check for pool->p.max_len should be enough, for driver developer to notice, that they also need to set offset. Maybe a comment close to pool->p.max_len check about "offset" will be enough. Given you return the "catch all" -EINVAL, we/you force driver devel to read code for page_pool_init(), which IMHO is sufficiently clear.
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
PATCH v4 3/3 net: mvneta: get rid of huge dma sync in mvneta_rx_refill
In reply to Lorenzo BianconiGet 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: ~585Kpps
--- drivers/net/ethernet/marvell/mvneta.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +15 -11
@@ -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);return 0 ;
@@ -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); }else { ret =MVNETA_XDP_REDIR ; }
@@ -2107,8 +2105,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,case XDP_TX : ret =mvneta_xdp_xmit_back (pp, xdp);if (ret !=MVNETA_XDP_TX ) -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);break ;default :bpf_warn_invalid_xdp_action (act);
@@ -2117,8 +2117,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,trace_xdp_exception (pp->dev, prog, act);/* fall through */ case XDP_DROP : -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); ret =MVNETA_XDP_DROPPED ;break ; }
@@ -3067,11 +3069,13 @@ static int mvneta_create_page_pool(struct mvneta_port *pp,struct bpf_prog *xdp_prog =READ_ONCE (pp->xdp_prog);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 , };int err;
Signature · 1 line
2.21.0
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 4 lines · On Mon, 18 Nov 2019 15:33:46 +0200 Lorenzo Bianconi <[email protected]> wrote:
drivers/net/ethernet/marvell/mvneta.c +0
-0
[...]
Quoted 27 lines
drivers/net/ethernet/marvell/mvneta.c +8
-4@@ -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); }else { ret =MVNETA_XDP_REDIR ; }@@ -2107,8 +2105,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,case XDP_TX : ret =mvneta_xdp_xmit_back (pp, xdp);if (ret !=MVNETA_XDP_TX ) -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);break ;default :bpf_warn_invalid_xdp_action (act);
drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2123
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2@@ -2117,8 +2117,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,trace_xdp_exception (pp->dev, prog, act);/* fall through */ case XDP_DROP : -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);
This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function name page_pool_recycle_direct. (I do considered calling this 'napi' instead, as it would be inline with networking use-cases, but it seemed limited if other subsystem end-up using this).
Does is 'page_pool_put_page_len' sound better?
But I want also want hide the bool 'allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)
Going for 'page_pool_put_page_len_napi' starts to be come rather long.
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 55 lines
Quoted 4 lines · On Mon, 18 Nov 2019 15:33:46 +0200 Lorenzo Bianconi <[email protected]> wrote:
drivers/net/ethernet/marvell/mvneta.c +0
-0[...]
Quoted 27 lines
drivers/net/ethernet/marvell/mvneta.c +8
-4@@ -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); }else { ret =MVNETA_XDP_REDIR ; }@@ -2107,8 +2105,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,case XDP_TX : ret =mvneta_xdp_xmit_back (pp, xdp);if (ret !=MVNETA_XDP_TX ) -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);break ;default :bpf_warn_invalid_xdp_action (act);drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2123
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2@@ -2117,8 +2117,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,trace_xdp_exception (pp->dev, prog, act);/* fall through */ case XDP_DROP : -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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function name
page_pool_recycle_direct. (I do considered calling this 'napi' instead, as it would be inline with networking use-cases, but it seemed limited if other subsystem end-up using this).
Quoted 6 lines
Does is '
page_pool_put_page_len' sound better?But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)Going for '
page_pool_put_page_len_napi' starts to be come rather long.
What about removing the second 'page'? Something like:
- page_pool_put_len_napi()
Regards,
Lorenzo
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
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 57 lines · On Tue, 19 Nov 2019 14:19:11 +0200 Lorenzo Bianconi…
Quoted 57 lines
Quoted 4 lines · On Mon, 18 Nov 2019 15:33:46 +0200 Lorenzo Bianconi <[email protected]> wrote:
drivers/net/ethernet/marvell/mvneta.c +0
-0[...]
Quoted 27 lines
drivers/net/ethernet/marvell/mvneta.c +8
-4@@ -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); }else { ret =MVNETA_XDP_REDIR ; }@@ -2107,8 +2105,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,case XDP_TX : ret =mvneta_xdp_xmit_back (pp, xdp);if (ret !=MVNETA_XDP_TX ) -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);break ;default :bpf_warn_invalid_xdp_action (act);drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2123
Quoted 9 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2@@ -2117,8 +2117,10 @@ mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,trace_xdp_exception (pp->dev, prog, act);/* fall through */ case XDP_DROP : -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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function name
page_pool_recycle_direct. (I do considered calling this 'napi' instead, as it would be inline with networking use-cases, but it seemed limited if other subsystem end-up using this).Does is '
page_pool_put_page_len' sound better?
Quoted 7 lines
Quoted 4 lines
But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)Going for '
page_pool_put_page_len_napi' starts to be come rather long.What about removing the second 'page'? Something like:
-page_pool_put_len_napi()
Well, we (unfortunately) already have page_pool_put(), which is used for refcnt on the page_pool object itself.
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
[...]
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2099
Quoted 6 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2-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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function name
page_pool_recycle_direct. (I do considered calling this 'napi' instead, as it would be inline with networking use-cases, but it seemed limited if other subsystem end-up using this).Does is '
page_pool_put_page_len' sound better?But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Going for '
page_pool_put_page_len_napi' starts to be come rather long.What about removing the second 'page'? Something like:
-page_pool_put_len_napi()Well, we (unfortunately) already have
page_pool_put(), which is used for refcnt on thepage_poolobject itself.
__page_pool_put_page(pp, data, len, true) is a more generic version of page_pool_recycle_direct where we can specify even the length. So what about:
- page_pool_recycle_len_direct
- page_pool_recycle_len_napi
Regards,
Lorenzo
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
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 28 lines · Lorenzo Bianconi
[...]
Quoted 27 lines
Quoted 26 lines
Quoted 17 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2099
Quoted 6 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2-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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function name
page_pool_recycle_direct. (I do considered calling this 'napi' instead, as it would be inline with networking use-cases, but it seemed limited if other subsystem end-up using this).Quoted 6 lines
Does is '
page_pool_put_page_len' sound better?But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)Going for '
page_pool_put_page_len_napi' starts to be come rather long.What about removing the second 'page'? Something like:
-page_pool_put_len_napi()
Quoted 8 lines
Quoted 2 lines
Well, we (unfortunately) already have
page_pool_put(), which is used for refcnt on thepage_poolobject itself.
__page_pool_put_page(pp, data, len, true)is a more generic version ofpage_pool_recycle_directwhere we can specify even the length. So what about:-
page_pool_recycle_len_direct
-page_pool_recycle_len_napi
I'd suggest:
/* elevated refcounts, page may seen by networking stack */page_pool_drain(pool, page, count) /* non napi, len = -1 */page_pool_drain_direct(pool, page, count) /* len = -1 */
page_pool_check_put_page(page) /* may not belong to pool */
/* recycle variants drain/expect refcount == 1 */page_pool_recycle(pool, page, len)page_pool_recycle_direct(pool, page, len)
page_pool_put_page(pool, page, len, mode) /* generic, for __xdp_return */
I'd rather add len as a parameter, than add more wrapper variants.
Signature · 1 line
Jonathan
Quoted 11 lines
Regards,
LorenzoQuoted 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
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 51 lines · Lorenzo Bianconi
Quoted 37 lines
[...]
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
drivers/net/ethernet/marvell/mvneta.c mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, · line 2099
Quoted 6 lines
drivers/net/ethernet/marvell/mvneta.c +4
-2-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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function
namepage_pool_recycle_direct. (I do considered calling this 'napi'
instead, as it would be inline with networking use-cases, but it seemed
limited if other subsystem end-up using this).Does is '
page_pool_put_page_len' sound better?But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Going for '
page_pool_put_page_len_napi' starts to be come rather long.What about removing the second 'page'? Something like:
-page_pool_put_len_napi()Well, we (unfortunately) already have
page_pool_put(), which is used for refcnt on thepage_poolobject itself.Quoted 6 lines
__page_pool_put_page(pp, data, len, true)is a more generic version ofpage_pool_recycle_directwhere we can specify even the length. So what about:-
page_pool_recycle_len_direct
-page_pool_recycle_len_napiI'd suggest:
/* elevated refcounts, page may seen by networking stack */
page_pool_drain(pool, page, count)/* non napi, len = -1 */page_pool_drain_direct(pool, page, count)/* len = -1 */
Quoted 7 lines
page_pool_check_put_page(page)/* may not belong to pool *//* recycle variants drain/expect refcount == 1 */
page_pool_recycle(pool, page, len)page_pool_recycle_direct(pool, page, len)
page_pool_put_page(pool, page, len, mode)/* generic, for__xdp_return
I am not against the suggestion but personally I would prefer to explicitate in the routine name where/how it is actually used. Moreover page_pool_recycle_direct or page_pool_put_page are currently used by multiple drivers and it seems to me out of the scope of this series. I think we can address it in a follow-up series and use __page_pool_put_page for the moment (it is actually just used by mvneta). Agree?
Regards,
Lorenzo
Quoted 20 lines
*/
I'd rather add len as a parameter, than add more wrapper variants.
Signature · 1 line
JonathanQuoted 11 lines
Regards,
LorenzoQuoted 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
Re: [PATCH v4 net-next 3/3] net: mvneta: get rid of huge dma sync in mvneta_rx_refill
Quoted 57 lines · Lorenzo Bianconi
Quoted 57 lines · Lorenzo Bianconi
Quoted 37 lines
[...]
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Review
Quoted 6 lines
(quoted diff) +4
-2- 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);This does beg for the question: Should we create an API wrapper for this in the header file?
But what to name it?
I know Jonathan doesn't like the "direct" part of the previous function
namepage_pool_recycle_direct. (I do considered calling this 'napi'
instead, as it would be inline with networking use-cases, but it seemed
limited if other subsystem end-up using this).Does is '
page_pool_put_page_len' sound better?But I want also want hide the bool '
allow_direct' in the API name. (As it makes it easier to identify users that uses this from softirq)Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Going for '
page_pool_put_page_len_napi' starts to be come rather long.What about removing the second 'page'? Something like:
-page_pool_put_len_napi()Well, we (unfortunately) already have
page_pool_put(), which is used for refcnt on thepage_poolobject itself.Quoted 6 lines
__page_pool_put_page(pp, data, len, true)is a more generic version ofpage_pool_recycle_directwhere we can specify even the length. So what about:-
page_pool_recycle_len_direct
-page_pool_recycle_len_napiI'd suggest:
/* elevated refcounts, page may seen by networking stack */
page_pool_drain(pool, page, count)/* non napi, len = -1 */page_pool_drain_direct(pool, page, count)/* len = -1 */
page_pool_check_put_page(page)/* may not belong to pool *//* recycle variants drain/expect refcount == 1 */
page_pool_recycle(pool, page, len)page_pool_recycle_direct(pool, page, len)
Quoted 8 lines
Quoted 1 line
page_pool_put_page(pool, page, len, mode)/* generic, for__xdp_returnI am not against the suggestion but personally I would prefer to explicitate in the routine name where/how it is actually used. Moreover
page_pool_recycle_directorpage_pool_put_pageare currently used by multiple drivers and it seems to me out of the scope of this series. I think we can address it in a follow-up series and use__page_pool_put_pagefor the moment (it is actually just used by mvneta). Agree?
Fine with me - I have a naming cleanup patch pending, I can roll it into this afterwards.
Signature · 1 line
Jonathan