Add remote reload stats to hold the history of actions performed due
devlink reload commands initiated by remote host. For example, in case
firmware activation with reset finished successfully but was initiated
by remote host.
The function devlink_remote_reload_actions_performed() is exported to
enable drivers update on remote reload actions performed as it was not
initiated by their own devlink instance.
Expose devlink remote 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
remote_reload_stats:
driver_reinit 0
fw_activate 0
fw_activate_no_reset 0
pci/0000:82:00.1:
stats:
reload_stats:
driver_reinit 1
fw_activate 0
fw_activate_no_reset 0
remote_reload_stats:
driver_reinit 1
fw_activate 1
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
} ],
"remote_reload_stats": [ {
"driver_reinit": 0
},{
"fw_activate": 0
},{
"fw_activate_no_reset": 0
} ]
}
},
"pci/0000:82:00.1": {
"stats": {
"reload_stats": [ {
"driver_reinit": 1
},{
"fw_activate": 0
},{
"fw_activate_no_reset": 0
} ],
"remote_reload_stats": [ {
"driver_reinit": 1
},{
"fw_activate": 1
},{
"fw_activate_no_reset": 0
} ]
}
}
}
}
Signed-off-by: Moshe Shemesh <[email protected]>
---
RFCv5 -> v1:
- Resplit this patch and the previous one by remote/local reload stats
instead of set/get reload stats
- Rename reload_action_stats to reload_stats
RFCv4 -> RFCv5:
- Add remote actions stats
- If devlink reload is not supported, show only remote_stats
RFCv3 -> RFCv4:
- Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to
DEVLINK_ATTR_RELOAD_ACTION_STAT
- Add stats per action per limit level
RFCv2 -> RFCv3:
- Add reload actions counters instead of supported reload actions
(reload actions counters are only for supported action so no need for
both)
RFCv1 -> RFCv2:
- Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL
- Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO
- Have actions instead of levels
---
include/net/devlink.h | 1 +
include/uapi/linux/devlink.h | 1 +
net/core/devlink.c | 49 +++++++++++++++++++++++++++++++-----
3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 0f3bd23b6c04..a4ccb83bbd2c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -42,6 +42,7 @@ struct devlink {
const struct devlink_ops *ops;
struct xarray snapshot_ids;
u32 reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
+ u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
struct device *dev;
possible_net_t _net;
struct mutex lock; /* Serializes access to devlink instance specific objects such as
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 97e0137f6201..f9887d8afdc7 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -530,6 +530,7 @@ enum devlink_attr {
DEVLINK_ATTR_RELOAD_STATS, /* nested */
DEVLINK_ATTR_RELOAD_STATS_ENTRY, /* nested */
DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */
+ DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */
/* add new attributes above here, update the policy in devlink.c */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 05516f1e4c3e..3b6bd3b4d346 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -523,28 +523,35 @@ static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
return -EMSGSIZE;
}
-static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink)
+static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
{
struct nlattr *reload_stats_attr;
int i, j, stat_idx;
u32 value;
- reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
+ if (!is_remote)
+ reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
+ else
+ reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_REMOTE_RELOAD_STATS);
if (!reload_stats_attr)
return -EMSGSIZE;
for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
- if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
+ if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
!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) ||
+ if ((!is_remote && !devlink_reload_action_is_supported(devlink, i)) ||
+ i == DEVLINK_RELOAD_ACTION_UNSPEC ||
devlink_reload_combination_is_invalid(i, j))
continue;
stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i;
- value = devlink->reload_stats[stat_idx];
+ if (!is_remote)
+ value = devlink->reload_stats[stat_idx];
+ else
+ value = devlink->remote_reload_stats[stat_idx];
if (devlink_reload_stat_put(msg, i, j, value))
goto nla_put_failure;
}
@@ -577,7 +584,9 @@ static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink,
if (!dev_stats)
goto nla_put_failure;
- if (devlink_reload_stats_put(msg, devlink))
+ if (devlink_reload_stats_put(msg, devlink, false))
+ goto dev_stats_nest_cancel;
+ if (devlink_reload_stats_put(msg, devlink, true))
goto dev_stats_nest_cancel;
nla_nest_end(msg, dev_stats);
@@ -3100,15 +3109,40 @@ devlink_reload_stats_update(struct devlink *devlink, enum devlink_reload_limit l
__devlink_reload_stats_update(devlink, devlink->reload_stats, limit, 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: reload limit
+ * @actions_performed: bitmask of actions performed
+ */
+void devlink_remote_reload_actions_performed(struct devlink *devlink,
+ enum devlink_reload_limit limit,
+ unsigned long actions_performed)
+{
+ __devlink_reload_stats_update(devlink, devlink->remote_reload_stats, limit,
+ 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_limit limit,
struct netlink_ext_ack *extack, unsigned long *actions_performed)
{
+ u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
int err;
if (!devlink->reload_enabled)
return -EOPNOTSUPP;
+ memcpy(remote_reload_stats, devlink->remote_reload_stats, sizeof(remote_reload_stats));
err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
if (err)
return err;
@@ -3122,6 +3156,9 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
return err;
WARN_ON(!test_bit(action, actions_performed));
+ /* Catch driver on updating the remote action within devlink reload */
+ WARN_ON(memcmp(remote_reload_stats, devlink->remote_reload_stats,
+ sizeof(remote_reload_stats)));
devlink_reload_stats_update(devlink, limit, *actions_performed);
return 0;
}
--
2.18.2
On Thu, 1 Oct 2020 16:59:08 +0300 Moshe Shemesh wrote:
> Add remote reload stats to hold the history of actions performed due
> devlink reload commands initiated by remote host. For example, in case
> firmware activation with reset finished successfully but was initiated
> by remote host.
>
> The function devlink_remote_reload_actions_performed() is exported to
> enable drivers update on remote reload actions performed as it was not
> initiated by their own devlink instance.
>
> Expose devlink remote reload stats to the user through devlink dev get
> command.
Reviewed-by: Jakub Kicinski <[email protected]>
> for (i = 0; i <= DEVLINK_RELOAD_ACTION_MAX; i++) {
> - if (!devlink_reload_action_is_supported(devlink, i) ||
> + if ((!is_remote && !devlink_reload_action_is_supported(devlink, i))
I see the point of these checks now, I guess it would have been cleaner
if they were added in this patch, but no big deal.
Thu, Oct 01, 2020 at 03:59:08PM CEST, [email protected] wrote:
>Add remote reload stats to hold the history of actions performed due
>devlink reload commands initiated by remote host. For example, in case
>firmware activation with reset finished successfully but was initiated
>by remote host.
>
>The function devlink_remote_reload_actions_performed() is exported to
>enable drivers update on remote reload actions performed as it was not
>initiated by their own devlink instance.
>
>Expose devlink remote 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
> remote_reload_stats:
> driver_reinit 0
> fw_activate 0
> fw_activate_no_reset 0
>pci/0000:82:00.1:
> stats:
> reload_stats:
> driver_reinit 1
> fw_activate 0
> fw_activate_no_reset 0
> remote_reload_stats:
> driver_reinit 1
> fw_activate 1
> 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
> } ],
> "remote_reload_stats": [ {
> "driver_reinit": 0
> },{
> "fw_activate": 0
> },{
> "fw_activate_no_reset": 0
> } ]
> }
> },
> "pci/0000:82:00.1": {
> "stats": {
> "reload_stats": [ {
> "driver_reinit": 1
> },{
> "fw_activate": 0
> },{
> "fw_activate_no_reset": 0
> } ],
> "remote_reload_stats": [ {
> "driver_reinit": 1
> },{
> "fw_activate": 1
> },{
> "fw_activate_no_reset": 0
> } ]
> }
> }
> }
>}
>
>Signed-off-by: Moshe Shemesh <[email protected]>
>---
>RFCv5 -> v1:
>- Resplit this patch and the previous one by remote/local reload stats
>instead of set/get reload stats
>- Rename reload_action_stats to reload_stats
>RFCv4 -> RFCv5:
>- Add remote actions stats
>- If devlink reload is not supported, show only remote_stats
>RFCv3 -> RFCv4:
>- Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to
> DEVLINK_ATTR_RELOAD_ACTION_STAT
>- Add stats per action per limit level
>RFCv2 -> RFCv3:
>- Add reload actions counters instead of supported reload actions
> (reload actions counters are only for supported action so no need for
> both)
>RFCv1 -> RFCv2:
>- Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL
>- Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO
>- Have actions instead of levels
>---
> include/net/devlink.h | 1 +
> include/uapi/linux/devlink.h | 1 +
> net/core/devlink.c | 49 +++++++++++++++++++++++++++++++-----
> 3 files changed, 45 insertions(+), 6 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 0f3bd23b6c04..a4ccb83bbd2c 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -42,6 +42,7 @@ struct devlink {
> const struct devlink_ops *ops;
> struct xarray snapshot_ids;
> u32 reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>+ u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
Perhaps a nested struct {} stats?
> struct device *dev;
> possible_net_t _net;
> struct mutex lock; /* Serializes access to devlink instance specific objects such as
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 97e0137f6201..f9887d8afdc7 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -530,6 +530,7 @@ enum devlink_attr {
> DEVLINK_ATTR_RELOAD_STATS, /* nested */
> DEVLINK_ATTR_RELOAD_STATS_ENTRY, /* nested */
> DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */
>+ DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */
>
> /* add new attributes above here, update the policy in devlink.c */
>
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 05516f1e4c3e..3b6bd3b4d346 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -523,28 +523,35 @@ static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
> return -EMSGSIZE;
> }
>
>-static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink)
>+static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
> {
> struct nlattr *reload_stats_attr;
> int i, j, stat_idx;
> u32 value;
>
>- reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>+ if (!is_remote)
>+ reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>+ else
>+ reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_REMOTE_RELOAD_STATS);
>
> if (!reload_stats_attr)
> return -EMSGSIZE;
>
> for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
>- if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>+ if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
I don't follow the check "!is_remote" here,
> !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) ||
>+ if ((!is_remote && !devlink_reload_action_is_supported(devlink, i)) ||
and here. Could you perhaps put in a comment to describe what are you
doing?
>+ i == DEVLINK_RELOAD_ACTION_UNSPEC ||
> devlink_reload_combination_is_invalid(i, j))
> continue;
>
> stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i;
>- value = devlink->reload_stats[stat_idx];
>+ if (!is_remote)
>+ value = devlink->reload_stats[stat_idx];
>+ else
>+ value = devlink->remote_reload_stats[stat_idx];
> if (devlink_reload_stat_put(msg, i, j, value))
> goto nla_put_failure;
> }
>@@ -577,7 +584,9 @@ static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink,
> if (!dev_stats)
> goto nla_put_failure;
>
>- if (devlink_reload_stats_put(msg, devlink))
>+ if (devlink_reload_stats_put(msg, devlink, false))
>+ goto dev_stats_nest_cancel;
>+ if (devlink_reload_stats_put(msg, devlink, true))
> goto dev_stats_nest_cancel;
>
> nla_nest_end(msg, dev_stats);
>@@ -3100,15 +3109,40 @@ devlink_reload_stats_update(struct devlink *devlink, enum devlink_reload_limit l
> __devlink_reload_stats_update(devlink, devlink->reload_stats, limit, 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: reload limit
>+ * @actions_performed: bitmask of actions performed
>+ */
>+void devlink_remote_reload_actions_performed(struct devlink *devlink,
>+ enum devlink_reload_limit limit,
>+ unsigned long actions_performed)
>+{
>+ __devlink_reload_stats_update(devlink, devlink->remote_reload_stats, limit,
>+ 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_limit limit,
> struct netlink_ext_ack *extack, unsigned long *actions_performed)
> {
>+ u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
> int err;
>
> if (!devlink->reload_enabled)
> return -EOPNOTSUPP;
>
>+ memcpy(remote_reload_stats, devlink->remote_reload_stats, sizeof(remote_reload_stats));
> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
> if (err)
> return err;
>@@ -3122,6 +3156,9 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
> return err;
>
> WARN_ON(!test_bit(action, actions_performed));
>+ /* Catch driver on updating the remote action within devlink reload */
>+ WARN_ON(memcmp(remote_reload_stats, devlink->remote_reload_stats,
>+ sizeof(remote_reload_stats)));
> devlink_reload_stats_update(devlink, limit, *actions_performed);
> return 0;
> }
>--
>2.18.2
>
On 10/3/2020 12:05 PM, Jiri Pirko wrote:
> Thu, Oct 01, 2020 at 03:59:08PM CEST, [email protected] wrote:
>> Add remote reload stats to hold the history of actions performed due
>> devlink reload commands initiated by remote host. For example, in case
>> firmware activation with reset finished successfully but was initiated
>> by remote host.
>>
>> The function devlink_remote_reload_actions_performed() is exported to
>> enable drivers update on remote reload actions performed as it was not
>> initiated by their own devlink instance.
>>
>> Expose devlink remote 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
>> remote_reload_stats:
>> driver_reinit 0
>> fw_activate 0
>> fw_activate_no_reset 0
>> pci/0000:82:00.1:
>> stats:
>> reload_stats:
>> driver_reinit 1
>> fw_activate 0
>> fw_activate_no_reset 0
>> remote_reload_stats:
>> driver_reinit 1
>> fw_activate 1
>> 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
>> } ],
>> "remote_reload_stats": [ {
>> "driver_reinit": 0
>> },{
>> "fw_activate": 0
>> },{
>> "fw_activate_no_reset": 0
>> } ]
>> }
>> },
>> "pci/0000:82:00.1": {
>> "stats": {
>> "reload_stats": [ {
>> "driver_reinit": 1
>> },{
>> "fw_activate": 0
>> },{
>> "fw_activate_no_reset": 0
>> } ],
>> "remote_reload_stats": [ {
>> "driver_reinit": 1
>> },{
>> "fw_activate": 1
>> },{
>> "fw_activate_no_reset": 0
>> } ]
>> }
>> }
>> }
>> }
>>
>> Signed-off-by: Moshe Shemesh <[email protected]>
>> ---
>> RFCv5 -> v1:
>> - Resplit this patch and the previous one by remote/local reload stats
>> instead of set/get reload stats
>> - Rename reload_action_stats to reload_stats
>> RFCv4 -> RFCv5:
>> - Add remote actions stats
>> - If devlink reload is not supported, show only remote_stats
>> RFCv3 -> RFCv4:
>> - Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to
>> DEVLINK_ATTR_RELOAD_ACTION_STAT
>> - Add stats per action per limit level
>> RFCv2 -> RFCv3:
>> - Add reload actions counters instead of supported reload actions
>> (reload actions counters are only for supported action so no need for
>> both)
>> RFCv1 -> RFCv2:
>> - Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL
>> - Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO
>> - Have actions instead of levels
>> ---
>> include/net/devlink.h | 1 +
>> include/uapi/linux/devlink.h | 1 +
>> net/core/devlink.c | 49 +++++++++++++++++++++++++++++++-----
>> 3 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 0f3bd23b6c04..a4ccb83bbd2c 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -42,6 +42,7 @@ struct devlink {
>> const struct devlink_ops *ops;
>> struct xarray snapshot_ids;
>> u32 reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>> + u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
> Perhaps a nested struct {} stats?
I guess you mean struct that holds these two arrays.
>
>> struct device *dev;
>> possible_net_t _net;
>> struct mutex lock; /* Serializes access to devlink instance specific objects such as
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index 97e0137f6201..f9887d8afdc7 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -530,6 +530,7 @@ enum devlink_attr {
>> DEVLINK_ATTR_RELOAD_STATS, /* nested */
>> DEVLINK_ATTR_RELOAD_STATS_ENTRY, /* nested */
>> DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */
>> + DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */
>>
>> /* add new attributes above here, update the policy in devlink.c */
>>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 05516f1e4c3e..3b6bd3b4d346 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -523,28 +523,35 @@ static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
>> return -EMSGSIZE;
>> }
>>
>> -static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink)
>> +static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
>> {
>> struct nlattr *reload_stats_attr;
>> int i, j, stat_idx;
>> u32 value;
>>
>> - reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>> + if (!is_remote)
>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>> + else
>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_REMOTE_RELOAD_STATS);
>>
>> if (!reload_stats_attr)
>> return -EMSGSIZE;
>>
>> for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
>> - if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>> + if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
> I don't follow the check "!is_remote" here,
We agreed that remote stats should be shown also for non supported
actions and limits, because its remote. So it makes this condition
different for remote stats. Rethinking about it, maybe that's wrong. I
mean if we had here reload actions as a result of remote driver, they
have common device, so it has to be the same type of driver and support
same actions/limits, right ?
>> !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) ||
>> + if ((!is_remote && !devlink_reload_action_is_supported(devlink, i)) ||
> and here. Could you perhaps put in a comment to describe what are you
> doing?
>
Yes, similar.
>
>> + i == DEVLINK_RELOAD_ACTION_UNSPEC ||
>> devlink_reload_combination_is_invalid(i, j))
>> continue;
>>
>> stat_idx = j * __DEVLINK_RELOAD_ACTION_MAX + i;
>> - value = devlink->reload_stats[stat_idx];
>> + if (!is_remote)
>> + value = devlink->reload_stats[stat_idx];
>> + else
>> + value = devlink->remote_reload_stats[stat_idx];
>> if (devlink_reload_stat_put(msg, i, j, value))
>> goto nla_put_failure;
>> }
>> @@ -577,7 +584,9 @@ static int devlink_nl_fill(struct sk_buff *msg, struct devlink *devlink,
>> if (!dev_stats)
>> goto nla_put_failure;
>>
>> - if (devlink_reload_stats_put(msg, devlink))
>> + if (devlink_reload_stats_put(msg, devlink, false))
>> + goto dev_stats_nest_cancel;
>> + if (devlink_reload_stats_put(msg, devlink, true))
>> goto dev_stats_nest_cancel;
>>
>> nla_nest_end(msg, dev_stats);
>> @@ -3100,15 +3109,40 @@ devlink_reload_stats_update(struct devlink *devlink, enum devlink_reload_limit l
>> __devlink_reload_stats_update(devlink, devlink->reload_stats, limit, 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: reload limit
>> + * @actions_performed: bitmask of actions performed
>> + */
>> +void devlink_remote_reload_actions_performed(struct devlink *devlink,
>> + enum devlink_reload_limit limit,
>> + unsigned long actions_performed)
>> +{
>> + __devlink_reload_stats_update(devlink, devlink->remote_reload_stats, limit,
>> + 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_limit limit,
>> struct netlink_ext_ack *extack, unsigned long *actions_performed)
>> {
>> + u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>> int err;
>>
>> if (!devlink->reload_enabled)
>> return -EOPNOTSUPP;
>>
>> + memcpy(remote_reload_stats, devlink->remote_reload_stats, sizeof(remote_reload_stats));
>> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
>> if (err)
>> return err;
>> @@ -3122,6 +3156,9 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>> return err;
>>
>> WARN_ON(!test_bit(action, actions_performed));
>> + /* Catch driver on updating the remote action within devlink reload */
>> + WARN_ON(memcmp(remote_reload_stats, devlink->remote_reload_stats,
>> + sizeof(remote_reload_stats)));
>> devlink_reload_stats_update(devlink, limit, *actions_performed);
>> return 0;
>> }
>> --
>> 2.18.2
>>
On 10/4/2020 12:09 AM, Moshe Shemesh wrote:
>
> On 10/3/2020 12:05 PM, Jiri Pirko wrote:
>> Thu, Oct 01, 2020 at 03:59:08PM CEST, [email protected] wrote:
>>> Add remote reload stats to hold the history of actions performed due
>>> devlink reload commands initiated by remote host. For example, in case
>>> firmware activation with reset finished successfully but was initiated
>>> by remote host.
>>>
>>> The function devlink_remote_reload_actions_performed() is exported to
>>> enable drivers update on remote reload actions performed as it was not
>>> initiated by their own devlink instance.
>>>
>>> Expose devlink remote 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
>>> remote_reload_stats:
>>> driver_reinit 0
>>> fw_activate 0
>>> fw_activate_no_reset 0
>>> pci/0000:82:00.1:
>>> stats:
>>> reload_stats:
>>> driver_reinit 1
>>> fw_activate 0
>>> fw_activate_no_reset 0
>>> remote_reload_stats:
>>> driver_reinit 1
>>> fw_activate 1
>>> 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
>>> } ],
>>> "remote_reload_stats": [ {
>>> "driver_reinit": 0
>>> },{
>>> "fw_activate": 0
>>> },{
>>> "fw_activate_no_reset": 0
>>> } ]
>>> }
>>> },
>>> "pci/0000:82:00.1": {
>>> "stats": {
>>> "reload_stats": [ {
>>> "driver_reinit": 1
>>> },{
>>> "fw_activate": 0
>>> },{
>>> "fw_activate_no_reset": 0
>>> } ],
>>> "remote_reload_stats": [ {
>>> "driver_reinit": 1
>>> },{
>>> "fw_activate": 1
>>> },{
>>> "fw_activate_no_reset": 0
>>> } ]
>>> }
>>> }
>>> }
>>> }
>>>
>>> Signed-off-by: Moshe Shemesh <[email protected]>
>>> ---
>>> RFCv5 -> v1:
>>> - Resplit this patch and the previous one by remote/local reload stats
>>> instead of set/get reload stats
>>> - Rename reload_action_stats to reload_stats
>>> RFCv4 -> RFCv5:
>>> - Add remote actions stats
>>> - If devlink reload is not supported, show only remote_stats
>>> RFCv3 -> RFCv4:
>>> - Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to
>>> DEVLINK_ATTR_RELOAD_ACTION_STAT
>>> - Add stats per action per limit level
>>> RFCv2 -> RFCv3:
>>> - Add reload actions counters instead of supported reload actions
>>> (reload actions counters are only for supported action so no need for
>>> both)
>>> RFCv1 -> RFCv2:
>>> - Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL
>>> - Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO
>>> - Have actions instead of levels
>>> ---
>>> include/net/devlink.h | 1 +
>>> include/uapi/linux/devlink.h | 1 +
>>> net/core/devlink.c | 49 +++++++++++++++++++++++++++++++-----
>>> 3 files changed, 45 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>> index 0f3bd23b6c04..a4ccb83bbd2c 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -42,6 +42,7 @@ struct devlink {
>>> const struct devlink_ops *ops;
>>> struct xarray snapshot_ids;
>>> u32 reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>>> + u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>> Perhaps a nested struct {} stats?
> I guess you mean struct that holds these two arrays.
>>
>>> struct device *dev;
>>> possible_net_t _net;
>>> struct mutex lock; /* Serializes access to devlink instance specific objects such as
>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>> index 97e0137f6201..f9887d8afdc7 100644
>>> --- a/include/uapi/linux/devlink.h
>>> +++ b/include/uapi/linux/devlink.h
>>> @@ -530,6 +530,7 @@ enum devlink_attr {
>>> DEVLINK_ATTR_RELOAD_STATS, /* nested */
>>> DEVLINK_ATTR_RELOAD_STATS_ENTRY, /* nested */
>>> DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */
>>> + DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */
>>>
>>> /* add new attributes above here, update the policy in devlink.c */
>>>
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>> index 05516f1e4c3e..3b6bd3b4d346 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -523,28 +523,35 @@ static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
>>> return -EMSGSIZE;
>>> }
>>>
>>> -static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink)
>>> +static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
>>> {
>>> struct nlattr *reload_stats_attr;
>>> int i, j, stat_idx;
>>> u32 value;
>>>
>>> - reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>>> + if (!is_remote)
>>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>>> + else
>>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_REMOTE_RELOAD_STATS);
>>>
>>> if (!reload_stats_attr)
>>> return -EMSGSIZE;
>>>
>>> for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
>>> - if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>>> + if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>> I don't follow the check "!is_remote" here,
>
>
> We agreed that remote stats should be shown also for non supported
> actions and limits, because its remote. So it makes this condition
> different for remote stats. Rethinking about it, maybe that's wrong. I
> mean if we had here reload actions as a result of remote driver, they
> have common device, so it has to be the same type of driver and support
> same actions/limits, right ?
>
Obviously it runs the same device but.. technically, couldn't the remote
device be running a different version of the driver? i.e. what if it
supports some new mode that this host doesn't yet understand? (or does
understand but has a driver which doesn't yet?)
On 10/5/2020 10:12 PM, Jacob Keller wrote:
>
> On 10/4/2020 12:09 AM, Moshe Shemesh wrote:
>> On 10/3/2020 12:05 PM, Jiri Pirko wrote:
>>> Thu, Oct 01, 2020 at 03:59:08PM CEST, [email protected] wrote:
>>>> Add remote reload stats to hold the history of actions performed due
>>>> devlink reload commands initiated by remote host. For example, in case
>>>> firmware activation with reset finished successfully but was initiated
>>>> by remote host.
>>>>
>>>> The function devlink_remote_reload_actions_performed() is exported to
>>>> enable drivers update on remote reload actions performed as it was not
>>>> initiated by their own devlink instance.
>>>>
>>>> Expose devlink remote 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
>>>> remote_reload_stats:
>>>> driver_reinit 0
>>>> fw_activate 0
>>>> fw_activate_no_reset 0
>>>> pci/0000:82:00.1:
>>>> stats:
>>>> reload_stats:
>>>> driver_reinit 1
>>>> fw_activate 0
>>>> fw_activate_no_reset 0
>>>> remote_reload_stats:
>>>> driver_reinit 1
>>>> fw_activate 1
>>>> 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
>>>> } ],
>>>> "remote_reload_stats": [ {
>>>> "driver_reinit": 0
>>>> },{
>>>> "fw_activate": 0
>>>> },{
>>>> "fw_activate_no_reset": 0
>>>> } ]
>>>> }
>>>> },
>>>> "pci/0000:82:00.1": {
>>>> "stats": {
>>>> "reload_stats": [ {
>>>> "driver_reinit": 1
>>>> },{
>>>> "fw_activate": 0
>>>> },{
>>>> "fw_activate_no_reset": 0
>>>> } ],
>>>> "remote_reload_stats": [ {
>>>> "driver_reinit": 1
>>>> },{
>>>> "fw_activate": 1
>>>> },{
>>>> "fw_activate_no_reset": 0
>>>> } ]
>>>> }
>>>> }
>>>> }
>>>> }
>>>>
>>>> Signed-off-by: Moshe Shemesh <[email protected]>
>>>> ---
>>>> RFCv5 -> v1:
>>>> - Resplit this patch and the previous one by remote/local reload stats
>>>> instead of set/get reload stats
>>>> - Rename reload_action_stats to reload_stats
>>>> RFCv4 -> RFCv5:
>>>> - Add remote actions stats
>>>> - If devlink reload is not supported, show only remote_stats
>>>> RFCv3 -> RFCv4:
>>>> - Renamed DEVLINK_ATTR_RELOAD_ACTION_CNT to
>>>> DEVLINK_ATTR_RELOAD_ACTION_STAT
>>>> - Add stats per action per limit level
>>>> RFCv2 -> RFCv3:
>>>> - Add reload actions counters instead of supported reload actions
>>>> (reload actions counters are only for supported action so no need for
>>>> both)
>>>> RFCv1 -> RFCv2:
>>>> - Removed DEVLINK_ATTR_RELOAD_DEFAULT_LEVEL
>>>> - Removed DEVLINK_ATTR_RELOAD_LEVELS_INFO
>>>> - Have actions instead of levels
>>>> ---
>>>> include/net/devlink.h | 1 +
>>>> include/uapi/linux/devlink.h | 1 +
>>>> net/core/devlink.c | 49 +++++++++++++++++++++++++++++++-----
>>>> 3 files changed, 45 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>> index 0f3bd23b6c04..a4ccb83bbd2c 100644
>>>> --- a/include/net/devlink.h
>>>> +++ b/include/net/devlink.h
>>>> @@ -42,6 +42,7 @@ struct devlink {
>>>> const struct devlink_ops *ops;
>>>> struct xarray snapshot_ids;
>>>> u32 reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>>>> + u32 remote_reload_stats[DEVLINK_RELOAD_STATS_ARRAY_SIZE];
>>> Perhaps a nested struct {} stats?
>> I guess you mean struct that holds these two arrays.
>>>> struct device *dev;
>>>> possible_net_t _net;
>>>> struct mutex lock; /* Serializes access to devlink instance specific objects such as
>>>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>>>> index 97e0137f6201..f9887d8afdc7 100644
>>>> --- a/include/uapi/linux/devlink.h
>>>> +++ b/include/uapi/linux/devlink.h
>>>> @@ -530,6 +530,7 @@ enum devlink_attr {
>>>> DEVLINK_ATTR_RELOAD_STATS, /* nested */
>>>> DEVLINK_ATTR_RELOAD_STATS_ENTRY, /* nested */
>>>> DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */
>>>> + DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */
>>>>
>>>> /* add new attributes above here, update the policy in devlink.c */
>>>>
>>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>>> index 05516f1e4c3e..3b6bd3b4d346 100644
>>>> --- a/net/core/devlink.c
>>>> +++ b/net/core/devlink.c
>>>> @@ -523,28 +523,35 @@ static int devlink_reload_stat_put(struct sk_buff *msg, enum devlink_reload_acti
>>>> return -EMSGSIZE;
>>>> }
>>>>
>>>> -static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink)
>>>> +static int devlink_reload_stats_put(struct sk_buff *msg, struct devlink *devlink, bool is_remote)
>>>> {
>>>> struct nlattr *reload_stats_attr;
>>>> int i, j, stat_idx;
>>>> u32 value;
>>>>
>>>> - reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>>>> + if (!is_remote)
>>>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_RELOAD_STATS);
>>>> + else
>>>> + reload_stats_attr = nla_nest_start(msg, DEVLINK_ATTR_REMOTE_RELOAD_STATS);
>>>>
>>>> if (!reload_stats_attr)
>>>> return -EMSGSIZE;
>>>>
>>>> for (j = 0; j <= DEVLINK_RELOAD_LIMIT_MAX; j++) {
>>>> - if (j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>>>> + if (!is_remote && j != DEVLINK_RELOAD_LIMIT_UNSPEC &&
>>> I don't follow the check "!is_remote" here,
>>
>> We agreed that remote stats should be shown also for non supported
>> actions and limits, because its remote. So it makes this condition
>> different for remote stats. Rethinking about it, maybe that's wrong. I
>> mean if we had here reload actions as a result of remote driver, they
>> have common device, so it has to be the same type of driver and support
>> same actions/limits, right ?
>>
> Obviously it runs the same device but.. technically, couldn't the remote
> device be running a different version of the driver? i.e. what if it
> supports some new mode that this host doesn't yet understand? (or does
> understand but has a driver which doesn't yet?)
Yes, also there is a possibility that one host function has privilege to
do an action that the other doesn't have. I see there are reasons to
keep this diff between remote stats and local. I will keep it. Thanks.