Re: [PATCH net-next RFC v4 03/15] devlink: Add reload action stats

[Moshe Shemesh <moshe@nvidia.com>]

On 9/15/2020 4:33 PM, Jiri Pirko wrote:
> Tue, Sep 15, 2020 at 02:30:19PM CEST, moshe@nvidia.com wrote:
>> On 9/14/2020 4:39 PM, Jiri Pirko wrote:
>>> Mon, Sep 14, 2020 at 08:07:50AM CEST, moshe@mellanox.com wrote:
> [..]
>
>
>>>> +/**
>>>> + *	devlink_reload_implicit_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_level: reload action limit level
>>>> + *	@actions_performed: bitmask of actions performed
>>>> + */
>>>> +void devlink_reload_implicit_actions_performed(struct devlink *devlink,
>>>> +					       enum devlink_reload_action_limit_level limit_level,
>>>> +					       unsigned long actions_performed)
>>> What I'm a bit scarred of that the driver would call this from withing
>>> reload_down()/up() ops. Perheps this could be WARN_ON'ed here (or in
>>> devlink_reload())?
>>>
>> Not sure how I know if it was called from devlink_reload_down()/up() ? Maybe
>> mutex ? So the warn will be actually mutex deadlock ?
> No. Don't abuse mutex for this.
> Just make sure that the counters do not move when you call
> reload_down/up().
>

Can make that, but actually I better take devlink->lock anyway in this 
function to avoid races, WDYT ?

>>>> +{
>>>> +	if (!devlink_reload_supported(devlink))
>>> Hmm. I think that the driver does not have to support the reload and can
>>> still be reloaded by another instance and update the stats here. Why
>>> not?
>>>
>> But I show counters only for supported reload actions and levels, otherwise
>> we will have these counters on devlink dev show output for other drivers that
>> don't have support for devlink reload and didn't implement any of these
>> including this function and these drivers may do some actions like
>> fw_activate in another way and don't update the stats and so that will make
>> these stats misleading. They will show history "stats" but they don't update
>> them as they didn't apply anything related to devlink reload.
> The case I tried to point at is the driver instance, that does not
> implement reload ops itself, but still it can be reloaded by someone else -
> the other driver instance outside.
>
> The counters should work no matter if the driver implements reload ops
> or not. Why wouldn't they? The user still likes to know that the devices
> was reloaded.
>

OK, so you say that every driver should show all counters no matter what 
actions it supports and if it supports devlink reload at all, right ?

>
>>>> +		return;
>>>> +	devlink_reload_action_stats_update(devlink, limit_level, actions_performed);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devlink_reload_implicit_actions_performed);
>>>> +
>>>> static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>>>> 			  enum devlink_reload_action action,
>>>> 			  enum devlink_reload_action_limit_level limit_level,
>>>> -			  struct netlink_ext_ack *extack, unsigned long *actions_performed)
>>>> +			  struct netlink_ext_ack *extack, unsigned long *actions_performed_out)
>>>> {
>>>> +	unsigned long actions_performed;
>>>> 	int err;
>>>>
>>>> 	if (!devlink->reload_enabled)
>>>> @@ -2998,9 +3045,14 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
>>>> 	if (dest_net && !net_eq(dest_net, devlink_net(devlink)))
>>>> 		devlink_reload_netns_change(devlink, dest_net);
>>>>
>>>> -	err = devlink->ops->reload_up(devlink, action, limit_level, extack, actions_performed);
>>>> +	err = devlink->ops->reload_up(devlink, action, limit_level, extack, &actions_performed);
>>>> 	devlink_reload_failed_set(devlink, !!err);
>>>> -	return err;
>>>> +	if (err)
>>>> +		return err;
>>>> +	devlink_reload_action_stats_update(devlink, limit_level, actions_performed);
>>>> +	if (actions_performed_out)
>>> Just make the caller to provide valid pointer, as I suggested in the
>>> other patch review.
>>
>> Ack.
>>
>>>> +		*actions_performed_out = actions_performed;
>>>> +	return 0;
>>>> }
>>>>
>>>> static int
>>>> -- 
>>>> 2.17.1
>>>>