Message-ID: <20220722111951.6a2fd39c@kernel.org>
Date:   Fri, 22 Jul 2022 11:19:51 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Yury Norov <yury.norov@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/4] netlink: add 'bitmap' attribute type and
 API

On Fri, 22 Jul 2022 16:55:14 +0200 Alexander Lobakin wrote:
> > I think that straight up bitmap with a fixed word is awkward and leads
> > to too much boilerplate code. People will avoid using it. What about
> > implementing a bigint type instead? Needing more than 64b is extremely
> > rare, so in 99% of the cases the code outside of parsing can keep using
> > its u8/u16/u32.  
> 
> In-kernel code can still use single unsigned long for some flags if
> it wouldn't need more than 64 bits in a couple decades and not
> bother with the bitmap API. Same with userspace -- a single 64 is
> fine for that API, just pass a pointer to it to send it as a bitmap
> to the kernel.

Single unsigned long - exactly. What I'm saying is that you need to
convince people who think they are "clever" by using u8 or u16 for
flags because it "saves space". Happens a lot, I can tell your from
reviewing such patches. And to avoid that we should give people a
"growable" type but starting from smaller sizes than a bitmap.

It's about human psychology as observed, not logic, just to be extra
clear.

A logical argument of smaller importance is that ID types have a
similar problem, although practically it's even more rare than for
flags  (I think it did happen once with inodes or some such, tho). 
So bigint has more uses.

I'd forgo the endianness BTW, it's a distraction at the netlink level.
There was more reason for it in fixed-size fields.

> Re 64b vs extremely rare -- I would say so 5 years go, but now more
> and more bitfields run out of 64 bits. Link modes, netdev features,
> ...

I like how you put the "..." there even tho these are the only two cases
either of us can think of, right? :) There are hundreds of flags in the
kernel, two needed to grow into a bitmap. And the problem you're
working on is with a 16 bit field, or 32 bit filed? Defo not 64b, right?

> Re bigint -- do you mean implementing u128 as a union, like
> 
> typedef union __u128 {
> 	struct {
> 		u32 b127_96;
> 		u32 b95_64;
> 		u32 b63_32;
> 		u32 b31_0;
> 	};
> 	struct {
> 		u64 b127_64;
> 		u64 b63_0;
> 	};
> #ifdef __HAVE_INT128
> 	__int128 b127_0;
> #endif
> } u128;
> 
> ? We have similar feature in one of our internal trees and planning
> to present generic u128 soon, but this doesn't work well for flags
> I think.

Actually once a field crosses the biggest natural int size I was
thinking that the user would go to a bitmap.

So at the netlink level the field is bigint (LE, don't care about BE).
Kernel side API is:

	nla_get_b8, nla_get_b16, nla_get_b32, nla_get_b64,
	nla_get_bitmap
	nla_put_b8, nla_put_b16 etc.

	u16 my_flags_are_so_toight;

	my_flags_are_so_toight = nla_get_b16(attr[BLAA_BLA_BLA_FLAGS]);

The point is - the representation can be more compact than u64 and will
therefore encourage anyone who doesn't have a strong reason to use
fixed size fields to switch to the bigint.

Honestly, if we were redoing netlink from scratch today I'd argue there
should be no fixed width integers in host endian.

> bitmap API and bitops are widely used and familiar to tons of folks,
> most platforms define their own machine-optimized bitops
> implementation, arrays of unsigned longs are native...
> 
> Re awkward -- all u64 <-> bitmap conversion is implemented in the
> core code in 4/4 and users won't need doing anything besides one
> get/set. And still use bitmap/bitops API. Userspace, as I said,
> can use a single __u64 as long as it fits into 64 bits.

Yes, but people will see a bitmap and think "I don't need a bitmap, 
I just need X bits".

> Summarizing, I feel like bigints would lead to much more boilerplate
> in both kernel and user spaces and need to implement a whole new API
> instead of using the already existing and well-used bitmap one.
> Continuation of using single objects with fixed size like %NLA_U* or
> %NLA_BITFIELD_U32 will lead to introducing a new Netlink attr every
> 32/64 bits (or even 16 like with IP tunnels, that was the initial
> reason why I started working on those 3 series). As Jake wrote me
> in PM earlier,

Every netlink attribute carries length. As long as you establish
correct expectations for parsing there is no need to create new types.

> "I like the concept of an NLA_BITMAP. I could have used this for
> some of the devlink interfaces we've done, and it definitely feels
> a bit more natural than being forced to a single u32 bitfield."

Slightly ironic to say "if I could use a larger type I would" 
and then not use the largest one available? ;) But the point is
about being flexible when it comes to size, I guess, more than
bitmap in particular.

Powered by blists - more mailing lists