Message-ID: <CAHS8izOeCdA+WVRYbieTqaCyadARsOpYttAXh7Lhu-B7RC3Tmg@mail.gmail.com>
Date: Sun, 17 Dec 2023 00:14:59 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: David Ahern <dsahern@...nel.org>
Cc: Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	bpf@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Sumit Semwal <sumit.semwal@...aro.org>, 
	Christian König <christian.koenig@....com>, 
	Michael Chan <michael.chan@...adcom.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, Wei Fang <wei.fang@....com>, 
	Shenwei Wang <shenwei.wang@....com>, Clark Wang <xiaoning.wang@....com>, 
	NXP Linux Team <linux-imx@....com>, Jeroen de Borst <jeroendb@...gle.com>, 
	Praveen Kaligineedi <pkaligineedi@...gle.com>, Shailend Chand <shailend@...gle.com>, 
	Yisen Zhuang <yisen.zhuang@...wei.com>, Salil Mehta <salil.mehta@...wei.com>, 
	Jesse Brandeburg <jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Marcin Wojtas <mw@...ihalf.com>, 
	Russell King <linux@...linux.org.uk>, Sunil Goutham <sgoutham@...vell.com>, 
	Geetha sowjanya <gakula@...vell.com>, Subbaraya Sundeep <sbhatta@...vell.com>, 
	hariprasad <hkelam@...vell.com>, Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>, 
	Sean Wang <sean.wang@...iatek.com>, Mark Lee <Mark-MC.Lee@...iatek.com>, 
	Lorenzo Bianconi <lorenzo@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
	Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>, 
	Horatiu Vultur <horatiu.vultur@...rochip.com>, UNGLinuxDriver@...rochip.com, 
	"K. Y. Srinivasan" <kys@...rosoft.com>, Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, 
	Dexuan Cui <decui@...rosoft.com>, Jassi Brar <jaswinder.singh@...aro.org>, 
	Ilias Apalodimas <ilias.apalodimas@...aro.org>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Siddharth Vadapalli <s-vadapalli@...com>, 
	Ravi Gunasekaran <r-gunasekaran@...com>, Roger Quadros <rogerq@...nel.org>, 
	Jiawen Wu <jiawenwu@...stnetic.com>, Mengyuan Lou <mengyuanlou@...-swift.com>, 
	Ronak Doshi <doshir@...are.com>, VMware PV-Drivers Reviewers <pv-drivers@...are.com>, 
	Ryder Lee <ryder.lee@...iatek.com>, Shayne Chen <shayne.chen@...iatek.com>, 
	Kalle Valo <kvalo@...nel.org>, Juergen Gross <jgross@...e.com>, 
	Stefano Stabellini <sstabellini@...nel.org>, 
	Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	Stefan Hajnoczi <stefanha@...hat.com>, Stefano Garzarella <sgarzare@...hat.com>, Shuah Khan <shuah@...nel.org>, 
	Mickaël Salaün <mic@...ikod.net>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
	Jason Gunthorpe <jgg@...dia.com>, Shakeel Butt <shakeelb@...gle.com>, 
	Yunsheng Lin <linyunsheng@...wei.com>, Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [RFC PATCH net-next v1 2/4] net: introduce abstraction for
 network memory

On Sat, Dec 16, 2023 at 5:45 PM David Ahern <dsahern@...nel.org> wrote:
>
> On 12/16/23 2:10 PM, Mina Almasry wrote:
> > On Fri, Dec 15, 2023 at 6:52 PM Jakub Kicinski <kuba@...nel.org> wrote:
> >>
> >> On Wed, 13 Dec 2023 18:05:25 -0800 Mina Almasry wrote:
> >>> +struct netmem {
> >>> +     union {
> >>> +             struct page page;
> >>> +
> >>> +             /* Stub to prevent compiler implicitly converting from page*
> >>> +              * to netmem_t* and vice versa.
> >>> +              *
> >>> +              * Other memory type(s) net stack would like to support
> >>> +              * can be added to this union.
> >>> +              */
> >>> +             void *addr;
> >>> +     };
> >>> +};
> >>
> >> My mind went to something like:
> >>
> >> typedef unsigned long __bitwise netmem_ref;
> >>
> >> instead. struct netmem does not exist, it's a handle which has
> >> to be converted to a real type using a helper.
> >
> > Sure thing I can do that. Is it better to do something like:
> >
> > struct netmem_ref;
> >
> > like in this patch:
> >
> > https://lore.kernel.org/linux-mm/20221108194139.57604-1-torvalds@linux-foundation.org/
> >
> > Asking because checkpatch tells me not to add typedefs to the kernel,
> > but checkpatch can be ignored if you think it's OK.
> >
> > Also with this approach I can't use container_of and I need to do a
> > cast, I assume that's fine.
> >
>
> Isn't that the whole point of this set - to introduce a new data type
> and avoid casts?

My understanding here the requirements from Jason are:

1. Never pass a non-page to an mm api.
2. If a mangle a pointer to indicate it's not a page, then I must not
call it mm's struct page*, I must add a new type.

I think both requirements are met regardless of whether
netmem_to_page() is implemented using union/container_of or straight
casts. folios implemented something similar being unioned with struct
page to avoid casts. I honestly could go either way on this. The union
provides some self documenting code and avoids casts. The
implementation without the union obfuscates the type and makes it much
more opaque.

I finished addressing the rest of the comments and I have this series
and the next devmem TCP series ready to go, so I fired v2 of this
patchset. If one feels strongly about this let me know and I will
re-spin.

-- 
Thanks,
Mina

Powered by blists - more mailing lists