[PATCH RFC 0/8] A possible proposal for intergating dmabuf to page pool

[PATCH RFC 0/8] A possible proposal for intergating dmabuf to page pool

[Yunsheng Lin]

This patchset is based on the [1] and [2], it is similar to
what patch [2] is doing in essence, the main differences is:
1. It reuses the 'struct page' to have more unified handling
   between normal page and devmem page for net stack.
2. It relies on the page->pp_frag_count to do reference counting
   instead of page->_refcount, in order to decouple the devmem
   page from mm subsystem.

As this patch is using normal memory page as devmem page as
prototyping, it is tested using simple iperf with some hack in
hns3 driver and in __skb_datagram_iter().

1. https://lkml.kernel.org/netdev/20230105214631.3939268-2-willy@infradead.org/
2. https://lore.kernel.org/all/20231106024413.2801438-1-almasrymina@google.com/

Jakub Kicinski (2):
  net: page_pool: factor out releasing DMA from releasing the page
  net: page_pool: create hooks for custom page providers

Mina Almasry (1):
  memory-provider: dmabuf devmem memory provider

Yunsheng Lin (5):
  skbuff: explicitize the semantics of skb_frag_fill_page_desc()
  skbuff: remove compound_head() related function calling
  skbuff: always try to do page pool frag reference counting
  net: hns3: temp hack for hns3 to use dmabuf memory provider
  net: temp hack for dmabuf page in __skb_datagram_iter()

 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c |   2 +-
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |   9 +-
 drivers/net/ethernet/sun/cassini.c            |   4 +-
 drivers/net/veth.c                            |   2 +-
 include/linux/skbuff.h                        |  26 ++-
 include/net/page_pool/types.h                 |  60 ++++++
 net/core/datagram.c                           |  10 +-
 net/core/page_pool.c                          | 197 ++++++++++++++++--
 net/core/skbuff.c                             |  10 +-
 net/tls/tls_device_fallback.c                 |   2 +-
 10 files changed, 282 insertions(+), 40 deletions(-)

-- 
2.33.0

[PATCH RFC 1/8] net: page_pool: factor out releasing DMA from releasing the page

[Yunsheng Lin]

From: Jakub Kicinski <kuba@kernel.org>

Releasing the DMA mapping will be useful for other types
of pages, so factor it out. Make sure compiler inlines it,
to avoid any regressions.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 net/core/page_pool.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 5e409b98aba0..578b6f2eeb46 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -514,21 +514,16 @@ static s32 page_pool_inflight(struct page_pool *pool)
 	return inflight;
 }
 
