Re: [PATCH net-next RFC v5 02/15] devlink: Add reload action limit level

[Jakub Kicinski]

On Thu, 24 Sep 2020 22:29:55 +0300 Moshe Shemesh wrote:
> >> @@ -3964,6 +3965,7 @@ static int mlx4_devlink_reload_down(struct devlink *devlink, bool netns_change,
> >>   }
> >>
> >>   static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_action action,
> >> +                               enum devlink_reload_action_limit_level limit_level,
> >>                                  struct netlink_ext_ack *extack, unsigned long *actions_performed)
> >>   {
> >>        struct mlx4_priv *priv = devlink_priv(devlink);
> >> @@ -3985,6 +3987,7 @@ static int mlx4_devlink_reload_up(struct devlink *devlink, enum devlink_reload_a
> >>   static const struct devlink_ops mlx4_devlink_ops = {
> >>        .port_type_set  = mlx4_devlink_port_type_set,
> >>        .supported_reload_actions = BIT(DEVLINK_RELOAD_ACTION_DRIVER_REINIT),
> >> +     .supported_reload_action_limit_levels = BIT(DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE),  
> > Please cut down the name lenghts, this is just lazy.
> >
> > 'supported_reload_limits' or 'cap_reload_limits' is perfectly
> > sufficient.
> >
> > 'reload_limits' would be even better. Cause what else would it be if
> > not a capability.  
> 
> Sounds good.
> 
> So instead of supported_reload_actions_limit_levels will have reload_limits.
> 
> Instead of supported_reload_actions will have reload_actions, OK ?

Sounds good.

> May also use reload_limit_level instead of reload_action_limit_level 
> everywhere if its clear enough.

I think reload_limits is clear. I'd also cut down the length of the
defines / enum names.

> > Besides I don't think drivers should have to fill negative attributes
> > (that they don't support something). Everyone is always going to
> > support NONE, since it's "unspecified" / "pick your favorite", right?  
> 
> Good point, will remove it.
> 
> >>        .reload_down    = mlx4_devlink_reload_down,
> >>        .reload_up      = mlx4_devlink_reload_up,
> >>   };
> >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> >> index fdba7ab58a79..0c5d942dcbd5 100644
> >> --- a/include/uapi/linux/devlink.h
> >> +++ b/include/uapi/linux/devlink.h
> >> @@ -289,6 +289,22 @@ enum devlink_reload_action {
> >>        DEVLINK_RELOAD_ACTION_MAX = __DEVLINK_RELOAD_ACTION_MAX - 1
> >>   };
> >>
> >> +/**
> >> + * enum devlink_reload_action_limit_level - Reload action limit level.
> >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE: No constrains on action. Action may include
> >> + *                                          reset or downtime as needed.
> >> + * @DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET: No reset allowed, no down time allowed,
> >> + *                                              no link flap and no configuration is lost.
> >> + */
> >> +enum devlink_reload_action_limit_level {  
> > You reserved UNSPEC for actions but not for limit level?  
> 
> 
> Yes, I used LIMIT_LEVEL_NONE = 0 as no limit needed, so I skipped UNSPEC.
> 
> Maybe should add UNSPEC and use UNSPEC as no limit needed. But UNSPEC is 
> kind of invalid.

Yeah, if we have UNSPEC then it should be invalid.

I'm mostly asking for consistency, either have UNSPEC for both actions
and limits or for neither.

> >> +     DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NONE,
> >> +     DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET,
> >> +
> >> +     /* Add new reload actions limit level above */
> >> +     __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX,
> >> +     DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX = __DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX - 1
> >> +};
> >> +
> >>   enum devlink_attr {
> >>        /* don't change the order or add anything between, this is ABI! */
> >>        DEVLINK_ATTR_UNSPEC,
> >> @@ -480,6 +496,7 @@ enum devlink_attr {
> >>
> >>        DEVLINK_ATTR_RELOAD_ACTION,             /* u8 */
> >>        DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED,  /* nested */
> >> +     DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */
> >>
> >>        /* add new attributes above here, update the policy in devlink.c */
> >>
> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> index 318ef29f81f2..fee6fcc7dead 100644
> >> --- a/net/core/devlink.c
> >> +++ b/net/core/devlink.c
> >> @@ -462,12 +462,45 @@ static int devlink_nl_put_handle(struct sk_buff *msg, struct devlink *devlink)
> >>        return 0;
> >>   }
> >>
> >> +struct devlink_reload_combination {
> >> +     enum devlink_reload_action action;
> >> +     enum devlink_reload_action_limit_level limit_level;
> >> +};
> >> +
> >> +static const struct devlink_reload_combination devlink_reload_invalid_combinations[] = {
> >> +     {
> >> +             /* can't reinitialize driver with no down time */
> >> +             .action = DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> >> +             .limit_level = DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_NO_RESET,
> >> +     },
> >> +};
> >> +
> >> +static bool
> >> +devlink_reload_combination_is_invalid(enum devlink_reload_action action,
> >> +                                   enum devlink_reload_action_limit_level limit_level)
> >> +{
> >> +     int i;
> >> +
> >> +     for (i = 0 ; i <  ARRAY_SIZE(devlink_reload_invalid_combinations) ; i++)  
> > Whitespace. Did you checkpatch?  
> 
> 
> Yes, checked it again now, it still pass. I think checkpatch doesn't see 
> double space.

And the spaces before semicolons? It's sad if checkpatch misses such
basic stuff :(

> But anyway, I missed it, I will fix.