Re: [PATCH net-next RFC v5 03/15] devlink: Add reload action stats

[Moshe Shemesh]

On 9/23/2020 9:42 PM, Jakub Kicinski wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, 18 Sep 2020 19:06:39 +0300 Moshe Shemesh wrote:
> > Add reload action stats to hold the history per reload action type and
> > limit level.
> >
> > For example, the number of times fw_activate has been performed on this
> > device since the driver module was added or if the firmware activation
> > was performed with or without reset.
> >
> > The function devlink_remote_reload_actions_performed() is exported to
> > enable also drivers update on reload actions performed, for example in
> > case firmware activation with reset finished successfully but was
> > initiated by remote host.
> >
> > Add devlink notification on stats update.
> >
> > Signed-off-by: Moshe Shemesh <moshe@xxxxxxxxxxxx>
> I'd split this and the next patch by stat type (remote vs local).
> Rather than split them by collect / report.


OK.

> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> > index d8c62d605381..f09f55a47d09 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -20,6 +20,9 @@
> >   #include <uapi/linux/devlink.h>
> >   #include <linux/xarray.h>
> >
> > +#define DEVLINK_RELOAD_ACTION_STATS_ARRAY_SIZE \
> > +     (__DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX * __DEVLINK_RELOAD_ACTION_MAX)
> > +
> >   struct devlink_ops;
> >
> >   struct devlink {
> > @@ -38,6 +41,8 @@ struct devlink {
> >        struct list_head trap_policer_list;
> >        const struct devlink_ops *ops;
> >        struct xarray snapshot_ids;
> > +     u32 reload_action_stats[DEVLINK_RELOAD_ACTION_STATS_ARRAY_SIZE];
> > +     u32 remote_reload_action_stats[DEVLINK_RELOAD_ACTION_STATS_ARRAY_SIZE];
> >        struct device *dev;
> >        possible_net_t _net;
> >        struct mutex lock; /* Serializes access to devlink instance specific objects such as
> > @@ -1400,6 +1405,9 @@ void
> >   devlink_health_reporter_recovery_done(struct devlink_health_reporter *reporter);
> >
> >   bool devlink_is_reload_failed(const struct devlink *devlink);
> > +void devlink_remote_reload_actions_performed(struct devlink *devlink,
> > +                                          enum devlink_reload_action_limit_level limit_level,
> > +                                          unsigned long actions_performed);
> >
> >   void devlink_flash_update_begin_notify(struct devlink *devlink);
> >   void devlink_flash_update_end_notify(struct devlink *devlink);
> > diff --git a/net/core/devlink.c b/net/core/devlink.c
> > index fee6fcc7dead..1509c2ffb98b 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -3007,16 +3007,74 @@ bool devlink_is_reload_failed(const struct devlink *devlink)
> >   }
> >   EXPORT_SYMBOL_GPL(devlink_is_reload_failed);
> >
> > +static void
> > +__devlink_reload_action_stats_update(struct devlink *devlink,
> > +                                  u32 *reload_action_stats,
> > +                                  enum devlink_reload_action_limit_level limit_level,
> > +                                  unsigned long actions_performed)
> > +{
> > +     int stat_idx;
> > +     int action;
> > +
> > +     if (!actions_performed)
> > +             return;
> > +
> > +     if (WARN_ON(limit_level > DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX))
> > +             return;
> > +     for (action = 0; action <= DEVLINK_RELOAD_ACTION_MAX; action++) {
> > +             if (!test_bit(action, &actions_performed))
> > +                     continue;
> > +             stat_idx = limit_level * __DEVLINK_RELOAD_ACTION_MAX + action;
> > +             reload_action_stats[stat_idx]++;
> > +     }
> > +     devlink_notify(devlink, DEVLINK_CMD_NEW);
> > +}
> > +
> > +static void
> > +devlink_reload_action_stats_update(struct devlink *devlink,
> > +                                enum devlink_reload_action_limit_level limit_level,
> > +                                unsigned long actions_performed)
> > +{
> > +     __devlink_reload_action_stats_update(devlink, devlink->reload_action_stats,
> > +                                          limit_level, actions_performed);
> > +}
> > +
> > +/**
> > + *   devlink_remote_reload_actions_performed - Update devlink on reload actions
> > + *     performed which are not a direct result of devlink reload call.
> > + *
> > + *   This should be called by a driver after performing reload actions in case it was not
> > + *   a result of devlink reload call. For example fw_activate was performed as a result
> > + *   of devlink reload triggered fw_activate on another host.
> > + *   The motivation for this function is to keep data on reload actions performed on this
> > + *   function whether it was done due to direct devlink reload call or not.
> > + *
> > + *   @devlink: devlink
> > + *   @limit_level: reload action limit level
> > + *   @actions_performed: bitmask of actions performed
> > + */
> > +void devlink_remote_reload_actions_performed(struct devlink *devlink,
> > +                                          enum devlink_reload_action_limit_level limit_level,
> > +                                          unsigned long actions_performed)
> > +{
> > +     __devlink_reload_action_stats_update(devlink, devlink->remote_reload_action_stats,
> > +                                          limit_level, actions_performed);
> > +}
> > +EXPORT_SYMBOL_GPL(devlink_remote_reload_actions_performed);
> > +
> >   static int devlink_reload(struct devlink *devlink, struct net *dest_net,
> >                          enum devlink_reload_action action,
> >                          enum devlink_reload_action_limit_level limit_level,
> >                          struct netlink_ext_ack *extack, unsigned long *actions_performed)
> >   {
> > +     u32 remote_reload_action_stats[DEVLINK_RELOAD_ACTION_STATS_ARRAY_SIZE];
> >        int err;
> >
> >        if (!devlink->reload_enabled)
> >                return -EOPNOTSUPP;
> >
> > +     memcpy(remote_reload_action_stats, devlink->remote_reload_action_stats,
> > +            sizeof(remote_reload_action_stats));
> >        err = devlink->ops->reload_down(devlink, !!dest_net, action, limit_level, extack);
> >        if (err)
> >                return err;
> > @@ -3030,6 +3088,10 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
> >                return err;
> >
> >        WARN_ON(!test_bit(action, actions_performed));
> > +     /* protect from driver updating the remote action within devlink reload */
> s/protect from/catch/


Ack.

> > +     WARN_ON(memcmp(remote_reload_action_stats, devlink->remote_reload_action_stats,
> > +                    sizeof(remote_reload_action_stats)));
> > +     devlink_reload_action_stats_update(devlink, limit_level, *actions_performed);
> >        return 0;
> >   }