Re: [PATCH net-next v2] page_pool: split types and declarations from page_pool.h

From: Alexander Duyck <redacted>
Date: Wed, 26 Jul 2023 08:30:17 -0700

On Wed, Jul 26, 2023 at 4:23 AM Yunsheng Lin [off-list ref] wrote:

quoted

On 2023/7/26 18:43, Alexander Lobakin wrote:

quoted

From: Alexander H Duyck <redacted>
Date: Tue, 25 Jul 2023 08:47:46 -0700

quoted

On Tue, 2023-07-25 at 21:12 +0800, Yunsheng Lin wrote:

quoted

Split types and pure function declarations from page_pool.h
and add them in page_pool/types.h, so that C sources can
include page_pool.h and headers should generally only include
page_pool/types.h as suggested by jakub.

Signed-off-by: Yunsheng Lin <redacted>
Suggested-by: Jakub Kicinski <kuba@kernel.org>
CC: Alexander Lobakin <aleksander.lobakin@intel.com>

[...]

quoted

quoted

+/* Caller must provide appropriate safe context, e.g. NAPI. */
+void page_pool_update_nid(struct page_pool *pool, int new_nid);
+
+#endif /* _NET_PAGE_POOL_H */

This seems kind of overkill for what is needed. It seems like the
general thought process with splitting this was so that you had just
the minimum of what is needed to support skbuff.h and the functions
declared there. The rest of this would then be added via the .h to the
.c files that will actually be calling the functions.

By that logic I think the only thing we really need is the function
declaration for page_pool_return_skb_page moved into skbuff.h. We could
then just remove page_pool.h from skbuff.h couldn't we?

This patch is not to drop page_pool.h include from skbuff.h.
This is more future-proof (since I'm dropping this include anyway in my
series) to have includes organized and prevent cases like that one with
skbuff.h from happening. And to save some CPU cycles on preprocessing if
that makes sense.

The suggestion is from below:

https://lore.kernel.org/all/20230710113841.482cbeac@kernel.org/ (local)

I get that. But it seemed like your types.h is full of inline
functions. That is what I was responding to. I would leave the inline

Ah, okay. So this was reply to my proposal, not Yunsheng's. I missed
that ._.

functions in page_pool.h unless there is some significant need for
them.

Only in order to not have the same functions defined in either types.h
or helpers.h depending on the kernel configuration -- this is
counter-intuitive and doesn't allow to use types.h when we don't need
driver-facing API.
Those inlines don't require any includes and 99% of them are empty (with
1% returning true/false depending on the kernel conf). What's the point
of breaking declaration consistency if we don't win anything?

quoted

quoted

quoted

Another thing we could consider doing is looking at splitting things up
so that we had a include file in net/core/page_pool.h to handle some of
the cases where we are just linking the page_pool bits to other core
file bits such as xdp.c and skbuff.c.

I suppose the above suggestion is about splitting or naming by
the user as the discussed in the below thread?
https://lore.kernel.org/all/20230721182942.0ca57663@kernel.org/ (local)

Actually my suggestion is more about defining boundaries for what is
meant to be used by drivers and what isn't. The stuff you could keep

helpers.h is to be used by drivers, types.h by kernel core. Mostly :D

in net/core/page_pool.h would only be usable by the files in net/core/
whereas the stuff you are keeping in the include/net/ folder is usable
by drivers. It is meant to prevent things like what you were
complaining about with the Mellanox drivers making use of interfaces

You mean abusing DMA allocation function by manual rewriting of device's
NUMA node? If you want to avoid that, you need to make struct device
private, I don't think it's a good idea. Otherwise, there will always be
a room for exploiting the internals up to some point.

you didn't intend them to use.

So for example you could pull out functions like
page_pool_return_skb_page, page_pool_use_xdp_mem,
page_pool_update_nid, and the like and look at relocating them into

update_nid() is used by drivers.

the net/core/ folder and thereby prevent abuse of those functions by
drivers.

I don't think there are that many internal functions, so that it would
be worth separate header.

Thanks,
Olek