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

[Ido Schimmel <idosch@idosch.org>]

On Tue, Mar 24, 2020 at 08:53:14PM -0700, Jakub Kicinski wrote:
> 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?

Trying to come up with a good reason why I didn't do it, but don't have
any :p

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

It seems fine:

NL_SET_ERR_MSG_MOD:

# devlink trap policer set pci/0000:01:00.0 policer 1337
Error: devlink: Device did not register this trap policer.
devlink answers: No such file or directory

NL_SET_ERR_MSG:

# devlink trap policer set pci/0000:01:00.0 policer 1337
Error: Device did not register this trap policer.
devlink answers: No such file or directory

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

I used do_setlink() as reference and it seems that it does not unwind
the changes. I can add extack message in case we did set action and
devlink_trap_group_set() failed.

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

I'm not sure it's really an issue, but I can flip the order just to be
on the safe side.

> 
> >  	return 0;
> >  }
> >  
>