Re: [patch net-next 02/10] tools: ynl-gen: introduce support for bitfield32 attribute type

From: Jacob Keller <jacob.e.keller@intel.com>
To: Jiri Pirko <jiri@resnulli.us>, Jakub Kicinski <kuba@kernel.org>
Cc: <netdev@vger.kernel.org>, <pabeni@redhat.com>,
	<davem@davemloft.net>, <edumazet@google.com>,
	<johannes@sipsolutions.net>
Subject: Re: [patch net-next 02/10] tools: ynl-gen: introduce support for bitfield32 attribute type
Date: Thu, 12 Oct 2023 14:06:57 -0700	[thread overview]
Message-ID: <b8705217-c26e-454d-a7f7-d24a4d8cbd0d@intel.com> (raw)
In-Reply-To: <ZSe8SGY3QeaJsYfg@nanopsycho>

On 10/12/2023 2:28 AM, Jiri Pirko wrote:
> Wed, Oct 11, 2023 at 08:25:37PM CEST, kuba@kernel.org wrote:
>> On Wed, 11 Oct 2023 19:04:42 +0200 Jiri Pirko wrote:
>>>>> Why? Should be usable for all, same as other types, no?  
>>>>
>>>> array-nest already isn't. I don't see much value in bitfiled32
>>>> and listing it means every future codegen for genetlink will
>>>> have to support it to be compatible. It's easier to add stuff
>>>> than to remove it, so let's not.  
>>>
>>> Interesting. You want to somehow mark bitfield32 obsolete? But why is
>>> it? I mean, what is the reason to discourage use of bitfield32?
>>
>> It's a tradeoff between simplicity of base types and usefulness.
>> bitfield32 is not bad in any way, but:
>>
>> - it's 32b, new features/caps like to start with 64b
> 

To me, this is the biggest annoyance with bitfield32: that its not
flexible in size, which is one of the big benefits of netlink. That
limits bitfield32 to be most useful in places where you don't expect any
such extension.

> That's fun. Back when Jamal (I think it was him) was pushing bitfield32,
> I argued that it would be better to make it flexible bitfield so it it
> future proof. IIRC DavidM said that it should be enough and that you can
> use extra attr in case the current one overflows.
> 
> Sigh :/
> 
I don't like that approach because it means you need different handling
for different sets of bits which can be a bit frustrating. I would have
also preferred flexible bitfield as well.

>> - it doesn't support "by name" operations so ethtool didn't use it
> 
> It follows the original Netlink rule: "all uapi should be well defined in
> enums/defines".
> 

What's the "by name" operation?

> 
>> - it can be trivially re-implemented with 2 attrs
> 
> Yeah, it's basically a wrapper to avoid unnecessary boilerplate and
> re-implementations. But I think that is a good thing. Or do you say it
> is not desirable to rather re-implement this with 2 attrs instead of
> using bitfield32 directly? 
> 

This reads to me as "its easy to re-implement with 2 attributes rather
than baking them into one", and those attributes can support varying
sizes instead of just bitfield32