| Message ID | 20231214020530.2267499-3-almasrymina@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Abstract page from net stack | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
On 12/13/23 7:05 PM, Mina Almasry wrote: > diff --git a/include/net/netmem.h b/include/net/netmem.h > new file mode 100644 > index 000000000000..e4309242d8be > --- /dev/null > +++ b/include/net/netmem.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * netmem.h > + * Author: Mina Almasry <almasrymina@google.com> > + * Copyright (C) 2023 Google LLC > + */ > + > +#ifndef _NET_NETMEM_H > +#define _NET_NETMEM_H > + > +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; > + }; > +}; > + > +static inline struct page *netmem_to_page(struct netmem *netmem) > +{ > + return &netmem->page; > +} > + > +static inline struct netmem *page_to_netmem(struct page *page) > +{ > + return (struct netmem *)page; container_of; no typecasts. > +} > + > +#endif /* _NET_NETMEM_H */
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.
On Fri, Dec 15, 2023 at 6:52 PM Jakub Kicinski <kuba@kernel.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.
On 12/16/23 2:10 PM, Mina Almasry wrote: > On Fri, Dec 15, 2023 at 6:52 PM Jakub Kicinski <kuba@kernel.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?
On Sat, Dec 16, 2023 at 5:45 PM David Ahern <dsahern@kernel.org> wrote: > > On 12/16/23 2:10 PM, Mina Almasry wrote: > > On Fri, Dec 15, 2023 at 6:52 PM Jakub Kicinski <kuba@kernel.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.
On Sun, 17 Dec 2023 00:14:59 -0800 Mina Almasry wrote: > > > 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? I don't see how we can avoid casts if the type of the referenced object is encoded on the low bits of the pointer. If we had a separate member we could so something like: struct netmem_ref { enum netmem_type type; union { struct page *p; struct page_pool_iov *pi; }; }; barring crazy things with endian-aware bitfields, we need at least one cast. > 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. Folios overlay a real struct page. It's completely different. > I honestly could go either way on this. The union > provides some self documenting code and avoids casts. Maybe you guys know some trick to mask out the bottom bit :S > The implementation without the union obfuscates the type and makes it much > more opaque. Some would say that that's the damn point of the wrapping.. You don't want non-core code futzing with the inside of the struct. > 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. You didn't address my feedback :| struct netmem which contains struct page by value is almost as bad as passing around pretend struct page pointers.
On Mon, Dec 18, 2023 at 2:06 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Sun, 17 Dec 2023 00:14:59 -0800 Mina Almasry wrote: > > > > 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? > > I don't see how we can avoid casts if the type of the referenced object > is encoded on the low bits of the pointer. If we had a separate member > we could so something like: > > struct netmem_ref { > enum netmem_type type; > union { > struct page *p; > struct page_pool_iov *pi; > }; > }; > > barring crazy things with endian-aware bitfields, we need at least one > cast. > > > 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. > > Folios overlay a real struct page. It's completely different. > > > I honestly could go either way on this. The union > > provides some self documenting code and avoids casts. > > Maybe you guys know some trick to mask out the bottom bit :S > > > The implementation without the union obfuscates the type and makes it much > > more opaque. > > Some would say that that's the damn point of the wrapping.. > > You don't want non-core code futzing with the inside of the struct. > > > 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. > > You didn't address my feedback :| > > struct netmem which contains struct page by value is almost as bad > as passing around pretend struct page pointers. Sorry about that. I misread your original request as 'here is something else you can do if you want', not something that you feel is critical. Honestly I missed the subtlety and the approaches seemed roughly equivalent to me. I will respin after the 24hr cooldown.
On Mon, Dec 18, 2023 at 2:39 PM Mina Almasry <almasrymina@google.com> wrote: > [...] > > > > You didn't address my feedback :| > > > > struct netmem which contains struct page by value is almost as bad > > as passing around pretend struct page pointers. > > Sorry about that. I misread your original request as 'here is > something else you can do if you want', not something that you feel is > critical. Honestly I missed the subtlety and the approaches seemed > roughly equivalent to me. I will respin after the 24hr cooldown. > Jakub's suggestion aligns more with the encoded_page approach as well, so let's proceed with that. Waiting for your respin.
diff --git a/include/net/netmem.h b/include/net/netmem.h new file mode 100644 index 000000000000..e4309242d8be --- /dev/null +++ b/include/net/netmem.h @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * netmem.h + * Author: Mina Almasry <almasrymina@google.com> + * Copyright (C) 2023 Google LLC + */ + +#ifndef _NET_NETMEM_H +#define _NET_NETMEM_H + +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; + }; +}; + +static inline struct page *netmem_to_page(struct netmem *netmem) +{ + return &netmem->page; +} + +static inline struct netmem *page_to_netmem(struct page *page) +{ + return (struct netmem *)page; +} + +#endif /* _NET_NETMEM_H */
Add the netmem_t type, an abstraction for network memory. To add support for new memory types to the net stack, we must first abstract the current memory type from the net stack. Currently parts of the net stack use struct page directly: - page_pool - drivers - skb_frag_t Originally the plan was to reuse struct page* for the new memory types, and to set the LSB on the page* to indicate it's not really a page. However, for compiler type checking we need to introduce a new type. netmem_t is introduced to abstract the underlying memory type. Currently it's a no-op abstraction that is always a struct page underneath. In parallel there is an undergoing effort to add support for devmem to the net stack: https://lore.kernel.org/netdev/20231208005250.2910004-1-almasrymina@google.com/ Signed-off-by: Mina Almasry <almasrymina@google.com> --- include/net/netmem.h | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 include/net/netmem.h