On Wed, Jul 26, 2023 at 8:30 AM Alexander Duyck <alexander.duyck@xxxxxxxxx> wrote: > > On Wed, Jul 26, 2023 at 4:23 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote: > > > > On 2023/7/26 18:43, Alexander Lobakin wrote: > > > From: Alexander H Duyck <alexander.duyck@xxxxxxxxx> > > > Date: Tue, 25 Jul 2023 08:47:46 -0700 > > > > > >> On Tue, 2023-07-25 at 21:12 +0800, Yunsheng Lin wrote: > > >>> 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 <linyunsheng@xxxxxxxxxx> > > >>> Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx> > > >>> CC: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx> > > > > > > [...] > > > > > >>> +/* 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@xxxxxxxxxx/ > > 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 > functions in page_pool.h unless there is some significant need for > them. > > > > > > >> > > >> 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@xxxxxxxxxx/ > > 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 > 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 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 > the net/core/ folder and thereby prevent abuse of those functions by > drivers. Okay, maybe not page_pool_update_nid. It looks like that is already in use in the form of page_pool_nid_changed by drivers..
