On Wed, Jul 26, 2023 at 4:23 AM Yunsheng Lin <[email protected]> wrote:
>
> On 2023/7/26 18:43, Alexander Lobakin wrote:
> > From: Alexander H Duyck <[email protected]>
> > 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 <[email protected]>
> >>> Suggested-by: Jakub Kicinski <[email protected]>
> >>> CC: Alexander Lobakin <[email protected]>
> >
> > [...]
> >
> >>> +/* 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/[email protected]/
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/[email protected]/
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.
On Wed, Jul 26, 2023 at 8:30 AM Alexander Duyck
<[email protected]> wrote:
>
> On Wed, Jul 26, 2023 at 4:23 AM Yunsheng Lin <[email protected]> wrote:
> >
> > On 2023/7/26 18:43, Alexander Lobakin wrote:
> > > From: Alexander H Duyck <[email protected]>
> > > 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 <[email protected]>
> > >>> Suggested-by: Jakub Kicinski <[email protected]>
> > >>> CC: Alexander Lobakin <[email protected]>
> > >
> > > [...]
> > >
> > >>> +/* 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/[email protected]/
>
> 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/[email protected]/
>
> 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..