Re: [PATCH net-next 05/15] devlink: Allow setting of packet trap group parameters

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 24 Mar 2020 21:32:40 +0200 Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
> 
> The previous patch allowed device drivers to publish their default
> binding between packet trap policers and packet trap groups. However,
> some users might not be content with this binding and would like to
> change it.
> 
> In case user space passed a packet trap policer identifier when setting
> a packet trap group, invoke the appropriate device driver callback and
> pass the new policer identifier.
> 
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/devlink.h |  9 +++++++++
>  net/core/devlink.c    | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 84c28e0f2d90..dea3c3fd9634 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -847,6 +847,15 @@ struct devlink_ops {
>  	 */
>  	int (*trap_group_init)(struct devlink *devlink,
>  			       const struct devlink_trap_group *group);
> +	/**
> +	 * @trap_group_set: Trap group parameters set function.
> +	 *
> +	 * Note: @policer can be NULL when a policer is being unbound from
> +	 * @group.
> +	 */
> +	int (*trap_group_set)(struct devlink *devlink,
> +			      const struct devlink_trap_group *group,
> +			      const struct devlink_trap_policer *policer);
>  	/**
>  	 * @trap_policer_init: Trap policer initialization function.
>  	 *
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 4ec7c7578709..e3042e131c1f 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6039,6 +6039,45 @@ devlink_trap_group_action_set(struct devlink *devlink,
>  	return 0;
>  }
>  
> +static int devlink_trap_group_set(struct devlink *devlink,
> +				  struct devlink_trap_group_item *group_item,
> +				  struct genl_info *info)
> +{
> +	struct devlink_trap_policer_item *policer_item;
> +	struct netlink_ext_ack *extack = info->extack;
> +	const struct devlink_trap_policer *policer;
> +	struct nlattr **attrs = info->attrs;
> +	int err;
> +

Why not:

	if (!attrs[DEVLINK_ATTR_TRAP_POLICER_ID])
		return 0?

> +	if (!devlink->ops->trap_group_set) {
> +		if (attrs[DEVLINK_ATTR_TRAP_POLICER_ID])
> +			return -EOPNOTSUPP;
> +		return 0;
> +	}
> +
> +	policer_item = group_item->policer_item;
> +	if (attrs[DEVLINK_ATTR_TRAP_POLICER_ID]) {
> +		u32 policer_id;
> +
> +		policer_id = nla_get_u32(attrs[DEVLINK_ATTR_TRAP_POLICER_ID]);
> +		policer_item = devlink_trap_policer_item_lookup(devlink,
> +								policer_id);
> +		if (policer_id && !policer_item) {
> +			NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap policer");

nit: is KBUILD_MODNAME still set if devlink can only be built-in now?

> +			return -ENOENT;
> +		}
> +	}
> +	policer = policer_item ? policer_item->policer : NULL;
> +
> +	err = devlink->ops->trap_group_set(devlink, group_item->group, policer);
> +	if (err)
> +		return err;
> +
> +	group_item->policer_item = policer_item;
> +
> +	return 0;
> +}
> +
>  static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
>  					      struct genl_info *info)
>  {
> @@ -6060,6 +6099,10 @@ static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
>  	if (err)
>  		return err;
>  
> +	err = devlink_trap_group_set(devlink, group_item, info);
> +	if (err)
> +		return err;

Should this unwind the action changes? Are the changes supposed to be
atomic? :S 

Also could it potentially be a problem if trap is being enabled and
policer applied - if we enable first the CPU may get overloaded and it
may be hard to apply the policer? Making sure the ordering is right
requires some careful checking, so IDK if its worth it..

>  	return 0;
>  }
>