On Wed, Jul 10, 2024 at 6:23 PM Jakub Kicinski kuba@kernel.org wrote:
On Wed, 10 Jul 2024 16:42:04 -0700 Mina Almasry wrote:
+static inline void netmem_set_pp(netmem_ref netmem, struct page_pool *pool) +{
__netmem_clear_lsb(netmem)->pp = pool;+}
Why is all this stuff in the main header? It's really low level. Please put helpers which are only used by the core in a header under net/core/, like net/core/dev.h
Sorry none of those are only used by net/core/*. Pretty much all of these are used by include/net/page_pool/helpers.h, and some have callers in net/core/devmem.c or net/core/skbuff.c
Would you like me to move these pp specific looking ones to include/net/page_pool/netmem.h or something similar?
That's because some things already in helpers have no real business being there either. Why is page_pool_set_pp_info() in helpers.h?
OK, I looked into this a bit. It looks like I can trivially move page_pool_set/clear_pp_info() to page_pool_priv.h, and that lets me move out a few of these netmem helpers to a header under net/core.
However, to move more of these netmem helpers to a private header, I think I need to move all the page pool dma helpers and reffing helpers to a private header or the .c file, which I think will uninline them as they're eventually called from drivers.
I had guessed the previous authors put those dma and ref helpers in the .h file to inline them as they're used in fast paths. Do you think the refactor and the uninling is desirable? Or should I just do with the trivial moving of the page_pool_set/clear_pp_info() to the private file?