Message-ID: <3374356e-5f4b-4a6f-bb19-8cb7c56103bc@gmail.com>
Date: Tue, 13 Feb 2024 13:18:26 +0000
From: Pavel Begunkov <asml.silence@...il.com>
To: Mina Almasry <almasrymina@...gle.com>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-alpha@...r.kernel.org, linux-mips@...r.kernel.org,
 linux-parisc@...r.kernel.org, sparclinux@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
 bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>,
 Richard Henderson <richard.henderson@...aro.org>,
 Ivan Kokshaysky <ink@...assic.park.msu.ru>, Matt Turner
 <mattst88@...il.com>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
 Helge Deller <deller@....de>, Jesper Dangaard Brouer <hawk@...nel.org>,
 Ilias Apalodimas <ilias.apalodimas@...aro.org>,
 Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Arnd Bergmann <arnd@...db.de>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Andrii Nakryiko <andrii@...nel.org>, David Ahern <dsahern@...nel.org>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 Shuah Khan <shuah@...nel.org>, Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 David Wei <dw@...idwei.uk>, Jason Gunthorpe <jgg@...pe.ca>,
 Yunsheng Lin <linyunsheng@...wei.com>, Shailend Chand <shailend@...gle.com>,
 Harshitha Ramamurthy <hramamurthy@...gle.com>,
 Shakeel Butt <shakeelb@...gle.com>, Jeroen de Borst <jeroendb@...gle.com>,
 Praveen Kaligineedi <pkaligineedi@...gle.com>
Subject: Re: [RFC PATCH net-next v5 07/14] page_pool: devmem support

On 12/18/23 02:40, Mina Almasry wrote:
> Convert netmem to be a union of struct page and struct netmem. Overload
> the LSB of struct netmem* to indicate that it's a net_iov, otherwise
> it's a page.
> 
> Currently these entries in struct page are rented by the page_pool and
> used exclusively by the net stack:
> 
> struct {
> 	unsigned long pp_magic;
> 	struct page_pool *pp;
> 	unsigned long _pp_mapping_pad;
> 	unsigned long dma_addr;
> 	atomic_long_t pp_ref_count;
> };
> 
> Mirror these (and only these) entries into struct net_iov and implement
> netmem helpers that can access these common fields regardless of
> whether the underlying type is page or net_iov.
> Implement checks for net_iov in netmem helpers which delegate to mm
> APIs, to ensure net_iov are never passed to the mm stack.
> 
> Signed-off-by: Mina Almasry <almasrymina@...gle.com>
> 
> ---
> 
> RFCv5:
> - Use netmem instead of page* with LSB set.
> - Use pp_ref_count for refcounting net_iov.
> - Removed many of the custom checks for netmem.
> 
> v1:
> - Disable fragmentation support for iov properly.
> - fix napi_pp_put_page() path (Yunsheng).
> - Use pp_frag_count for devmem refcounting.
> 
> ---
>   include/net/netmem.h            | 145 ++++++++++++++++++++++++++++++--
>   include/net/page_pool/helpers.h |  25 +++---
>   net/core/page_pool.c            |  26 +++---
>   net/core/skbuff.c               |   9 +-
>   4 files changed, 164 insertions(+), 41 deletions(-)
> 
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 31f338f19da0..7557aecc0f78 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -12,11 +12,47 @@
>   
>   /* net_iov */
>   
> +DECLARE_STATIC_KEY_FALSE(page_pool_mem_providers);
> +
> +/*  We overload the LSB of the struct page pointer to indicate whether it's
> + *  a page or net_iov.
> + */
> +#define NET_IOV 0x01UL
> +
>   struct net_iov {
> +	unsigned long __unused_padding;
> +	unsigned long pp_magic;
> +	struct page_pool *pp;
>   	struct dmabuf_genpool_chunk_owner *owner;
>   	unsigned long dma_addr;
> +	atomic_long_t pp_ref_count;
>   };

I wonder if it would be better to extract a common sub-struct
used in struct page, struct_group_tagged can help to avoid
touching old code:

struct page {
	unsigned long flags;
	union {
		...
		struct_group_tagged(<struct_name>, ...,
			/**
			 * @pp_magic: magic value to avoid recycling non
			 * page_pool allocated pages.
			 */
			unsigned long pp_magic;
			struct page_pool *pp;
			unsigned long _pp_mapping_pad;
			unsigned long dma_addr;
			atomic_long_t pp_ref_count;
		);
	};
}

struct net_iov {
	unsigned long pad;
	struct <struct_name> p;
};


A bit of a churn with the padding and nesting net_iov but looks
sturdier. No duplication, and you can just check positions of the
structure instead of per-field NET_IOV_ASSERT_OFFSET, which you
have to not forget to update e.g. when adding a new field. Also,
with the change __netmem_clear_lsb can return a pointer to that
structure, casting struct net_iov when it's a page is a bit iffy.

And the next question would be whether it'd be a good idea to encode
iov vs page not by setting a bit but via one of the fields in the
structure, maybe pp_magic.

With that said I'm a bit concerned about the net_iov size. If each
represents 4096 bytes and you're registering 10MB, then you need
30 pages worth of memory just for the iov array. Makes kvmalloc
a must even for relatively small sizes.

And the final bit, I don't believe the overlay is necessary in
this series. Optimisations are great, but this one is a bit more on
the controversial side. Unless I missed something and it does make
things easier, it might make sense to do it separately later.


> +/* These fields in struct page are used by the page_pool and net stack:
> + *
> + *	struct {
> + *		unsigned long pp_magic;
> + *		struct page_pool *pp;
> + *		unsigned long _pp_mapping_pad;
> + *		unsigned long dma_addr;
> + *		atomic_long_t pp_ref_count;
> + *	};
> + *
> + * We mirror the page_pool fields here so the page_pool can access these fields
> + * without worrying whether the underlying fields belong to a page or net_iov.
> + *
> + * The non-net stack fields of struct page are private to the mm stack and must
> + * never be mirrored to net_iov.
> + */
> +#define NET_IOV_ASSERT_OFFSET(pg, iov)             \
> +	static_assert(offsetof(struct page, pg) == \
> +		      offsetof(struct net_iov, iov))
> +NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> +NET_IOV_ASSERT_OFFSET(pp, pp);
> +NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> +NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> +#undef NET_IOV_ASSERT_OFFSET
> +
>   static inline struct dmabuf_genpool_chunk_owner *
>   net_iov_owner(const struct net_iov *niov)
>   {
> @@ -47,19 +83,25 @@ net_iov_binding(const struct net_iov *niov)
>   struct netmem {
>   	union {
>   		struct page page;
> -
> -		/* Stub to prevent compiler implicitly converting from page*
> -		 * to netmem_t* and vice versa.
> -		 *
> -		 * Other memory type(s) net stack would like to support
> -		 * can be added to this union.
> -		 */
> -		void *addr;
> +		struct net_iov niov;
>   	};
>   };
>   
...

-- 
Pavel Begunkov

Powered by blists - more mailing lists