On 9/15/2020 4:33 PM, Jiri Pirko wrote:
Tue, Sep 15, 2020 at 02:30:19PM CEST, mo...@nvidia.com wrote:
On 9/14/2020 4:39 PM, Jiri Pirko wrote:
Mon, Sep 14, 2020 at 08:07:50AM CEST, mo...@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
