Re: [PATCH net-next 04/16] devlink: Add reload stats

[Moshe Shemesh]

On 10/2/2020 12:25 AM, Jakub Kicinski wrote:
External email: Use caution opening links or attachments


On Thu,  1 Oct 2020 16:59:07 +0300 Moshe Shemesh wrote:
Add reload stats to hold the history per reload action type and limit.

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.

Add devlink notification on stats update.

Expose devlink reload stats to the user through devlink dev get command.

Examples:
$ devlink dev show
pci/0000:82:00.0:
   stats:
       reload_stats:
         driver_reinit 2
         fw_activate 1
         fw_activate_no_reset 0
pci/0000:82:00.1:
   stats:
       reload_stats:
         driver_reinit 1
         fw_activate 0
         fw_activate_no_reset 0

$ devlink dev show -jp
{
     "dev": {
         "pci/0000:82:00.0": {
             "stats": {
                 "reload_stats": [ {
                         "driver_reinit": 2
                     },{
                         "fw_activate": 1
                     },{
                         "fw_activate_no_reset": 0
                     } ]
             }
         },
         "pci/0000:82:00.1": {
             "stats": {
                 "reload_stats": [ {
                         "driver_reinit": 1
                     },{
                         "fw_activate": 0
                     },{
                         "fw_activate_no_reset": 0
                     } ]
This will be a question to the user space part but IDK why every stat
is in a separate object?

Instead of doing an array of objects -> [ {}, {}, {} ]
make "reload_stats" itself an object.


I first thought that there should be an array of stats, looking at it 
again, as long as each stat contains just sting and value, object of 
pairs will perfectly fit. I will change it.

             }
         }
     }
}

Signed-off-by: Moshe Shemesh <mo...@mellanox.com>
Minor nits, looks good overall:

Reviewed-by: Jakub Kicinski <k...@kernel.org>

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6de7d6aa6ed1..05516f1e4c3e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -500,10 +500,68 @@ devlink_reload_limit_is_supported(struct devlink 
*devlink, enum devlink_reload_l
       return test_bit(limit, &devlink->ops->reload_limits);
  }

+static int devlink_reload_stat_put(struct sk_buff *msg, enum 
devlink_reload_action action,
+                                enum devlink_reload_limit limit, u32 value)
+{
+     struct nlattr *reload_stats_entry;
+
+     reload_stats_entry = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS_ENTRY);
+     if (!reload_stats_entry)
+             return -EMSGSIZE;
+
+     if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, action))
+             goto nla_put_failure;
+     if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_LIMIT, limit))
+             goto nla_put_failure;
+     if (nla_put_u32(msg, DEVLINK_ATTR_RELOAD_STATS_VALUE, value))
+             goto nla_put_failure;
nit: it's common to combine such expressions into:

if (nla_put...() ||
     nla_put...() ||
     nla_put...())
     goto ...;


Ack.

+     nla_nest_end(msg, reload_stats_entry);
+     return 0;
+
+nla_put_failure:
+     nla_nest_cancel(msg, reload_stats_entry);
+     return -EMSGSIZE;
+}
+
+static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink 
*devlink)
+{
+     struct nlattr *reload_stats_attr;
+     int i, j, stat_idx;
+     u32 value;
+
+     reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
+
+     if (!reload_stats_attr)
+             return -EMSGSIZE;
+
+     for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
+             if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
Why check this? It can't be supported.
Show stats of the actions with unspecified limit.
+                 !devlink_reload_limit_is_supported(devlink, j))
+                     continue;
+             for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) {
+                     if (!devlink_reload_action_is_supported(devlink, i) ||
+                         devlink_reload_combination_is_invalid(i, j))
Again, devlink instance would not have been registered with invalid
combinations, right?


It does register actions and register limits, there can be invalid 
combination, but devlink will block it before it gets to the driver.

+                             continue;
+
+                     stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i;
+                     value = devlink->reload_stats[stat_idx];
+                     if (devlink_reload_stat_put(msg, i, j, value))
+                             goto nla_put_failure;
+             }
+     }
+     nla_nest_end(msg, reload_stats_attr);
+     return 0;
+
+nla_put_failure:
+     nla_nest_cancel(msg, reload_stats_attr);
+     return -EMSGSIZE;
+}

@@ -3004,6 +3072,34 @@ bool devlink_is_reload_failed(const struct devlink 
*devlink)
  }
  EXPORT_SYMBOL_GPL(devlink_is_reload_failed);

+static void
+__devlink_reload_stats_update(struct devlink *devlink, u32 *reload_stats,
+                           enum devlink_reload_limit limit, unsigned long 
actions_performed)
+     for (action = 0; action <= DEVLINK_RELOAD_ACTION_MAX; action++) {
nit: for_each_set_bit
Ack.
+             if (!test_bit(action, &actions_performed))
+                     continue;
+             stat_idx = limit * __DEVLINK_RELOAD_ACTION_MAX + action;
+             reload_stats[stat_idx]++;
+     }
+     devlink_notify(devlink, DEVLINK_CMD_NEW);
+}