-/* 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
- * page-allocator via put_page).
- */
-static void page_pool_return_page(struct page_pool *pool, struct page *page)
+static __always_inline
+void __page_pool_release_page_dma(struct page_pool *pool, struct page *page)
 {
 	dma_addr_t dma;
-	int count;
 
 	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
 		/* Always account for inflight pages, even if we didn't
 		 * map them
 		 */
-		goto skip_dma_unmap;
+		return;
 
 	dma = page_pool_get_dma_addr(page);
 
@@ -537,7 +532,19 @@ static void page_pool_return_page(struct page_pool *pool, struct page *page)
 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
 	page_pool_set_dma_addr(page, 0);
-skip_dma_unmap:
+}
+
+/* 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
+ * page-allocator via put_page).
+ */
+void page_pool_return_page(struct page_pool *pool, struct page *page)
+{
+	int count;
+
+	__page_pool_release_page_dma(pool, page);
+
 	page_pool_clear_pp_info(page);
 
 	/* This may be the last page returned, releasing the pool, so
-- 
2.33.0

[PATCH RFC 2/8] net: page_pool: create hooks for custom page providers

[Yunsheng Lin]

From: Jakub Kicinski <kuba@kernel.org>

The page providers which try to reuse the same pages will
need to hold onto the ref, even if page gets released from
the pool - as in releasing the page from the pp just transfers
the "ownership" reference from pp to the provider, and provider
will wait for other references to be gone before feeding this
page back into the pool.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 include/net/page_pool/types.h | 19 ++++++++
 net/core/page_pool.c          | 81 +++++++++++++++++++++++++++++------
 2 files changed, 87 insertions(+), 13 deletions(-)

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 6fc5134095ed..5e4fcd45ba50 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -60,6 +60,8 @@ struct page_pool_params {
 	int		nid;
 	struct device	*dev;
 	struct napi_struct *napi;
+	u8		memory_provider;
+	void            *mp_priv;
 	enum dma_data_direction dma_dir;
 	unsigned int	max_len;
 	unsigned int	offset;
@@ -118,6 +120,20 @@ struct page_pool_stats {
 };
 #endif
 
+struct mem_provider;
+
+enum pp_memory_provider_type {
+	__PP_MP_NONE, /* Use system allocator directly */
+};
+
+struct pp_memory_provider_ops {
+	int (*init)(struct page_pool *pool);
+	void (*destroy)(struct page_pool *pool);
+	struct page *(*alloc_pages)(struct page_pool *pool, gfp_t gfp);
+	void (*release_page)(struct page_pool *pool, struct page *page);
+	void (*free_pages)(struct page_pool *pool, struct page *page);
+};
+
 struct page_pool {
 	struct page_pool_params p;
 
@@ -165,6 +181,9 @@ struct page_pool {
 	 */
 	struct ptr_ring ring;
 
+	const struct pp_memory_provider_ops *mp_ops;
+	void *mp_priv;
+
 #ifdef CONFIG_PAGE_POOL_STATS
 	/* recycle stats are per-cpu to avoid locking */
 	struct page_pool_recycle_stats __percpu *recycle_stats;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 578b6f2eeb46..6c502bea842b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -23,6 +23,8 @@
 
 #include <trace/events/page_pool.h>
 
+static DEFINE_STATIC_KEY_FALSE(page_pool_mem_providers);
+
 #define DEFER_TIME (msecs_to_jiffies(1000))
 #define DEFER_WARN_INTERVAL (60 * HZ)
 
@@ -172,6 +174,7 @@ static int page_pool_init(struct page_pool *pool,
 			  const struct page_pool_params *params)
 {
 	unsigned int ring_qsize = 1024; /* Default */
+	int err;
 
 	memcpy(&pool->p, params, sizeof(pool->p));
 
@@ -225,10 +228,34 @@ static int page_pool_init(struct page_pool *pool,
 	/* Driver calling page_pool_create() also call page_pool_destroy() */
 	refcount_set(&pool->user_cnt, 1);
 
+	switch (pool->p.memory_provider) {
+	case __PP_MP_NONE:
+		break;
+	default:
+		err = -EINVAL;
+		goto free_ptr_ring;
+	}
+
+	pool->mp_priv = pool->p.mp_priv;
+	if (pool->mp_ops) {
+		err = pool->mp_ops->init(pool);
+		if (err) {
+			pr_warn("%s() mem-provider init failed %d\n",
+				__func__, err);
+			goto free_ptr_ring;
+		}
+
+		static_branch_inc(&page_pool_mem_providers);
+	}
+
 	if (pool->p.flags & PP_FLAG_DMA_MAP)
 		get_device(pool->p.dev);
 
 	return 0;
+
+free_ptr_ring:
+	ptr_ring_cleanup(&pool->ring, NULL);
+	return err;
 }
 
 /**
@@ -257,18 +284,13 @@ EXPORT_SYMBOL(page_pool_create);
 
 static void page_pool_return_page(struct page_pool *pool, struct page *page);
 
-noinline
-static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
+static bool page_pool_page_in_pref_node(struct page_pool *pool, struct page *page)
 {
-	struct ptr_ring *r = &pool->ring;
-	struct page *page;
 	int pref_nid; /* preferred NUMA node */
 
-	/* Quicker fallback, avoid locks when ring is empty */
-	if (__ptr_ring_empty(r)) {
-		alloc_stat_inc(pool, empty);
-		return NULL;
-	}
+	/* Always assume page is in pref node for mem providers */
+	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
+		return true;
 
 	/* Softirq guarantee CPU and thus NUMA node is stable. This,
 	 * assumes CPU refilling driver RX-ring will also run RX-NAPI.
@@ -280,13 +302,31 @@ static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
 	pref_nid = numa_mem_id(); /* will be zero like page_to_nid() */
 #endif
 
+	if (page_to_nid(page) == pref_nid)
+		return true;
+
+	return false;
+}
+
+noinline
+static struct page *page_pool_refill_alloc_cache(struct page_pool *pool)
+{
+	struct ptr_ring *r = &pool->ring;
+	struct page *page;
+
+	/* Quicker fallback, avoid locks when ring is empty */
+	if (__ptr_ring_empty(r)) {
+		alloc_stat_inc(pool, empty);
+		return NULL;
+	}
+
 	/* Refill alloc array, but only if NUMA match */
 	do {
 		page = __ptr_ring_consume(r);
 		if (unlikely(!page))
 			break;
 
-		if (likely(page_to_nid(page) == pref_nid)) {
+		if (likely(page_pool_page_in_pref_node(pool, page))) {
 			pool->alloc.cache[pool->alloc.count++] = page;
 		} else {
 			/* NUMA mismatch;
@@ -490,7 +530,10 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
 		return page;
 
 	/* Slow-path: cache empty, do real allocation */
-	page = __page_pool_alloc_pages_slow(pool, gfp);
+	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
+		page = pool->mp_ops->alloc_pages(pool, gfp);
+	else
+		page = __page_pool_alloc_pages_slow(pool, gfp);
 	return page;
 }
 EXPORT_SYMBOL(page_pool_alloc_pages);
@@ -543,7 +586,10 @@ void page_pool_return_page(struct page_pool *pool, struct page *page)
 {
 	int count;
 
-	__page_pool_release_page_dma(pool, page);
+	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
+		pool->mp_ops->release_page(pool, page);
+	else
+		__page_pool_release_page_dma(pool, page);
 
 	page_pool_clear_pp_info(page);
 
@@ -553,7 +599,11 @@ void page_pool_return_page(struct page_pool *pool, struct page *page)
 	count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
 	trace_page_pool_state_release(pool, page, count);
 
-	put_page(page);
+	if (static_branch_unlikely(&page_pool_mem_providers) && pool->mp_ops)
+		pool->mp_ops->free_pages(pool, page);
+	else
+		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
 	 * __page_cache_release() call).
@@ -821,6 +871,11 @@ static void __page_pool_destroy(struct page_pool *pool)
 	if (pool->disconnect)
 		pool->disconnect(pool);
 
+	if (pool->mp_ops) {
+		pool->mp_ops->destroy(pool);
+		static_branch_dec(&page_pool_mem_providers);
+	}
+
 	ptr_ring_cleanup(&pool->ring, NULL);
 
 	if (pool->p.flags & PP_FLAG_DMA_MAP)
-- 
2.33.0

[PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

From: Mina Almasry <almasrymina@google.com>

Implement a memory provider that allocates dmabuf devmem page_pool_iovs.

Support of PP_FLAG_DMA_MAP and PP_FLAG_DMA_SYNC_DEV is omitted for
simplicity.

The provider receives a reference to the struct netdev_dmabuf_binding
via the pool->mp_priv pointer. The driver needs to set this pointer for
the provider in the page_pool_params.

The provider obtains a reference on the netdev_dmabuf_binding which
guarantees the binding and the underlying mapping remains alive until
the provider is destroyed.

Signed-off-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 include/net/page_pool/types.h | 28 +++++++++++
 net/core/page_pool.c          | 93 +++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)

diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 5e4fcd45ba50..52e4cf98ebc6 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -124,6 +124,7 @@ struct mem_provider;
 
 enum pp_memory_provider_type {
 	__PP_MP_NONE, /* Use system allocator directly */
+	PP_MP_DMABUF_DEVMEM, /* dmabuf devmem provider */
 };
 
 struct pp_memory_provider_ops {
@@ -134,6 +135,33 @@ struct pp_memory_provider_ops {
 	void (*free_pages)(struct page_pool *pool, struct page *page);
 };
 
+extern const struct pp_memory_provider_ops dmabuf_devmem_ops;
+
+struct page_pool_iov {
+	unsigned long res0;
+	unsigned long pp_magic;
+	struct page_pool *pp;
+	struct page *page;  /* dmabuf memory provider specific field */
+	unsigned long dma_addr;
+	atomic_long_t pp_frag_count;
+	unsigned int res1;
+	refcount_t _refcount;
+};
+
+#define PAGE_POOL_MATCH(pg, iov)				\
+	static_assert(offsetof(struct page, pg) ==		\
+		      offsetof(struct page_pool_iov, iov))
+PAGE_POOL_MATCH(flags, res0);
+PAGE_POOL_MATCH(pp_magic, pp_magic);
+PAGE_POOL_MATCH(pp, pp);
+PAGE_POOL_MATCH(_pp_mapping_pad, page);
+PAGE_POOL_MATCH(dma_addr, dma_addr);
+PAGE_POOL_MATCH(pp_frag_count, pp_frag_count);
+PAGE_POOL_MATCH(_mapcount, res1);
+PAGE_POOL_MATCH(_refcount, _refcount);
+#undef PAGE_POOL_MATCH
+static_assert(sizeof(struct page_pool_iov) <= sizeof(struct page));
+
 struct page_pool {
 	struct page_pool_params p;
 
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 6c502bea842b..1bd7a2306f09 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -231,6 +231,9 @@ static int page_pool_init(struct page_pool *pool,
 	switch (pool->p.memory_provider) {
 	case __PP_MP_NONE:
 		break;
+	case PP_MP_DMABUF_DEVMEM:
+		pool->mp_ops = &dmabuf_devmem_ops;
+		break;
 	default:
 		err = -EINVAL;
 		goto free_ptr_ring;
@@ -1010,3 +1013,93 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
 	}
 }
 EXPORT_SYMBOL(page_pool_update_nid);
+
+/*** "Dmabuf devmem memory provider" ***/
+
+static int mp_dmabuf_devmem_init(struct page_pool *pool)
+{
+	if (pool->p.flags & PP_FLAG_DMA_MAP ||
+	    pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+		return -EOPNOTSUPP;
+	return 0;
+}
+
+static struct page *mp_dmabuf_devmem_alloc_pages(struct page_pool *pool,
+						 gfp_t gfp)
+{
+	struct page_pool_iov *ppiov;
+	struct page *page;
+	dma_addr_t dma;
+
+	ppiov = kvmalloc(sizeof(*ppiov), gfp | __GFP_ZERO);
+	if (!ppiov)
+		return NULL;
+
+	page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
+	if (!page) {
+		kvfree(ppiov);
+		return NULL;
+	}
+
+	dma = dma_map_page_attrs(pool->p.dev, page, 0,
+				 (PAGE_SIZE << pool->p.order),
+				 pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
+						  DMA_ATTR_WEAK_ORDERING);
+	if (dma_mapping_error(pool->p.dev, dma)) {
+		put_page(page);
+		kvfree(ppiov);
+		return NULL;
+	}
+
+	ppiov->pp = pool;
+	ppiov->pp_magic = PP_SIGNATURE;
+	ppiov->page = page;
+	refcount_set(&ppiov->_refcount, 1);
+	page_pool_fragment_page((struct page *)ppiov, 1);
+	page_pool_set_dma_addr((struct page *)ppiov, dma);
+	pool->pages_state_hold_cnt++;
+	trace_page_pool_state_hold(pool, (struct page *)ppiov,
+				   pool->pages_state_hold_cnt);
+	return (struct page *)ppiov;
+}
+
+static void mp_dmabuf_devmem_destroy(struct page_pool *pool)
+{
+}
+
+static void mp_dmabuf_devmem_release_page(struct page_pool *pool,
+					  struct page *page)
+{
+	struct page_pool_iov *ppiov = (struct page_pool_iov *)page;
+	dma_addr_t dma;
+
+	dma = page_pool_get_dma_addr(page);
+
+	/* When page is unmapped, it cannot be returned to our pool */
+	dma_unmap_page_attrs(pool->p.dev, dma,
+			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
+			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
+	page_pool_set_dma_addr(page, 0);
+
+	put_page(ppiov->page);
+}
+
+static void mp_dmabuf_devmem_free_pages(struct page_pool *pool,
+					struct page *page)
+{
+	int count;
+
+	count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
+	trace_page_pool_state_release(pool, page, count);
+
+	kvfree(page);
+}
+
+const struct pp_memory_provider_ops dmabuf_devmem_ops = {
+	.init			= mp_dmabuf_devmem_init,
+	.destroy		= mp_dmabuf_devmem_destroy,
+	.alloc_pages		= mp_dmabuf_devmem_alloc_pages,
+	.release_page		= mp_dmabuf_devmem_release_page,
+	.free_pages		= mp_dmabuf_devmem_free_pages,
+};
+EXPORT_SYMBOL(dmabuf_devmem_ops);
-- 
2.33.0

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Mon, Nov 13, 2023 at 5:00 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> From: Mina Almasry <almasrymina@google.com>
>
> Implement a memory provider that allocates dmabuf devmem page_pool_iovs.
>
> Support of PP_FLAG_DMA_MAP and PP_FLAG_DMA_SYNC_DEV is omitted for
> simplicity.
>
> The provider receives a reference to the struct netdev_dmabuf_binding
> via the pool->mp_priv pointer. The driver needs to set this pointer for
> the provider in the page_pool_params.
>
> The provider obtains a reference on the netdev_dmabuf_binding which
> guarantees the binding and the underlying mapping remains alive until
> the provider is destroyed.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Kaiyuan Zhang <kaiyuanz@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>  include/net/page_pool/types.h | 28 +++++++++++
>  net/core/page_pool.c          | 93 +++++++++++++++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
>
> diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
> index 5e4fcd45ba50..52e4cf98ebc6 100644
> --- a/include/net/page_pool/types.h
> +++ b/include/net/page_pool/types.h
> @@ -124,6 +124,7 @@ struct mem_provider;
>
>  enum pp_memory_provider_type {
>         __PP_MP_NONE, /* Use system allocator directly */
> +       PP_MP_DMABUF_DEVMEM, /* dmabuf devmem provider */
>  };
>
>  struct pp_memory_provider_ops {
> @@ -134,6 +135,33 @@ struct pp_memory_provider_ops {
>         void (*free_pages)(struct page_pool *pool, struct page *page);
>  };
>
> +extern const struct pp_memory_provider_ops dmabuf_devmem_ops;
> +
> +struct page_pool_iov {
> +       unsigned long res0;
> +       unsigned long pp_magic;
> +       struct page_pool *pp;
> +       struct page *page;  /* dmabuf memory provider specific field */
> +       unsigned long dma_addr;
> +       atomic_long_t pp_frag_count;
> +       unsigned int res1;
> +       refcount_t _refcount;
> +};
> +
> +#define PAGE_POOL_MATCH(pg, iov)                               \
> +       static_assert(offsetof(struct page, pg) ==              \
> +                     offsetof(struct page_pool_iov, iov))
> +PAGE_POOL_MATCH(flags, res0);
> +PAGE_POOL_MATCH(pp_magic, pp_magic);
> +PAGE_POOL_MATCH(pp, pp);
> +PAGE_POOL_MATCH(_pp_mapping_pad, page);
> +PAGE_POOL_MATCH(dma_addr, dma_addr);
> +PAGE_POOL_MATCH(pp_frag_count, pp_frag_count);
> +PAGE_POOL_MATCH(_mapcount, res1);
> +PAGE_POOL_MATCH(_refcount, _refcount);
> +#undef PAGE_POOL_MATCH
> +static_assert(sizeof(struct page_pool_iov) <= sizeof(struct page));
> +

You're doing exactly what I think you're doing, and what was nacked in RFC v1.

You've converted 'struct page_pool_iov' to essentially become a
duplicate of 'struct page'. Then, you're casting page_pool_iov* into
struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
the mm stack into thinking dma-buf memory is a struct page.

RFC v1 was almost exactly the same, except instead of creating a
duplicate definition of struct page, it just allocated 'struct page'
instead of allocating another struct that is identical to struct page
and casting it into struct page.

I don't think what you're doing here reverses the nacks I got in RFC
v1. You also did not CC any dma-buf or mm people on this proposal that
would bring up these concerns again.

>  struct page_pool {
>         struct page_pool_params p;
>
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 6c502bea842b..1bd7a2306f09 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -231,6 +231,9 @@ static int page_pool_init(struct page_pool *pool,
>         switch (pool->p.memory_provider) {
>         case __PP_MP_NONE:
>                 break;
> +       case PP_MP_DMABUF_DEVMEM:
> +               pool->mp_ops = &dmabuf_devmem_ops;
> +               break;
>         default:
>                 err = -EINVAL;
>                 goto free_ptr_ring;
> @@ -1010,3 +1013,93 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
>         }
>  }
>  EXPORT_SYMBOL(page_pool_update_nid);
> +
> +/*** "Dmabuf devmem memory provider" ***/
> +
> +static int mp_dmabuf_devmem_init(struct page_pool *pool)
> +{
> +       if (pool->p.flags & PP_FLAG_DMA_MAP ||
> +           pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> +               return -EOPNOTSUPP;
> +       return 0;
> +}
> +
> +static struct page *mp_dmabuf_devmem_alloc_pages(struct page_pool *pool,
> +                                                gfp_t gfp)
> +{
> +       struct page_pool_iov *ppiov;
> +       struct page *page;
> +       dma_addr_t dma;
> +
> +       ppiov = kvmalloc(sizeof(*ppiov), gfp | __GFP_ZERO);
> +       if (!ppiov)
> +               return NULL;
> +
> +       page = alloc_pages_node(pool->p.nid, gfp, pool->p.order);
> +       if (!page) {
> +               kvfree(ppiov);
> +               return NULL;
> +       }
> +
> +       dma = dma_map_page_attrs(pool->p.dev, page, 0,
> +                                (PAGE_SIZE << pool->p.order),
> +                                pool->p.dma_dir, DMA_ATTR_SKIP_CPU_SYNC |
> +                                                 DMA_ATTR_WEAK_ORDERING);
> +       if (dma_mapping_error(pool->p.dev, dma)) {
> +               put_page(page);
> +               kvfree(ppiov);
> +               return NULL;
> +       }
> +
> +       ppiov->pp = pool;
> +       ppiov->pp_magic = PP_SIGNATURE;
> +       ppiov->page = page;
> +       refcount_set(&ppiov->_refcount, 1);
> +       page_pool_fragment_page((struct page *)ppiov, 1);
> +       page_pool_set_dma_addr((struct page *)ppiov, dma);
> +       pool->pages_state_hold_cnt++;
> +       trace_page_pool_state_hold(pool, (struct page *)ppiov,
> +                                  pool->pages_state_hold_cnt);
> +       return (struct page *)ppiov;
> +}
> +
> +static void mp_dmabuf_devmem_destroy(struct page_pool *pool)
> +{
> +}
> +
> +static void mp_dmabuf_devmem_release_page(struct page_pool *pool,
> +                                         struct page *page)
> +{
> +       struct page_pool_iov *ppiov = (struct page_pool_iov *)page;
> +       dma_addr_t dma;
> +
> +       dma = page_pool_get_dma_addr(page);
> +
> +       /* When page is unmapped, it cannot be returned to our pool */
> +       dma_unmap_page_attrs(pool->p.dev, dma,
> +                            PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> +                            DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
> +       page_pool_set_dma_addr(page, 0);
> +
> +       put_page(ppiov->page);
> +}
> +
> +static void mp_dmabuf_devmem_free_pages(struct page_pool *pool,
> +                                       struct page *page)
> +{
> +       int count;
> +
> +       count = atomic_inc_return_relaxed(&pool->pages_state_release_cnt);
> +       trace_page_pool_state_release(pool, page, count);
> +
> +       kvfree(page);
> +}
> +
> +const struct pp_memory_provider_ops dmabuf_devmem_ops = {
> +       .init                   = mp_dmabuf_devmem_init,
> +       .destroy                = mp_dmabuf_devmem_destroy,
> +       .alloc_pages            = mp_dmabuf_devmem_alloc_pages,
> +       .release_page           = mp_dmabuf_devmem_release_page,
> +       .free_pages             = mp_dmabuf_devmem_free_pages,
> +};
> +EXPORT_SYMBOL(dmabuf_devmem_ops);
> --
> 2.33.0
>


-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Jakub Kicinski]

On Mon, 13 Nov 2023 05:42:16 -0800 Mina Almasry wrote:
> You're doing exactly what I think you're doing, and what was nacked in RFC v1.
> 
> You've converted 'struct page_pool_iov' to essentially become a
> duplicate of 'struct page'. Then, you're casting page_pool_iov* into
> struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
> mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
> the mm stack into thinking dma-buf memory is a struct page.
> 
> RFC v1 was almost exactly the same, except instead of creating a
> duplicate definition of struct page, it just allocated 'struct page'
> instead of allocating another struct that is identical to struct page
> and casting it into struct page.
> 
> I don't think what you're doing here reverses the nacks I got in RFC
> v1. You also did not CC any dma-buf or mm people on this proposal that
> would bring up these concerns again.

Right, but the mirror struct has some appeal to a non-mm person like
myself. The problem IIUC is that this patch is the wrong way around, we
should be converting everyone who can deal with non-host mem to struct
page_pool_iov. Using page_address() on ppiov which hns3 seems to do in
this series does not compute for me.

Then we can turn the existing non-iov helpers to be a thin wrapper with
just a cast from struct page to struct page_pool_iov, and a call of the
iov helper. Again - never cast the other way around.

Also I think this conversion can be done completely separately from the
mem provider changes. Just add struct page_pool_iov and start using it.

Does that make more sense?

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

+cc Christian, Jason and Willy

On 2023/11/14 7:05, Jakub Kicinski wrote:
> On Mon, 13 Nov 2023 05:42:16 -0800 Mina Almasry wrote:
>> You're doing exactly what I think you're doing, and what was nacked in RFC v1.
>>
>> You've converted 'struct page_pool_iov' to essentially become a
>> duplicate of 'struct page'. Then, you're casting page_pool_iov* into
>> struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
>> mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
>> the mm stack into thinking dma-buf memory is a struct page.

Yes, something like above, but I am not sure about the 'fooled the mm
stack into thinking dma-buf memory is a struct page' part, because:
1. We never let the 'struct page' for devmem leaking out of net stacking
   through the 'not kmap()able and not readable' checking in your patchset.
2. We inititiate page->_refcount for devmem to one and it remains as one,
   we will never call page_ref_inc()/page_ref_dec()/get_page()/put_page(),
   instead, we use page pool's pp_frag_count to do reference counting for
   devmem page in patch 6.

>>
>> RFC v1 was almost exactly the same, except instead of creating a
>> duplicate definition of struct page, it just allocated 'struct page'
>> instead of allocating another struct that is identical to struct page
>> and casting it into struct page.

Perhaps it is more accurate to say this is something between RFC v1 and
RFC v3, in order to decouple 'struct page' for devmem from mm subsystem,
but still have most unified handling for both normal memory and devmem
in page pool and net stack.

The main difference between this patchset and RFC v1:
1. The mm subsystem is not supposed to see the 'struct page' for devmem
   in this patchset, I guess we could say it is decoupled from the mm
   subsystem even though we still call PageTail()/page_ref_count()/
   page_is_pfmemalloc() on 'struct page' for devmem.

The main difference between this patchset and RFC v3:
1. It reuses the 'struct page' to have more unified handling between
   normal page and devmem page for net stack.
2. It relies on the page->pp_frag_count to do reference counting.

>>
>> I don't think what you're doing here reverses the nacks I got in RFC
>> v1. You also did not CC any dma-buf or mm people on this proposal that
>> would bring up these concerns again.
> 
> Right, but the mirror struct has some appeal to a non-mm person like
> myself. The problem IIUC is that this patch is the wrong way around, we
> should be converting everyone who can deal with non-host mem to struct
> page_pool_iov. Using page_address() on ppiov which hns3 seems to do in
> this series does not compute for me.

The hacking use of ppiov in hns3 is only used to do the some prototype
testing, so ignore it.

> 
> Then we can turn the existing non-iov helpers to be a thin wrapper with
> just a cast from struct page to struct page_pool_iov, and a call of the
> iov helper. Again - never cast the other way around.

I am agreed that a cast from struct page to struct page_pool_iov is allowed,
but a cast from struct page_pool_iov to struct page is not allowed if I am
understanding you correctly.

Before we can also completely decouple 'struct page' allocated using buddy
allocator directly from mm subsystem in netstack, below is what I have in
mind in order to support different memory provider.

                                +--------------+
                                |   Netstack   |
                                |'struct page' |
                                +--------------+
                                        ^
                                        |
                                        |
                                        v
                              +---------------------+
+----------------------+      |                     |      +---------------+
|      devmem MP       |<---->|     Page pool       |----->|    **** MP    |
|'struct page_pool_iov'|      |   'struct page'     |      |'struct **_iov'|
+----------------------+      |                     |      +---------------+
                              +---------------------+
                                        ^
                                        |
                                        |
                                        v
                                +---------------+
                                |    Driver     |
                                | 'struct page' |
                                +---------------+

I would expect net stack, page pool, driver still see the 'struct page',
only memory provider see the specific struct for itself, for the above,
devmem memory provider sees the 'struct page_pool_iov'.

The reason I still expect driver to see the 'struct page' is that driver
will still need to support normal memory besides devmem.

> 
> Also I think this conversion can be done completely separately from the
> mem provider changes. Just add struct page_pool_iov and start using it.

I am not sure I understand what does "Just add struct page_pool_iov and
start using it" mean yet.

> 
> Does that make more sense?
> 
> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Tue, Nov 14, 2023 at 12:23 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> +cc Christian, Jason and Willy
>
> On 2023/11/14 7:05, Jakub Kicinski wrote:
> > On Mon, 13 Nov 2023 05:42:16 -0800 Mina Almasry wrote:
> >> You're doing exactly what I think you're doing, and what was nacked in RFC v1.
> >>
> >> You've converted 'struct page_pool_iov' to essentially become a
> >> duplicate of 'struct page'. Then, you're casting page_pool_iov* into
> >> struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
> >> mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
> >> the mm stack into thinking dma-buf memory is a struct page.
>
> Yes, something like above, but I am not sure about the 'fooled the mm
> stack into thinking dma-buf memory is a struct page' part, because:
> 1. We never let the 'struct page' for devmem leaking out of net stacking
>    through the 'not kmap()able and not readable' checking in your patchset.

RFC never used dma-buf pages outside the net stack, so that is the same.

You are not able to get rid of the 'net kmap()able and not readable'
checking with this approach, because dma-buf memory is fundamentally
unkmapable and unreadable. This approach would still need
skb_frags_not_readable checks in net stack, so that is also the same.

> 2. We inititiate page->_refcount for devmem to one and it remains as one,
>    we will never call page_ref_inc()/page_ref_dec()/get_page()/put_page(),
>    instead, we use page pool's pp_frag_count to do reference counting for
>    devmem page in patch 6.
>

I'm not sure that moves the needle in terms of allowing dma-buf
memory to look like struct pages.

> >>
> >> RFC v1 was almost exactly the same, except instead of creating a
> >> duplicate definition of struct page, it just allocated 'struct page'
> >> instead of allocating another struct that is identical to struct page
> >> and casting it into struct page.
>
> Perhaps it is more accurate to say this is something between RFC v1 and
> RFC v3, in order to decouple 'struct page' for devmem from mm subsystem,
> but still have most unified handling for both normal memory and devmem
> in page pool and net stack.
>
> The main difference between this patchset and RFC v1:
> 1. The mm subsystem is not supposed to see the 'struct page' for devmem
>    in this patchset, I guess we could say it is decoupled from the mm
>    subsystem even though we still call PageTail()/page_ref_count()/
>    page_is_pfmemalloc() on 'struct page' for devmem.
>

In this patchset you pretty much allocate a struct page for your
dma-buf memory, and then cast it into a struct page, so all the mm
calls in page_pool.c are seeing a struct page when it's really dma-buf
memory.

'even though we still call
PageTail()/page_ref_count()/page_is_pfmemalloc() on 'struct page' for
devmem' is basically making dma-buf memory look like struct pages.

Actually because you put the 'strtuct page for devmem' in
skb->bv_frag, the net stack will grab the 'struct page' for devmem
using skb_frag_page() then call things like page_address(), kmap,
get_page, put_page, etc, etc, etc.

> The main difference between this patchset and RFC v3:
> 1. It reuses the 'struct page' to have more unified handling between
>    normal page and devmem page for net stack.

This is what was nacked in RFC v1.

> 2. It relies on the page->pp_frag_count to do reference counting.
>

I don't see you change any of the page_ref_* calls in page_pool.c, for
example this one:

https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601

So the reference the page_pool is seeing is actually page->_refcount,
not page->pp_frag_count? I'm confused here. Is this a bug in the
patchset?

> >>
> >> I don't think what you're doing here reverses the nacks I got in RFC
> >> v1. You also did not CC any dma-buf or mm people on this proposal that
> >> would bring up these concerns again.
> >
> > Right, but the mirror struct has some appeal to a non-mm person like
> > myself. The problem IIUC is that this patch is the wrong way around, we
> > should be converting everyone who can deal with non-host mem to struct
> > page_pool_iov. Using page_address() on ppiov which hns3 seems to do in
> > this series does not compute for me.
>
> The hacking use of ppiov in hns3 is only used to do the some prototype
> testing, so ignore it.
>
> >
> > Then we can turn the existing non-iov helpers to be a thin wrapper with
> > just a cast from struct page to struct page_pool_iov, and a call of the
> > iov helper. Again - never cast the other way around.
>
> I am agreed that a cast from struct page to struct page_pool_iov is allowed,
> but a cast from struct page_pool_iov to struct page is not allowed if I am
> understanding you correctly.
>
> Before we can also completely decouple 'struct page' allocated using buddy
> allocator directly from mm subsystem in netstack, below is what I have in
> mind in order to support different memory provider.
>
>                                 +--------------+
>                                 |   Netstack   |
>                                 |'struct page' |
>                                 +--------------+
>                                         ^
>                                         |
>                                         |
>                                         v
>                               +---------------------+
> +----------------------+      |                     |      +---------------+
> |      devmem MP       |<---->|     Page pool       |----->|    **** MP    |
> |'struct page_pool_iov'|      |   'struct page'     |      |'struct **_iov'|
> +----------------------+      |                     |      +---------------+
>                               +---------------------+
>                                         ^
>                                         |
>                                         |
>                                         v
>                                 +---------------+
>                                 |    Driver     |
>                                 | 'struct page' |
>                                 +---------------+
>
> I would expect net stack, page pool, driver still see the 'struct page',
> only memory provider see the specific struct for itself, for the above,
> devmem memory provider sees the 'struct page_pool_iov'.
>
> The reason I still expect driver to see the 'struct page' is that driver
> will still need to support normal memory besides devmem.
>
> >
> > Also I think this conversion can be done completely separately from the
> > mem provider changes. Just add struct page_pool_iov and start using it.
>
> I am not sure I understand what does "Just add struct page_pool_iov and
> start using it" mean yet.
>
> >
> > Does that make more sense?
> >
> > .
> >



--
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/14 20:21, Mina Almasry wrote:
> On Tue, Nov 14, 2023 at 12:23 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> +cc Christian, Jason and Willy
>>
>> On 2023/11/14 7:05, Jakub Kicinski wrote:
>>> On Mon, 13 Nov 2023 05:42:16 -0800 Mina Almasry wrote:
>>>> You're doing exactly what I think you're doing, and what was nacked in RFC v1.
>>>>
>>>> You've converted 'struct page_pool_iov' to essentially become a
>>>> duplicate of 'struct page'. Then, you're casting page_pool_iov* into
>>>> struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
>>>> mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
>>>> the mm stack into thinking dma-buf memory is a struct page.
>>
>> Yes, something like above, but I am not sure about the 'fooled the mm
>> stack into thinking dma-buf memory is a struct page' part, because:
>> 1. We never let the 'struct page' for devmem leaking out of net stacking
>>    through the 'not kmap()able and not readable' checking in your patchset.
> 
> RFC never used dma-buf pages outside the net stack, so that is the same.
> 
> You are not able to get rid of the 'net kmap()able and not readable'
> checking with this approach, because dma-buf memory is fundamentally
> unkmapable and unreadable. This approach would still need
> skb_frags_not_readable checks in net stack, so that is also the same.

Yes, I am agreed that checking is still needed whatever the proposal is.

> 
>> 2. We inititiate page->_refcount for devmem to one and it remains as one,
>>    we will never call page_ref_inc()/page_ref_dec()/get_page()/put_page(),
>>    instead, we use page pool's pp_frag_count to do reference counting for
>>    devmem page in patch 6.
>>
> 
> I'm not sure that moves the needle in terms of allowing dma-buf
> memory to look like struct pages.
> 
>>>>
>>>> RFC v1 was almost exactly the same, except instead of creating a
>>>> duplicate definition of struct page, it just allocated 'struct page'
>>>> instead of allocating another struct that is identical to struct page
>>>> and casting it into struct page.
>>
>> Perhaps it is more accurate to say this is something between RFC v1 and
>> RFC v3, in order to decouple 'struct page' for devmem from mm subsystem,
>> but still have most unified handling for both normal memory and devmem
>> in page pool and net stack.
>>
>> The main difference between this patchset and RFC v1:
>> 1. The mm subsystem is not supposed to see the 'struct page' for devmem
>>    in this patchset, I guess we could say it is decoupled from the mm
>>    subsystem even though we still call PageTail()/page_ref_count()/
>>    page_is_pfmemalloc() on 'struct page' for devmem.
>>
> 
> In this patchset you pretty much allocate a struct page for your
> dma-buf memory, and then cast it into a struct page, so all the mm
> calls in page_pool.c are seeing a struct page when it's really dma-buf
> memory.
> 
> 'even though we still call
> PageTail()/page_ref_count()/page_is_pfmemalloc() on 'struct page' for
> devmem' is basically making dma-buf memory look like struct pages.
> 
> Actually because you put the 'strtuct page for devmem' in
> skb->bv_frag, the net stack will grab the 'struct page' for devmem
> using skb_frag_page() then call things like page_address(), kmap,
> get_page, put_page, etc, etc, etc.

Yes, as above, skb_frags_not_readable() checking is still needed for
kmap() and page_address().

get_page, put_page related calling is avoided in page_pool_frag_ref()
and napi_pp_put_page() for devmem page as the above checking is true
for devmem page:
(pp_iov->pp_magic & ~0x3UL) == PP_SIGNATURE

> 
>> The main difference between this patchset and RFC v3:
>> 1. It reuses the 'struct page' to have more unified handling between
>>    normal page and devmem page for net stack.
> 
> This is what was nacked in RFC v1.
> 
>> 2. It relies on the page->pp_frag_count to do reference counting.
>>
> 
> I don't see you change any of the page_ref_* calls in page_pool.c, for
> example this one:
> 
> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601
> 
> So the reference the page_pool is seeing is actually page->_refcount,
> not page->pp_frag_count? I'm confused here. Is this a bug in the
> patchset?

page->_refcount is the same as page_pool_iov->_refcount for devmem, which
is ensured by the 'PAGE_POOL_MATCH(_refcount, _refcount);', and
page_pool_iov->_refcount is set to one in mp_dmabuf_devmem_alloc_pages()
by calling 'refcount_set(&ppiov->_refcount, 1)' and always remains as one.

So the 'page_ref_count(page) == 1' checking is always true for devmem page.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Tue, Nov 14, 2023 at 4:49 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2023/11/14 20:21, Mina Almasry wrote:
> > On Tue, Nov 14, 2023 at 12:23 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>
> >> +cc Christian, Jason and Willy
> >>
> >> On 2023/11/14 7:05, Jakub Kicinski wrote:
> >>> On Mon, 13 Nov 2023 05:42:16 -0800 Mina Almasry wrote:
> >>>> You're doing exactly what I think you're doing, and what was nacked in RFC v1.
> >>>>
> >>>> You've converted 'struct page_pool_iov' to essentially become a
> >>>> duplicate of 'struct page'. Then, you're casting page_pool_iov* into
> >>>> struct page* in mp_dmabuf_devmem_alloc_pages(), then, you're calling
> >>>> mm APIs like page_ref_*() on the page_pool_iov* because you've fooled
> >>>> the mm stack into thinking dma-buf memory is a struct page.
> >>
> >> Yes, something like above, but I am not sure about the 'fooled the mm
> >> stack into thinking dma-buf memory is a struct page' part, because:
> >> 1. We never let the 'struct page' for devmem leaking out of net stacking
> >>    through the 'not kmap()able and not readable' checking in your patchset.
> >
> > RFC never used dma-buf pages outside the net stack, so that is the same.
> >
> > You are not able to get rid of the 'net kmap()able and not readable'
> > checking with this approach, because dma-buf memory is fundamentally
> > unkmapable and unreadable. This approach would still need
> > skb_frags_not_readable checks in net stack, so that is also the same.
>
> Yes, I am agreed that checking is still needed whatever the proposal is.
>
> >
> >> 2. We inititiate page->_refcount for devmem to one and it remains as one,
> >>    we will never call page_ref_inc()/page_ref_dec()/get_page()/put_page(),
> >>    instead, we use page pool's pp_frag_count to do reference counting for
> >>    devmem page in patch 6.
> >>
> >
> > I'm not sure that moves the needle in terms of allowing dma-buf
> > memory to look like struct pages.
> >
> >>>>
> >>>> RFC v1 was almost exactly the same, except instead of creating a
> >>>> duplicate definition of struct page, it just allocated 'struct page'
> >>>> instead of allocating another struct that is identical to struct page
> >>>> and casting it into struct page.
> >>
> >> Perhaps it is more accurate to say this is something between RFC v1 and
> >> RFC v3, in order to decouple 'struct page' for devmem from mm subsystem,
> >> but still have most unified handling for both normal memory and devmem
> >> in page pool and net stack.
> >>
> >> The main difference between this patchset and RFC v1:
> >> 1. The mm subsystem is not supposed to see the 'struct page' for devmem
> >>    in this patchset, I guess we could say it is decoupled from the mm
> >>    subsystem even though we still call PageTail()/page_ref_count()/
> >>    page_is_pfmemalloc() on 'struct page' for devmem.
> >>
> >
> > In this patchset you pretty much allocate a struct page for your
> > dma-buf memory, and then cast it into a struct page, so all the mm
> > calls in page_pool.c are seeing a struct page when it's really dma-buf
> > memory.
> >
> > 'even though we still call
> > PageTail()/page_ref_count()/page_is_pfmemalloc() on 'struct page' for
> > devmem' is basically making dma-buf memory look like struct pages.
> >
> > Actually because you put the 'strtuct page for devmem' in
> > skb->bv_frag, the net stack will grab the 'struct page' for devmem
> > using skb_frag_page() then call things like page_address(), kmap,
> > get_page, put_page, etc, etc, etc.
>
> Yes, as above, skb_frags_not_readable() checking is still needed for
> kmap() and page_address().
>
> get_page, put_page related calling is avoided in page_pool_frag_ref()
> and napi_pp_put_page() for devmem page as the above checking is true
> for devmem page:
> (pp_iov->pp_magic & ~0x3UL) == PP_SIGNATURE
>

So, devmem needs special handling with if statement for refcounting,
even after using struct pages for devmem, which is not allowed (IIUC
the dma-buf maintainer).

> >
> >> The main difference between this patchset and RFC v3:
> >> 1. It reuses the 'struct page' to have more unified handling between
> >>    normal page and devmem page for net stack.
> >
> > This is what was nacked in RFC v1.
> >
> >> 2. It relies on the page->pp_frag_count to do reference counting.
> >>
> >
> > I don't see you change any of the page_ref_* calls in page_pool.c, for
> > example this one:
> >
> > https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601
> >
> > So the reference the page_pool is seeing is actually page->_refcount,
> > not page->pp_frag_count? I'm confused here. Is this a bug in the
> > patchset?
>
> page->_refcount is the same as page_pool_iov->_refcount for devmem, which
> is ensured by the 'PAGE_POOL_MATCH(_refcount, _refcount);', and
> page_pool_iov->_refcount is set to one in mp_dmabuf_devmem_alloc_pages()
> by calling 'refcount_set(&ppiov->_refcount, 1)' and always remains as one.
>
> So the 'page_ref_count(page) == 1' checking is always true for devmem page.

Which, of course, is a bug in the patchset, and it only works because
it's a POC for you. devmem pages (which shouldn't exist according to
the dma-buf maintainer, IIUC) can't be recycled all the time. See
SO_DEVMEM_DONTNEED patch in my RFC and refcounting needed for devmem.

-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/14 20:58, Mina Almasry wrote:

>>
>> Yes, as above, skb_frags_not_readable() checking is still needed for
>> kmap() and page_address().
>>
>> get_page, put_page related calling is avoided in page_pool_frag_ref()
>> and napi_pp_put_page() for devmem page as the above checking is true
>> for devmem page:
>> (pp_iov->pp_magic & ~0x3UL) == PP_SIGNATURE
>>
> 
> So, devmem needs special handling with if statement for refcounting,
> even after using struct pages for devmem, which is not allowed (IIUC
> the dma-buf maintainer).

It reuses the already existing checking or optimization, that is the point
of 'mirror struct'.

> 
>>>
>>>> The main difference between this patchset and RFC v3:
>>>> 1. It reuses the 'struct page' to have more unified handling between
>>>>    normal page and devmem page for net stack.
>>>
>>> This is what was nacked in RFC v1.
>>>
>>>> 2. It relies on the page->pp_frag_count to do reference counting.
>>>>
>>>
>>> I don't see you change any of the page_ref_* calls in page_pool.c, for
>>> example this one:
>>>
>>> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601
>>>
>>> So the reference the page_pool is seeing is actually page->_refcount,
>>> not page->pp_frag_count? I'm confused here. Is this a bug in the
>>> patchset?
>>
>> page->_refcount is the same as page_pool_iov->_refcount for devmem, which
>> is ensured by the 'PAGE_POOL_MATCH(_refcount, _refcount);', and
>> page_pool_iov->_refcount is set to one in mp_dmabuf_devmem_alloc_pages()
>> by calling 'refcount_set(&ppiov->_refcount, 1)' and always remains as one.
>>
>> So the 'page_ref_count(page) == 1' checking is always true for devmem page.
> 
> Which, of course, is a bug in the patchset, and it only works because
> it's a POC for you. devmem pages (which shouldn't exist according to
> the dma-buf maintainer, IIUC) can't be recycled all the time. See
> SO_DEVMEM_DONTNEED patch in my RFC and refcounting needed for devmem.

I am not sure dma-buf maintainer's concern is still there with this patchset.

Whatever name you calling it for the struct, however you arrange each field
in the struct, some metadata is always needed for dmabuf to intergrate into
page pool.

If the above is true, why not utilize the 'struct page' to have more unified
handling?

> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Willem de Bruijn]

Yunsheng Lin wrote:
> On 2023/11/14 20:58, Mina Almasry wrote:
> 
> >>
> >> Yes, as above, skb_frags_not_readable() checking is still needed for
> >> kmap() and page_address().
> >>
> >> get_page, put_page related calling is avoided in page_pool_frag_ref()
> >> and napi_pp_put_page() for devmem page as the above checking is true
> >> for devmem page:
> >> (pp_iov->pp_magic & ~0x3UL) == PP_SIGNATURE
> >>
> > 
> > So, devmem needs special handling with if statement for refcounting,
> > even after using struct pages for devmem, which is not allowed (IIUC
> > the dma-buf maintainer).
> 
> It reuses the already existing checking or optimization, that is the point
> of 'mirror struct'.
> 
> > 
> >>>
> >>>> The main difference between this patchset and RFC v3:
> >>>> 1. It reuses the 'struct page' to have more unified handling between
> >>>>    normal page and devmem page for net stack.
> >>>
> >>> This is what was nacked in RFC v1.
> >>>
> >>>> 2. It relies on the page->pp_frag_count to do reference counting.
> >>>>
> >>>
> >>> I don't see you change any of the page_ref_* calls in page_pool.c, for
> >>> example this one:
> >>>
> >>> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L601
> >>>
> >>> So the reference the page_pool is seeing is actually page->_refcount,
> >>> not page->pp_frag_count? I'm confused here. Is this a bug in the
> >>> patchset?
> >>
> >> page->_refcount is the same as page_pool_iov->_refcount for devmem, which
> >> is ensured by the 'PAGE_POOL_MATCH(_refcount, _refcount);', and
> >> page_pool_iov->_refcount is set to one in mp_dmabuf_devmem_alloc_pages()
> >> by calling 'refcount_set(&ppiov->_refcount, 1)' and always remains as one.
> >>
> >> So the 'page_ref_count(page) == 1' checking is always true for devmem page.
> > 
> > Which, of course, is a bug in the patchset, and it only works because
> > it's a POC for you. devmem pages (which shouldn't exist according to
> > the dma-buf maintainer, IIUC) can't be recycled all the time. See
> > SO_DEVMEM_DONTNEED patch in my RFC and refcounting needed for devmem.
> 
> I am not sure dma-buf maintainer's concern is still there with this patchset.
> 
> Whatever name you calling it for the struct, however you arrange each field
> in the struct, some metadata is always needed for dmabuf to intergrate into
> page pool.
> 
> If the above is true, why not utilize the 'struct page' to have more unified
> handling?

My understanding is that there is a general preference to simplify struct
page, and at the least not move in the other direction by overloading the
struct in new ways.

If using struct page for something that is not memory, there is ZONE_DEVICE.
But using that correctly is non-trivial:

https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/ 

Since all we need is a handle that does not leave the network stack,
a network specific struct like page_pool_iov entirely avoids this issue.
RFC v3 seems like a good simplification over RFC v1 in that regard to me.
I was also pleasantly surprised how minimal the change to the users of
skb_frag_t actually proved to be.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/14 23:41, Willem de Bruijn wrote:
>>
>> I am not sure dma-buf maintainer's concern is still there with this patchset.
>>
>> Whatever name you calling it for the struct, however you arrange each field
>> in the struct, some metadata is always needed for dmabuf to intergrate into
>> page pool.
>>
>> If the above is true, why not utilize the 'struct page' to have more unified
>> handling?
> 
> My understanding is that there is a general preference to simplify struct
> page, and at the least not move in the other direction by overloading the
> struct in new ways.

As my understanding, the new struct is just mirroring the struct page pool
is already using, see:
https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119

If there is simplifying to the struct page_pool is using, I think the new
stuct the devmem memory provider is using can adjust accordingly.

As a matter of fact, I think the way 'struct page' for devmem is decoupled
from mm subsystem may provide a way to simplify or decoupled the already
existing 'struct page' used in netstack from mm subsystem, before this
patchset, it seems we have the below types of 'struct page':
1. page allocated in the netstack using page pool.
2. page allocated in the netstack using buddy allocator.
3. page allocated in other subsystem and passed to the netstack, such as
   zcopy or spliced page?

If we can decouple 'struct page' for devmem from mm subsystem, we may be able
to decouple the above 'struct page' from mm subsystem one by one.

> 
> If using struct page for something that is not memory, there is ZONE_DEVICE.
> But using that correctly is non-trivial:
> 
> https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/ 
> 
> Since all we need is a handle that does not leave the network stack,
> a network specific struct like page_pool_iov entirely avoids this issue.

Yes, I am agree about the network specific struct.
I am wondering if we can make the struct more generic if we want to
intergrate it into page_pool and use it in net stack.

> RFC v3 seems like a good simplification over RFC v1 in that regard to me.
> I was also pleasantly surprised how minimal the change to the users of
> skb_frag_t actually proved to be.

Yes, I am agreed about that too. Maybe we can make it simpler by using
a more abstract struct as page_pool, and utilize some features of
page_pool too.

For example, from the page_pool doc, page_pool have fast cache and
ptr-ring cache as below, but if napi_frag_unref() call
page_pool_page_put_many() and return the dmabuf chunk directly to
gen_pool in the memory provider, then it seems we are bypassing the
below caches in the page_pool.

    +------------------+
    |       Driver     |
    +------------------+
            ^
            |
            |
            |
            v
    +--------------------------------------------+
    |                request memory              |
    +--------------------------------------------+
        ^                                  ^
        |                                  |
        | Pool empty                       | Pool has entries
        |                                  |
        v                                  v
    +-----------------------+     +------------------------+
    | alloc (and map) pages |     |  get page from cache   |
    +-----------------------+     +------------------------+
                                    ^                    ^
                                    |                    |
                                    | cache available    | No entries, refill
                                    |                    | from ptr-ring
                                    |                    |
                                    v                    v
                          +-----------------+     +------------------+
                          |   Fast cache    |     |  ptr-ring cache  |
                          +-----------------+     +------------------+


> 
> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Wed, Nov 15, 2023 at 1:29 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2023/11/14 23:41, Willem de Bruijn wrote:
> >>
> >> I am not sure dma-buf maintainer's concern is still there with this patchset.
> >>
> >> Whatever name you calling it for the struct, however you arrange each field
> >> in the struct, some metadata is always needed for dmabuf to intergrate into
> >> page pool.
> >>
> >> If the above is true, why not utilize the 'struct page' to have more unified
> >> handling?
> >
> > My understanding is that there is a general preference to simplify struct
> > page, and at the least not move in the other direction by overloading the
> > struct in new ways.
>
> As my understanding, the new struct is just mirroring the struct page pool
> is already using, see:
> https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119
>
> If there is simplifying to the struct page_pool is using, I think the new
> stuct the devmem memory provider is using can adjust accordingly.
>
> As a matter of fact, I think the way 'struct page' for devmem is decoupled
> from mm subsystem may provide a way to simplify or decoupled the already
> existing 'struct page' used in netstack from mm subsystem, before this
> patchset, it seems we have the below types of 'struct page':
> 1. page allocated in the netstack using page pool.
> 2. page allocated in the netstack using buddy allocator.
> 3. page allocated in other subsystem and passed to the netstack, such as
>    zcopy or spliced page?
>
> If we can decouple 'struct page' for devmem from mm subsystem, we may be able
> to decouple the above 'struct page' from mm subsystem one by one.
>
> >
> > If using struct page for something that is not memory, there is ZONE_DEVICE.
> > But using that correctly is non-trivial:
> >
> > https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/
> >
> > Since all we need is a handle that does not leave the network stack,
> > a network specific struct like page_pool_iov entirely avoids this issue.
>
> Yes, I am agree about the network specific struct.
> I am wondering if we can make the struct more generic if we want to
> intergrate it into page_pool and use it in net stack.
>
> > RFC v3 seems like a good simplification over RFC v1 in that regard to me.
> > I was also pleasantly surprised how minimal the change to the users of
> > skb_frag_t actually proved to be.
>
> Yes, I am agreed about that too. Maybe we can make it simpler by using
> a more abstract struct as page_pool, and utilize some features of
> page_pool too.
>
> For example, from the page_pool doc, page_pool have fast cache and
> ptr-ring cache as below, but if napi_frag_unref() call
> page_pool_page_put_many() and return the dmabuf chunk directly to
> gen_pool in the memory provider, then it seems we are bypassing the
> below caches in the page_pool.
>

I think you're just misunderstanding the code. The page recycling
works with my patchset. napi_frag_unref() calls napi_pp_put_page() if
recycle == true, and that works the same with devmem as with regular
pages.

If recycle == false, we call page_pool_page_put_many() which will call
put_page() for regular pages and page_pool_iov_put_many() for devmem
pages. So, the memory recycling works exactly the same as before with
devmem as with regular pages. In my tests I do see the devmem being
recycled correctly. We are not bypassing any caches.


>     +------------------+
>     |       Driver     |
>     +------------------+
>             ^
>             |
>             |
>             |
>             v
>     +--------------------------------------------+
>     |                request memory              |
>     +--------------------------------------------+
>         ^                                  ^
>         |                                  |
>         | Pool empty                       | Pool has entries
>         |                                  |
>         v                                  v
>     +-----------------------+     +------------------------+
>     | alloc (and map) pages |     |  get page from cache   |
>     +-----------------------+     +------------------------+
>                                     ^                    ^
>                                     |                    |
>                                     | cache available    | No entries, refill
>                                     |                    | from ptr-ring
>                                     |                    |
>                                     v                    v
>                           +-----------------+     +------------------+
>                           |   Fast cache    |     |  ptr-ring cache  |
>                           +-----------------+     +------------------+
>
>
> >
> > .
> >



-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Wed, Nov 15, 2023 at 10:07 AM Mina Almasry <almasrymina@google.com> wrote:
>
> On Wed, Nov 15, 2023 at 1:29 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >
> > On 2023/11/14 23:41, Willem de Bruijn wrote:
> > >>
> > >> I am not sure dma-buf maintainer's concern is still there with this patchset.
> > >>
> > >> Whatever name you calling it for the struct, however you arrange each field
> > >> in the struct, some metadata is always needed for dmabuf to intergrate into
> > >> page pool.
> > >>
> > >> If the above is true, why not utilize the 'struct page' to have more unified
> > >> handling?
> > >
> > > My understanding is that there is a general preference to simplify struct
> > > page, and at the least not move in the other direction by overloading the
> > > struct in new ways.
> >
> > As my understanding, the new struct is just mirroring the struct page pool
> > is already using, see:
> > https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119
> >
> > If there is simplifying to the struct page_pool is using, I think the new
> > stuct the devmem memory provider is using can adjust accordingly.
> >
> > As a matter of fact, I think the way 'struct page' for devmem is decoupled
> > from mm subsystem may provide a way to simplify or decoupled the already
> > existing 'struct page' used in netstack from mm subsystem, before this
> > patchset, it seems we have the below types of 'struct page':
> > 1. page allocated in the netstack using page pool.
> > 2. page allocated in the netstack using buddy allocator.
> > 3. page allocated in other subsystem and passed to the netstack, such as
> >    zcopy or spliced page?
> >
> > If we can decouple 'struct page' for devmem from mm subsystem, we may be able
> > to decouple the above 'struct page' from mm subsystem one by one.
> >
> > >
> > > If using struct page for something that is not memory, there is ZONE_DEVICE.
> > > But using that correctly is non-trivial:
> > >
> > > https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/
> > >
> > > Since all we need is a handle that does not leave the network stack,
> > > a network specific struct like page_pool_iov entirely avoids this issue.
> >
> > Yes, I am agree about the network specific struct.
> > I am wondering if we can make the struct more generic if we want to
> > intergrate it into page_pool and use it in net stack.
> >
> > > RFC v3 seems like a good simplification over RFC v1 in that regard to me.
> > > I was also pleasantly surprised how minimal the change to the users of
> > > skb_frag_t actually proved to be.
> >
> > Yes, I am agreed about that too. Maybe we can make it simpler by using
> > a more abstract struct as page_pool, and utilize some features of
> > page_pool too.
> >
> > For example, from the page_pool doc, page_pool have fast cache and
> > ptr-ring cache as below, but if napi_frag_unref() call
> > page_pool_page_put_many() and return the dmabuf chunk directly to
> > gen_pool in the memory provider, then it seems we are bypassing the
> > below caches in the page_pool.
> >
>
> I think you're just misunderstanding the code. The page recycling
> works with my patchset. napi_frag_unref() calls napi_pp_put_page() if
> recycle == true, and that works the same with devmem as with regular
> pages.
>
> If recycle == false, we call page_pool_page_put_many() which will call
> put_page() for regular pages and page_pool_iov_put_many() for devmem
> pages. So, the memory recycling works exactly the same as before with
> devmem as with regular pages. In my tests I do see the devmem being
> recycled correctly. We are not bypassing any caches.
>
>

Ah, taking a closer look here, the devmem recycling works for me but I
think that's a side effect to the fact that the page_pool support I
implemented with GVE is unusual. I currently allocate pages from the
page_pool but do not set skb_mark_for_recycle(). The page recycling
still happens when GVE is done with the page and calls
page_pool_put_full_pgae(), as that eventually checks the refcount on
the devmem and recycles it.

I will fix up the GVE to call skb_mark_for_recycle() and ensure the
napi_pp_put_page() path recycles the devmem or page correctly in the
next version.

> >     +------------------+
> >     |       Driver     |
> >     +------------------+
> >             ^
> >             |
> >             |
> >             |
> >             v
> >     +--------------------------------------------+
> >     |                request memory              |
> >     +--------------------------------------------+
> >         ^                                  ^
> >         |                                  |
> >         | Pool empty                       | Pool has entries
> >         |                                  |
> >         v                                  v
> >     +-----------------------+     +------------------------+
> >     | alloc (and map) pages |     |  get page from cache   |
> >     +-----------------------+     +------------------------+
> >                                     ^                    ^
> >                                     |                    |
> >                                     | cache available    | No entries, refill
> >                                     |                    | from ptr-ring
> >                                     |                    |
> >                                     v                    v
> >                           +-----------------+     +------------------+
> >                           |   Fast cache    |     |  ptr-ring cache  |
> >                           +-----------------+     +------------------+
> >
> >
> > >
> > > .
> > >
>
>
>
> --
> Thanks,
> Mina



-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/16 3:05, Mina Almasry wrote:
> On Wed, Nov 15, 2023 at 10:07 AM Mina Almasry <almasrymina@google.com> wrote:
>>
>> On Wed, Nov 15, 2023 at 1:29 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>
>>> On 2023/11/14 23:41, Willem de Bruijn wrote:
>>>>>
>>>>> I am not sure dma-buf maintainer's concern is still there with this patchset.
>>>>>
>>>>> Whatever name you calling it for the struct, however you arrange each field
>>>>> in the struct, some metadata is always needed for dmabuf to intergrate into
>>>>> page pool.
>>>>>
>>>>> If the above is true, why not utilize the 'struct page' to have more unified
>>>>> handling?
>>>>
>>>> My understanding is that there is a general preference to simplify struct
>>>> page, and at the least not move in the other direction by overloading the
>>>> struct in new ways.
>>>
>>> As my understanding, the new struct is just mirroring the struct page pool
>>> is already using, see:
>>> https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119
>>>
>>> If there is simplifying to the struct page_pool is using, I think the new
>>> stuct the devmem memory provider is using can adjust accordingly.
>>>
>>> As a matter of fact, I think the way 'struct page' for devmem is decoupled
>>> from mm subsystem may provide a way to simplify or decoupled the already
>>> existing 'struct page' used in netstack from mm subsystem, before this
>>> patchset, it seems we have the below types of 'struct page':
>>> 1. page allocated in the netstack using page pool.
>>> 2. page allocated in the netstack using buddy allocator.
>>> 3. page allocated in other subsystem and passed to the netstack, such as
>>>    zcopy or spliced page?
>>>
>>> If we can decouple 'struct page' for devmem from mm subsystem, we may be able
>>> to decouple the above 'struct page' from mm subsystem one by one.
>>>
>>>>
>>>> If using struct page for something that is not memory, there is ZONE_DEVICE.
>>>> But using that correctly is non-trivial:
>>>>
>>>> https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/
>>>>
>>>> Since all we need is a handle that does not leave the network stack,
>>>> a network specific struct like page_pool_iov entirely avoids this issue.
>>>
>>> Yes, I am agree about the network specific struct.
>>> I am wondering if we can make the struct more generic if we want to
>>> intergrate it into page_pool and use it in net stack.
>>>
>>>> RFC v3 seems like a good simplification over RFC v1 in that regard to me.
>>>> I was also pleasantly surprised how minimal the change to the users of
>>>> skb_frag_t actually proved to be.
>>>
>>> Yes, I am agreed about that too. Maybe we can make it simpler by using
>>> a more abstract struct as page_pool, and utilize some features of
>>> page_pool too.
>>>
>>> For example, from the page_pool doc, page_pool have fast cache and
>>> ptr-ring cache as below, but if napi_frag_unref() call
>>> page_pool_page_put_many() and return the dmabuf chunk directly to
>>> gen_pool in the memory provider, then it seems we are bypassing the
>>> below caches in the page_pool.
>>>
>>
>> I think you're just misunderstanding the code. The page recycling
>> works with my patchset. napi_frag_unref() calls napi_pp_put_page() if
>> recycle == true, and that works the same with devmem as with regular
>> pages.
>>
>> If recycle == false, we call page_pool_page_put_many() which will call
>> put_page() for regular pages and page_pool_iov_put_many() for devmem
>> pages. So, the memory recycling works exactly the same as before with
>> devmem as with regular pages. In my tests I do see the devmem being
>> recycled correctly. We are not bypassing any caches.
>>
>>
> 
> Ah, taking a closer look here, the devmem recycling works for me but I
> think that's a side effect to the fact that the page_pool support I
> implemented with GVE is unusual. I currently allocate pages from the
> page_pool but do not set skb_mark_for_recycle(). The page recycling
> still happens when GVE is done with the page and calls
> page_pool_put_full_pgae(), as that eventually checks the refcount on
> the devmem and recycles it.
> 
> I will fix up the GVE to call skb_mark_for_recycle() and ensure the
> napi_pp_put_page() path recycles the devmem or page correctly in the
> next version.

What about other features? Like dma mapping optimization and frag support
in the page pool.

I understand that you use some trick in the gen_gool to avoid the per chunk
dma_addr storage in the 'struct page_pool_iov' and do not need frag support
for now.

But for other memory provider, if they need those supports, we probably need
to extend 'struct page_pool_iov' to support those features, then we may have
a 'struct page_pool_iov' to be looking alike to the sepcific union for page_pool
in 'struct page', see:

https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119

We currently don't have a way to decouple the existing 'struct page' from mm
subsystem yet as my understanding, if we don't mirror the above union in the
'struct page', we may have more 'if' checking added in the future.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Thu, Nov 16, 2023 at 3:12 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2023/11/16 3:05, Mina Almasry wrote:
> > On Wed, Nov 15, 2023 at 10:07 AM Mina Almasry <almasrymina@google.com> wrote:
> >>
> >> On Wed, Nov 15, 2023 at 1:29 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>>
> >>> On 2023/11/14 23:41, Willem de Bruijn wrote:
> >>>>>
> >>>>> I am not sure dma-buf maintainer's concern is still there with this patchset.
> >>>>>
> >>>>> Whatever name you calling it for the struct, however you arrange each field
> >>>>> in the struct, some metadata is always needed for dmabuf to intergrate into
> >>>>> page pool.
> >>>>>
> >>>>> If the above is true, why not utilize the 'struct page' to have more unified
> >>>>> handling?
> >>>>
> >>>> My understanding is that there is a general preference to simplify struct
> >>>> page, and at the least not move in the other direction by overloading the
> >>>> struct in new ways.
> >>>
> >>> As my understanding, the new struct is just mirroring the struct page pool
> >>> is already using, see:
> >>> https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119
> >>>
> >>> If there is simplifying to the struct page_pool is using, I think the new
> >>> stuct the devmem memory provider is using can adjust accordingly.
> >>>
> >>> As a matter of fact, I think the way 'struct page' for devmem is decoupled
> >>> from mm subsystem may provide a way to simplify or decoupled the already
> >>> existing 'struct page' used in netstack from mm subsystem, before this
> >>> patchset, it seems we have the below types of 'struct page':
> >>> 1. page allocated in the netstack using page pool.
> >>> 2. page allocated in the netstack using buddy allocator.
> >>> 3. page allocated in other subsystem and passed to the netstack, such as
> >>>    zcopy or spliced page?
> >>>
> >>> If we can decouple 'struct page' for devmem from mm subsystem, we may be able
> >>> to decouple the above 'struct page' from mm subsystem one by one.
> >>>
> >>>>
> >>>> If using struct page for something that is not memory, there is ZONE_DEVICE.
> >>>> But using that correctly is non-trivial:
> >>>>
> >>>> https://lore.kernel.org/all/ZKyZBbKEpmkFkpWV@ziepe.ca/
> >>>>
> >>>> Since all we need is a handle that does not leave the network stack,
> >>>> a network specific struct like page_pool_iov entirely avoids this issue.
> >>>
> >>> Yes, I am agree about the network specific struct.
> >>> I am wondering if we can make the struct more generic if we want to
> >>> intergrate it into page_pool and use it in net stack.
> >>>
> >>>> RFC v3 seems like a good simplification over RFC v1 in that regard to me.
> >>>> I was also pleasantly surprised how minimal the change to the users of
> >>>> skb_frag_t actually proved to be.
> >>>
> >>> Yes, I am agreed about that too. Maybe we can make it simpler by using
> >>> a more abstract struct as page_pool, and utilize some features of
> >>> page_pool too.
> >>>
> >>> For example, from the page_pool doc, page_pool have fast cache and
> >>> ptr-ring cache as below, but if napi_frag_unref() call
> >>> page_pool_page_put_many() and return the dmabuf chunk directly to
> >>> gen_pool in the memory provider, then it seems we are bypassing the
> >>> below caches in the page_pool.
> >>>
> >>
> >> I think you're just misunderstanding the code. The page recycling
> >> works with my patchset. napi_frag_unref() calls napi_pp_put_page() if
> >> recycle == true, and that works the same with devmem as with regular
> >> pages.
> >>
> >> If recycle == false, we call page_pool_page_put_many() which will call
> >> put_page() for regular pages and page_pool_iov_put_many() for devmem
> >> pages. So, the memory recycling works exactly the same as before with
> >> devmem as with regular pages. In my tests I do see the devmem being
> >> recycled correctly. We are not bypassing any caches.
> >>
> >>
> >
> > Ah, taking a closer look here, the devmem recycling works for me but I
> > think that's a side effect to the fact that the page_pool support I
> > implemented with GVE is unusual. I currently allocate pages from the
> > page_pool but do not set skb_mark_for_recycle(). The page recycling
> > still happens when GVE is done with the page and calls
> > page_pool_put_full_pgae(), as that eventually checks the refcount on
> > the devmem and recycles it.
> >
> > I will fix up the GVE to call skb_mark_for_recycle() and ensure the
> > napi_pp_put_page() path recycles the devmem or page correctly in the
> > next version.
>
> What about other features? Like dma mapping optimization and frag support
> in the page pool.
>

PP_FLAG_DMA_MAP will be supported and required in the next version per
Jakub's request.

frag support is something I disabled in the initial versions of the
patchset, but only out of convenience and to simplify the initial
implementation. At google we typically use page aligned MSS and the
frag support isn't really that useful for us. I don't see an issue
extending frag support to devmem and iovs in the future if needed.
We'd probably add the pp_frag_count field to page_pool_iov and handle
it similarly to how it's handled for pages.

> I understand that you use some trick in the gen_gool to avoid the per chunk
> dma_addr storage in the 'struct page_pool_iov' and do not need frag support
> for now.
>
> But for other memory provider, if they need those supports, we probably need
> to extend 'struct page_pool_iov' to support those features, then we may have
> a 'struct page_pool_iov' to be looking alike to the sepcific union for page_pool
> in 'struct page', see:
>
> https://elixir.free-electrons.com/linux/v6.7-rc1/source/include/linux/mm_types.h#L119
>
> We currently don't have a way to decouple the existing 'struct page' from mm
> subsystem yet as my understanding, if we don't mirror the above union in the
> 'struct page', we may have more 'if' checking added in the future.



-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Jason Gunthorpe]

On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:

> Actually because you put the 'strtuct page for devmem' in
> skb->bv_frag, the net stack will grab the 'struct page' for devmem
> using skb_frag_page() then call things like page_address(), kmap,
> get_page, put_page, etc, etc, etc.

Yikes, please no. If net has its own struct page look alike it has to
stay entirely inside net. A non-mm owned struct page should not be
passed into mm calls. It is just way too hacky to be seriously
considered :(

> > I would expect net stack, page pool, driver still see the 'struct page',
> > only memory provider see the specific struct for itself, for the above,
> > devmem memory provider sees the 'struct page_pool_iov'.
> >
> > The reason I still expect driver to see the 'struct page' is that driver
> > will still need to support normal memory besides devmem.

I wouldn't say this approach is unreasonable, but it does have to be
done carefully to isolate the mm. Keeping the struct page in the API
is going to make this very hard.

Jason

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Christian König]

Am 14.11.23 um 14:16 schrieb Jason Gunthorpe:
> A non-mm owned struct page should not be
> passed into mm calls. It is just way too hacky to be seriously
> considered :(

Can we please print this sentence on T-Shirts? Or framed on the wall of 
meeting rooms?

I don't know how often I had to repeat that to people.

Regards,
Christian.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/14 21:16, Jason Gunthorpe wrote:
> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
> 
>> Actually because you put the 'strtuct page for devmem' in
>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>> using skb_frag_page() then call things like page_address(), kmap,
>> get_page, put_page, etc, etc, etc.
> 
> Yikes, please no. If net has its own struct page look alike it has to
> stay entirely inside net. A non-mm owned struct page should not be
> passed into mm calls. It is just way too hacky to be seriously
> considered :(

Yes, that is something this patchset is trying to do, defining its own
struct page look alike for page pool to support devmem.

struct page for devmem will not be called into the mm subsystem, so most
of the mm calls is avoided by calling into the devmem memory provider'
ops instead of calling mm calls.

As far as I see for now, only page_ref_count(), page_is_pfmemalloc() and
PageTail() is called for devmem page, which should be easy to ensure that
those call for devmem page is consistent with the struct page owned by mm.
I am not sure if we can use some kind of compile/runtime checking to ensure
those kinds of consistency?

> 
>>> I would expect net stack, page pool, driver still see the 'struct page',
>>> only memory provider see the specific struct for itself, for the above,
>>> devmem memory provider sees the 'struct page_pool_iov'.
>>>
>>> The reason I still expect driver to see the 'struct page' is that driver
>>> will still need to support normal memory besides devmem.
> 
> I wouldn't say this approach is unreasonable, but it does have to be
> done carefully to isolate the mm. Keeping the struct page in the API
> is going to make this very hard.

I would expect that most of the isolation is done in page pool, as far as
I can see:

1. For control part: the driver may need to tell the page pool which memory
                     provider it want to use. Or the administrator specifies
                     which memory provider to use by some netlink-based cmd.

2. For data part: I am thinking that driver should only call page_pool_alloc(),
                  page_pool_free() and page_pool_get_dma_addr related function.

Of course the driver may need to be aware of that if it can call kmap() or
page_address() on the page returned from page_pool_alloc(), and maybe tell
net stack that those pages is not kmap()'able and page_address()'able.

> 
> Jason
> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Jason Gunthorpe]

On Wed, Nov 15, 2023 at 05:21:02PM +0800, Yunsheng Lin wrote:

> >>> I would expect net stack, page pool, driver still see the 'struct page',
> >>> only memory provider see the specific struct for itself, for the above,
> >>> devmem memory provider sees the 'struct page_pool_iov'.
> >>>
> >>> The reason I still expect driver to see the 'struct page' is that driver
> >>> will still need to support normal memory besides devmem.
> > 
> > I wouldn't say this approach is unreasonable, but it does have to be
> > done carefully to isolate the mm. Keeping the struct page in the API
> > is going to make this very hard.
> 
> I would expect that most of the isolation is done in page pool, as far as
> I can see:

It is the sort of thing that is important enough it should have
compiler help via types to prove that it is being done
properly. Otherwise it will be full of mistakes over time.

Jason

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/15 21:38, Jason Gunthorpe wrote:
> On Wed, Nov 15, 2023 at 05:21:02PM +0800, Yunsheng Lin wrote:
> 
>>>>> I would expect net stack, page pool, driver still see the 'struct page',
>>>>> only memory provider see the specific struct for itself, for the above,
>>>>> devmem memory provider sees the 'struct page_pool_iov'.
>>>>>
>>>>> The reason I still expect driver to see the 'struct page' is that driver
>>>>> will still need to support normal memory besides devmem.
>>>
>>> I wouldn't say this approach is unreasonable, but it does have to be
>>> done carefully to isolate the mm. Keeping the struct page in the API
>>> is going to make this very hard.
>>
>> I would expect that most of the isolation is done in page pool, as far as
>> I can see:
> 
> It is the sort of thing that is important enough it should have
> compiler help via types to prove that it is being done
> properly. Otherwise it will be full of mistakes over time.

Yes, agreed.

I have done something similar as willy has done for some of
folio conversion as below:

+#define PAGE_POOL_MATCH(pg, iov)				\
+	static_assert(offsetof(struct page, pg) ==		\
+		      offsetof(struct page_pool_iov, iov))
+PAGE_POOL_MATCH(flags, res0);
+PAGE_POOL_MATCH(pp_magic, pp_magic);
+PAGE_POOL_MATCH(pp, pp);
...

Not sure if we need to add new API for driver to use when the
driver need the devmem support yet.


> 
> Jason
> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Jason Gunthorpe]

On Thu, Nov 16, 2023 at 07:10:01PM +0800, Yunsheng Lin wrote:
> On 2023/11/15 21:38, Jason Gunthorpe wrote:
> > On Wed, Nov 15, 2023 at 05:21:02PM +0800, Yunsheng Lin wrote:
> > 
> >>>>> I would expect net stack, page pool, driver still see the 'struct page',
> >>>>> only memory provider see the specific struct for itself, for the above,
> >>>>> devmem memory provider sees the 'struct page_pool_iov'.
> >>>>>
> >>>>> The reason I still expect driver to see the 'struct page' is that driver
> >>>>> will still need to support normal memory besides devmem.
> >>>
> >>> I wouldn't say this approach is unreasonable, but it does have to be
> >>> done carefully to isolate the mm. Keeping the struct page in the API
> >>> is going to make this very hard.
> >>
> >> I would expect that most of the isolation is done in page pool, as far as
> >> I can see:
> > 
> > It is the sort of thing that is important enough it should have
> > compiler help via types to prove that it is being done
> > properly. Otherwise it will be full of mistakes over time.
> 
> Yes, agreed.
> 
> I have done something similar as willy has done for some of
> folio conversion as below:

That is not at all what I mean, I mean you should not use
struct page * types at all in code that flows from the _iov version
except via limited accessors that can be audited and have appropriate
assertions.

Just releasing struct page * that is not a struct page * everywhere
without type safety will never be correct long term.

Jason

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Mina Almasry]

On Wed, Nov 15, 2023 at 1:21 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2023/11/14 21:16, Jason Gunthorpe wrote:
> > On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
> >
> >> Actually because you put the 'strtuct page for devmem' in
> >> skb->bv_frag, the net stack will grab the 'struct page' for devmem
> >> using skb_frag_page() then call things like page_address(), kmap,
> >> get_page, put_page, etc, etc, etc.
> >
> > Yikes, please no. If net has its own struct page look alike it has to
> > stay entirely inside net. A non-mm owned struct page should not be
> > passed into mm calls. It is just way too hacky to be seriously
> > considered :(
>
> Yes, that is something this patchset is trying to do, defining its own
> struct page look alike for page pool to support devmem.
>
> struct page for devmem will not be called into the mm subsystem, so most
> of the mm calls is avoided by calling into the devmem memory provider'
> ops instead of calling mm calls.
>
> As far as I see for now, only page_ref_count(), page_is_pfmemalloc() and
> PageTail() is called for devmem page, which should be easy to ensure that
> those call for devmem page is consistent with the struct page owned by mm.

I'm not sure this is true. These 3 calls are just the calls you're
aware of. In your proposal you're casting mirror pages into page* and
releasing them into the net stack. You need to scrub the entire net
stack for mm calls, i.e. all driver code and all skb_frag_page() call
sites. Of the top of my head, the driver is probably calling
page_address() and illegal_highdma() is calling PageHighMem(). TCP
zerocopy receive is calling vm_insert_pages().

> I am not sure if we can use some kind of compile/runtime checking to ensure
> those kinds of consistency?
>
> >
> >>> I would expect net stack, page pool, driver still see the 'struct page',
> >>> only memory provider see the specific struct for itself, for the above,
> >>> devmem memory provider sees the 'struct page_pool_iov'.
> >>>
> >>> The reason I still expect driver to see the 'struct page' is that driver
> >>> will still need to support normal memory besides devmem.
> >
> > I wouldn't say this approach is unreasonable, but it does have to be
> > done carefully to isolate the mm. Keeping the struct page in the API
> > is going to make this very hard.
>
> I would expect that most of the isolation is done in page pool, as far as
> I can see:
>
> 1. For control part: the driver may need to tell the page pool which memory
>                      provider it want to use. Or the administrator specifies
>                      which memory provider to use by some netlink-based cmd.
>
> 2. For data part: I am thinking that driver should only call page_pool_alloc(),
>                   page_pool_free() and page_pool_get_dma_addr related function.
>
> Of course the driver may need to be aware of that if it can call kmap() or
> page_address() on the page returned from page_pool_alloc(), and maybe tell
> net stack that those pages is not kmap()'able and page_address()'able.
>
> >
> > Jason
> > .
> >



-- 
Thanks,
Mina

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/16 1:44, Mina Almasry wrote:
> On Wed, Nov 15, 2023 at 1:21 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2023/11/14 21:16, Jason Gunthorpe wrote:
>>> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
>>>
>>>> Actually because you put the 'strtuct page for devmem' in
>>>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>>>> using skb_frag_page() then call things like page_address(), kmap,
>>>> get_page, put_page, etc, etc, etc.
>>>
>>> Yikes, please no. If net has its own struct page look alike it has to
>>> stay entirely inside net. A non-mm owned struct page should not be
>>> passed into mm calls. It is just way too hacky to be seriously
>>> considered :(
>>
>> Yes, that is something this patchset is trying to do, defining its own
>> struct page look alike for page pool to support devmem.
>>
>> struct page for devmem will not be called into the mm subsystem, so most
>> of the mm calls is avoided by calling into the devmem memory provider'
>> ops instead of calling mm calls.
>>
>> As far as I see for now, only page_ref_count(), page_is_pfmemalloc() and
>> PageTail() is called for devmem page, which should be easy to ensure that
>> those call for devmem page is consistent with the struct page owned by mm.
> 
> I'm not sure this is true. These 3 calls are just the calls you're
> aware of. In your proposal you're casting mirror pages into page* and
> releasing them into the net stack. You need to scrub the entire net
> stack for mm calls, i.e. all driver code and all skb_frag_page() call
> sites. Of the top of my head, the driver is probably calling
> page_address() and illegal_highdma() is calling PageHighMem(). TCP
> zerocopy receive is calling vm_insert_pages().

For net stack part, I believe your patch below is handling to aovid those
mm calls? I don't include it in this patchset as I thought it is obvious
that whatever the proposal is, we always need those checking.
Maybe we should have included it to avoid this kind of confusion.
https://lore.kernel.org/all/20231106024413.2801438-10-almasrymina@google.com/

For driver part, I was thinking if the driver supports devmem, it should check
that if it can call page_address() related call on a specific 'stuct page', or
maybe we should introduce a new helper to make it obvious?

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[David Ahern]

On 11/15/23 2:21 AM, Yunsheng Lin wrote:
> On 2023/11/14 21:16, Jason Gunthorpe wrote:
>> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
>>
>>> Actually because you put the 'strtuct page for devmem' in
>>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>>> using skb_frag_page() then call things like page_address(), kmap,
>>> get_page, put_page, etc, etc, etc.
>>
>> Yikes, please no. If net has its own struct page look alike it has to
>> stay entirely inside net. A non-mm owned struct page should not be
>> passed into mm calls. It is just way too hacky to be seriously
>> considered :(
> 
> Yes, that is something this patchset is trying to do, defining its own
> struct page look alike for page pool to support devmem.
> 

Networking needs to be able to move away from struct page references.
The devmem and host memory for Rx use cases do not need to be page based.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/16 1:57, David Ahern wrote:
> On 11/15/23 2:21 AM, Yunsheng Lin wrote:
>> On 2023/11/14 21:16, Jason Gunthorpe wrote:
>>> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
>>>
>>>> Actually because you put the 'strtuct page for devmem' in
>>>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>>>> using skb_frag_page() then call things like page_address(), kmap,
>>>> get_page, put_page, etc, etc, etc.
>>>
>>> Yikes, please no. If net has its own struct page look alike it has to
>>> stay entirely inside net. A non-mm owned struct page should not be
>>> passed into mm calls. It is just way too hacky to be seriously
>>> considered :(
>>
>> Yes, that is something this patchset is trying to do, defining its own
>> struct page look alike for page pool to support devmem.
>>
> 
> Networking needs to be able to move away from struct page references.
> The devmem and host memory for Rx use cases do not need to be page based.

Yes, I am agreed the ultimate goal is to move away from struct page
references. But I am not sure if we can do it right away as there
still are different types of existing 'struct page' in the netstack,
see:

https://lore.kernel.org/all/8b7d25eb-1f10-3e37-8753-92b42da3fb34@huawei.com/

> 
> 
> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[David Ahern]

On 11/16/23 4:12 AM, Yunsheng Lin wrote:
> On 2023/11/16 1:57, David Ahern wrote:
>> On 11/15/23 2:21 AM, Yunsheng Lin wrote:
>>> On 2023/11/14 21:16, Jason Gunthorpe wrote:
>>>> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
>>>>
>>>>> Actually because you put the 'strtuct page for devmem' in
>>>>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>>>>> using skb_frag_page() then call things like page_address(), kmap,
>>>>> get_page, put_page, etc, etc, etc.
>>>>
>>>> Yikes, please no. If net has its own struct page look alike it has to
>>>> stay entirely inside net. A non-mm owned struct page should not be
>>>> passed into mm calls. It is just way too hacky to be seriously
>>>> considered :(
>>>
>>> Yes, that is something this patchset is trying to do, defining its own
>>> struct page look alike for page pool to support devmem.
>>>
>>
>> Networking needs to be able to move away from struct page references.
>> The devmem and host memory for Rx use cases do not need to be page based.
> 
> Yes, I am agreed the ultimate goal is to move away from struct page
> references. But I am not sure if we can do it right away as there
> still are different types of existing 'struct page' in the netstack,
> see:
> 
> https://lore.kernel.org/all/8b7d25eb-1f10-3e37-8753-92b42da3fb34@huawei.com/

yes, that is the point of a blended approach -- pages and buffers (or
iov) -- leveraging the LSB of the address. That proposal is the right
direction to be moving for co-existence. Adding fake struct page
instances is the wrong direction.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/16 23:58, David Ahern wrote:
> On 11/16/23 4:12 AM, Yunsheng Lin wrote:
>> On 2023/11/16 1:57, David Ahern wrote:
>>> On 11/15/23 2:21 AM, Yunsheng Lin wrote:
>>>> On 2023/11/14 21:16, Jason Gunthorpe wrote:
>>>>> On Tue, Nov 14, 2023 at 04:21:26AM -0800, Mina Almasry wrote:
>>>>>
>>>>>> Actually because you put the 'strtuct page for devmem' in
>>>>>> skb->bv_frag, the net stack will grab the 'struct page' for devmem
>>>>>> using skb_frag_page() then call things like page_address(), kmap,
>>>>>> get_page, put_page, etc, etc, etc.
>>>>>
>>>>> Yikes, please no. If net has its own struct page look alike it has to
>>>>> stay entirely inside net. A non-mm owned struct page should not be
>>>>> passed into mm calls. It is just way too hacky to be seriously
>>>>> considered :(
>>>>
>>>> Yes, that is something this patchset is trying to do, defining its own
>>>> struct page look alike for page pool to support devmem.
>>>>
>>>
>>> Networking needs to be able to move away from struct page references.
>>> The devmem and host memory for Rx use cases do not need to be page based.
>>
>> Yes, I am agreed the ultimate goal is to move away from struct page
>> references. But I am not sure if we can do it right away as there
>> still are different types of existing 'struct page' in the netstack,
>> see:
>>
>> https://lore.kernel.org/all/8b7d25eb-1f10-3e37-8753-92b42da3fb34@huawei.com/
> 
> yes, that is the point of a blended approach -- pages and buffers (or
> iov) -- leveraging the LSB of the address. That proposal is the right

I am not sure leveraging the LSB of the address is necessary yet, as it
does not seems to provide the type check protection, it seems to just
provide a way to demux between pages(including page pool owned page and
non-page pool owned page) and page pool owned buffer.
That info is avaliable through the page->pp_magic and page->pp->mp_*
too if we mirror the page pool specific union in 'struct page'.

> direction to be moving for co-existence. Adding fake struct page
> instances is the wrong direction.

Perhaps a fake struct page with type check protection is the right
direction?

Intergrating devmem to page pool without a unified metadata between
pages and buffers or without a proper abstract layer does not seems
like the good direction either.

> .
> 

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Jakub Kicinski]

On Tue, 14 Nov 2023 16:23:29 +0800 Yunsheng Lin wrote:
> I would expect net stack, page pool, driver still see the 'struct page',
> only memory provider see the specific struct for itself, for the above,
> devmem memory provider sees the 'struct page_pool_iov'.

You can't lie to the driver that an _iov is a page either.
The driver must explicitly "opt-in" to using the _iov variant,
by calling the _iov set of APIs.

Only drivers which can support header-data split can reasonably
use the _iov API, for data pages.

Re: [PATCH RFC 3/8] memory-provider: dmabuf devmem memory provider

[Yunsheng Lin]

On 2023/11/15 6:25, Jakub Kicinski wrote:
> On Tue, 14 Nov 2023 16:23:29 +0800 Yunsheng Lin wrote:
>> I would expect net stack, page pool, driver still see the 'struct page',
>> only memory provider see the specific struct for itself, for the above,
>> devmem memory provider sees the 'struct page_pool_iov'.
> 
> You can't lie to the driver that an _iov is a page either.

Yes, agreed about that.

As a matter of fact, the driver should be awared of what kind of
memory provider is using when it calls page_pool_create() during
init process.

> The driver must explicitly "opt-in" to using the _iov variant,
> by calling the _iov set of APIs.
> 
> Only drivers which can support header-data split can reasonably
> use the _iov API, for data pages.

But those drivers can still allow allocating normal memory, right?
sometimes for data and header part, and sometimes for the header part.

Do those drivers need to support two sets of APIs? the one with _iov
for devmem, and the one without _iov for normal memory. It seems somewhat
unnecessary from driver' point of veiw to support two sets of APIs?
The driver seems to know which type of page it is expecting when calling
page_pool_alloc() with a specific page_pool instance.

Or do we use the API with _iov to allocate both devmem and normal memory
in the new driver supporting devmem page?  If that is the case, does it
really matter if the API is with _iov or not?

> .
> 

[PATCH RFC 4/8] skbuff: explicitize the semantics of skb_frag_fill_page_desc()

[Yunsheng Lin]

As we have ensured that the page and the offset to the page
for the skb frag is passed togetherly in one function call,
see [1], make it explicit that skb_frag_fill_page_desc()
expect a base page or head page for a compound page, so
that we can avoid the compound_head() in the net stack.

Log a warning if the caller passes a tail page of compoud
page and adjust 'page' to point to it's head page and
'offset' to point to start of it's head page. The warning
can be removed if we have ensured all the caller is passing
non tail page to skb_frag_fill_page_desc() in the future.

1. https://lore.kernel.org/all/20230511011213.59091-2-linyunsheng@huawei.com/

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 include/linux/skbuff.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 27998f73183e..3e2f806c8ed8 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2423,6 +2423,14 @@ static inline void skb_frag_fill_page_desc(skb_frag_t *frag,
 					   struct page *page,
 					   int off, int size)
 {
+	/* expect head page for compound page */
+	if (WARN_ON_ONCE(PageTail(page))) {
+		struct page *head = compound_head(page);
+
+		off += (page_to_pfn(page) - page_to_pfn(head)) * PAGE_SIZE;
+		page = head;
+	}
+
 	frag->bv_page = page;
 	frag->bv_offset = off;
 	skb_frag_size_set(frag, size);
-- 
2.33.0

[PATCH RFC 5/8] skbuff: remove compound_head() related function calling

[Yunsheng Lin]

As we have ensured that the page for the skb frag is
always a base page or head page for a compound page, we
can remove some compound_head() related function.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 include/linux/skbuff.h | 8 +++++---
 net/core/skbuff.c      | 4 +---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3e2f806c8ed8..1889b0968be0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2473,13 +2473,15 @@ static inline void skb_len_add(struct sk_buff *skb, int delta)
 static inline void __skb_fill_page_desc(struct sk_buff *skb, int i,
 					struct page *page, int off, int size)
 {
-	__skb_fill_page_desc_noacc(skb_shinfo(skb), i, page, off, size);
+	skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+
+	skb_frag_fill_page_desc(frag, page, off, size);
 
 	/* Propagate page pfmemalloc to the skb if we can. The problem is
 	 * that not all callers have unique ownership of the page but rely
 	 * on page_is_pfmemalloc doing the right thing(tm).
 	 */
-	page = compound_head(page);
+	page = frag->bv_page;
 	if (page_is_pfmemalloc(page))
 		skb->pfmemalloc	= true;
 }
@@ -3429,7 +3431,7 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
  */
 static inline void __skb_frag_ref(skb_frag_t *frag)
 {
-	get_page(skb_frag_page(frag));
+	page_ref_inc(skb_frag_page(frag));
 }
 
 /**
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b157efea5dea..ada3da4fe221 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -896,8 +896,6 @@ bool napi_pp_put_page(struct page *page, bool napi_safe)
 	bool allow_direct = false;
 	struct page_pool *pp;
 
-	page = compound_head(page);
-
 	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
 	 * in order to preserve any existing bits, such as bit 0 for the
 	 * head page of compound page and bit 1 for pfmemalloc page, so
@@ -939,7 +937,7 @@ static bool skb_pp_recycle(struct sk_buff *skb, void *data, bool napi_safe)
 {
 	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
 		return false;
-	return napi_pp_put_page(virt_to_page(data), napi_safe);
+	return napi_pp_put_page(virt_to_head_page(data), napi_safe);
 }
 
 static void skb_kfree_head(void *head, unsigned int end_offset)
-- 
2.33.0

[PATCH RFC 6/8] skbuff: always try to do page pool frag reference counting

[Yunsheng Lin]

As we have unified the frag_count handling in [1], we can
do frag reference counting instead of native page reference
counting whenever possible in net stack, in order to enable
the best possibility of recycling as we assume other
subsystem other than page pool is also hodling on to a page
if page_ref_count(page) != 1, so we stop the recycling and
release the page from page pool.

As the page from the devmem, we reuse most of the fields in
'struct page' for devmem in order to have unified handling
for both normal memory and devmem, but the meta data is not
really tied to mm, so we can't really use page->_refcount
as reference counting for devmem, instead it relies on page
pool's frag reference counting.

After this patch, pp_frag_count is not appropriate name as
we don't ensure only one user holding on to a frag, we may
rename it to frag_refcount in the future to avoid confusion.

1. https://lore.kernel.org/all/20231020095952.11055-1-linyunsheng@huawei.com/

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c       |  2 +-
 drivers/net/ethernet/sun/cassini.c                  |  4 ++--
 drivers/net/veth.c                                  |  2 +-
 include/linux/skbuff.h                              | 12 +++++++++---
 include/net/page_pool/types.h                       | 13 +++++++++++++
 net/core/skbuff.c                                   |  6 +++---
 net/tls/tls_device_fallback.c                       |  2 +-
 7 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index 6482728794dd..87fda3e9d7d1 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -1658,7 +1658,7 @@ static void chcr_ktls_copy_record_in_skb(struct sk_buff *nskb,
 	for (i = 0; i < record->num_frags; i++) {
 		skb_shinfo(nskb)->frags[i] = record->frags[i];
 		/* increase the frag ref count */
-		__skb_frag_ref(&skb_shinfo(nskb)->frags[i]);
+		skb_frag_ref(nskb, i);
 	}
 
 	skb_shinfo(nskb)->nr_frags = record->num_frags;
diff --git a/drivers/net/ethernet/sun/cassini.c b/drivers/net/ethernet/sun/cassini.c
index b317b9486455..8b818809c721 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -1999,7 +1999,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 		skb->len      += hlen - swivel;
 
 		skb_frag_fill_page_desc(frag, page->buffer, off, hlen - swivel);
-		__skb_frag_ref(frag);
+		__skb_frag_ref(frag, skb->pp_recycle);
 
 		/* any more data? */
 		if ((words[0] & RX_COMP1_SPLIT_PKT) && ((dlen -= hlen) > 0)) {
@@ -2023,7 +2023,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 			frag++;
 
 			skb_frag_fill_page_desc(frag, page->buffer, 0, hlen);
-			__skb_frag_ref(frag);
+			__skb_frag_ref(frag, skb->pp_recycle);
 			RX_USED_ADD(page, hlen + cp->crc_size);
 		}
 
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 9980517ed8b0..8ee323a1184b 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -724,7 +724,7 @@ static void veth_xdp_get(struct xdp_buff *xdp)
 		return;
 
 	for (i = 0; i < sinfo->nr_frags; i++)
-		__skb_frag_ref(&sinfo->frags[i]);
+		__skb_frag_ref(&sinfo->frags[i], false);
 }
 
 static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1889b0968be0..b77043a2b446 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -37,6 +37,7 @@
 #endif
 #include <net/net_debug.h>
 #include <net/dropreason-core.h>
+#include <net/page_pool/types.h>
 
 /**
  * DOC: skb checksums
@@ -3429,9 +3430,14 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
  *
  * Takes an additional reference on the paged fragment @frag.
  */
-static inline void __skb_frag_ref(skb_frag_t *frag)
+static inline void __skb_frag_ref(skb_frag_t *frag, bool recycle)
 {
-	page_ref_inc(skb_frag_page(frag));
+	struct page *page = skb_frag_page(frag);
+
+	if (page_pool_frag_ref(page, recycle))
+		return;
+
+	page_ref_inc(page);
 }
 
 /**
@@ -3443,7 +3449,7 @@ static inline void __skb_frag_ref(skb_frag_t *frag)
  */
 static inline void skb_frag_ref(struct sk_buff *skb, int f)
 {
-	__skb_frag_ref(&skb_shinfo(skb)->frags[f]);
+	__skb_frag_ref(&skb_shinfo(skb)->frags[f], skb->pp_recycle);
 }
 
 bool napi_pp_put_page(struct page *page, bool napi_safe);
diff --git a/include/net/page_pool/types.h b/include/net/page_pool/types.h
index 52e4cf98ebc6..ea4e654168bd 100644
--- a/include/net/page_pool/types.h
+++ b/include/net/page_pool/types.h
@@ -275,6 +275,19 @@ static inline bool is_page_pool_compiled_in(void)
 #endif
 }
 
+static inline bool page_pool_frag_ref(struct page *page, bool recycle)
+{
+	if (recycle && (page->pp_magic & ~0x3UL) == PP_SIGNATURE) {
+		atomic_long_inc(&page->pp_frag_count);
+		return true;
+	}
+
+	BUG_ON((page->pp_magic & ~0x3UL) == PP_SIGNATURE &&
+	       page->pp->mp_ops);
+
+	return false;
+}
+
 /* Caller must provide appropriate safe context, e.g. NAPI. */
 void page_pool_update_nid(struct page_pool *pool, int new_nid);
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ada3da4fe221..a33df6e49694 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4046,7 +4046,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 			to++;
 
 		} else {
-			__skb_frag_ref(fragfrom);
+			__skb_frag_ref(fragfrom, skb->pp_recycle);
 			skb_frag_page_copy(fragto, fragfrom);
 			skb_frag_off_copy(fragto, fragfrom);
 			skb_frag_size_set(fragto, todo);
@@ -4695,7 +4695,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 			}
 
 			*nskb_frag = (i < 0) ? skb_head_frag_to_page_desc(frag_skb) : *frag;
-			__skb_frag_ref(nskb_frag);
+			__skb_frag_ref(nskb_frag, nskb->pp_recycle);
 			size = skb_frag_size(nskb_frag);
 
 			if (pos < offset) {
@@ -5830,7 +5830,7 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	 * since we set nr_frags to 0.
 	 */
 	for (i = 0; i < from_shinfo->nr_frags; i++)
-		__skb_frag_ref(&from_shinfo->frags[i]);
+		__skb_frag_ref(&from_shinfo->frags[i], from->pp_recycle);
 
 	to->truesize += delta;
 	to->len += len;
diff --git a/net/tls/tls_device_fallback.c b/net/tls/tls_device_fallback.c
index 4e7228f275fa..d4000b4a1f7d 100644
--- a/net/tls/tls_device_fallback.c
+++ b/net/tls/tls_device_fallback.c
@@ -277,7 +277,7 @@ static int fill_sg_in(struct scatterlist *sg_in,
 	for (i = 0; remaining > 0; i++) {
 		skb_frag_t *frag = &record->frags[i];
 
-		__skb_frag_ref(frag);
+		__skb_frag_ref(frag, false);
 		sg_set_page(sg_in + i, skb_frag_page(frag),
 			    skb_frag_size(frag), skb_frag_off(frag));
 
-- 
2.33.0

[PATCH RFC 7/8] net: hns3: temp hack for hns3 to use dmabuf memory provider

[Yunsheng Lin]

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 06117502001f..43454aa6a279 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -3371,6 +3371,8 @@ static int hns3_alloc_buffer(struct hns3_enet_ring *ring,
 	struct page *p;
 
 	if (ring->page_pool) {
+		struct page_pool_iov *ppiov;
+
 		p = page_pool_dev_alloc_frag(ring->page_pool,
 					     &cb->page_offset,
 					     hns3_buf_size(ring));
@@ -3378,7 +3380,8 @@ static int hns3_alloc_buffer(struct hns3_enet_ring *ring,
 			return -ENOMEM;
 
 		cb->priv = p;
-		cb->buf = page_address(p);
+		ppiov = cb->priv;
+		cb->buf = page_address(ppiov->page);
 		cb->dma = page_pool_get_dma_addr(p);
 		cb->type = DESC_TYPE_PP_FRAG;
 		cb->reuse_flag = 0;
@@ -4940,7 +4943,7 @@ static void hns3_put_ring_config(struct hns3_nic_priv *priv)
 static void hns3_alloc_page_pool(struct hns3_enet_ring *ring)
 {
 	struct page_pool_params pp_params = {
-		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
+		.flags = 0,//PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
 		.order = hns3_page_order(ring),
 		.pool_size = ring->desc_num * hns3_buf_size(ring) /
 				(PAGE_SIZE << hns3_page_order(ring)),
@@ -4949,6 +4952,8 @@ static void hns3_alloc_page_pool(struct hns3_enet_ring *ring)
 		.dma_dir = DMA_FROM_DEVICE,
 		.offset = 0,
 		.max_len = PAGE_SIZE << hns3_page_order(ring),
+		.memory_provider = PP_MP_DMABUF_DEVMEM,
+		.mp_priv = &dmabuf_devmem_ops,
 	};
 
 	ring->page_pool = page_pool_create(&pp_params);
-- 
2.33.0

[PATCH RFC 8/8] net: temp hack for dmabuf page in __skb_datagram_iter()

[Yunsheng Lin]

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 net/core/datagram.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index 103d46fa0eeb..5556782ac658 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -436,7 +436,15 @@ static int __skb_datagram_iter(const struct sk_buff *skb, int offset,
 		end = start + skb_frag_size(frag);
 		if ((copy = end - offset) > 0) {
 			struct page *page = skb_frag_page(frag);
-			u8 *vaddr = kmap(page);
+			u8 *vaddr;
+
+			if ((page->pp_magic & ~0x3UL) == PP_SIGNATURE) {
+				struct page_pool_iov *ppiov = (struct page_pool_iov *)page;
+
+				page = ppiov->page;
+			}
+
+			vaddr = kmap(page);
 
 			if (copy > len)
 				copy = len;
-- 
2.33.0