From: Moshe Shemesh <moshe@nvidia.com>
To: Jakub Kicinski <kuba@kernel.org>, Moshe Shemesh <moshe@mellanox.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jiri Pirko <jiri@nvidia.com>, <netdev@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 04/16] devlink: Add reload stats
Date: Fri, 2 Oct 2020 18:07:05 +0300 [thread overview]
Message-ID: <df68d59b-8885-d685-ab53-2c5d8f7b56e4@nvidia.com> (raw)
In-Reply-To: <20201001142521.0c9ac25c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
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);
>> +}
next prev parent reply other threads:[~2020-10-02 15:07 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 [this message]
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
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=df68d59b-8885-d685-ab53-2c5d8f7b56e4@nvidia.com \
--to=moshe@nvidia.com \
--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).