Messages in this thread | | | Subject | Re: [PATCH net-next RFC v5 04/15] devlink: Add reload actions stats to dev get | From | Moshe Shemesh <> | Date | Thu, 24 Sep 2020 22:34:23 +0300 |
| |
On 9/23/2020 9:50 PM, Jakub Kicinski wrote: > On Fri, 18 Sep 2020 19:06:40 +0300 Moshe Shemesh wrote: >> Expose devlink reload actions stats to the user through devlink dev >> get command. >> >> Examples: >> $ devlink dev show >> pci/0000:82:00.0: >> stats: >> reload_action_stats: >> driver_reinit 2 >> fw_activate 1 >> fw_activate_no_reset 0 >> remote_driver_reinit 0 >> remote_fw_activate 0 >> remote_fw_activate_no_reset 0 >> pci/0000:82:00.1: >> stats: >> reload_action_stats: >> driver_reinit 0 >> fw_activate 0 >> fw_activate_no_reset 0 >> remote_driver_reinit 1 >> remote_fw_activate 1 >> remote_fw_activate_no_reset 0 >> >> $ devlink dev show -jp >> { >> "dev": { >> "pci/0000:82:00.0": { >> "stats": { >> "reload_action_stats": [ { >> "driver_reinit": 2 >> },{ >> "fw_activate": 1 >> },{ >> "fw_activate_no_reset": 0 >> },{ >> "remote_driver_reinit": 0 >> },{ >> "remote_fw_activate": 0 >> },{ >> "remote_fw_activate_no_reset": 0 >> } ] >> } >> }, >> "pci/0000:82:00.1": { >> "stats": { >> "reload_action_stats": [ { >> "driver_reinit": 0 >> },{ >> "fw_activate": 0 >> },{ >> "fw_activate_no_reset": 0 >> },{ >> "remote_driver_reinit": 1 >> },{ >> "remote_fw_activate": 1 >> },{ >> "remote_fw_activate_no_reset": 0 >> } ] >> } >> } >> } >> } >> >> Signed-off-by: Moshe Shemesh <moshe@mellanox.com> >> --- >> v4 -> v5: >> - Add remote actions stats >> - If devlink reload is not supported, show only remote_stats >> v3 -> v4: >> - Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to >> DEVLINK_ATTR_RELOAD_ACTION_STAT >> - Add stats per action per limit level >> v2 -> v3: >> - Add reload actions counters instead of supported reload actions >> (reload actions counters are only for supported action so no need for >> both) >> v1 -> v2: >> - Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL >> - Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO >> - Have actions instead of levels >> --- >> include/uapi/linux/devlink.h | 5 +++ >> net/core/devlink.c | 70 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 75 insertions(+) >> >> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h >> index 0c5d942dcbd5..648d53be691e 100644 >> --- a/include/uapi/linux/devlink.h >> +++ b/include/uapi/linux/devlink.h >> @@ -497,7 +497,12 @@ enum devlink_attr { >> DEVLINK_ATTR_RELOAD_ACTION, /* u8 */ >> DEVLINK_ATTR_RELOAD_ACTIONS_PERFORMED, /* nested */ >> DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, /* u8 */ >> + DEVLINK_ATTR_RELOAD_ACTION_STATS, /* nested */ >> + DEVLINK_ATTR_RELOAD_ACTION_STAT, /* nested */ >> + DEVLINK_ATTR_RELOAD_ACTION_STAT_REMOTE, /* flag */ >> + DEVLINK_ATTR_RELOAD_ACTION_STAT_VALUE, /* u32 */ >> >> + DEVLINK_ATTR_DEV_STATS, /* nested */ >> /* add new attributes above here, update the policy in devlink.c */ > I'd propose this nesting: > > [DEV_STATS] > [RELOAD_STATS] > [DEV_STATS_ENTRY] > [ACTION] > [LIMIT] > [VALUE] > [DEV_STATS_ENTRY] > [...] > [REMOTE_RELOAD_STATS] > [DEV_STATS_ENTRY] > [ACTION] > [LIMIT] > [VALUE] > [DEV_STATS_ENTRY] > [...] > > Then you can fill in the inside of the [REMOTE_]RELOAD_STATS nests with > a helper, and similarly user space can separate the two in JSON more > cleanly than string concat.
Right, will fix, thanks.
>> __DEVLINK_ATTR_MAX, >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 1509c2ffb98b..71aeda259e6a 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -501,10 +501,39 @@ devlink_reload_action_limit_level_is_supported(struct devlink *devlink, >> return test_bit(limit_level, &devlink->ops->supported_reload_action_limit_levels); >> } >> >> +static int devlink_reload_action_stat_put(struct sk_buff *msg, enum devlink_reload_action action, >> + enum devlink_reload_action_limit_level limit_level, >> + bool is_remote, u32 value) >> +{ >> + struct nlattr *reload_action_stat; >> + >> + reload_action_stat = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTION_STAT); >> + if (!reload_action_stat) >> + return -EMSGSIZE; >> + >> + if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION, action)) >> + goto nla_put_failure; >> + if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_ACTION_LIMIT_LEVEL, limit_level)) >> + goto nla_put_failure; >> + if (is_remote && nla_put_flag(msg, DEVLINK_ATTR_RELOAD_ACTION_STAT_REMOTE)) >> + goto nla_put_failure; >> + if (nla_put_u32(msg, DEVLINK_ATTR_RELOAD_ACTION_STAT_VALUE, value)) >> + goto nla_put_failure; >> + nla_nest_end(msg, reload_action_stat); >> + return 0; >> + >> +nla_put_failure: >> + nla_nest_cancel(msg, reload_action_stat); >> + return -EMSGSIZE; >> +} >> + >> static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, >> enum devlink_command cmd, u32 portid, >> u32 seq, int flags) >> { >> + struct nlattr *dev_stats, *reload_action_stats; >> + int i, j, stat_idx; >> + u32 value; >> void *hdr; >> >> hdr = genlmsg_put(msg, portid, seq, &devlink_nl_family, flags, cmd); >> @@ -516,9 +545,50 @@ static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink, >> if (nla_put_u8(msg, DEVLINK_ATTR_RELOAD_FAILED, devlink->reload_failed)) >> goto nla_put_failure; >> >> + dev_stats = nla_nest_start(msg, DEVLINK_ATTR_DEV_STATS); >> + if (!dev_stats) >> + goto nla_put_failure; >> + reload_action_stats = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_ACTION_STATS); >> + if (!reload_action_stats) >> + goto dev_stats_nest_cancel; >> + >> + for (j = 0; j <= DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX; j++) { >> + if (!devlink_reload_action_limit_level_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)) >> + continue; >> + >> + stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i; >> + value = devlink->reload_action_stats[stat_idx]; >> + if (devlink_reload_action_stat_put(msg, i, j, false, value)) >> + goto reload_action_stats_nest_cancel; >> + } >> + } >> + >> + for (j = 0; j <= DEVLINK_RELOAD_ACTION_LIMIT_LEVEL_MAX; j++) { >> + for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) { >> + if (i == DEVLINK_RELOAD_ACTION_UNSPEC || >> + devlink_reload_combination_is_invalid(i, j)) >> + continue; >> + >> + stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i; >> + value = devlink->remote_reload_action_stats[stat_idx]; >> + if (devlink_reload_action_stat_put(msg, i, j, true, value)) >> + goto reload_action_stats_nest_cancel; >> + } >> + } > This calls for a helper. Ack.
| |