lkml.org 
[lkml]   [2022]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net-next 0/4] netlink: add 'bitmap' attribute type and API
Date
From: Jakub Kicinski <kuba@kernel.org>
Date: Mon, 25 Jul 2022 11:53:24 -0700

> On Mon, 25 Jul 2022 15:02:55 +0200 Alexander Lobakin wrote:
> > > 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?
>
> Not exactly. I'd prefer if the netlink level was in u32 increments.
> u64 requires padding (so the nla_put..() calls will have more args).
> Netlink requires platform alignment and rounds up to 4B, so u32 is much
> more convenient than u64. Similarly - it doesn't make sense to represent
> sizes smaller than 4B because of the rounding up, so nla_put_b8() can
> be a define to nla_put_b32(). Ethool's choice of u32 is not without
> merit.
>
> > 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]);
> >
> > ?
>
> Yes.
>
> > > 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 ._.
>
> Modulo the fact that I do still want to pack to u32. Especially a
> single u32 - perhaps once we cross 8B we can switch to requiring 8B
> increments.
>
> The nla_len is 16bit, which means that attrs nested inside other attrs
> are quite tight for space (see the sad story of VF attrs in
> RTM_GETLINK). If we don't represent u8/u16/u32 in a netlink-level
> efficient manner we're back to people arguing for raw u32s rather than
> using the new type.

Ah, got it. I think you're right. The only thing that bothers me
a bit is that we'll be converting arrays of u32s to unsigned longs
each time, unlike u64s <--> longs which are 1:1 for 64 bits and LEs.
But I guess it's better anyway than waste Netlink attrs for
paddings?

So I'd like to summarize for a v2:

* represent as u32s, not u64s;
* present it as "bigints", not bitmaps. It can carry bitmaps as
well, but also ->
* add getters/setters for u8, 16, 32, 64s. Bitmaps are already here.
Probably u32 arrays as well? Just copy them directly. And maybe
u64 arrays, too (convert Netlink u32s to target u64s)?
* should I drop Endianness stuff? With it still in place, it would
be easier to use this new API to exchange with e.g. __be64.
* hope I didn't miss anything?

Thanks a lot, some great ideas here from your side :)

Olek

\
 
 \ /
  Last update: 2022-07-26 12:44    [W:0.054 / U:0.696 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site