| Message ID | 20231010110828.200709-3-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: finish conversion to generated split_ops | expand |
On Tue, 2023-10-10 at 13:08 +0200, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Introduce support for forgotten attribute type bitfield32. > Note that since the generated code works with struct nla_bitfiel32, ^ typo Otherwise, looks good. Reviewed-by: Johannes Berg <johannes@sipsolutions.net> johannes
On Tue, 10 Oct 2023 13:08:21 +0200 Jiri Pirko wrote: > Introduce support for forgotten attribute type bitfield32. s/forgotten//, no family need it so far > Note that since the generated code works with struct nla_bitfiel32, > the generator adds netlink.h to the list of includes for userspace > headers. Regenerate the headers. If all we need it for is bitfield32 it should be added dynamically. bitfiled32 is an odd concept. > diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml > index f9366aaddd21..8192b87b3046 100644 > --- a/Documentation/netlink/genetlink-c.yaml > +++ b/Documentation/netlink/genetlink-c.yaml > @@ -144,7 +144,7 @@ properties: > name: > type: string > type: &attr-type > - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, > + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, > string, nest, array-nest, nest-type-value ] Just for genetlink-legacy, please. Also I think you need to update Documentation. > +class TypeBitfield32(Type): > + def arg_member(self, ri): > + return [f"const struct nla_bitfield32 *{self.c_name}"] > + > + def struct_member(self, ri): > + ri.cw.p(f"struct nla_bitfield32 {self.c_name};") I think that you can re-implement _complex_member_type() instead of these two?
On Tue, 10 Oct 2023 13:08:21 +0200 Jiri Pirko wrote: > tools/net/ynl/lib/ynl.c | 6 ++++ > tools/net/ynl/lib/ynl.h | 1 + > tools/net/ynl/ynl-gen-c.py | 31 +++++++++++++++++++++ "forgotten" to add support to tools/net/ynl/lib/ynl.py ? :]
Tue, Oct 10, 2023 at 09:01:30PM CEST, kuba@kernel.org wrote: >On Tue, 10 Oct 2023 13:08:21 +0200 Jiri Pirko wrote: >> tools/net/ynl/lib/ynl.c | 6 ++++ >> tools/net/ynl/lib/ynl.h | 1 + >> tools/net/ynl/ynl-gen-c.py | 31 +++++++++++++++++++++ > >"forgotten" to add support to tools/net/ynl/lib/ynl.py ? :] Will check.
Tue, Oct 10, 2023 at 08:58:04PM CEST, kuba@kernel.org wrote: >On Tue, 10 Oct 2023 13:08:21 +0200 Jiri Pirko wrote: >> Introduce support for forgotten attribute type bitfield32. > >s/forgotten//, no family need it so far > >> Note that since the generated code works with struct nla_bitfiel32, >> the generator adds netlink.h to the list of includes for userspace >> headers. Regenerate the headers. > >If all we need it for is bitfield32 it should be added dynamically. >bitfiled32 is an odd concept. What do you mean by "added dynamically"? > >> diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml >> index f9366aaddd21..8192b87b3046 100644 >> --- a/Documentation/netlink/genetlink-c.yaml >> +++ b/Documentation/netlink/genetlink-c.yaml >> @@ -144,7 +144,7 @@ properties: >> name: >> type: string >> type: &attr-type >> - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, >> + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, >> string, nest, array-nest, nest-type-value ] > >Just for genetlink-legacy, please. Why? Should be usable for all, same as other types, no? >Also I think you need to update Documentation. > >> +class TypeBitfield32(Type): >> + def arg_member(self, ri): >> + return [f"const struct nla_bitfield32 *{self.c_name}"] >> + >> + def struct_member(self, ri): >> + ri.cw.p(f"struct nla_bitfield32 {self.c_name};") > >I think that you can re-implement _complex_member_type() instead >of these two? Will check. >
On Wed, 11 Oct 2023 08:07:12 +0200 Jiri Pirko wrote: > >> Note that since the generated code works with struct nla_bitfiel32, > >> the generator adds netlink.h to the list of includes for userspace > >> headers. Regenerate the headers. > > > >If all we need it for is bitfield32 it should be added dynamically. > >bitfiled32 is an odd concept. > > What do you mean by "added dynamically"? Scan the family, see if it has any bitfields and only then add the include? It's not that common, no point slowing down compilation for all families if the header is not otherwise needed. > >> diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml > >> index f9366aaddd21..8192b87b3046 100644 > >> --- a/Documentation/netlink/genetlink-c.yaml > >> +++ b/Documentation/netlink/genetlink-c.yaml > >> @@ -144,7 +144,7 @@ properties: > >> name: > >> type: string > >> type: &attr-type > >> - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, > >> + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, > >> string, nest, array-nest, nest-type-value ] > > > >Just for genetlink-legacy, please. > > 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.
Wed, Oct 11, 2023 at 06:52:36PM CEST, kuba@kernel.org wrote: >On Wed, 11 Oct 2023 08:07:12 +0200 Jiri Pirko wrote: >> >> Note that since the generated code works with struct nla_bitfiel32, >> >> the generator adds netlink.h to the list of includes for userspace >> >> headers. Regenerate the headers. >> > >> >If all we need it for is bitfield32 it should be added dynamically. >> >bitfiled32 is an odd concept. >> >> What do you mean by "added dynamically"? > >Scan the family, see if it has any bitfields and only then add >the include? It's not that common, no point slowing down compilation >for all families if the header is not otherwise needed. Got it. Will try. > >> >> diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml >> >> index f9366aaddd21..8192b87b3046 100644 >> >> --- a/Documentation/netlink/genetlink-c.yaml >> >> +++ b/Documentation/netlink/genetlink-c.yaml >> >> @@ -144,7 +144,7 @@ properties: >> >> name: >> >> type: string >> >> type: &attr-type >> >> - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, >> >> + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, >> >> string, nest, array-nest, nest-type-value ] >> > >> >Just for genetlink-legacy, please. >> >> 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?
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 - it doesn't support "by name" operations so ethtool didn't use it - it can be trivially re-implemented with 2 attrs all in all there aren't very many new uses. So I think we should put it in legacy for now. Maybe somehow mark it as being there due to limited applicability rather than being "bad"?
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 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 :/ > - 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". > - 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? > >all in all there aren't very many new uses. So I think we should >put it in legacy for now. Maybe somehow mark it as being there due >to limited applicability rather than being "bad"? I think it is odd, but if you insists, sure. Your the boss.
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
On Thu, 12 Oct 2023 14:06:57 -0700 Jacob Keller wrote: > >> - 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? Instead of sending the full bit mask sending the list of bits and what state we want them in. And that list can either have bit numbers or names. Looking at ethnl_parse_bit() could be helpful.
On Thu, 12 Oct 2023 11:28:40 +0200 Jiri Pirko wrote: >> all in all there aren't very many new uses. So I think we should >> put it in legacy for now. Maybe somehow mark it as being there due >> to limited applicability rather than being "bad"? > > I think it is odd, but if you insists, sure. Your the boss. Just to be clear here - we're talking about what goes into which level of compatibility within YNL. So yes, I wrote YNL, I'd prefer to retain the ability to make some decisions about it :(
On 10/12/2023 5:15 PM, Jakub Kicinski wrote: > On Thu, 12 Oct 2023 14:06:57 -0700 Jacob Keller wrote: >>>> - 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? > > Instead of sending the full bit mask sending the list of bits and what > state we want them in. And that list can either have bit numbers or > names. Looking at ethnl_parse_bit() could be helpful. > Ah, yep. Thanks for the clarification. -Jake
diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml index f9366aaddd21..8192b87b3046 100644 --- a/Documentation/netlink/genetlink-c.yaml +++ b/Documentation/netlink/genetlink-c.yaml @@ -144,7 +144,7 @@ properties: name: type: string type: &attr-type - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, string, nest, array-nest, nest-type-value ] doc: description: Documentation of the attribute. diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index a6a490333a1a..8b867b5b9966 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -120,7 +120,7 @@ properties: type: string type: description: The netlink attribute type - enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string, binary ] + enum: [ u8, u16, u32, u64, s8, s16, s32, s64, string, binary, bitfield32 ] len: $ref: '#/$defs/len-or-define' byte-order: @@ -187,7 +187,7 @@ properties: type: string type: &attr-type description: The netlink attribute type - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, string, nest, array-nest, nest-type-value ] doc: description: Documentation of the attribute. diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml index 2b788e607a14..5cde1b030e8e 100644 --- a/Documentation/netlink/genetlink.yaml +++ b/Documentation/netlink/genetlink.yaml @@ -117,7 +117,7 @@ properties: name: type: string type: &attr-type - enum: [ unused, pad, flag, binary, u8, u16, u32, u64, s32, s64, + enum: [ unused, pad, flag, binary, bitfield32, u8, u16, u32, u64, s32, s64, string, nest, array-nest, nest-type-value ] doc: description: Documentation of the attribute. diff --git a/tools/net/ynl/generated/devlink-user.h b/tools/net/ynl/generated/devlink-user.h index 4b686d147613..a490466fb98a 100644 --- a/tools/net/ynl/generated/devlink-user.h +++ b/tools/net/ynl/generated/devlink-user.h @@ -9,6 +9,7 @@ #include <stdlib.h> #include <string.h> #include <linux/types.h> +#include <linux/netlink.h> #include <linux/devlink.h> struct ynl_sock; diff --git a/tools/net/ynl/generated/ethtool-user.h b/tools/net/ynl/generated/ethtool-user.h index ddc1a5209992..f7bce36f8485 100644 --- a/tools/net/ynl/generated/ethtool-user.h +++ b/tools/net/ynl/generated/ethtool-user.h @@ -10,6 +10,7 @@ #include <stdlib.h> #include <string.h> #include <linux/types.h> +#include <linux/netlink.h> #include <linux/ethtool.h> struct ynl_sock; diff --git a/tools/net/ynl/generated/fou-user.h b/tools/net/ynl/generated/fou-user.h index a8f860892540..2ae6d1b66393 100644 --- a/tools/net/ynl/generated/fou-user.h +++ b/tools/net/ynl/generated/fou-user.h @@ -9,6 +9,7 @@ #include <stdlib.h> #include <string.h> #include <linux/types.h> +#include <linux/netlink.h> #include <linux/fou.h> struct ynl_sock; diff --git a/tools/net/ynl/generated/handshake-user.h b/tools/net/ynl/generated/handshake-user.h index 2b34acc608de..1007f8db5c5e 100644 --- a/tools/net/ynl/generated/handshake-user.h +++ b/tools/net/ynl/generated/handshake-user.h @@ -9,6 +9,7 @@ #include <stdlib.h> #include <string.h> #include <linux/types.h> +#include <linux/netlink.h> #include <linux/handshake.h> struct ynl_sock; diff --git a/tools/net/ynl/generated/netdev-user.h b/tools/net/ynl/generated/netdev-user.h index b4351ff34595..d6ffc0c8ccf4 100644 --- a/tools/net/ynl/generated/netdev-user.h +++ b/tools/net/ynl/generated/netdev-user.h @@ -9,6 +9,7 @@ #include <stdlib.h> #include <string.h> #include <linux/types.h> +#include <linux/netlink.h> #include <linux/netdev.h> struct ynl_sock; diff --git a/tools/net/ynl/lib/ynl.c b/tools/net/ynl/lib/ynl.c index 514e0d69e731..4a94ef092b6e 100644 --- a/tools/net/ynl/lib/ynl.c +++ b/tools/net/ynl/lib/ynl.c @@ -373,6 +373,12 @@ int ynl_attr_validate(struct ynl_parse_arg *yarg, const struct nlattr *attr) yerr(yarg->ys, YNL_ERROR_ATTR_INVALID, "Invalid attribute (string %s)", policy->name); return -1; + case YNL_PT_BITFIELD32: + if (len == sizeof(struct nla_bitfield32)) + break; + yerr(yarg->ys, YNL_ERROR_ATTR_INVALID, + "Invalid attribute (bitfield32 %s)", policy->name); + return -1; default: yerr(yarg->ys, YNL_ERROR_ATTR_INVALID, "Invalid attribute (unknown %s)", policy->name); diff --git a/tools/net/ynl/lib/ynl.h b/tools/net/ynl/lib/ynl.h index 9eafa3552c16..813b26a08145 100644 --- a/tools/net/ynl/lib/ynl.h +++ b/tools/net/ynl/lib/ynl.h @@ -134,6 +134,7 @@ enum ynl_policy_type { YNL_PT_U32, YNL_PT_U64, YNL_PT_NUL_STR, + YNL_PT_BITFIELD32, }; struct ynl_policy_attr { diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py index f125b5f704ba..27bbe376054d 100755 --- a/tools/net/ynl/ynl-gen-c.py +++ b/tools/net/ynl/ynl-gen-c.py @@ -433,6 +433,34 @@ class TypeBinary(Type): f'memcpy({member}, {self.c_name}, {presence}_len);'] +class TypeBitfield32(Type): + def arg_member(self, ri): + return [f"const struct nla_bitfield32 *{self.c_name}"] + + def struct_member(self, ri): + ri.cw.p(f"struct nla_bitfield32 {self.c_name};") + + def _attr_typol(self): + return f'.type = YNL_PT_BITFIELD32, ' + + def _attr_policy(self, policy): + if not 'enum' in self.attr: + raise Exception('Enum required for bitfield32 attr') + enum = self.family.consts[self.attr['enum']] + mask = enum.get_mask(as_flags=True) + return f"NLA_POLICY_BITFIELD32({mask})" + + def attr_put(self, ri, var): + line = f"mnl_attr_put(nlh, {self.enum_name}, sizeof(struct nla_bitfield32), &{var}->{self.c_name})" + self._attr_put_line(ri, var, line) + + def _attr_get(self, ri, var): + return f"memcpy(&{var}->{self.c_name}, mnl_attr_get_payload(attr), sizeof(struct nla_bitfield32));", None, None + + def _setter_lines(self, ri, member, presence): + return [f"memcpy(&{member}, {self.c_name}, sizeof(struct nla_bitfield32));"] + + class TypeNest(Type): def _complex_member_type(self, ri): return self.nested_struct_type @@ -735,6 +763,8 @@ class AttrSet(SpecAttrSet): t = TypeString(self.family, self, elem, value) elif elem['type'] == 'binary': t = TypeBinary(self.family, self, elem, value) + elif elem['type'] == 'bitfield32': + t = TypeBitfield32(self.family, self, elem, value) elif elem['type'] == 'nest': t = TypeNest(self.family, self, elem, value) elif elem['type'] == 'array-nest': @@ -2406,6 +2436,7 @@ def main(): cw.p('#include <string.h>') if args.header: cw.p('#include <linux/types.h>') + cw.p('#include <linux/netlink.h>') else: cw.p(f'#include "{parsed.name}-user.h"') cw.p('#include "ynl.h"')