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/ > >> >> 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/ > > Thanks, > Olek > . >
