Messages in this thread | | | Date | Fri, 19 May 2023 13:45:45 -0700 | From | Jakub Kicinski <> | Subject | Re: [Intel-wired-lan] [PATCH net-next 07/11] net: page_pool: add DMA-sync-for-CPU inline helpers |
| |
On Fri, 19 May 2023 15:56:40 +0200 Alexander Lobakin wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Thu, 18 May 2023 13:36:27 -0700 > >> I'll definitely take a look, I also like the idea of minimalistic and > >> lightweight headers. > >> page_pool.h and page_pool_drv.h? :D > > > > What I've been doing lately is split like this: > > > > include/net/something.h (simply includes all other headers) > > include/net/something/types.h (structs, defines, enums) > > include/net/something/functions.h (inlines and function declarations) > > > > If that's reasonable -- we should put the helpers under > > > > include/net/page_pool/functions.h ? > > Hmm, all files that need something from page_pool.h usually need both > types and functions. Not sure we'll benefit anything here.
Ack, in the scheme above most places (source files) would include something.h, the something/types.h is just for other headers. something/functions.h is basically never included directly.
> OTOH leaving > those sync-for-cpu inlines alone allows to avoid including dma-mapping.h > and currently only IAVF needs them. So my idea is: > > - you need smth from PP, but not sync-for-cpu -- more lightweight > page_pool.h is for you; > - you need sync-for-cpu (or maybe something else with heavy deps in the > future) -- just include page_pool_drv.h.
The idea makes sense in isolation, but I'm trying to figure out a convention which would not require case-by-case discussions.
> I tried moving something else, but couldn't find anything that would > give any win. <linux/mm.h> and <linux/ptr_ring.h> are needed to define > `struct page_pool`, i.e. even being structured like in your example they > would've gone into pp/types.h =\ > `struct ptr_ring` itself doesn't require any MM-related definitions, so > would we split it into ptr_ring/{types,functions}.h, we could probably > avoid a couple includes :D
Ack, not saying that we need to split now, it's just about the naming (everyone's favorite topic).
I think that it's a touch weird to name the header _drv.h and then include it in the core in multiple places (*cough* xdp_sock_drv.h). Also If someone needs to add another "heavy" static line for use by the core they will try to put it in page_pool.h rather than _drv.h...
I'd rather split the includes by the basic language-level contents, first, then by the intended consumer, only if necessary. Language level sorting require less thinking :)
But none of this is important, if you don't wanna to do it, just keep the new helpers in page_pool.h (let's not do another _drv.h).
| |