Message-Id: <20220725130255.3943438-1-alexandr.lobakin@intel.com>
Date:   Mon, 25 Jul 2022 15:02:55 +0200
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        "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

From: Jakub Kicinski <kuba@...nel.org>
Date: Fri, 22 Jul 2022 11:19:51 -0700

> 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
  ^^^^^^^^^^^^^^^^^^^^^^

Oh true!

> "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.

Hmm yeah, and I don't use explicit-endian arr64s in the IP tunnel
changeset. Probably not worth it, I did it only for some... uhm,
_flexibility_.

> 
> > 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

Hahaha you got me!

> 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?

It's even worse, IP tunnel flags is __be16, so I have to redo all
the use sites either case, whether I change them to u64 or bitmap
or even __be64 -- no way to optimize it to a noop >_<

Initially I've converted them to u64 IIRC. Then I looked at the
timeline of adding new flags and calculated that in the best case
we'll run out of u64 in 20 years. But there's a spike in ~5 new
flags during the last n months, so... Given that it took a lot of
routine non-so-exicing adjusting all the use sites (I can't imagine
how that guy who's converting netdev features to bitmaps right now
managed finish redoing 120+ drivers under drivers/net/), if/when
`u64 flags` comes to its end, there will be a new patch series
doing the same job again...

> 
> > 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.

Ahh looks like I got it! So you mean that at Netlink level we should
exchange with bigint/u64arrs, but there should be an option to
get/set only 16/32/64 bits from them to simplify (or keep simple)
users? Like, if we have `u16 uuid`, to not do

	unsigned long uuid_bitmap;

	nla_get_bitmap(attr[FLAGS], &uuid_bitmap, BITS_PER_TYPE(u16));
	uuid = (u16)uuid_bitmap;

but instead

	uuid = nla_get_b16(attr[FLAGS]);

?

> 
> 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.

Right, but %NLA_BITFIELD_U32 landed +/- recently IIRC :)


> 
> > "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

:D Agree

> about being flexible when it comes to size, I guess, more than
> bitmap in particular.

Probably, but you also said above that for anything bigger than
longs you'd go for bitmaps, didn't you? So I guess that series
goes in the right direction, just needs a couple more inlines
to be able to get/put u{32, 64; 8, 16 -- not sure about these two
after reading your follow-up mail} as easy as nla_{get,put}<size>()
and probably dropping Endianness stuff? Hope I got it right ._.

Thanks,
Olek

Powered by blists - more mailing lists