Re: [PATCH net-next 05/16] devlink: Add remote reload stats

[Jacob Keller <jacob.e.keller@intel.com>]

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?)