| Message ID | 156045052249.29115.2357668905441684019.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v1,01/11] net: page_pool: add helper function to retrieve dma addresses | expand |
Hi Jesper, Minot nit picks mostly, On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: > This patch is needed before we can allow drivers to use page_pool for > DMA-mappings. Today with page_pool and XDP return API, it is possible to > remove the page_pool object (from rhashtable), while there are still > in-flight packet-pages. This is safely handled via RCU and failed lookups in > __xdp_return() fallback to call put_page(), when page_pool object is gone. > In-case page is still DMA mapped, this will result in page note getting > correctly DMA unmapped. > > To solve this, the page_pool is extended with tracking in-flight pages. And > XDP disconnect system queries page_pool and waits, via workqueue, for all > in-flight pages to be returned. > > To avoid killing performance when tracking in-flight pages, the implement > use two (unsigned) counters, that in placed on different cache-lines, and > can be used to deduct in-flight packets. This is done by mapping the > unsigned "sequence" counters onto signed Two's complement arithmetic > operations. This is e.g. used by kernel's time_after macros, described in > kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982. > > The trick is these two incrementing counters only need to be read and > compared, when checking if it's safe to free the page_pool structure. Which > will only happen when driver have disconnected RX/alloc side. Thus, on a > non-fast-path. > > It is chosen that page_pool tracking is also enabled for the non-DMA > use-case, as this can be used for statistics later. > > After this patch, using page_pool requires more strict resource "release", > e.g. via page_pool_release_page() that was introduced in this patchset, and > previous patches implement/fix this more strict requirement. > > Drivers no-longer call page_pool_destroy(). Drivers already call > xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will > attempt to disconnect the mem id, and if attempt fails schedule the > disconnect for later via delayed workqueue. > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 - > include/net/page_pool.h | 41 ++++++++++--- > net/core/page_pool.c | 62 +++++++++++++++----- > net/core/xdp.c | 65 +++++++++++++++++++-- > 4 files changed, 136 insertions(+), 35 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index 2f647be292b6..6c9d4d7defbc 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq) > } > > xdp_rxq_info_unreg(&rq->xdp_rxq); > - if (rq->page_pool) > - page_pool_destroy(rq->page_pool); > - > mlx5_wq_destroy(&rq->wq_ctrl); > } > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 754d980700df..f09b3f1994e6 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -16,14 +16,16 @@ > * page_pool_alloc_pages() call. Drivers should likely use > * page_pool_dev_alloc_pages() replacing dev_alloc_pages(). > * > - * If page_pool handles DMA mapping (use page->private), then API user > - * is responsible for invoking page_pool_put_page() once. In-case of > - * elevated refcnt, the DMA state is released, assuming other users of > - * the page will eventually call put_page(). > + * API keeps track of in-flight pages, in-order to let API user know > + * when it is safe to dealloactor page_pool object. Thus, API users > + * must make sure to call page_pool_release_page() when a page is > + * "leaving" the page_pool. Or call page_pool_put_page() where > + * appropiate. For maintaining correct accounting. > * > - * If no DMA mapping is done, then it can act as shim-layer that > - * fall-through to alloc_page. As no state is kept on the page, the > - * regular put_page() call is sufficient. > + * API user must only call page_pool_put_page() once on a page, as it > + * will either recycle the page, or in case of elevated refcnt, it > + * will release the DMA mapping and in-flight state accounting. We > + * hope to lift this requirement in the future. > */ > #ifndef _NET_PAGE_POOL_H > #define _NET_PAGE_POOL_H > @@ -66,9 +68,10 @@ struct page_pool_params { > }; > > struct page_pool { > - struct rcu_head rcu; > struct page_pool_params p; > > + u32 pages_state_hold_cnt; Maybe mention the different cache-line placement for pages_state_hold_cnt and pages_state_release_cnt as described in the commit message for future editors? > + > /* > * Data structure for allocation side > * > @@ -96,6 +99,8 @@ struct page_pool { > * TODO: Implement bulk return pages into this structure. > */ > struct ptr_ring ring; > + > + atomic_t pages_state_release_cnt; As we discussed this can change to per-cpu release variables if atomics end up hurting performance. I am fine with leaving this as-is and optimize if we spot any bottlenecks > }; > > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp); > @@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool) > > struct page_pool *page_pool_create(const struct page_pool_params *params); > > -void page_pool_destroy(struct page_pool *pool); > - > void __page_pool_free(struct page_pool *pool); > static inline void page_pool_free(struct page_pool *pool) > { > @@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, > __page_pool_put_page(pool, page, true); > } > > +/* API user MUST have disconnected alloc-side (not allowed to call > + * page_pool_alloc_pages()) before calling this. The free-side can > + * still run concurrently, to handle in-flight packet-pages. > + * > + * A request to shutdown can fail (with false) if there are still > + * in-flight packet-pages. > + */ > +bool __page_pool_request_shutdown(struct page_pool *pool); > +static inline bool page_pool_request_shutdown(struct page_pool *pool) > +{ > + /* When page_pool isn't compiled-in, net/core/xdp.c doesn't > + * allow registering MEM_TYPE_PAGE_POOL, but shield linker. > + */ > +#ifdef CONFIG_PAGE_POOL > + return __page_pool_request_shutdown(pool); > +#endif > +} > + > /* Disconnects a page (from a page_pool). API users can have a need > * to disconnect a page (from a page_pool), to allow it to be used as > * a regular page (that will eventually be returned to the normal > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > index 41391b5dc14c..8679e24fd665 100644 > --- a/net/core/page_pool.c > +++ b/net/core/page_pool.c > @@ -43,6 +43,8 @@ static int page_pool_init(struct page_pool *pool, > if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) > return -ENOMEM; > > + atomic_set(&pool->pages_state_release_cnt, 0); > + > return 0; > } > > @@ -151,6 +153,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool, > page->dma_addr = dma; > > skip_dma_map: > + /* Track how many pages are held 'in-flight' */ > + pool->pages_state_hold_cnt++; > + > /* When page just alloc'ed is should/must have refcnt 1. */ > return page; > } > @@ -173,6 +178,33 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp) > } > EXPORT_SYMBOL(page_pool_alloc_pages); > > +/* Calculate distance between two u32 values, valid if distance is below 2^(31) > + * https://en.wikipedia.org/wiki/Serial_number_arithmetic#General_Solution > + */ > +#define _distance(a, b) (s32)((a) - (b)) > + > +static s32 page_pool_inflight(struct page_pool *pool) > +{ > + u32 release_cnt = atomic_read(&pool->pages_state_release_cnt); > + u32 hold_cnt = READ_ONCE(pool->pages_state_hold_cnt); > + s32 distance; > + > + distance = _distance(hold_cnt, release_cnt); > + > + /* TODO: Add tracepoint here */ > + return distance; > +} > + > +static bool __page_pool_safe_to_destroy(struct page_pool *pool) > +{ > + s32 inflight = page_pool_inflight(pool); > + > + /* The distance should not be able to become negative */ > + WARN(inflight < 0, "Negative(%d) inflight packet-pages", inflight); > + > + return (inflight == 0); > +} > + > /* Cleanup page_pool state from page */ > static void __page_pool_clean_page(struct page_pool *pool, > struct page *page) > @@ -180,7 +212,7 @@ static void __page_pool_clean_page(struct page_pool *pool, > dma_addr_t dma; > > if (!(pool->p.flags & PP_FLAG_DMA_MAP)) > - return; > + goto skip_dma_unmap; > > dma = page->dma_addr; > /* DMA unmap */ > @@ -188,11 +220,16 @@ static void __page_pool_clean_page(struct page_pool *pool, > PAGE_SIZE << pool->p.order, pool->p.dma_dir, > DMA_ATTR_SKIP_CPU_SYNC); > page->dma_addr = 0; > +skip_dma_unmap: > + atomic_inc(&pool->pages_state_release_cnt); > } > > /* unmap the page and clean our state */ > void page_pool_unmap_page(struct page_pool *pool, struct page *page) > { > + /* When page is unmapped, this implies page will not be > + * returned to page_pool. > + */ > __page_pool_clean_page(pool, page); > } > EXPORT_SYMBOL(page_pool_unmap_page); > @@ -201,6 +238,7 @@ EXPORT_SYMBOL(page_pool_unmap_page); > static void __page_pool_return_page(struct page_pool *pool, struct page *page) > { > __page_pool_clean_page(pool, page); > + > put_page(page); > /* An optimization would be to call __free_pages(page, pool->p.order) > * knowing page is not part of page-cache (thus avoiding a > @@ -296,24 +334,17 @@ void __page_pool_free(struct page_pool *pool) > { > WARN(pool->alloc.count, "API usage violation"); > WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty"); > + WARN(!__page_pool_safe_to_destroy(pool), "still in-flight pages"); > > ptr_ring_cleanup(&pool->ring, NULL); > kfree(pool); > } > EXPORT_SYMBOL(__page_pool_free); > > -static void __page_pool_destroy_rcu(struct rcu_head *rcu) > -{ > - struct page_pool *pool; > - > - pool = container_of(rcu, struct page_pool, rcu); > - > - __page_pool_empty_ring(pool); > - __page_pool_free(pool); > -} > - > -/* Cleanup and release resources */ > -void page_pool_destroy(struct page_pool *pool) > +/* Request to shutdown: release pages cached by page_pool, and check > + * for in-flight pages > + */ > +bool __page_pool_request_shutdown(struct page_pool *pool) > { > struct page *page; > > @@ -331,7 +362,6 @@ void page_pool_destroy(struct page_pool *pool) > */ > __page_pool_empty_ring(pool); > > - /* An xdp_mem_allocator can still ref page_pool pointer */ > - call_rcu(&pool->rcu, __page_pool_destroy_rcu); > + return __page_pool_safe_to_destroy(pool); > } > -EXPORT_SYMBOL(page_pool_destroy); > +EXPORT_SYMBOL(__page_pool_request_shutdown); > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 179d90570afe..2b7bad227030 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -38,6 +38,7 @@ struct xdp_mem_allocator { > }; > struct rhash_head node; > struct rcu_head rcu; > + struct delayed_work defer_wq; > }; > > static u32 xdp_mem_id_hashfn(const void *data, u32 len, u32 seed) > @@ -79,13 +80,13 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu) > > xa = container_of(rcu, struct xdp_mem_allocator, rcu); > > + /* Allocator have indicated safe to remove before this is called */ > + if (xa->mem.type == MEM_TYPE_PAGE_POOL) > + page_pool_free(xa->page_pool); > + > /* Allow this ID to be reused */ > ida_simple_remove(&mem_id_pool, xa->mem.id); > > - /* Notice, driver is expected to free the *allocator, > - * e.g. page_pool, and MUST also use RCU free. > - */ > - > /* Poison memory */ > xa->mem.id = 0xFFFF; > xa->mem.type = 0xF0F0; > @@ -94,6 +95,46 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu) > kfree(xa); > } > > +bool __mem_id_disconnect(int id) > +{ > + struct xdp_mem_allocator *xa; > + bool safe_to_remove = true; > + > + mutex_lock(&mem_id_lock); > + > + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params); > + if (!xa) { > + mutex_unlock(&mem_id_lock); > + WARN(1, "Request remove non-existing id(%d), driver bug?", id); > + return true; > + } > + > + /* Detects in-flight packet-pages for page_pool */ > + if (xa->mem.type == MEM_TYPE_PAGE_POOL) > + safe_to_remove = page_pool_request_shutdown(xa->page_pool); > + > + if (safe_to_remove && > + !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params)) > + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free); > + > + mutex_unlock(&mem_id_lock); > + return safe_to_remove; > +} > + > +#define DEFER_TIME (msecs_to_jiffies(1000)) > + > +static void mem_id_disconnect_defer_retry(struct work_struct *wq) > +{ > + struct delayed_work *dwq = to_delayed_work(wq); > + struct xdp_mem_allocator *xa = container_of(dwq, typeof(*xa), defer_wq); > + > + if (__mem_id_disconnect(xa->mem.id)) > + return; > + > + /* Still not ready to be disconnected, retry later */ > + schedule_delayed_work(&xa->defer_wq, DEFER_TIME); > +} > + > void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq) Again as we already discussed, the naming for destroying a registered page_pool is mixed with xdp_ prefixes. That's fine but we'll need to document it properly once real drivers start using this > { > struct xdp_mem_allocator *xa; > @@ -112,16 +153,28 @@ void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq) > if (id == 0) > return; > > + if (__mem_id_disconnect(id)) > + return; > + > + /* Could not disconnect, defer new disconnect attempt to later */ > mutex_lock(&mem_id_lock); > > xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params); > - if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params)) > - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free); > + if (!xa) { > + mutex_unlock(&mem_id_lock); > + return; > + } > > + INIT_DELAYED_WORK(&xa->defer_wq, mem_id_disconnect_defer_retry); > mutex_unlock(&mem_id_lock); > + schedule_delayed_work(&xa->defer_wq, DEFER_TIME); > } > EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg_mem_model); > > +/* This unregister operation will also cleanup and destroy the > + * allocator. The page_pool_free() operation is first called when it's > + * safe to remove, possibly deferred to a workqueue. > + */ > void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq) > { > /* Simplify driver cleanup code paths, allow unreg "unused" */ > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: Hi, Jesper >This patch is needed before we can allow drivers to use page_pool for >DMA-mappings. Today with page_pool and XDP return API, it is possible to >remove the page_pool object (from rhashtable), while there are still >in-flight packet-pages. This is safely handled via RCU and failed lookups in >__xdp_return() fallback to call put_page(), when page_pool object is gone. >In-case page is still DMA mapped, this will result in page note getting >correctly DMA unmapped. > >To solve this, the page_pool is extended with tracking in-flight pages. And >XDP disconnect system queries page_pool and waits, via workqueue, for all >in-flight pages to be returned. > >To avoid killing performance when tracking in-flight pages, the implement >use two (unsigned) counters, that in placed on different cache-lines, and >can be used to deduct in-flight packets. This is done by mapping the >unsigned "sequence" counters onto signed Two's complement arithmetic >operations. This is e.g. used by kernel's time_after macros, described in >kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982. > >The trick is these two incrementing counters only need to be read and >compared, when checking if it's safe to free the page_pool structure. Which >will only happen when driver have disconnected RX/alloc side. Thus, on a >non-fast-path. > >It is chosen that page_pool tracking is also enabled for the non-DMA >use-case, as this can be used for statistics later. > >After this patch, using page_pool requires more strict resource "release", >e.g. via page_pool_release_page() that was introduced in this patchset, and >previous patches implement/fix this more strict requirement. > >Drivers no-longer call page_pool_destroy(). Drivers already call >xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will >attempt to disconnect the mem id, and if attempt fails schedule the >disconnect for later via delayed workqueue. > >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >--- > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 - > include/net/page_pool.h | 41 ++++++++++--- > net/core/page_pool.c | 62 +++++++++++++++----- > net/core/xdp.c | 65 +++++++++++++++++++-- > 4 files changed, 136 insertions(+), 35 deletions(-) > >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >index 2f647be292b6..6c9d4d7defbc 100644 >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c [...] >--- a/net/core/xdp.c >+++ b/net/core/xdp.c >@@ -38,6 +38,7 @@ struct xdp_mem_allocator { > }; > struct rhash_head node; > struct rcu_head rcu; >+ struct delayed_work defer_wq; > }; > > static u32 xdp_mem_id_hashfn(const void *data, u32 len, u32 seed) >@@ -79,13 +80,13 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu) > > xa = container_of(rcu, struct xdp_mem_allocator, rcu); > >+ /* Allocator have indicated safe to remove before this is called */ >+ if (xa->mem.type == MEM_TYPE_PAGE_POOL) >+ page_pool_free(xa->page_pool); >+ What would you recommend to do for the following situation: Same receive queue is shared between 2 network devices. The receive ring is filled by pages from page_pool, but you don't know the actual port (ndev) filling this ring, because a device is recognized only after packet is received. The API is so that xdp rxq is bind to network device, each frame has reference on it, so rxq ndev must be static. That means each netdev has it's own rxq instance even no need in it. Thus, after your changes, page must be returned to the pool it was taken from, or released from old pool and recycled in new one somehow. And that is inconvenience at least. It's hard to move pages between pools w/o performance penalty. No way to use common pool either, as unreg_rxq now drops the pool and 2 rxqa can't reference same pool.
On 6/15/2019 12:33 PM, Ivan Khoronzhuk wrote: > On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: > Hi, Jesper > >> This patch is needed before we can allow drivers to use page_pool for >> DMA-mappings. Today with page_pool and XDP return API, it is possible to >> remove the page_pool object (from rhashtable), while there are still >> in-flight packet-pages. This is safely handled via RCU and failed >> lookups in >> __xdp_return() fallback to call put_page(), when page_pool object is >> gone. >> In-case page is still DMA mapped, this will result in page note getting >> correctly DMA unmapped. >> >> To solve this, the page_pool is extended with tracking in-flight >> pages. And >> XDP disconnect system queries page_pool and waits, via workqueue, for all >> in-flight pages to be returned. >> >> To avoid killing performance when tracking in-flight pages, the implement >> use two (unsigned) counters, that in placed on different cache-lines, and >> can be used to deduct in-flight packets. This is done by mapping the >> unsigned "sequence" counters onto signed Two's complement arithmetic >> operations. This is e.g. used by kernel's time_after macros, described in >> kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in >> RFC1982. >> >> The trick is these two incrementing counters only need to be read and >> compared, when checking if it's safe to free the page_pool structure. >> Which >> will only happen when driver have disconnected RX/alloc side. Thus, on a >> non-fast-path. >> >> It is chosen that page_pool tracking is also enabled for the non-DMA >> use-case, as this can be used for statistics later. >> >> After this patch, using page_pool requires more strict resource >> "release", >> e.g. via page_pool_release_page() that was introduced in this >> patchset, and >> previous patches implement/fix this more strict requirement. >> >> Drivers no-longer call page_pool_destroy(). Drivers already call >> xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which >> will >> attempt to disconnect the mem id, and if attempt fails schedule the >> disconnect for later via delayed workqueue. >> >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> --- >> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 - >> include/net/page_pool.h | 41 ++++++++++--- >> net/core/page_pool.c | 62 >> +++++++++++++++----- >> net/core/xdp.c | 65 >> +++++++++++++++++++-- >> 4 files changed, 136 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> index 2f647be292b6..6c9d4d7defbc 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > [...] > >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -38,6 +38,7 @@ struct xdp_mem_allocator { >> }; >> struct rhash_head node; >> struct rcu_head rcu; >> + struct delayed_work defer_wq; >> }; >> >> static u32 xdp_mem_id_hashfn(const void *data, u32 len, u32 seed) >> @@ -79,13 +80,13 @@ static void __xdp_mem_allocator_rcu_free(struct >> rcu_head *rcu) >> >> xa = container_of(rcu, struct xdp_mem_allocator, rcu); >> >> + /* Allocator have indicated safe to remove before this is called */ >> + if (xa->mem.type == MEM_TYPE_PAGE_POOL) >> + page_pool_free(xa->page_pool); >> + > > What would you recommend to do for the following situation: > > Same receive queue is shared between 2 network devices. The receive ring is > filled by pages from page_pool, but you don't know the actual port (ndev) > filling this ring, because a device is recognized only after packet is > received. > > The API is so that xdp rxq is bind to network device, each frame has > reference > on it, so rxq ndev must be static. That means each netdev has it's own rxq > instance even no need in it. Thus, after your changes, page must be > returned to > the pool it was taken from, or released from old pool and recycled in > new one > somehow. > > And that is inconvenience at least. It's hard to move pages between > pools w/o > performance penalty. No way to use common pool either, as unreg_rxq now > drops > the pool and 2 rxqa can't reference same pool. > Within the single netdev, separate page_pool instances are anyway created for different RX rings, working under different NAPI's. So I don't understand your comment above about breaking some multi-netdev pool sharing use case... Tariq
On Sun, Jun 16, 2019 at 10:56:25AM +0000, Tariq Toukan wrote: Hi Tariq > > >On 6/15/2019 12:33 PM, Ivan Khoronzhuk wrote: >> On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: >> Hi, Jesper >> >>> This patch is needed before we can allow drivers to use page_pool for >>> DMA-mappings. Today with page_pool and XDP return API, it is possible to >>> remove the page_pool object (from rhashtable), while there are still >>> in-flight packet-pages. This is safely handled via RCU and failed >>> lookups in >>> __xdp_return() fallback to call put_page(), when page_pool object is >>> gone. >>> In-case page is still DMA mapped, this will result in page note getting >>> correctly DMA unmapped. >>> >>> To solve this, the page_pool is extended with tracking in-flight >>> pages. And >>> XDP disconnect system queries page_pool and waits, via workqueue, for all >>> in-flight pages to be returned. >>> >>> To avoid killing performance when tracking in-flight pages, the implement >>> use two (unsigned) counters, that in placed on different cache-lines, and >>> can be used to deduct in-flight packets. This is done by mapping the >>> unsigned "sequence" counters onto signed Two's complement arithmetic >>> operations. This is e.g. used by kernel's time_after macros, described in >>> kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in >>> RFC1982. >>> >>> The trick is these two incrementing counters only need to be read and >>> compared, when checking if it's safe to free the page_pool structure. >>> Which >>> will only happen when driver have disconnected RX/alloc side. Thus, on a >>> non-fast-path. >>> >>> It is chosen that page_pool tracking is also enabled for the non-DMA >>> use-case, as this can be used for statistics later. >>> >>> After this patch, using page_pool requires more strict resource >>> "release", >>> e.g. via page_pool_release_page() that was introduced in this >>> patchset, and >>> previous patches implement/fix this more strict requirement. >>> >>> Drivers no-longer call page_pool_destroy(). Drivers already call >>> xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which >>> will >>> attempt to disconnect the mem id, and if attempt fails schedule the >>> disconnect for later via delayed workqueue. >>> >>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>> --- >>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 - >>> include/net/page_pool.h | 41 ++++++++++--- >>> net/core/page_pool.c | 62 >>> +++++++++++++++----- >>> net/core/xdp.c | 65 >>> +++++++++++++++++++-- >>> 4 files changed, 136 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> index 2f647be292b6..6c9d4d7defbc 100644 >>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> >> [...] >> >>> --- a/net/core/xdp.c >>> +++ b/net/core/xdp.c >>> @@ -38,6 +38,7 @@ struct xdp_mem_allocator { >>> }; >>> struct rhash_head node; >>> struct rcu_head rcu; >>> + struct delayed_work defer_wq; >>> }; >>> >>> static u32 xdp_mem_id_hashfn(const void *data, u32 len, u32 seed) >>> @@ -79,13 +80,13 @@ static void __xdp_mem_allocator_rcu_free(struct >>> rcu_head *rcu) >>> >>> xa = container_of(rcu, struct xdp_mem_allocator, rcu); >>> >>> + /* Allocator have indicated safe to remove before this is called */ >>> + if (xa->mem.type == MEM_TYPE_PAGE_POOL) >>> + page_pool_free(xa->page_pool); >>> + >> >> What would you recommend to do for the following situation: >> >> Same receive queue is shared between 2 network devices. The receive ring is >> filled by pages from page_pool, but you don't know the actual port (ndev) >> filling this ring, because a device is recognized only after packet is >> received. >> >> The API is so that xdp rxq is bind to network device, each frame has >> reference >> on it, so rxq ndev must be static. That means each netdev has it's own rxq >> instance even no need in it. Thus, after your changes, page must be >> returned to >> the pool it was taken from, or released from old pool and recycled in >> new one >> somehow. >> >> And that is inconvenience at least. It's hard to move pages between >> pools w/o >> performance penalty. No way to use common pool either, as unreg_rxq now >> drops >> the pool and 2 rxqa can't reference same pool. >> > >Within the single netdev, separate page_pool instances are anyway >created for different RX rings, working under different NAPI's. The circumstances are so that same RX ring is shared between 2 netdevs... and netdev can be known only after descriptor/packet is received. Thus, while filling RX ring, there is no actual device, but when packet is received it has to be recycled to appropriate net device pool. Before this change there were no difference from which pool the page was allocated to fill RX ring, as there were no owner. After this change there is owner - netdev page pool. For cpsw the dma unmap is common for both netdevs and no difference who is freeing the page, but there is difference which pool it's freed to. So that, while filling RX ring the page is taken from page pool of ndev1, but packet is received for ndev2, it has to be later returned/recycled to page pool of ndev1, but when xdp buffer is handed over to xdp prog the xdp_rxq_info has reference on ndev2 ... And no way to predict the final ndev before packet is received, so no way to choose appropriate page pool as now it becomes page owner. So, while RX ring filling, the page/dma recycling is needed but should be some way to identify page owner only after receiving packet. Roughly speaking, something like: pool->pages_state_hold_cnt++; outside of page allocation API, after packet is received. and free of the counter while allocation (w/o owing the page).
Hi Ivan, Tariq, > >>>+ [...] > >> > >>What would you recommend to do for the following situation: > >> > >>Same receive queue is shared between 2 network devices. The receive ring is > >>filled by pages from page_pool, but you don't know the actual port (ndev) > >>filling this ring, because a device is recognized only after packet is > >>received. > >> > >>The API is so that xdp rxq is bind to network device, each frame has > >>reference > >>on it, so rxq ndev must be static. That means each netdev has it's own rxq > >>instance even no need in it. Thus, after your changes, page must be > >>returned to > >>the pool it was taken from, or released from old pool and recycled in > >>new one > >>somehow. > >> > >>And that is inconvenience at least. It's hard to move pages between > >>pools w/o > >>performance penalty. No way to use common pool either, as unreg_rxq now > >>drops > >>the pool and 2 rxqa can't reference same pool. > >> > > > >Within the single netdev, separate page_pool instances are anyway > >created for different RX rings, working under different NAPI's. > > The circumstances are so that same RX ring is shared between 2 > netdevs... and netdev can be known only after descriptor/packet is > received. Thus, while filling RX ring, there is no actual device, > but when packet is received it has to be recycled to appropriate > net device pool. Before this change there were no difference from > which pool the page was allocated to fill RX ring, as there were no > owner. After this change there is owner - netdev page pool. > > For cpsw the dma unmap is common for both netdevs and no difference > who is freeing the page, but there is difference which pool it's > freed to. Since 2 netdevs are sharing one queue you'll need locking right? (Assuming that the rx-irq per device can end up on a different core) We discussed that ideally page pools should be alocated per hardware queue. If you indeed need locking (and pay the performance penalty anyway) i wonder if there's anything preventing you from keeping the same principle, i.e allocate a pool per queue and handle the recycling to the proper ndev internally. That way only the first device will be responsible of allocating/recycling/maintaining the pool state. > > So that, while filling RX ring the page is taken from page pool of > ndev1, but packet is received for ndev2, it has to be later > returned/recycled to page pool of ndev1, but when xdp buffer is > handed over to xdp prog the xdp_rxq_info has reference on ndev2 ... > > And no way to predict the final ndev before packet is received, so no > way to choose appropriate page pool as now it becomes page owner. > > So, while RX ring filling, the page/dma recycling is needed but should > be some way to identify page owner only after receiving packet. > > Roughly speaking, something like: > > pool->pages_state_hold_cnt++; > > outside of page allocation API, after packet is received. > and free of the counter while allocation (w/o owing the page). If handling it internally is not an option maybe we can sort something out for special devices Thanks /Ilias
On Tue, Jun 18, 2019 at 04:30:12PM +0300, Ilias Apalodimas wrote: >Hi Ivan, Tariq, > >> >>>+ >[...] >> >> >> >>What would you recommend to do for the following situation: >> >> >> >>Same receive queue is shared between 2 network devices. The receive ring is >> >>filled by pages from page_pool, but you don't know the actual port (ndev) >> >>filling this ring, because a device is recognized only after packet is >> >>received. >> >> >> >>The API is so that xdp rxq is bind to network device, each frame has >> >>reference >> >>on it, so rxq ndev must be static. That means each netdev has it's own rxq >> >>instance even no need in it. Thus, after your changes, page must be >> >>returned to >> >>the pool it was taken from, or released from old pool and recycled in >> >>new one >> >>somehow. >> >> >> >>And that is inconvenience at least. It's hard to move pages between >> >>pools w/o >> >>performance penalty. No way to use common pool either, as unreg_rxq now >> >>drops >> >>the pool and 2 rxqa can't reference same pool. >> >> >> > >> >Within the single netdev, separate page_pool instances are anyway >> >created for different RX rings, working under different NAPI's. >> >> The circumstances are so that same RX ring is shared between 2 >> netdevs... and netdev can be known only after descriptor/packet is >> received. Thus, while filling RX ring, there is no actual device, >> but when packet is received it has to be recycled to appropriate >> net device pool. Before this change there were no difference from >> which pool the page was allocated to fill RX ring, as there were no >> owner. After this change there is owner - netdev page pool. >> >> For cpsw the dma unmap is common for both netdevs and no difference >> who is freeing the page, but there is difference which pool it's >> freed to. >Since 2 netdevs are sharing one queue you'll need locking right? >(Assuming that the rx-irq per device can end up on a different core) No, rx-irq is not per device, same queue is shared only for descs, farther it's separate queue with separate pool. Even rx-irq is per device, no issues, as I said, it has it's own page pool, every queue and every ndev, no need in locking, for pools... >We discussed that ideally page pools should be alocated per hardware queue. This is about one hw queue shared between ndevs. Page pool is separate for each hw queues and each ndevs ofc. >If you indeed need locking (and pay the performance penalty anyway) i wonder if >there's anything preventing you from keeping the same principle, i.e allocate a >pool per queue page pool is per queue. >pool per queue and handle the recycling to the proper ndev internally. >That way only the first device will be responsible of >allocating/recycling/maintaining the pool state. No. There is more dependencies then it looks like, see rxq_info ... The final recycling is ended not internally.
On Tue, 18 Jun 2019 15:54:33 +0300 Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: > On Sun, Jun 16, 2019 at 10:56:25AM +0000, Tariq Toukan wrote: > > > >On 6/15/2019 12:33 PM, Ivan Khoronzhuk wrote: > >> On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: [...] > >> > >> What would you recommend to do for the following situation: > >> > >> Same receive queue is shared between 2 network devices. The receive ring is > >> filled by pages from page_pool, but you don't know the actual port (ndev) > >> filling this ring, because a device is recognized only after packet is > >> received. > >> > >> The API is so that xdp rxq is bind to network device, each frame has > >> reference > >> on it, so rxq ndev must be static. That means each netdev has it's own rxq > >> instance even no need in it. Thus, after your changes, page must be > >> returned to > >> the pool it was taken from, or released from old pool and recycled in > >> new one > >> somehow. > >> > >> And that is inconvenience at least. It's hard to move pages between > >> pools w/o performance penalty. No way to use common pool either, > >> as unreg_rxq now drops the pool and 2 rxqa can't reference same > >> pool. > > > >Within the single netdev, separate page_pool instances are anyway > >created for different RX rings, working under different NAPI's. > > The circumstances are so that same RX ring is shared between 2 > netdevs... and netdev can be known only after descriptor/packet is > received. Thus, while filling RX ring, there is no actual device, > but when packet is received it has to be recycled to appropriate > net device pool. Before this change there were no difference from > which pool the page was allocated to fill RX ring, as there were no > owner. After this change there is owner - netdev page pool. It not really a dependency added in this patchset. A page_pool is strictly bound to a single RX-queue, for performance, as this allow us a NAPI fast-path return used for early drop (XDP_DROP). I can see that the API xdp_rxq_info_reg_mem_model() make it possible to call it on different xdp_rxq_info structs with the same page_pool pointer. But it was never intended to be used like that, and I consider it an API usage violation. I originally wanted to add the allocator pointer to xdp_rxq_info_reg() call, but the API was extended in different versions, so I didn't want to break users. I've actually tried hard to catch when drivers use the API wrong, via WARN(), but I guess you found a loop hole. Besides, we already have a dependency from the RX-queue to the netdev in the xdp_rxq_info structure. E.g. the xdp_rxq_info->dev is sort of central, and dereferenced by BPF-code to read xdp_md->ingress_ifindex, and also used by cpumap when creating SKBs. > For cpsw the dma unmap is common for both netdevs and no difference > who is freeing the page, but there is difference which pool it's > freed to. > > So that, while filling RX ring the page is taken from page pool of > ndev1, but packet is received for ndev2, it has to be later > returned/recycled to page pool of ndev1, but when xdp buffer is > handed over to xdp prog the xdp_rxq_info has reference on ndev2 ... > > And no way to predict the final ndev before packet is received, so no > way to choose appropriate page pool as now it becomes page owner. > > So, while RX ring filling, the page/dma recycling is needed but should > be some way to identify page owner only after receiving packet. > > Roughly speaking, something like: > > pool->pages_state_hold_cnt++; > > outside of page allocation API, after packet is received. Don't EVER manipulate the internal state outside of page allocation API. That kills the purpose of defining any API. > and free of the counter while allocation (w/o owing the page). You use-case of two netdev's sharing the same RX-queue sounds dubious, and very hardware specific. I'm not sure why we want to bend the APIs to support this? If we had to allow page_pool to be registered twice, via xdp_rxq_info_reg_mem_model() then I guess we could extend page_pool with a usage/users reference count, and then only really free the page_pool when refcnt reach zero. But it just seems and looks wrong (in the code) as the hole trick to get performance is to only have one user.
On Tue, Jun 18, 2019 at 05:19:51PM +0200, Jesper Dangaard Brouer wrote: Hi, Jesper > >On Tue, 18 Jun 2019 15:54:33 +0300 Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote: > >> On Sun, Jun 16, 2019 at 10:56:25AM +0000, Tariq Toukan wrote: >> > >> >On 6/15/2019 12:33 PM, Ivan Khoronzhuk wrote: >> >> On Thu, Jun 13, 2019 at 08:28:42PM +0200, Jesper Dangaard Brouer wrote: >[...] >> >> >> >> What would you recommend to do for the following situation: >> >> >> >> Same receive queue is shared between 2 network devices. The receive ring is >> >> filled by pages from page_pool, but you don't know the actual port (ndev) >> >> filling this ring, because a device is recognized only after packet is >> >> received. >> >> >> >> The API is so that xdp rxq is bind to network device, each frame has >> >> reference >> >> on it, so rxq ndev must be static. That means each netdev has it's own rxq >> >> instance even no need in it. Thus, after your changes, page must be >> >> returned to >> >> the pool it was taken from, or released from old pool and recycled in >> >> new one >> >> somehow. >> >> >> >> And that is inconvenience at least. It's hard to move pages between >> >> pools w/o performance penalty. No way to use common pool either, >> >> as unreg_rxq now drops the pool and 2 rxqa can't reference same >> >> pool. >> > >> >Within the single netdev, separate page_pool instances are anyway >> >created for different RX rings, working under different NAPI's. >> >> The circumstances are so that same RX ring is shared between 2 >> netdevs... and netdev can be known only after descriptor/packet is >> received. Thus, while filling RX ring, there is no actual device, >> but when packet is received it has to be recycled to appropriate >> net device pool. Before this change there were no difference from >> which pool the page was allocated to fill RX ring, as there were no >> owner. After this change there is owner - netdev page pool. > >It not really a dependency added in this patchset. A page_pool is >strictly bound to a single RX-queue, for performance, as this allow us >a NAPI fast-path return used for early drop (XDP_DROP). > >I can see that the API xdp_rxq_info_reg_mem_model() make it possible to >call it on different xdp_rxq_info structs with the same page_pool >pointer. But it was never intended to be used like that, and I >consider it an API usage violation. I originally wanted to add the >allocator pointer to xdp_rxq_info_reg() call, but the API was extended >in different versions, so I didn't want to break users. I've actually >tried hard to catch when drivers use the API wrong, via WARN(), but I >guess you found a loop hole. > >Besides, we already have a dependency from the RX-queue to the netdev >in the xdp_rxq_info structure. >E.g. the xdp_rxq_info->dev is sort of >central, and dereferenced by BPF-code to read xdp_md->ingress_ifindex, >and also used by cpumap when creating SKBs. Yes, That's I'm talking about. > > >> For cpsw the dma unmap is common for both netdevs and no difference >> who is freeing the page, but there is difference which pool it's >> freed to. >> >> So that, while filling RX ring the page is taken from page pool of >> ndev1, but packet is received for ndev2, it has to be later >> returned/recycled to page pool of ndev1, but when xdp buffer is >> handed over to xdp prog the xdp_rxq_info has reference on ndev2 ... >> >> And no way to predict the final ndev before packet is received, so no >> way to choose appropriate page pool as now it becomes page owner. >> >> So, while RX ring filling, the page/dma recycling is needed but should >> be some way to identify page owner only after receiving packet. >> >> Roughly speaking, something like: >> >> pool->pages_state_hold_cnt++; >> >> outside of page allocation API, after packet is received. > >Don't EVER manipulate the internal state outside of page allocation >API. That kills the purpose of defining any API. And I don't, that's rough propose that can be covered by newer API and here kind of material for explanation. This RX ring is filled by internal descriptors for 2 netdevs, it's common only internally. But to bind descriptor with a page, the page must be allocated from pool bind to ndev, In case of cpsw, ndev is identified only after the descriptor/packet is received. Then, it can be related to some ndev and thus pool. The example in question only shows what API needs to allow underneath. For this, the following is needed: 1) Allocate page from page pool, but w/o counter inc, so driver can became the owner and bind it with a descriptor and put it to RX ring for receiving a packet by h/w. 2) after a packet is received the counter needs to be inc for a pool the packet is destined for and let xdp infrastructure do the rest. That's it. I don't propose to change the API for those who doesn't require it, just let to use more detailed variant for those who can't use it in regular way. And it's not final decision would be nice to hear another constructive propositions. > >> and free of the counter while allocation (w/o owing the page). > >You use-case of two netdev's sharing the same RX-queue sounds dubious, >and very hardware specific. I'm not sure why we want to bend the APIs >to support this? The question is rather why not? The allocator is supposed to be used as one of the main generic solutions for XDP, if not say more, it should be able to cover most of the hardware, not only for hi speed solutions, but for other usecases. I don't propose to change main API, left it as is, just extend it that not only single ndev hardware can use it... I can add it myself if you don't want to bother. > If we had to allow page_pool to be registered twice, via >xdp_rxq_info_reg_mem_model() then I guess we could extend page_pool >with a usage/users reference count, and then only really free the >page_pool when refcnt reach zero. But it just seems and looks wrong >(in the code) as the hole trick to get performance is to only have one >user. I don't propose this. > >-- >Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer
On Tue, Jun 18, 2019 at 08:54:07PM +0300, Ivan Khoronzhuk wrote: Hi, Jesper >On Tue, Jun 18, 2019 at 05:19:51PM +0200, Jesper Dangaard Brouer wrote: > [...] >>If we had to allow page_pool to be registered twice, via >>xdp_rxq_info_reg_mem_model() then I guess we could extend page_pool >>with a usage/users reference count, and then only really free the >>page_pool when refcnt reach zero. But it just seems and looks wrong >>(in the code) as the hole trick to get performance is to only have one >>user. Let's try this variant. This might be used only in case if no choice. If no issues and provide appropriate comments in the code it can look more a while normal.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 2f647be292b6..6c9d4d7defbc 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq) } xdp_rxq_info_unreg(&rq->xdp_rxq); - if (rq->page_pool) - page_pool_destroy(rq->page_pool); - mlx5_wq_destroy(&rq->wq_ctrl); } diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 754d980700df..f09b3f1994e6 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -16,14 +16,16 @@ * page_pool_alloc_pages() call. Drivers should likely use * page_pool_dev_alloc_pages() replacing dev_alloc_pages(). * - * If page_pool handles DMA mapping (use page->private), then API user - * is responsible for invoking page_pool_put_page() once. In-case of - * elevated refcnt, the DMA state is released, assuming other users of - * the page will eventually call put_page(). + * API keeps track of in-flight pages, in-order to let API user know + * when it is safe to dealloactor page_pool object. Thus, API users + * must make sure to call page_pool_release_page() when a page is + * "leaving" the page_pool. Or call page_pool_put_page() where + * appropiate. For maintaining correct accounting. * - * If no DMA mapping is done, then it can act as shim-layer that - * fall-through to alloc_page. As no state is kept on the page, the - * regular put_page() call is sufficient. + * API user must only call page_pool_put_page() once on a page, as it + * will either recycle the page, or in case of elevated refcnt, it + * will release the DMA mapping and in-flight state accounting. We + * hope to lift this requirement in the future. */ #ifndef _NET_PAGE_POOL_H #define _NET_PAGE_POOL_H @@ -66,9 +68,10 @@ struct page_pool_params { }; struct page_pool { - struct rcu_head rcu; struct page_pool_params p; + u32 pages_state_hold_cnt; + /* * Data structure for allocation side * @@ -96,6 +99,8 @@ struct page_pool { * TODO: Implement bulk return pages into this structure. */ struct ptr_ring ring; + + atomic_t pages_state_release_cnt; }; struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp); @@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool) struct page_pool *page_pool_create(const struct page_pool_params *params); -void page_pool_destroy(struct page_pool *pool); - void __page_pool_free(struct page_pool *pool); static inline void page_pool_free(struct page_pool *pool) { @@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool, __page_pool_put_page(pool, page, true); } +/* API user MUST have disconnected alloc-side (not allowed to call + * page_pool_alloc_pages()) before calling this. The free-side can + * still run concurrently, to handle in-flight packet-pages. + * + * A request to shutdown can fail (with false) if there are still + * in-flight packet-pages. + */ +bool __page_pool_request_shutdown(struct page_pool *pool); +static inline bool page_pool_request_shutdown(struct page_pool *pool) +{ + /* When page_pool isn't compiled-in, net/core/xdp.c doesn't + * allow registering MEM_TYPE_PAGE_POOL, but shield linker. + */ +#ifdef CONFIG_PAGE_POOL + return __page_pool_request_shutdown(pool); +#endif +} + /* Disconnects a page (from a page_pool). API users can have a need * to disconnect a page (from a page_pool), to allow it to be used as * a regular page (that will eventually be returned to the normal diff --git a/net/core/page_pool.c b/net/core/page_pool.c index 41391b5dc14c..8679e24fd665 100644 --- a/net/core/page_pool.c +++ b/net/core/page_pool.c @@ -43,6 +43,8 @@ static int page_pool_init(struct page_pool *pool, if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0) return -ENOMEM; + atomic_set(&pool->pages_state_release_cnt, 0); + return 0; } @@ -151,6 +153,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool, page->dma_addr = dma; skip_dma_map: + /* Track how many pages are held 'in-flight' */ + pool->pages_state_hold_cnt++; + /* When page just alloc'ed is should/must have refcnt 1. */ return page; } @@ -173,6 +178,33 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp) } EXPORT_SYMBOL(page_pool_alloc_pages); +/* Calculate distance between two u32 values, valid if distance is below 2^(31) + * https://en.wikipedia.org/wiki/Serial_number_arithmetic#General_Solution + */ +#define _distance(a, b) (s32)((a) - (b)) + +static s32 page_pool_inflight(struct page_pool *pool) +{ + u32 release_cnt = atomic_read(&pool->pages_state_release_cnt); + u32 hold_cnt = READ_ONCE(pool->pages_state_hold_cnt); + s32 distance; + + distance = _distance(hold_cnt, release_cnt); + + /* TODO: Add tracepoint here */ + return distance; +} + +static bool __page_pool_safe_to_destroy(struct page_pool *pool) +{ + s32 inflight = page_pool_inflight(pool); + + /* The distance should not be able to become negative */ + WARN(inflight < 0, "Negative(%d) inflight packet-pages", inflight); + + return (inflight == 0); +} + /* Cleanup page_pool state from page */ static void __page_pool_clean_page(struct page_pool *pool, struct page *page) @@ -180,7 +212,7 @@ static void __page_pool_clean_page(struct page_pool *pool, dma_addr_t dma; if (!(pool->p.flags & PP_FLAG_DMA_MAP)) - return; + goto skip_dma_unmap; dma = page->dma_addr; /* DMA unmap */ @@ -188,11 +220,16 @@ static void __page_pool_clean_page(struct page_pool *pool, PAGE_SIZE << pool->p.order, pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC); page->dma_addr = 0; +skip_dma_unmap: + atomic_inc(&pool->pages_state_release_cnt); } /* unmap the page and clean our state */ void page_pool_unmap_page(struct page_pool *pool, struct page *page) { + /* When page is unmapped, this implies page will not be + * returned to page_pool. + */ __page_pool_clean_page(pool, page); } EXPORT_SYMBOL(page_pool_unmap_page); @@ -201,6 +238,7 @@ EXPORT_SYMBOL(page_pool_unmap_page); static void __page_pool_return_page(struct page_pool *pool, struct page *page) { __page_pool_clean_page(pool, page); + put_page(page); /* An optimization would be to call __free_pages(page, pool->p.order) * knowing page is not part of page-cache (thus avoiding a @@ -296,24 +334,17 @@ void __page_pool_free(struct page_pool *pool) { WARN(pool->alloc.count, "API usage violation"); WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty"); + WARN(!__page_pool_safe_to_destroy(pool), "still in-flight pages"); ptr_ring_cleanup(&pool->ring, NULL); kfree(pool); } EXPORT_SYMBOL(__page_pool_free); -static void __page_pool_destroy_rcu(struct rcu_head *rcu) -{ - struct page_pool *pool; - - pool = container_of(rcu, struct page_pool, rcu); - - __page_pool_empty_ring(pool); - __page_pool_free(pool); -} - -/* Cleanup and release resources */ -void page_pool_destroy(struct page_pool *pool) +/* Request to shutdown: release pages cached by page_pool, and check + * for in-flight pages + */ +bool __page_pool_request_shutdown(struct page_pool *pool) { struct page *page; @@ -331,7 +362,6 @@ void page_pool_destroy(struct page_pool *pool) */ __page_pool_empty_ring(pool); - /* An xdp_mem_allocator can still ref page_pool pointer */ - call_rcu(&pool->rcu, __page_pool_destroy_rcu); + return __page_pool_safe_to_destroy(pool); } -EXPORT_SYMBOL(page_pool_destroy); +EXPORT_SYMBOL(__page_pool_request_shutdown); diff --git a/net/core/xdp.c b/net/core/xdp.c index 179d90570afe..2b7bad227030 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -38,6 +38,7 @@ struct xdp_mem_allocator { }; struct rhash_head node; struct rcu_head rcu; + struct delayed_work defer_wq; }; static u32 xdp_mem_id_hashfn(const void *data, u32 len, u32 seed) @@ -79,13 +80,13 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu) xa = container_of(rcu, struct xdp_mem_allocator, rcu); + /* Allocator have indicated safe to remove before this is called */ + if (xa->mem.type == MEM_TYPE_PAGE_POOL) + page_pool_free(xa->page_pool); + /* Allow this ID to be reused */ ida_simple_remove(&mem_id_pool, xa->mem.id); - /* Notice, driver is expected to free the *allocator, - * e.g. page_pool, and MUST also use RCU free. - */ - /* Poison memory */ xa->mem.id = 0xFFFF; xa->mem.type = 0xF0F0; @@ -94,6 +95,46 @@ static void __xdp_mem_allocator_rcu_free(struct rcu_head *rcu) kfree(xa); } +bool __mem_id_disconnect(int id) +{ + struct xdp_mem_allocator *xa; + bool safe_to_remove = true; + + mutex_lock(&mem_id_lock); + + xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params); + if (!xa) { + mutex_unlock(&mem_id_lock); + WARN(1, "Request remove non-existing id(%d), driver bug?", id); + return true; + } + + /* Detects in-flight packet-pages for page_pool */ + if (xa->mem.type == MEM_TYPE_PAGE_POOL) + safe_to_remove = page_pool_request_shutdown(xa->page_pool); + + if (safe_to_remove && + !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params)) + call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free); + + mutex_unlock(&mem_id_lock); + return safe_to_remove; +} + +#define DEFER_TIME (msecs_to_jiffies(1000)) + +static void mem_id_disconnect_defer_retry(struct work_struct *wq) +{ + struct delayed_work *dwq = to_delayed_work(wq); + struct xdp_mem_allocator *xa = container_of(dwq, typeof(*xa), defer_wq); + + if (__mem_id_disconnect(xa->mem.id)) + return; + + /* Still not ready to be disconnected, retry later */ + schedule_delayed_work(&xa->defer_wq, DEFER_TIME); +} + void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq) { struct xdp_mem_allocator *xa; @@ -112,16 +153,28 @@ void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq) if (id == 0) return; + if (__mem_id_disconnect(id)) + return; + + /* Could not disconnect, defer new disconnect attempt to later */ mutex_lock(&mem_id_lock); xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params); - if (xa && !rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params)) - call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free); + if (!xa) { + mutex_unlock(&mem_id_lock); + return; + } + INIT_DELAYED_WORK(&xa->defer_wq, mem_id_disconnect_defer_retry); mutex_unlock(&mem_id_lock); + schedule_delayed_work(&xa->defer_wq, DEFER_TIME); } EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg_mem_model); +/* This unregister operation will also cleanup and destroy the + * allocator. The page_pool_free() operation is first called when it's + * safe to remove, possibly deferred to a workqueue. + */ void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq) { /* Simplify driver cleanup code paths, allow unreg "unused" */
This patch is needed before we can allow drivers to use page_pool for DMA-mappings. Today with page_pool and XDP return API, it is possible to remove the page_pool object (from rhashtable), while there are still in-flight packet-pages. This is safely handled via RCU and failed lookups in __xdp_return() fallback to call put_page(), when page_pool object is gone. In-case page is still DMA mapped, this will result in page note getting correctly DMA unmapped. To solve this, the page_pool is extended with tracking in-flight pages. And XDP disconnect system queries page_pool and waits, via workqueue, for all in-flight pages to be returned. To avoid killing performance when tracking in-flight pages, the implement use two (unsigned) counters, that in placed on different cache-lines, and can be used to deduct in-flight packets. This is done by mapping the unsigned "sequence" counters onto signed Two's complement arithmetic operations. This is e.g. used by kernel's time_after macros, described in kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982. The trick is these two incrementing counters only need to be read and compared, when checking if it's safe to free the page_pool structure. Which will only happen when driver have disconnected RX/alloc side. Thus, on a non-fast-path. It is chosen that page_pool tracking is also enabled for the non-DMA use-case, as this can be used for statistics later. After this patch, using page_pool requires more strict resource "release", e.g. via page_pool_release_page() that was introduced in this patchset, and previous patches implement/fix this more strict requirement. Drivers no-longer call page_pool_destroy(). Drivers already call xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will attempt to disconnect the mem id, and if attempt fails schedule the disconnect for later via delayed workqueue. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 - include/net/page_pool.h | 41 ++++++++++--- net/core/page_pool.c | 62 +++++++++++++++----- net/core/xdp.c | 65 +++++++++++++++++++-- 4 files changed, 136 insertions(+), 35 deletions(-)