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@google.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;
...