Date: Thu, 4 Jan 2024 14:15:52 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Shakeel Butt <shakeelb@...gle.com>, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, kvm@...r.kernel.org, virtualization@...ts.linux.dev, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Stefan Hajnoczi <stefanha@...hat.com>, 
	Stefano Garzarella <sgarzare@...hat.com>, David Howells <dhowells@...hat.com>, 
	Jason Gunthorpe <jgg@...dia.com>, Christian König <christian.koenig@....com>, 
	Yunsheng Lin <linyunsheng@...wei.com>, Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net-next v3 2/3] net: introduce abstraction for network memory

On Thu, Jan 4, 2024 at 1:44 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 21 Dec 2023 15:44:22 -0800 Mina Almasry wrote:
> > The warning is like so:
> >
> > ./include/net/page_pool/helpers.h: In function ‘page_pool_alloc’:
> > ./include/linux/stddef.h:8:14: warning: returning ‘void *’ from a
> > function with return type ‘netmem_ref’ {aka ‘long unsigned int’} makes
> > integer from pointer without a cast [-Wint-conversion]
> >     8 | #define NULL ((void *)0)
> >       |              ^
> > ./include/net/page_pool/helpers.h:132:24: note: in expansion of macro
> > ‘NULL’
> >   132 |                 return NULL;
> >       |                        ^~~~
> >
> > And happens in all the code where:
> >
> > netmem_ref func()
> > {
> >     return NULL;
> > }
> >
> > It's fixable by changing the return to `return (netmem_ref NULL);` or
> > `return 0;`, but I feel like netmem_ref should be some type which
> > allows a cast from NULL implicitly.
>
> Why do you think we should be able to cast NULL implicitly?
> netmem_ref is a handle, it could possibly be some form of
> an ID in the future, rather than a pointer. Or have more low
> bits stolen for specific use cases.
>
> unsigned long, and returning 0 as "no handle" makes perfect sense to me.
>
> Note that 0 is a special case, bitwise types are allowed to convert
> to 0/bool and 0 is implicitly allowed to become a bitwise type.
> This will pass without a warning:
>
> typedef unsigned long __bitwise netmem_ref;
>
> netmem_ref some_code(netmem_ref ref)
> {
>         // direct test is fine
>         if (!ref)
>                 // 0 "upgrades" without casts
>                 return 0;
>         // 1 does not, we need __force
>         return (__force netmem_ref)1 | ref;
> }
>
> The __bitwise annotation will make catching people trying
> to cast to struct page * trivial.
>
> You seem to be trying hard to make struct netmem a thing.
> Perhaps you have a reason I'm not getting?

There are a number of functions that return struct page* today that I
convert to return struct netmem* later in the child devmem series, one
example is something like:

struct page *page_pool_alloc(...); // returns NULL on failure.

becomes:

struct netmem *page_pool_alloc(...); // also returns NULL on failure.

rather than,

netmem_ref page_pool_alloc(...); // returns 0 on failure.

I guess in my mind having NULL be castable to the new type makes it so
that I can avoid the additional code churn of converting a bunch of
`return NULL;` to `return 0;`, and maybe the transition from page
pointers to netmem pointers can be more easily done if they're both
compatible pointer types.

But that is not any huge blocker or critical point in my mind, I just
thought this approach is preferred. If conversion to unsigned long
makes more sense to you, I'll respin this like that and do the `NULL
-> 0` conversion everywhere as needed.

-- 
Thanks,
Mina

Powered by blists - more mailing lists