From: Jiri Pirko <jiri@resnulli.us>
To: Moshe Shemesh <moshe@mellanox.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@nvidia.com>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 05/16] devlink: Add remote reload stats
Date: Sat, 3 Oct 2020 11:05:42 +0200 [thread overview]
Message-ID: <20201003090542.GF3159@nanopsycho.orion> (raw)
In-Reply-To: <1601560759-11030-6-git-send-email-moshe@mellanox.com>
Thu, Oct 01, 2020 at 03:59:08PM CEST, moshe@mellanox.com 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 <moshe@mellanox.com>
>---
>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
>
next prev parent reply other threads:[~2020-10-03 9:05 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-01 13:59 [PATCH net-next 00/16] Add devlink reload action and limit options Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 01/16] devlink: Change devlink_reload_supported() param type Moshe Shemesh
2020-10-01 20:54 ` Jakub Kicinski
2020-10-02 15:13 ` Jiri Pirko
2020-10-05 18:34 ` Jacob Keller
2020-10-01 13:59 ` [PATCH net-next 02/16] devlink: Add reload action option to devlink reload command Moshe Shemesh
2020-10-01 20:59 ` Jakub Kicinski
2020-10-02 15:19 ` Jiri Pirko
2020-10-04 6:30 ` Moshe Shemesh
2020-10-03 7:52 ` Jiri Pirko
2020-10-04 6:45 ` Moshe Shemesh
2020-10-05 18:39 ` Jacob Keller
2020-10-01 13:59 ` [PATCH net-next 03/16] devlink: Add devlink reload limit option Moshe Shemesh
2020-10-01 21:14 ` Jakub Kicinski
2020-10-02 15:01 ` Moshe Shemesh
2020-10-01 21:15 ` Jakub Kicinski
2020-10-03 7:51 ` Jiri Pirko
2020-10-03 15:04 ` Jakub Kicinski
2020-10-04 7:18 ` Moshe Shemesh
2020-10-05 18:53 ` Jacob Keller
2020-10-04 6:42 ` Moshe Shemesh
2020-10-05 12:18 ` Jiri Pirko
2020-10-05 18:45 ` Jacob Keller
2020-10-01 13:59 ` [PATCH net-next 04/16] devlink: Add reload stats Moshe Shemesh
2020-10-01 21:25 ` Jakub Kicinski
2020-10-02 15:07 ` Moshe Shemesh
2020-10-03 9:00 ` Jiri Pirko
2020-10-04 6:59 ` Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 05/16] devlink: Add remote " Moshe Shemesh
2020-10-01 21:48 ` Jakub Kicinski
2020-10-03 9:05 ` Jiri Pirko [this message]
2020-10-04 7:09 ` Moshe Shemesh
2020-10-05 19:12 ` Jacob Keller
2020-10-07 5:41 ` Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 06/16] net/mlx5: Add functions to set/query MFRL register Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 07/16] net/mlx5: Set cap for pci sync for fw update event Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 08/16] net/mlx5: Handle sync reset request event Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 09/16] net/mlx5: Handle sync reset now event Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 10/16] net/mlx5: Handle sync reset abort event Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 11/16] net/mlx5: Add support for devlink reload action fw activate Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 12/16] devlink: Add enable_remote_dev_reset generic parameter Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 13/16] net/mlx5: Add devlink param enable_remote_dev_reset support Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 14/16] net/mlx5: Add support for fw live patch event Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 15/16] net/mlx5: Add support for devlink reload limit no reset Moshe Shemesh
2020-10-01 21:52 ` Jakub Kicinski
2020-10-02 15:08 ` Moshe Shemesh
2020-10-01 13:59 ` [PATCH net-next 16/16] devlink: Add Documentation/networking/devlink/devlink-reload.rst Moshe Shemesh
2020-10-03 9:14 ` Jiri Pirko
2020-10-04 7:15 ` Moshe Shemesh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201003090542.GF3159@nanopsycho.orion \
--to=jiri@resnulli.us \
--cc=davem@davemloft.net \
--cc=jiri@nvidia.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=moshe@mellanox.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).