Messages in this thread | | | Subject | Re: [PATCH net-next 04/16] devlink: Add reload stats | From | Moshe Shemesh <> | Date | Fri, 2 Oct 2020 18:07:05 +0300 |
| |
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 <moshe@mellanox.com> > Minor nits, looks good overall: > > Reviewed-by: Jakub Kicinski <kuba@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); >> +}
| |