From: Moshe Shemesh <moshe@nvidia.com>
To: Jacob Keller <jacob.e.keller@intel.com>,
Jiri Pirko <jiri@resnulli.us>, 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: Wed, 7 Oct 2020 08:41:28 +0300 [thread overview]
Message-ID: <2cd57697-a1e1-cab8-6a7d-f139b5af1420@nvidia.com> (raw)
In-Reply-To: <f0ae9141-3ed2-f296-b3ae-84408a87b2d9@intel.com>
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, 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?
>> 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.
next prev parent reply other threads:[~2020-10-07 5:41 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
2020-10-04 7:09 ` Moshe Shemesh
2020-10-05 19:12 ` Jacob Keller
2020-10-07 5:41 ` Moshe Shemesh [this message]
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=2cd57697-a1e1-cab8-6a7d-f139b5af1420@nvidia.com \
--to=moshe@nvidia.com \
--cc=davem@davemloft.net \
--cc=jacob.e.keller@intel.com \
--cc=jiri@nvidia.com \
--cc=jiri@resnulli.us \
--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).