Genetlink per cmd policies

Genetlink per cmd policies

[Jakub Kicinski]

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

Re: Genetlink per cmd policies

[Michal Kubecek]

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

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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 🤔

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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
> 

Re: Genetlink per cmd policies

[Jakub Kicinski]

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?

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Michal Kubecek]

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 

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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.

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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,

Re: Genetlink per cmd policies

[Johannes Berg]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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.

Re: Genetlink per cmd policies

[Andrew Lunn]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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>

Re: Genetlink per cmd policies

[Andrew Lunn]

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

Re: Genetlink per cmd policies

[Jakub Kicinski]

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.

Re: Genetlink per cmd policies

[Johannes Berg]

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