* Genetlink per cmd policies @ 2020-09-30 15:49 Jakub Kicinski 2020-09-30 16:03 ` Michal Kubecek 2020-09-30 16:06 ` Johannes Berg 0 siblings, 2 replies; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 15:49 UTC (permalink / raw) To: Jiri Pirko, johannes, Michal Kubecek, dsahern, pablo; +Cc: netdev Hi! I'd like to be able to dump ethtool nl policies, but right now policy dumping is only supported for "global" family policies. Is there any reason not to put the policy in struct genl_ops, or just nobody got to it, yet? I get the feeling that this must have been discussed before... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 15:49 Genetlink per cmd policies Jakub Kicinski @ 2020-09-30 16:03 ` Michal Kubecek 2020-09-30 16:11 ` Jakub Kicinski 2020-09-30 16:06 ` Johannes Berg 1 sibling, 1 reply; 22+ messages in thread From: Michal Kubecek @ 2020-09-30 16:03 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Jiri Pirko, johannes, dsahern, pablo, netdev On Wed, Sep 30, 2020 at 08:49:55AM -0700, Jakub Kicinski wrote: > Hi! > > I'd like to be able to dump ethtool nl policies, but right now policy > dumping is only supported for "global" family policies. > > Is there any reason not to put the policy in struct genl_ops, > or just nobody got to it, yet? > > I get the feeling that this must have been discussed before... It used to be per-cmd but with common maxattr which didn't make much sense. In 5.2 cycle, commit 3b0f31f2b8c9 ("genetlink: make policy common to family") changed it to common policy for each family. Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 16:03 ` Michal Kubecek @ 2020-09-30 16:11 ` Jakub Kicinski 0 siblings, 0 replies; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 16:11 UTC (permalink / raw) To: Michal Kubecek; +Cc: Jiri Pirko, johannes, dsahern, pablo, netdev On Wed, 30 Sep 2020 18:03:03 +0200 Michal Kubecek wrote: > On Wed, Sep 30, 2020 at 08:49:55AM -0700, Jakub Kicinski wrote: > > Hi! > > > > I'd like to be able to dump ethtool nl policies, but right now policy > > dumping is only supported for "global" family policies. > > > > Is there any reason not to put the policy in struct genl_ops, > > or just nobody got to it, yet? > > > > I get the feeling that this must have been discussed before... > > It used to be per-cmd but with common maxattr which didn't make much > sense. In 5.2 cycle, commit 3b0f31f2b8c9 ("genetlink: make policy common > to family") changed it to common policy for each family. Interesting 🤔 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 15:49 Genetlink per cmd policies Jakub Kicinski 2020-09-30 16:03 ` Michal Kubecek @ 2020-09-30 16:06 ` Johannes Berg 2020-09-30 16:17 ` Johannes Berg 2020-09-30 16:18 ` Jakub Kicinski 1 sibling, 2 replies; 22+ messages in thread From: Johannes Berg @ 2020-09-30 16:06 UTC (permalink / raw) To: Jakub Kicinski, Jiri Pirko, Michal Kubecek, dsahern, pablo; +Cc: netdev Hi Jakub, > I'd like to be able to dump ethtool nl policies, but right now policy > dumping is only supported for "global" family policies. Did ethtool add per-command policies? > Is there any reason not to put the policy in struct genl_ops, > or just nobody got to it, yet? > > I get the feeling that this must have been discussed before... Sort of, yeah. We actually *had* per-command policies, and I removed it in commit 3b0f31f2b8c9 ("genetlink: make policy common to family"), mostly because the maxattr is actually in the family not the op, so having a policy in each op was never really a good idea and well-supported. Additionally, having the pointer in each op (and if you want to do it right you also need maxattr in each op) significantly increases the binary size there, as well as boilerplate code to type it out, though the latter could be avoided by "falling back" to the family policy. So at the time, that reduced nl80211 size by 824 bytes, which I guess means we had 103 ops at the time. Doing it right with maxattr would cost twice as much as just the policy pointer, and we probably have a few more ops now, so we're looking at a cost of ~1.6k for it. Personally, I'm not really convinced that there really is a need for it, but then I haven't really looked that much at ethtool. In most cases, you want some "common" attributes for the family, even if it's only the interface index or such, because also on the userspace side that's awkward if you have to really build *everything* including such identifying information per command. The one or two families that actually had different policies per command (at the time I removed it) actually solved this by "aliasing" the same attribute number between the different policies, but again that feels rather awkward ... That's the historic info I guess - I'll take a look at ethtool later and see what it's doing there. johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 16:06 ` Johannes Berg @ 2020-09-30 16:17 ` Johannes Berg 2020-09-30 16:44 ` Jakub Kicinski 2020-09-30 16:18 ` Jakub Kicinski 1 sibling, 1 reply; 22+ messages in thread From: Johannes Berg @ 2020-09-30 16:17 UTC (permalink / raw) To: Jakub Kicinski, Jiri Pirko, Michal Kubecek, dsahern, pablo; +Cc: netdev On Wed, 2020-09-30 at 18:06 +0200, Johannes Berg wrote: > > That's the historic info I guess - I'll take a look at ethtool later and > see what it's doing there. Oh, ok, I see how that works ... you *do* have a sort of common/aliased attribute inside each per-op family that then carries common sub- attributes. That can be linked into the policy. I guess that's not a bad idea. I'd still prefer not to add maxattr/policy into the ops struct because like I said, that's a large amount of wasted space? Perhaps then a "struct nla_policy *get_policy(int cmd, int *maxattr)" function (method) could work, and fall back to just "->policy" and"- >maxattr" if unset, and then you'd just have to write a few lines of code for this case? Seems like overall that'd still be smaller than putting the pointer/maxattr into each and every op struct. johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 16:17 ` Johannes Berg @ 2020-09-30 16:44 ` Jakub Kicinski 2020-09-30 18:36 ` Johannes Berg 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 16:44 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 18:17:47 +0200 Johannes Berg wrote: > On Wed, 2020-09-30 at 18:06 +0200, Johannes Berg wrote: > > > > That's the historic info I guess - I'll take a look at ethtool later and > > see what it's doing there. > > Oh, ok, I see how that works ... you *do* have a sort of common/aliased > attribute inside each per-op family that then carries common sub- > attributes. That can be linked into the policy. > > I guess that's not a bad idea. I'd still prefer not to add > maxattr/policy into the ops struct because like I said, that's a large > amount of wasted space? > > Perhaps then a "struct nla_policy *get_policy(int cmd, int *maxattr)" > function (method) could work, and fall back to just "->policy" and"- > >maxattr" if unset, and then you'd just have to write a few lines of > code for this case? Seems like overall that'd still be smaller than > putting the pointer/maxattr into each and every op struct. I started with a get_policy() callback, but I didn't like it much. Static data is much more pleasant for a client of the API IMHO. What do you think about "ops light"? Insufficiently flexible? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 16:44 ` Jakub Kicinski @ 2020-09-30 18:36 ` Johannes Berg 2020-09-30 18:42 ` Michal Kubecek 2020-09-30 19:01 ` Jakub Kicinski 0 siblings, 2 replies; 22+ messages in thread From: Johannes Berg @ 2020-09-30 18:36 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote: > I started with a get_policy() callback, but I didn't like it much. > Static data is much more pleasant for a client of the API IMHO. Yeah, true. > What do you think about "ops light"? Insufficiently flexible? TBH, I'm not really sure how you'd do it? Admittedly, it _would_ be nice to reduce struct genl_ops further, I could imagine, assuming that doit is far more common than anything else, perhaps something like diff --git a/include/net/genetlink.h b/include/net/genetlink.h index b9eb92f3fe86..a5abab50673c 100644 --- a/include/net/genetlink.h +++ b/include/net/genetlink.h @@ -125,23 +125,34 @@ genl_dumpit_info(struct netlink_callback *cb) return cb->data; } +/** + * struct genl_ops_ext - full generic netlink operations + * @start: start callback for dumps + * @dumpit: callback for dumpers + * @done: completion callback for dumps + * @policy: policy if different from the family policy + * @maxattr: max attr for the policy + */ +struct genl_ops_ext { + int (*start)(struct netlink_callback *cb); + int (*dumpit)(struct sk_buff *skb, + struct netlink_callback *cb); + int (*done)(struct netlink_callback *cb); + const struct nla_policy *policy; + unsigned int maxattr; +}; + /** * struct genl_ops - generic netlink operations * @cmd: command identifier * @internal_flags: flags used by the family * @flags: flags * @doit: standard command callback - * @start: start callback for dumps - * @dumpit: callback for dumpers - * @done: completion callback for dumps */ struct genl_ops { int (*doit)(struct sk_buff *skb, struct genl_info *info); - int (*start)(struct netlink_callback *cb); - int (*dumpit)(struct sk_buff *skb, - struct netlink_callback *cb); - int (*done)(struct netlink_callback *cb); + struct genl_ops_ext *extops; u8 cmd; u8 internal_flags; u8 flags; ... or perhaps even diff --git a/include/net/genetlink.h b/include/net/genetlink.h index b9eb92f3fe86..9be3fc051400 100644 --- a/include/net/genetlink.h +++ b/include/net/genetlink.h @@ -125,29 +125,45 @@ genl_dumpit_info(struct netlink_callback *cb) return cb->data; } +/** + * struct genl_ops_ext - full generic netlink operations + * @start: start callback for dumps + * @dumpit: callback for dumpers + * @done: completion callback for dumps + * @doit: standard command callback + * @policy: policy if different from the family policy + * @maxattr: max attr for the policy + */ +struct genl_ops_ext { + int (*start)(struct netlink_callback *cb); + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb); + int (*done)(struct netlink_callback *cb); + int (*doit)(struct sk_buff *skb, struct genl_info *info); + const struct nla_policy *policy; + unsigned int maxattr; +}; + /** * struct genl_ops - generic netlink operations * @cmd: command identifier * @internal_flags: flags used by the family * @flags: flags * @doit: standard command callback - * @start: start callback for dumps - * @dumpit: callback for dumpers - * @done: completion callback for dumps + * @extops: extended ops if needed, must use GENL_EXTOPS() */ struct genl_ops { - int (*doit)(struct sk_buff *skb, - struct genl_info *info); - int (*start)(struct netlink_callback *cb); - int (*dumpit)(struct sk_buff *skb, - struct netlink_callback *cb); - int (*done)(struct netlink_callback *cb); + union { + int (*doit)(struct sk_buff *skb, struct genl_info *info); + struct genl_ops_ext *extops; + }; u8 cmd; u8 internal_flags; u8 flags; u8 validate; }; +#define GENL_EXT_OPS(ptr) ((struct genl_ops_ext *)((uintptr_t)(ptr) | 1)) + int genl_register_family(struct genl_family *family); int genl_unregister_family(const struct genl_family *family); void genl_notify(const struct genl_family *family, struct sk_buff *skb, But both sort of feel awkward, you have to declare another structure, and link it manually to the right place? There isn't really a _good_ way to express such a thing easily in C? johannes ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 18:36 ` Johannes Berg @ 2020-09-30 18:42 ` Michal Kubecek 2020-09-30 18:42 ` Johannes Berg 2020-09-30 19:01 ` Jakub Kicinski 1 sibling, 1 reply; 22+ messages in thread From: Michal Kubecek @ 2020-09-30 18:42 UTC (permalink / raw) To: Johannes Berg; +Cc: Jakub Kicinski, Jiri Pirko, dsahern, pablo, netdev On Wed, Sep 30, 2020 at 08:36:24PM +0200, Johannes Berg wrote: > On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote: > > > I started with a get_policy() callback, but I didn't like it much. > > Static data is much more pleasant for a client of the API IMHO. > > Yeah, true. > > > What do you think about "ops light"? Insufficiently flexible? > > TBH, I'm not really sure how you'd do it? > > Admittedly, it _would_ be nice to reduce struct genl_ops further, I > could imagine, assuming that doit is far more common than anything else, > perhaps something like > > diff --git a/include/net/genetlink.h b/include/net/genetlink.h > index b9eb92f3fe86..a5abab50673c 100644 > --- a/include/net/genetlink.h > +++ b/include/net/genetlink.h > @@ -125,23 +125,34 @@ genl_dumpit_info(struct netlink_callback *cb) > return cb->data; > } > > +/** > + * struct genl_ops_ext - full generic netlink operations > + * @start: start callback for dumps > + * @dumpit: callback for dumpers > + * @done: completion callback for dumps > + * @policy: policy if different from the family policy > + * @maxattr: max attr for the policy > + */ > +struct genl_ops_ext { > + int (*start)(struct netlink_callback *cb); > + int (*dumpit)(struct sk_buff *skb, > + struct netlink_callback *cb); > + int (*done)(struct netlink_callback *cb); > + const struct nla_policy *policy; > + unsigned int maxattr; > +}; > + > /** > * struct genl_ops - generic netlink operations > * @cmd: command identifier > * @internal_flags: flags used by the family > * @flags: flags > * @doit: standard command callback > - * @start: start callback for dumps > - * @dumpit: callback for dumpers > - * @done: completion callback for dumps > */ > struct genl_ops { > int (*doit)(struct sk_buff *skb, > struct genl_info *info); > - int (*start)(struct netlink_callback *cb); > - int (*dumpit)(struct sk_buff *skb, > - struct netlink_callback *cb); > - int (*done)(struct netlink_callback *cb); > + struct genl_ops_ext *extops; > u8 cmd; > u8 internal_flags; > u8 flags; > > ... > > or perhaps even > > diff --git a/include/net/genetlink.h b/include/net/genetlink.h > index b9eb92f3fe86..9be3fc051400 100644 > --- a/include/net/genetlink.h > +++ b/include/net/genetlink.h > @@ -125,29 +125,45 @@ genl_dumpit_info(struct netlink_callback *cb) > return cb->data; > } > > +/** > + * struct genl_ops_ext - full generic netlink operations > + * @start: start callback for dumps > + * @dumpit: callback for dumpers > + * @done: completion callback for dumps > + * @doit: standard command callback > + * @policy: policy if different from the family policy > + * @maxattr: max attr for the policy > + */ > +struct genl_ops_ext { > + int (*start)(struct netlink_callback *cb); > + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb); > + int (*done)(struct netlink_callback *cb); > + int (*doit)(struct sk_buff *skb, struct genl_info *info); > + const struct nla_policy *policy; > + unsigned int maxattr; > +}; > + > /** > * struct genl_ops - generic netlink operations > * @cmd: command identifier > * @internal_flags: flags used by the family > * @flags: flags > * @doit: standard command callback > - * @start: start callback for dumps > - * @dumpit: callback for dumpers > - * @done: completion callback for dumps > + * @extops: extended ops if needed, must use GENL_EXTOPS() > */ > struct genl_ops { > - int (*doit)(struct sk_buff *skb, > - struct genl_info *info); > - int (*start)(struct netlink_callback *cb); > - int (*dumpit)(struct sk_buff *skb, > - struct netlink_callback *cb); > - int (*done)(struct netlink_callback *cb); > + union { > + int (*doit)(struct sk_buff *skb, struct genl_info *info); > + struct genl_ops_ext *extops; > + }; > u8 cmd; > u8 internal_flags; > u8 flags; > u8 validate; > }; > > +#define GENL_EXT_OPS(ptr) ((struct genl_ops_ext *)((uintptr_t)(ptr) | 1)) > + > int genl_register_family(struct genl_family *family); > int genl_unregister_family(const struct genl_family *family); > void genl_notify(const struct genl_family *family, struct sk_buff *skb, > > > > > But both sort of feel awkward, you have to declare another structure, > and link it manually to the right place? > > There isn't really a _good_ way to express such a thing easily in C? Not really good either but how about embedding struct genl_ops as first member of struct genl_ops_ext like we do with sockets? Michal ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 18:42 ` Michal Kubecek @ 2020-09-30 18:42 ` Johannes Berg 0 siblings, 0 replies; 22+ messages in thread From: Johannes Berg @ 2020-09-30 18:42 UTC (permalink / raw) To: Michal Kubecek; +Cc: Jakub Kicinski, Jiri Pirko, dsahern, pablo, netdev On Wed, 2020-09-30 at 20:42 +0200, Michal Kubecek wrote: > > Not really good either but how about embedding struct genl_ops as first > member of struct genl_ops_ext like we do with sockets? That won't work, we need to make an *array* out of these... johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 18:36 ` Johannes Berg 2020-09-30 18:42 ` Michal Kubecek @ 2020-09-30 19:01 ` Jakub Kicinski 2020-09-30 19:03 ` Johannes Berg 1 sibling, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 19:01 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 20:36:24 +0200 Johannes Berg wrote: > On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote: > > > I started with a get_policy() callback, but I didn't like it much. > > Static data is much more pleasant for a client of the API IMHO. > > Yeah, true. > > > What do you think about "ops light"? Insufficiently flexible? > > TBH, I'm not really sure how you'd do it? There are very few users who actually access ops, I was thinking that callers to genl_get_cmd() should declare a full struct genl_ops on the stack (or in some context, not sure yet), and then genl_get_cmd() will fill it in. If family has full ops it will do a memcpy(); if the ops are "light" it can assign the right pointers. Plus it can propagate the policy and maxattr from family if needed in both cases. Don't have the code yet, but shouldnt take long to get a PoC... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 19:01 ` Jakub Kicinski @ 2020-09-30 19:03 ` Johannes Berg 2020-09-30 19:14 ` Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Johannes Berg @ 2020-09-30 19:03 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 2020-09-30 at 12:01 -0700, Jakub Kicinski wrote: > On Wed, 30 Sep 2020 20:36:24 +0200 Johannes Berg wrote: > > On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote: > > > > > I started with a get_policy() callback, but I didn't like it much. > > > Static data is much more pleasant for a client of the API IMHO. > > > > Yeah, true. > > > > > What do you think about "ops light"? Insufficiently flexible? > > > > TBH, I'm not really sure how you'd do it? > > There are very few users who actually access ops, I was thinking that > callers to genl_get_cmd() should declare a full struct genl_ops on the > stack (or in some context, not sure yet), and then genl_get_cmd() will > fill it in. > > If family has full ops it will do a memcpy(); if the ops are "light" it > can assign the right pointers. > > Plus it can propagate the policy and maxattr from family if needed in > both cases. Oh, so you were thinking you'd have to sort of decide on the *family* level whether you want "light" or "heavy" ops? Hm. I guess you could even have both? struct genl_ops *ops; struct genl_ops_ext *extops; and then search both arrays, no need for memcpy/pointer assignment? johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 19:03 ` Johannes Berg @ 2020-09-30 19:14 ` Jakub Kicinski 2020-09-30 19:15 ` Johannes Berg 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 19:14 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 21:03:08 +0200 Johannes Berg wrote: > On Wed, 2020-09-30 at 12:01 -0700, Jakub Kicinski wrote: > > On Wed, 30 Sep 2020 20:36:24 +0200 Johannes Berg wrote: > > > On Wed, 2020-09-30 at 09:44 -0700, Jakub Kicinski wrote: > > > > > > > I started with a get_policy() callback, but I didn't like it much. > > > > Static data is much more pleasant for a client of the API IMHO. > > > > > > Yeah, true. > > > > > > > What do you think about "ops light"? Insufficiently flexible? > > > > > > TBH, I'm not really sure how you'd do it? > > > > There are very few users who actually access ops, I was thinking that > > callers to genl_get_cmd() should declare a full struct genl_ops on the > > stack (or in some context, not sure yet), and then genl_get_cmd() will > > fill it in. > > > > If family has full ops it will do a memcpy(); if the ops are "light" it > > can assign the right pointers. > > > > Plus it can propagate the policy and maxattr from family if needed in > > both cases. > > Oh, so you were thinking you'd have to sort of decide on the *family* > level whether you want "light" or "heavy" ops? > > Hm. I guess you could even have both? > > struct genl_ops *ops; > struct genl_ops_ext *extops; > > and then search both arrays, no need for memcpy/pointer assignment? Yup, both should work quite nicely, too. No reason to force one or the other. Extra n_ops_ext should be fine, I think I can make n_ops a u8 in the first place, since commands themselves are u8s. And 0 is commonly unused. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 19:14 ` Jakub Kicinski @ 2020-09-30 19:15 ` Johannes Berg 2020-09-30 19:46 ` Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Johannes Berg @ 2020-09-30 19:15 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 2020-09-30 at 12:14 -0700, Jakub Kicinski wrote: > > Hm. I guess you could even have both? > > > > struct genl_ops *ops; > > struct genl_ops_ext *extops; > > > > and then search both arrays, no need for memcpy/pointer assignment? > > Yup, both should work quite nicely, too. No reason to force one or the > other. Indeed. > Extra n_ops_ext should be fine, I think I can make n_ops a u8 in > the first place, since commands themselves are u8s. And 0 is commonly > unused. True. I'm not really worried about the extra pointer in the *family* though, there aren't really all that many families :) johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 19:15 ` Johannes Berg @ 2020-09-30 19:46 ` Jakub Kicinski 2020-09-30 20:13 ` Johannes Berg 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 19:46 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 21:15:33 +0200 Johannes Berg wrote: > On Wed, 2020-09-30 at 12:14 -0700, Jakub Kicinski wrote: > > > > Hm. I guess you could even have both? > > > > > > struct genl_ops *ops; > > > struct genl_ops_ext *extops; > > > > > > and then search both arrays, no need for memcpy/pointer assignment? > > > > Yup, both should work quite nicely, too. No reason to force one or the > > other. > > Indeed. > > > Extra n_ops_ext should be fine, I think I can make n_ops a u8 in > > the first place, since commands themselves are u8s. And 0 is commonly > > unused. > > True. I'm not really worried about the extra pointer in the *family* > though, there aren't really all that many families :) This builds (I think) - around 100 extra LoC: include/net/genetlink.h | 25 +++++++++ net/netlink/genetlink.c | 138 +++++++++++++++++++++++++++++++++++++++--------- net/wireless/nl80211.c | 5 ++ 3 files changed, 142 insertions(+), 26 deletions(-) diff --git a/include/net/genetlink.h b/include/net/genetlink.h index ee1d0ab3a89f..28e162d89426 100644 --- a/include/net/genetlink.h +++ b/include/net/genetlink.h @@ -41,6 +41,8 @@ struct genl_info; * (private) * @ops: the operations supported by this family * @n_ops: number of operations supported by this family + * @light_ops: the small-struct operations supported by this family + * @n_light_ops: number of small-struct operations supported by this family */ struct genl_family { int id; /* private */ @@ -52,6 +54,7 @@ struct genl_family { u8 netnsok:1; u8 parallel_ops:1; u8 n_ops; + u8 n_light_ops; u8 n_mcgrps; const struct nla_policy *policy; int (*pre_doit)(const struct genl_ops *ops, @@ -61,6 +64,7 @@ struct genl_family { struct sk_buff *skb, struct genl_info *info); const struct genl_ops * ops; + const struct genl_light_ops *light_ops; const struct genl_multicast_group *mcgrps; struct module *module; }; @@ -125,6 +129,27 @@ genl_dumpit_info(struct netlink_callback *cb) return cb->data; } +/** + * struct genl_light_ops - generic netlink operations (small version) + * @cmd: command identifier + * @internal_flags: flags used by the family + * @flags: flags + * @validate: validation flags from enum genl_validate_flags + * @doit: standard command callback + * @dumpit: callback for dumpers + * + * This is a cut-down version of struct genl_ops for users who don't need + * most of the ancillary infra and want to save space. + */ +struct genl_light_ops { + int (*doit)(struct sk_buff *skb, struct genl_info *info); + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb); + u8 cmd; + u8 internal_flags; + u8 flags; + u8 validate; +}; + /** * struct genl_ops - generic netlink operations * @cmd: command identifier diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index b6a7c560772f..2a2330e149bc 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -107,16 +107,89 @@ static const struct genl_family *genl_family_find_byname(char *name) return NULL; } -static const struct genl_ops *genl_get_cmd(u8 cmd, - const struct genl_family *family) +static int genl_get_cmd_cnt(const struct genl_family *family) +{ + return family->n_ops + family->n_light_ops; +} + +static void genl_op_from_full(const struct genl_family *family, + unsigned int i, struct genl_ops *op) +{ + memcpy(op, &family->ops[i], sizeof(*op)); + + if (!op->maxattr) + op->maxattr = family->maxattr; + if (!op->policy) + op->policy = family->policy; +} + +static int genl_get_cmd_full(u8 cmd, const struct genl_family *family, + struct genl_ops *op) { int i; for (i = 0; i < family->n_ops; i++) - if (family->ops[i].cmd == cmd) - return &family->ops[i]; + if (family->ops[i].cmd == cmd) { + genl_op_from_full(family, i, op); + return 0; + } - return NULL; + return -ENOENT; +} + +static void genl_op_from_light(const struct genl_family *family, + unsigned int i, struct genl_ops *op) +{ + memset(op, 0, sizeof(*op)); + op->doit = family->light_ops[i].doit; + op->dumpit = family->light_ops[i].dumpit; + op->cmd = family->light_ops[i].cmd; + op->internal_flags = family->light_ops[i].internal_flags; + op->flags = family->light_ops[i].flags; + op->validate = family->light_ops[i].validate; + + op->maxattr = family->maxattr; + op->policy = family->policy; +} + +static int genl_get_cmd_light(u8 cmd, const struct genl_family *family, + struct genl_ops *op) +{ + int i; + + for (i = 0; i < family->n_light_ops; i++) + if (family->light_ops[i].cmd == cmd) { + genl_op_from_light(family, i, op); + return 0; + } + + return -ENOENT; +} + +static int genl_get_cmd(u8 cmd, const struct genl_family *family, + struct genl_ops *op) +{ + if (!genl_get_cmd_full(cmd, family, op)) + return 0; + return genl_get_cmd_light(cmd, family, op); +} + +static int genl_get_cmd_by_index(unsigned int i, + const struct genl_family *family, + struct genl_ops *op) +{ + if (i < family->n_ops) { + genl_op_from_full(family, i, op); + return 0; + } + i -= family->n_ops; + + if (i < family->n_light_ops) { + genl_op_from_light(family, i, op); + return 0; + } + + return -EINVAL; } static int genl_allocate_reserve_groups(int n_groups, int *first_id) @@ -286,22 +359,32 @@ static void genl_unregister_mc_groups(const struct genl_family *family) static int genl_validate_ops(const struct genl_family *family) { - const struct genl_ops *ops = family->ops; - unsigned int n_ops = family->n_ops; + unsigned int n_ops; int i, j; - if (WARN_ON(n_ops && !ops)) + if (WARN_ON(family->n_ops && !family->ops) || + WARN_ON(family->n_light_ops && !family->light_ops)) return -EINVAL; + n_ops = genl_get_cmd_cnt(family); if (!n_ops) return 0; for (i = 0; i < n_ops; i++) { - if (ops[i].dumpit == NULL && ops[i].doit == NULL) + struct genl_ops op; + + if (genl_get_cmd_by_index(i, family, &op)) return -EINVAL; - for (j = i + 1; j < n_ops; j++) - if (ops[i].cmd == ops[j].cmd) + if (op.dumpit == NULL && op.doit == NULL) + return -EINVAL; + for (j = i + 1; j < n_ops; j++) { + struct genl_ops op2; + + if (genl_get_cmd_by_index(j, family, &op2)) return -EINVAL; + if (op.cmd == op2.cmd) + return -EINVAL; + } } return 0; @@ -687,9 +770,9 @@ static int genl_family_rcv_msg(const struct genl_family *family, struct nlmsghdr *nlh, struct netlink_ext_ack *extack) { - const struct genl_ops *ops; struct net *net = sock_net(skb->sk); struct genlmsghdr *hdr = nlmsg_data(nlh); + struct genl_ops op; int hdrlen; /* this family doesn't exist in this netns */ @@ -700,24 +783,23 @@ static int genl_family_rcv_msg(const struct genl_family *family, if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) return -EINVAL; - ops = genl_get_cmd(hdr->cmd, family); - if (ops == NULL) + if (genl_get_cmd(hdr->cmd, family, &op)) return -EOPNOTSUPP; - if ((ops->flags & GENL_ADMIN_PERM) && + if ((op.flags & GENL_ADMIN_PERM) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if ((ops->flags & GENL_UNS_ADMIN_PERM) && + if ((op.flags & GENL_UNS_ADMIN_PERM) && !netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) return -EPERM; if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) return genl_family_rcv_msg_dumpit(family, skb, nlh, extack, - ops, hdrlen, net); + &op, hdrlen, net); else return genl_family_rcv_msg_doit(family, skb, nlh, extack, - ops, hdrlen, net); + &op, hdrlen, net); } static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, @@ -770,7 +852,7 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq, nla_put_u32(skb, CTRL_ATTR_MAXATTR, family->maxattr)) goto nla_put_failure; - if (family->n_ops) { + if (genl_get_cmd_cnt(family)) { struct nlattr *nla_ops; int i; @@ -778,23 +860,27 @@ static int ctrl_fill_info(const struct genl_family *family, u32 portid, u32 seq, if (nla_ops == NULL) goto nla_put_failure; - for (i = 0; i < family->n_ops; i++) { + for (i = 0; i < genl_get_cmd_cnt(family); i++) { struct nlattr *nest; - const struct genl_ops *ops = &family->ops[i]; - u32 op_flags = ops->flags; + struct genl_ops op; + u32 op_flags; + + if (genl_get_cmd_by_index(i, family, &op)) + goto nla_put_failure; - if (ops->dumpit) + op_flags = op.flags; + if (op.dumpit) op_flags |= GENL_CMD_CAP_DUMP; - if (ops->doit) + if (op.doit) op_flags |= GENL_CMD_CAP_DO; - if (family->policy || ops->policy) + if (op.policy) op_flags |= GENL_CMD_CAP_HASPOL; nest = nla_nest_start_noflag(skb, i + 1); if (nest == NULL) goto nla_put_failure; - if (nla_put_u32(skb, CTRL_ATTR_OP_ID, ops->cmd) || + if (nla_put_u32(skb, CTRL_ATTR_OP_ID, op.cmd) || nla_put_u32(skb, CTRL_ATTR_OP_FLAGS, op_flags)) goto nla_put_failure; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 1a212db7a300..2bff951dbb0a 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -14603,6 +14603,9 @@ static const struct genl_ops nl80211_ops[] = { .internal_flags = NL80211_FLAG_NEED_WIPHY | NL80211_FLAG_NEED_RTNL, }, +}; + +static const struct genl_light_ops nl80211_light_ops[] = { { .cmd = NL80211_CMD_SET_WIPHY, .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, @@ -15464,6 +15467,8 @@ static struct genl_family nl80211_fam __ro_after_init = { .module = THIS_MODULE, .ops = nl80211_ops, .n_ops = ARRAY_SIZE(nl80211_ops), + .light_ops = nl80211_light_ops, + .n_light_ops = ARRAY_SIZE(nl80211_light_ops), .mcgrps = nl80211_mcgrps, .n_mcgrps = ARRAY_SIZE(nl80211_mcgrps), .parallel_ops = true, ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 19:46 ` Jakub Kicinski @ 2020-09-30 20:13 ` Johannes Berg 2020-09-30 20:47 ` Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Johannes Berg @ 2020-09-30 20:13 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 2020-09-30 at 12:46 -0700, Jakub Kicinski wrote: > This builds (I think) - around 100 extra LoC: Looks good to me, couple of comments below. > +/** > + * struct genl_light_ops - generic netlink operations (small version) > + * @cmd: command identifier > + * @internal_flags: flags used by the family > + * @flags: flags > + * @validate: validation flags from enum genl_validate_flags > + * @doit: standard command callback > + * @dumpit: callback for dumpers > + * > + * This is a cut-down version of struct genl_ops for users who don't need > + * most of the ancillary infra and want to save space. > + */ > +struct genl_light_ops { > + int (*doit)(struct sk_buff *skb, struct genl_info *info); > + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb); Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe remove that even? It's a bit more juggling in nl80211 to actually use it, but I'm certainly happy to do that myself. > +static void genl_op_from_full(const struct genl_family *family, > + unsigned int i, struct genl_ops *op) > +{ > + memcpy(op, &family->ops[i], sizeof(*op)); What's wrong with struct assignment? :) *op = family->ops[i]; > + if (!op->maxattr) > + op->maxattr = family->maxattr; > + if (!op->policy) > + op->policy = family->policy; That doesn't build as is, I think? Or did you have some other patch below it? > static int genl_validate_ops(const struct genl_family *family) > { [...] > + n_ops = genl_get_cmd_cnt(family); > if (!n_ops) > return 0; Come to think of it, that check is kinda pointless, the loop won't run if it's 0 and then we return 0 immediately anyway... whatever :) > for (i = 0; i < n_ops; i++) { > - if (ops[i].dumpit == NULL && ops[i].doit == NULL) > + struct genl_ops op; > + > + if (genl_get_cmd_by_index(i, family, &op)) > return -EINVAL; Maybe WARN_ON() or something? It really ought to not be possible for that to fail, since you're only iterating to n_ops, so you'd have to have some consistency issues if that happens. > - for (j = i + 1; j < n_ops; j++) > - if (ops[i].cmd == ops[j].cmd) > + if (op.dumpit == NULL && op.doit == NULL) > + return -EINVAL; > + for (j = i + 1; j < n_ops; j++) { > + struct genl_ops op2; > + > + if (genl_get_cmd_by_index(j, family, &op2)) > return -EINVAL; same here > + for (i = 0; i < genl_get_cmd_cnt(family); i++) { > struct nlattr *nest; > - const struct genl_ops *ops = &family->ops[i]; > - u32 op_flags = ops->flags; > + struct genl_ops op; > + u32 op_flags; > + > + if (genl_get_cmd_by_index(i, family, &op)) > + goto nla_put_failure; but actually, same here, so maybe it should just not even be able to return an error but WARN_ON instead and clear the op, so you have everything NULL in that case? I don't really see a case where you'd have the index coming from userspace and would have to protect against it being bad, or something? johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 20:13 ` Johannes Berg @ 2020-09-30 20:47 ` Jakub Kicinski 2020-09-30 23:38 ` Andrew Lunn 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 20:47 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 22:13:07 +0200 Johannes Berg wrote: > > +/** > > + * struct genl_light_ops - generic netlink operations (small version) > > + * @cmd: command identifier > > + * @internal_flags: flags used by the family > > + * @flags: flags > > + * @validate: validation flags from enum genl_validate_flags > > + * @doit: standard command callback > > + * @dumpit: callback for dumpers > > + * > > + * This is a cut-down version of struct genl_ops for users who don't need > > + * most of the ancillary infra and want to save space. > > + */ > > +struct genl_light_ops { > > + int (*doit)(struct sk_buff *skb, struct genl_info *info); > > + int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb); > > Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe > remove that even? It's a bit more juggling in nl80211 to actually use > it, but I'm certainly happy to do that myself. Hm. 16 / 44 for devlink if I'm counting right. For nl80211 we'd go from: 107 * 24 = 2471 to: 97 * 16 + 10 * 48 = 2032 But for devlink we go from: 44 * 24 = 1056 to 16 * 16 + 28 * 48 = 1600 No strong feelings but I think the API is a little easier to grasp when families with a global policy can use exclusively the "light" ops. > > +static void genl_op_from_full(const struct genl_family *family, > > + unsigned int i, struct genl_ops *op) > > +{ > > + memcpy(op, &family->ops[i], sizeof(*op)); > > What's wrong with struct assignment? :) > > *op = family->ops[i]; Code size :) text data bss dec hex 22657 3590 64 26311 66c7 memcpy 23103 3590 64 26757 6885 struct > > + if (!op->maxattr) > > + op->maxattr = family->maxattr; > > + if (!op->policy) > > + op->policy = family->policy; > > That doesn't build as is, I think? Or did you have some other patch > below it? Heh, right, I already have policy in ops in my tree. I'll send a fuller RFC series by the end of the day. > > static int genl_validate_ops(const struct genl_family *family) > > { > [...] > > + n_ops = genl_get_cmd_cnt(family); > > if (!n_ops) > > return 0; > > Come to think of it, that check is kinda pointless, the loop won't run > if it's 0 and then we return 0 immediately anyway... whatever :) Good point :) > > for (i = 0; i < n_ops; i++) { > > - if (ops[i].dumpit == NULL && ops[i].doit == NULL) > > + struct genl_ops op; > > + > > + if (genl_get_cmd_by_index(i, family, &op)) > > return -EINVAL; > > Maybe WARN_ON() or something? It really ought to not be possible for > that to fail, since you're only iterating to n_ops, so you'd have to > have some consistency issues if that happens. > > > - for (j = i + 1; j < n_ops; j++) > > - if (ops[i].cmd == ops[j].cmd) > > + if (op.dumpit == NULL && op.doit == NULL) > > + return -EINVAL; > > + for (j = i + 1; j < n_ops; j++) { > > + struct genl_ops op2; > > + > > + if (genl_get_cmd_by_index(j, family, &op2)) > > return -EINVAL; > > same here Ack, will add it to the helper itself. > > + for (i = 0; i < genl_get_cmd_cnt(family); i++) { > > struct nlattr *nest; > > - const struct genl_ops *ops = &family->ops[i]; > > - u32 op_flags = ops->flags; > > + struct genl_ops op; > > + u32 op_flags; > > + > > + if (genl_get_cmd_by_index(i, family, &op)) > > + goto nla_put_failure; > > but actually, same here, so maybe it should just not even be able to > return an error but WARN_ON instead and clear the op, so you have > everything NULL in that case? > > I don't really see a case where you'd have the index coming from > userspace and would have to protect against it being bad, or something? Yeah, this is entirely an internal error. I'll double check if cleared out op doesn't break anything and make the helper void. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 20:47 ` Jakub Kicinski @ 2020-09-30 23:38 ` Andrew Lunn 2020-10-01 0:23 ` Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Andrew Lunn @ 2020-09-30 23:38 UTC (permalink / raw) To: Jakub Kicinski Cc: Johannes Berg, Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev > > > +static void genl_op_from_full(const struct genl_family *family, > > > + unsigned int i, struct genl_ops *op) > > > +{ > > > + memcpy(op, &family->ops[i], sizeof(*op)); > > > > What's wrong with struct assignment? :) > > > > *op = family->ops[i]; > > Code size :) > > text data bss dec hex > 22657 3590 64 26311 66c7 memcpy > 23103 3590 64 26757 6885 struct You might want to show that to the compiler people. Did you look at the assembly? Andrew ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 23:38 ` Andrew Lunn @ 2020-10-01 0:23 ` Jakub Kicinski 2020-10-01 1:53 ` Andrew Lunn 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-10-01 0:23 UTC (permalink / raw) To: Andrew Lunn Cc: Johannes Berg, Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Thu, 1 Oct 2020 01:38:17 +0200 Andrew Lunn wrote: > > > > +static void genl_op_from_full(const struct genl_family *family, > > > > + unsigned int i, struct genl_ops *op) > > > > +{ > > > > + memcpy(op, &family->ops[i], sizeof(*op)); > > > > > > What's wrong with struct assignment? :) > > > > > > *op = family->ops[i]; > > > > Code size :) > > > > text data bss dec hex > > 22657 3590 64 26311 66c7 memcpy > > 23103 3590 64 26757 6885 struct > > You might want to show that to the compiler people. Did you look at > the assembly? Somewhere along the line I lost the ability to decipher compiler code :( Two snippets follow (both with -Os to prevent inlining) Assignment: static void genl_op_from_full(const struct genl_family *family, unsigned int i, struct genl_ops *op) { 0: e8 00 00 00 00 callq 5 <genl_op_from_full+0x5> 5: 41 54 push %r12 7: 49 89 d4 mov %rdx,%r12 a: 55 push %rbp b: 89 f5 mov %esi,%ebp d: 53 push %rbx e: 48 89 fb mov %rdi,%rbx *op = family->ops[i]; 11: 48 6b ed 30 imul $0x30,%rbp,%rbp 15: 48 83 c7 40 add $0x40,%rdi 19: e8 00 00 00 00 callq 1e <genl_op_from_full+0x1e> 1e: 48 03 6b 40 add 0x40(%rbx),%rbp 22: be 30 00 00 00 mov $0x30,%esi 27: 4c 89 e7 mov %r12,%rdi 2a: e8 00 00 00 00 callq 2f <genl_op_from_full+0x2f> 2f: be 30 00 00 00 mov $0x30,%esi 34: 48 89 ef mov %rbp,%rdi 37: e8 00 00 00 00 callq 3c <genl_op_from_full+0x3c> 3c: b9 0c 00 00 00 mov $0xc,%ecx 41: 4c 89 e7 mov %r12,%rdi 44: 48 89 ee mov %rbp,%rsi 47: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) if (!op->maxattr) 49: 49 8d 7c 24 28 lea 0x28(%r12),%rdi 4e: e8 00 00 00 00 callq 53 <genl_op_from_full+0x53> 53: 41 83 7c 24 28 00 cmpl $0x0,0x28(%r12) 59: 75 11 jne 6c <genl_op_from_full+0x6c> op->maxattr = family->maxattr; 5b: 48 8d 7b 1c lea 0x1c(%rbx),%rdi 5f: e8 00 00 00 00 callq 64 <genl_op_from_full+0x64> 64: 8b 43 1c mov 0x1c(%rbx),%eax 67: 41 89 44 24 28 mov %eax,0x28(%r12) Memcpy: 00000000000001a2 <genl_op_from_full>: { 1a2: e8 00 00 00 00 callq 1a7 <genl_op_from_full+0x5> 1a7: 41 54 push %r12 1a9: 49 89 fc mov %rdi,%r12 memcpy(op, &family->ops[i], sizeof(*op)); 1ac: 48 83 c7 40 add $0x40,%rdi { 1b0: 55 push %rbp 1b1: 89 f5 mov %esi,%ebp 1b3: 53 push %rbx 1b4: 48 89 d3 mov %rdx,%rbx memcpy(op, &family->ops[i], sizeof(*op)); 1b7: e8 00 00 00 00 callq 1bc <genl_op_from_full+0x1a> 1bc: 89 ee mov %ebp,%esi if (q_size < size) __read_overflow2(); } if (p_size < size || q_size < size) fortify_panic(__func__); return __underlying_memcpy(p, q, size); 1be: ba 30 00 00 00 mov $0x30,%edx 1c3: 48 89 df mov %rbx,%rdi 1c6: 48 6b f6 30 imul $0x30,%rsi,%rsi 1ca: 49 03 74 24 40 add 0x40(%r12),%rsi 1cf: e8 00 00 00 00 callq 1d4 <genl_op_from_full+0x32> if (!op->maxattr) 1d4: 48 8d 7b 28 lea 0x28(%rbx),%rdi 1d8: e8 00 00 00 00 callq 1dd <genl_op_from_full+0x3b> 1dd: 83 7b 28 00 cmpl $0x0,0x28(%rbx) 1e1: 75 12 jne 1f5 <genl_op_from_full+0x53> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-10-01 0:23 ` Jakub Kicinski @ 2020-10-01 1:53 ` Andrew Lunn 2020-10-01 15:50 ` Jakub Kicinski 0 siblings, 1 reply; 22+ messages in thread From: Andrew Lunn @ 2020-10-01 1:53 UTC (permalink / raw) To: Jakub Kicinski Cc: Johannes Berg, Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, Sep 30, 2020 at 05:23:17PM -0700, Jakub Kicinski wrote: > On Thu, 1 Oct 2020 01:38:17 +0200 Andrew Lunn wrote: > > > > > +static void genl_op_from_full(const struct genl_family *family, > > > > > + unsigned int i, struct genl_ops *op) > > > > > +{ > > > > > + memcpy(op, &family->ops[i], sizeof(*op)); > > > > > > > > What's wrong with struct assignment? :) > > > > > > > > *op = family->ops[i]; > > > > > > Code size :) > > > > > > text data bss dec hex > > > 22657 3590 64 26311 66c7 memcpy > > > 23103 3590 64 26757 6885 struct > > > > You might want to show that to the compiler people. Did you look at > > the assembly? > > Somewhere along the line I lost the ability to decipher compiler code :( Yah, Z80 and 6809 i could sometimes just read the byte codes. That has long gone. I tend to read ARM assembly now a days being mostly in the embedded world. So the memcpy version just calls memcpy by the looks of it. I thought it might of inlined it, but it has not. Maybe because of the -Os. The struct assignment is interesting because it appears to be calling three functions to do the work. I wonder if it is avoiding copying the padding in the structure? But still, that does not explain an extra 400 bytes in the text segment. Andrew ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-10-01 1:53 ` Andrew Lunn @ 2020-10-01 15:50 ` Jakub Kicinski 2020-10-01 15:52 ` Johannes Berg 0 siblings, 1 reply; 22+ messages in thread From: Jakub Kicinski @ 2020-10-01 15:50 UTC (permalink / raw) To: Andrew Lunn Cc: Johannes Berg, Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Thu, 1 Oct 2020 03:53:23 +0200 Andrew Lunn wrote: > On Wed, Sep 30, 2020 at 05:23:17PM -0700, Jakub Kicinski wrote: > > On Thu, 1 Oct 2020 01:38:17 +0200 Andrew Lunn wrote: > > > > > > +static void genl_op_from_full(const struct genl_family *family, > > > > > > + unsigned int i, struct genl_ops *op) > > > > > > +{ > > > > > > + memcpy(op, &family->ops[i], sizeof(*op)); > > > > > > > > > > What's wrong with struct assignment? :) > > > > > > > > > > *op = family->ops[i]; > > > > > > > > Code size :) > > > > > > > > text data bss dec hex > > > > 22657 3590 64 26311 66c7 memcpy > > > > 23103 3590 64 26757 6885 struct > > > > > > You might want to show that to the compiler people. Did you look at > > > the assembly? > > > > Somewhere along the line I lost the ability to decipher compiler code :( > > Yah, Z80 and 6809 i could sometimes just read the byte codes. That has > long gone. I tend to read ARM assembly now a days being mostly in the > embedded world. > > So the memcpy version just calls memcpy by the looks of it. I thought > it might of inlined it, but it has not. Maybe because of the -Os. > > The struct assignment is interesting because it appears to be calling > three functions to do the work. I wonder if it is avoiding copying the > padding in the structure? > > But still, that does not explain an extra 400 bytes in the text > segment. FWIW the 400 was without the -Os with -Os it's more like 50. So I'll just go for it and do the struct assignment. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-10-01 15:50 ` Jakub Kicinski @ 2020-10-01 15:52 ` Johannes Berg 0 siblings, 0 replies; 22+ messages in thread From: Johannes Berg @ 2020-10-01 15:52 UTC (permalink / raw) To: Jakub Kicinski, Andrew Lunn Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Thu, 2020-10-01 at 08:50 -0700, Jakub Kicinski wrote: > > > > > > > + memcpy(op, &family->ops[i], sizeof(*op)); > > > > > > > > > > > > What's wrong with struct assignment? :) > FWIW the 400 was without the -Os with -Os it's more like 50. So I'll > just go for it and do the struct assignment. FWIW, I really don't think it actually _matters_. I just started using struct assignments more since they're type safe and you don't trip up any of the static checkers :-) More in non-kernel projects than the kernel though, to be honest. johannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Genetlink per cmd policies 2020-09-30 16:06 ` Johannes Berg 2020-09-30 16:17 ` Johannes Berg @ 2020-09-30 16:18 ` Jakub Kicinski 1 sibling, 0 replies; 22+ messages in thread From: Jakub Kicinski @ 2020-09-30 16:18 UTC (permalink / raw) To: Johannes Berg; +Cc: Jiri Pirko, Michal Kubecek, dsahern, pablo, netdev On Wed, 30 Sep 2020 18:06:28 +0200 Johannes Berg wrote: > Hi Jakub, > > > I'd like to be able to dump ethtool nl policies, but right now policy > > dumping is only supported for "global" family policies. > > Did ethtool add per-command policies? Yup. > > Is there any reason not to put the policy in struct genl_ops, > > or just nobody got to it, yet? > > > > I get the feeling that this must have been discussed before... > > Sort of, yeah. > > We actually *had* per-command policies, and I removed it in commit > 3b0f31f2b8c9 ("genetlink: make policy common to family"), mostly because > the maxattr is actually in the family not the op, so having a policy in > each op was never really a good idea and well-supported. > > Additionally, having the pointer in each op (and if you want to do it > right you also need maxattr in each op) significantly increases the > binary size there, as well as boilerplate code to type it out, though > the latter could be avoided by "falling back" to the family policy. Yup, it's like 2 or 3 "ops->family ? : rt->family" and we're good on fallback AFAICT. > So at the time, that reduced nl80211 size by 824 bytes, which I guess > means we had 103 ops at the time. Doing it right with maxattr would cost > twice as much as just the policy pointer, and we probably have a few > more ops now, so we're looking at a cost of ~1.6k for it. Hm. If we really care it wouldn't be too crazy to have "lightweight" ops and normal ops. At a cost of an extra pointer in the family. Core genl can deal with that. And we could save another 16B per op if we drop .start and .done which nl80211 barely uses. > Personally, I'm not really convinced that there really is a need for it, > but then I haven't really looked that much at ethtool. In most cases, > you want some "common" attributes for the family, even if it's only the > interface index or such, because also on the userspace side that's > awkward if you have to really build *everything* including such > identifying information per command. The one or two families that > actually had different policies per command (at the time I removed it) > actually solved this by "aliasing" the same attribute number between the > different policies, but again that feels rather awkward ... ethtool has common and per command parts. Global policies don't jell well with strict checking in my mind. > That's the historic info I guess - I'll take a look at ethtool later and > see what it's doing there. > > johannes > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-10-01 15:52 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 15:49 Genetlink per cmd policies Jakub Kicinski 2020-09-30 16:03 ` Michal Kubecek 2020-09-30 16:11 ` Jakub Kicinski 2020-09-30 16:06 ` Johannes Berg 2020-09-30 16:17 ` Johannes Berg 2020-09-30 16:44 ` Jakub Kicinski 2020-09-30 18:36 ` Johannes Berg 2020-09-30 18:42 ` Michal Kubecek 2020-09-30 18:42 ` Johannes Berg 2020-09-30 19:01 ` Jakub Kicinski 2020-09-30 19:03 ` Johannes Berg 2020-09-30 19:14 ` Jakub Kicinski 2020-09-30 19:15 ` Johannes Berg 2020-09-30 19:46 ` Jakub Kicinski 2020-09-30 20:13 ` Johannes Berg 2020-09-30 20:47 ` Jakub Kicinski 2020-09-30 23:38 ` Andrew Lunn 2020-10-01 0:23 ` Jakub Kicinski 2020-10-01 1:53 ` Andrew Lunn 2020-10-01 15:50 ` Jakub Kicinski 2020-10-01 15:52 ` Johannes Berg 2020-09-30 16:18 ` Jakub Kicinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.