Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Sep 2020 15:33:09 +0200
From:   Jiri Pirko <jiri@resnulli.us>
To:     Moshe Shemesh <moshe@nvidia.com>
Cc:     Moshe Shemesh <moshe@mellanox.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jiri Pirko <jiri@mellanox.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next RFC v4 03/15] devlink: Add reload action stats
Message-ID: <20200915133309.GP2236@nanopsycho.orion>
References: <1600063682-17313-1-git-send-email-moshe@mellanox.com>
 <1600063682-17313-4-git-send-email-moshe@mellanox.com>
 <20200914133939.GG2236@nanopsycho.orion>
 <a5b7cbd5-ef55-1d74-a21e-5fb962307773@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a5b7cbd5-ef55-1d74-a21e-5fb962307773@nvidia.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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().


>
>> > +{
>> > +	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.


>
>> > +		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
>> >