[RFC net-next 00/10] devlink: remove the wait-for-references on unregister

[RFC net-next 00/10] devlink: remove the wait-for-references on unregister

[Jakub Kicinski]

This set is on top of the previous RFC.

Move the registration and unregistration of the devlink instances
under their instance locks. Don't perform the netdev-style wait
for all references when unregistering the instance.

Jakub Kicinski (10):
  devlink: bump the instance index directly when iterating
  devlink: update the code in netns move to latest helpers
  devlink: protect devlink->dev by the instance lock
  devlink: always check if the devlink instance is registered
  devlink: remove the registration guarantee of references
  devlink: don't require setting features before registration
  netdevsim: rename a label
  netdevsim: move devlink registration under the instance lock
  devlink: allow registering parameters after the instance
  netdevsim: register devlink instance before sub-objects

 drivers/net/netdevsim/dev.c |  15 +++--
 include/net/devlink.h       |   3 +
 net/devlink/basic.c         |  64 ++++++++++++------
 net/devlink/core.c          | 127 +++++++++++++++++-------------------
 net/devlink/devl_internal.h |  20 ++----
 net/devlink/netlink.c       |  19 ++++--
 6 files changed, 136 insertions(+), 112 deletions(-)

-- 
2.38.1

[RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jakub Kicinski]

We use a clever find_first() / find_after() scheme currently,
which works nicely as xarray will write the "current" index
into the variable we pass.

We can't do the same thing during the "dump walk" because
there we must not increment the index until we're sure
that the instance has been fully dumped.

Since we have a precedent and a requirement for manually futzing
with the increment of the index, let's switch
devlinks_xa_for_each_registered_get() to do the same thing.
It removes some indirections.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/devlink/core.c          | 31 +++++++++----------------------
 net/devlink/devl_internal.h | 17 ++++-------------
 2 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/net/devlink/core.c b/net/devlink/core.c
index 371d6821315d..88c88b8053e2 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -91,16 +91,13 @@ void devlink_put(struct devlink *devlink)
 		call_rcu(&devlink->rcu, __devlink_put_rcu);
 }
 
-struct devlink *
-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
-					  unsigned long, xa_mark_t))
+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
 {
-	struct devlink *devlink;
+	struct devlink *devlink = NULL;
 
 	rcu_read_lock();
 retry:
-	devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
+	devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
 	if (!devlink)
 		goto unlock;
 
@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,
 	 * This prevents live-lock of devlink_unregister() wait for completion.
 	 */
 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
-		goto retry;
+		goto next;
 
-	/* For a possible retry, the xa_find_after() should be always used */
-	xa_find_fn = xa_find_after;
 	if (!devlink_try_get(devlink))
-		goto retry;
+		goto next;
 	if (!net_eq(devlink_net(devlink), net)) {
 		devlink_put(devlink);
-		goto retry;
+		goto next;
 	}
 unlock:
 	rcu_read_unlock();
 	return devlink;
-}
-
-struct devlink *
-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp)
-{
-	return devlinks_xa_find_get(net, indexp, xa_find);
-}
 
-struct devlink *
-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp)
-{
-	return devlinks_xa_find_get(net, indexp, xa_find_after);
+next:
+	(*indexp)++;
+	goto retry;
 }
 
 /**
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index 1d7ab11f2f7e..ef0369449592 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
  * in loop body in order to release the reference.
  */
 #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
-	for (index = 0,							\
-	     devlink = devlinks_xa_find_get_first(net, &index);	\
-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
-
-struct devlink *
-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
-					  unsigned long, xa_mark_t));
-struct devlink *
-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
-struct devlink *
-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)
+
+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp);
 
 /* Netlink */
 #define DEVLINK_NL_FLAG_NEED_PORT		BIT(0)
@@ -133,7 +124,7 @@ struct devlink_gen_cmd {
  */
 #define devlink_dump_for_each_instance_get(msg, dump, devlink)		\
 	for (; (devlink = devlinks_xa_find_get(sock_net(msg->sk),	\
-					       &dump->instance, xa_find)); \
+					       &dump->instance));	\
 	     dump->instance++, dump->idx = 0)
 
 extern const struct genl_small_ops devlink_nl_ops[56];
-- 
2.38.1

[RFC net-next 02/10] devlink: update the code in netns move to latest helpers

[Jakub Kicinski]

devlink_pernet_pre_exit() is the only obvious place which takes
the instance lock without using the devl_ helpers. Update the code
and move the error print after releasing the reference
(having unlock and put together feels slightly idiomatic).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/devlink/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/devlink/core.c b/net/devlink/core.c
index 88c88b8053e2..d3b8336946fd 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -299,15 +299,16 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 	 */
 	devlinks_xa_for_each_registered_get(net, index, devlink) {
 		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
-		mutex_lock(&devlink->lock);
+		devl_lock(devlink);
 		err = devlink_reload(devlink, &init_net,
 				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
 				     DEVLINK_RELOAD_LIMIT_UNSPEC,
 				     &actions_performed, NULL);
-		mutex_unlock(&devlink->lock);
+		devl_unlock(devlink);
+		devlink_put(devlink);
+
 		if (err && err != -EOPNOTSUPP)
 			pr_warn("Failed to reload devlink instance into init_net\n");
-		devlink_put(devlink);
 	}
 }
 
-- 
2.38.1

[RFC net-next 03/10] devlink: protect devlink->dev by the instance lock

[Jakub Kicinski]

devlink->dev is assumed to be always valid as long as any
outstanding reference to the devlink instance exists.

In prep for weakening of the references take the instance lock.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/devlink/basic.c         | 7 +++----
 net/devlink/devl_internal.h | 3 ++-
 net/devlink/netlink.c       | 9 ++++++---
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/devlink/basic.c b/net/devlink/basic.c
index ceac4343698b..5f33d74eef83 100644
--- a/net/devlink/basic.c
+++ b/net/devlink/basic.c
@@ -6314,12 +6314,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,
 
 	start_offset = dump->start_offset;
 
-	devlink = devlink_get_from_attrs(sock_net(cb->skb->sk), attrs);
+	devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
 	if (IS_ERR(devlink))
 		return PTR_ERR(devlink);
 
-	devl_lock(devlink);
-
 	if (!attrs[DEVLINK_ATTR_REGION_NAME]) {
 		NL_SET_ERR_MSG(cb->extack, "No region name provided");
 		err = -EINVAL;
@@ -7735,9 +7733,10 @@ devlink_health_reporter_get_from_cb(struct netlink_callback *cb)
 	struct nlattr **attrs = info->attrs;
 	struct devlink *devlink;
 
-	devlink = devlink_get_from_attrs(sock_net(cb->skb->sk), attrs);
+	devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
 	if (IS_ERR(devlink))
 		return NULL;
+	devl_unlock(devlink);
 
 	reporter = devlink_health_reporter_get_from_attrs(devlink, attrs);
 	devlink_put(devlink);
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index ef0369449592..c3977c69552a 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -129,7 +129,8 @@ struct devlink_gen_cmd {
 
 extern const struct genl_small_ops devlink_nl_ops[56];
 
-struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs);
+struct devlink *
+devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs);
 
 void devlink_notify_unregister(struct devlink *devlink);
 void devlink_notify_register(struct devlink *devlink);
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index 7eb39ccb2ae7..b38df704be1c 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -82,7 +82,8 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
 	[DEVLINK_ATTR_REGION_DIRECT] = { .type = NLA_FLAG },
 };
 
-struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs)
+struct devlink *
+devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
 {
 	struct devlink *devlink;
 	unsigned long index;
@@ -96,9 +97,11 @@ struct devlink *devlink_get_from_attrs(struct net *net, struct nlattr **attrs)
 	devname = nla_data(attrs[DEVLINK_ATTR_DEV_NAME]);
 
 	devlinks_xa_for_each_registered_get(net, index, devlink) {
+		devl_lock(devlink);
 		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
 		    strcmp(dev_name(devlink->dev), devname) == 0)
 			return devlink;
+		devl_unlock(devlink);
 		devlink_put(devlink);
 	}
 
@@ -113,10 +116,10 @@ static int devlink_nl_pre_doit(const struct genl_split_ops *ops,
 	struct devlink *devlink;
 	int err;
 
-	devlink = devlink_get_from_attrs(genl_info_net(info), info->attrs);
+	devlink = devlink_get_from_attrs_lock(genl_info_net(info), info->attrs);
 	if (IS_ERR(devlink))
 		return PTR_ERR(devlink);
-	devl_lock(devlink);
+
 	info->user_ptr[0] = devlink;
 	if (ops->internal_flags & DEVLINK_NL_FLAG_NEED_PORT) {
 		devlink_port = devlink_port_get_from_info(devlink, info);
-- 
2.38.1

[RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

Always check under the instance lock whether the devlink instance
is still / already registered.

This is a no-op for the most part, as the unregistration path currently
waits for all references. On the init path, however, we may temporarily
open up a race with netdev code, if netdevs are registered before the
devlink instance. This is temporary, the next change fixes it, and this
commit has been split out for the ease of review.

Note that in case of iterating over sub-objects which have their
own lock (regions and line cards) we assume an implicit dependency
between those objects existing and devlink unregistration.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/devlink.h |  1 +
 net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
 net/devlink/core.c    | 25 +++++++++++++++++++++----
 net/devlink/netlink.c | 10 ++++++++--
 4 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6a2e4f21779f..36e013d3aa52 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
 void devl_lock(struct devlink *devlink);
 int devl_trylock(struct devlink *devlink);
 void devl_unlock(struct devlink *devlink);
+bool devl_is_alive(struct devlink *devlink);
 void devl_assert_locked(struct devlink *devlink);
 bool devl_lock_is_held(struct devlink *devlink);
 
diff --git a/net/devlink/basic.c b/net/devlink/basic.c
index 5f33d74eef83..6b18e70a39fd 100644
--- a/net/devlink/basic.c
+++ b/net/devlink/basic.c
@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
 		int idx = 0;
 
 		mutex_lock(&devlink->linecards_lock);
+		if (!devl_is_alive(devlink))
+			goto next_devlink;
+
 		list_for_each_entry(linecard, &devlink->linecard_list, list) {
 			if (idx < dump->idx) {
 				idx++;
@@ -2151,6 +2154,7 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
 			}
 			idx++;
 		}
+next_devlink:
 		mutex_unlock(&devlink->linecards_lock);
 		devlink_put(devlink);
 	}
@@ -7809,6 +7813,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 		int idx = 0;
 
 		mutex_lock(&devlink->reporters_lock);
+		if (!devl_is_alive(devlink)) {
+			mutex_unlock(&devlink->reporters_lock);
+			devlink_put(devlink);
+			continue;
+		}
+
 		list_for_each_entry(reporter, &devlink->reporter_list,
 				    list) {
 			if (idx < dump->idx) {
@@ -7830,6 +7840,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 		mutex_unlock(&devlink->reporters_lock);
 
 		devl_lock(devlink);
+		if (!devl_is_alive(devlink))
+			goto next_devlink;
+
 		xa_for_each(&devlink->ports, port_index, port) {
 			mutex_lock(&port->reporters_lock);
 			list_for_each_entry(reporter, &port->reporter_list, list) {
@@ -7853,6 +7866,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
 			}
 			mutex_unlock(&port->reporters_lock);
 		}
+next_devlink:
 		devl_unlock(devlink);
 		devlink_put(devlink);
 	}
@@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
 		return;
 
 	devl_lock(devlink);
-	__devlink_compat_running_version(devlink, buf, len);
+	if (devl_is_alive(devlink))
+		__devlink_compat_running_version(devlink, buf, len);
 	devl_unlock(devlink);
 }
 
@@ -12227,20 +12242,28 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
 	struct devlink_flash_update_params params = {};
 	int ret;
 
-	if (!devlink->ops->flash_update)
-		return -EOPNOTSUPP;
+	devl_lock(devlink);
+	if (!devl_is_alive(devlink)) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (!devlink->ops->flash_update) {
+		ret = -EOPNOTSUPP;
+		goto out_unlock;
+	}
 
 	ret = request_firmware(&params.fw, file_name, devlink->dev);
 	if (ret)
-		return ret;
+		goto out_unlock;
 
-	devl_lock(devlink);
 	devlink_flash_update_begin_notify(devlink);
 	ret = devlink->ops->flash_update(devlink, &params, NULL);
 	devlink_flash_update_end_notify(devlink);
-	devl_unlock(devlink);
 
 	release_firmware(params.fw);
+out_unlock:
+	devl_unlock(devlink);
 
 	return ret;
 }
diff --git a/net/devlink/core.c b/net/devlink/core.c
index d3b8336946fd..2abad8247597 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -67,6 +67,21 @@ void devl_unlock(struct devlink *devlink)
 }
 EXPORT_SYMBOL_GPL(devl_unlock);
 
+bool devl_is_alive(struct devlink *devlink)
+{
+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+}
+EXPORT_SYMBOL_GPL(devl_is_alive);
+
+/**
+ * devlink_try_get() - try to obtain a reference on a devlink instance
+ * @devlink: instance to reference
+ *
+ * Obtain a reference on a devlink instance. A reference on a devlink instance
+ * only implies that it's safe to take the instance lock. It does not imply
+ * that the instance is registered, use devl_is_alive() after taking
+ * the instance lock to check registration status.
+ */
 struct devlink *__must_check devlink_try_get(struct devlink *devlink)
 {
 	if (refcount_inc_not_zero(&devlink->refcount))
@@ -300,10 +315,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 	devlinks_xa_for_each_registered_get(net, index, devlink) {
 		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
 		devl_lock(devlink);
-		err = devlink_reload(devlink, &init_net,
-				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
-				     DEVLINK_RELOAD_LIMIT_UNSPEC,
-				     &actions_performed, NULL);
+		err = 0;
+		if (devl_is_alive(devlink))
+			err = devlink_reload(devlink, &init_net,
+					     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
+					     DEVLINK_RELOAD_LIMIT_UNSPEC,
+					     &actions_performed, NULL);
 		devl_unlock(devlink);
 		devlink_put(devlink);
 
diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
index b38df704be1c..773efaabb6ad 100644
--- a/net/devlink/netlink.c
+++ b/net/devlink/netlink.c
@@ -98,7 +98,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
 
 	devlinks_xa_for_each_registered_get(net, index, devlink) {
 		devl_lock(devlink);
-		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
+		if (devl_is_alive(devlink) &&
+		    strcmp(devlink->dev->bus->name, busname) == 0 &&
 		    strcmp(dev_name(devlink->dev), devname) == 0)
 			return devlink;
 		devl_unlock(devlink);
@@ -210,7 +211,12 @@ int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
 
 	devlink_dump_for_each_instance_get(msg, dump, devlink) {
 		devl_lock(devlink);
-		err = cmd->dump_one(msg, devlink, cb);
+
+		if (devl_is_alive(devlink))
+			err = cmd->dump_one(msg, devlink, cb);
+		else
+			err = 0;
+
 		devl_unlock(devlink);
 		devlink_put(devlink);
 
-- 
2.38.1

[RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jakub Kicinski]

The objective of exposing the devlink instance locks to
drivers was to let them use these locks to prevent user space
from accessing the device before it's fully initialized.
This is difficult because devlink_unregister() waits for all
references to be released, meaning that devlink_unregister()
can't itself be called under the instance lock.

To avoid this issue devlink_register() was moved after subobject
registration a while ago. Unfortunately the netdev paths get
a hold of the devlink instances _before_ they are registered.
Ideally netdev should wait for devlink init to finish (synchronizing
on the instance lock). This can't work because we don't know if the
instance will _ever_ be registered (in case of failures it may not).
The other option of returning an error until devlink_register()
is called is unappealing (user space would get a notification
netdev exist but would have to wait arbitrary amount of time
before accessing some of its attributes).

Weaken the guarantees of the devlink references.

Holding a reference will now only guarantee that the memory
of the object is around. Another way of looking at it is that
the reference now protects the object not its "registered" status.
Use devlink instance lock to synchronize unregistration.

This implies that releasing of the "main" reference of the devlink
instance moves from devlink_unregister() to devlink_free().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/devlink.h       |  2 ++
 net/devlink/core.c          | 64 ++++++++++++++++---------------------
 net/devlink/devl_internal.h |  2 --
 3 files changed, 30 insertions(+), 38 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 36e013d3aa52..cc910612b3f4 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1648,6 +1648,8 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
 	return devlink_alloc_ns(ops, priv_size, &init_net, dev);
 }
 void devlink_set_features(struct devlink *devlink, u64 features);
+int devl_register(struct devlink *devlink);
+void devl_unregister(struct devlink *devlink);
 void devlink_register(struct devlink *devlink);
 void devlink_unregister(struct devlink *devlink);
 void devlink_free(struct devlink *devlink);
diff --git a/net/devlink/core.c b/net/devlink/core.c
index 2abad8247597..413b92534ad6 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -89,21 +89,10 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
 	return NULL;
 }
 
-static void __devlink_put_rcu(struct rcu_head *head)
-{
-	struct devlink *devlink = container_of(head, struct devlink, rcu);
-
-	complete(&devlink->comp);
-}
-
 void devlink_put(struct devlink *devlink)
 {
 	if (refcount_dec_and_test(&devlink->refcount))
-		/* Make sure unregister operation that may await the completion
-		 * is unblocked only after all users are after the end of
-		 * RCU grace period.
-		 */
-		call_rcu(&devlink->rcu, __devlink_put_rcu);
+		kfree_rcu(devlink, rcu);
 }
 
 struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
@@ -116,13 +105,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
 	if (!devlink)
 		goto unlock;
 
-	/* In case devlink_unregister() was already called and "unregistering"
-	 * mark was set, do not allow to get a devlink reference here.
-	 * This prevents live-lock of devlink_unregister() wait for completion.
-	 */
-	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
-		goto next;
-
 	if (!devlink_try_get(devlink))
 		goto next;
 	if (!net_eq(devlink_net(devlink), net)) {
@@ -158,37 +140,48 @@ void devlink_set_features(struct devlink *devlink, u64 features)
 EXPORT_SYMBOL_GPL(devlink_set_features);
 
 /**
- *	devlink_register - Register devlink instance
- *
- *	@devlink: devlink
+ * devl_register - Register devlink instance
+ * @devlink: devlink
  */
-void devlink_register(struct devlink *devlink)
+int devl_register(struct devlink *devlink)
 {
 	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-	/* Make sure that we are in .probe() routine */
+	devl_assert_locked(devlink);
 
 	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
 	devlink_notify_register(devlink);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devl_register);
+
+void devlink_register(struct devlink *devlink)
+{
+	devl_lock(devlink);
+	devl_register(devlink);
+	devl_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_register);
 
 /**
- *	devlink_unregister - Unregister devlink instance
- *
- *	@devlink: devlink
+ * devl_unregister - Unregister devlink instance
+ * @devlink: devlink
  */
-void devlink_unregister(struct devlink *devlink)
+void devl_unregister(struct devlink *devlink)
 {
 	ASSERT_DEVLINK_REGISTERED(devlink);
-	/* Make sure that we are in .remove() routine */
-
-	xa_set_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
-	devlink_put(devlink);
-	wait_for_completion(&devlink->comp);
+	devl_assert_locked(devlink);
 
 	devlink_notify_unregister(devlink);
 	xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
-	xa_clear_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
+}
+EXPORT_SYMBOL_GPL(devl_unregister);
+
+void devlink_unregister(struct devlink *devlink)
+{
+	devl_lock(devlink);
+	devl_unregister(devlink);
+	devl_unlock(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_unregister);
 
@@ -252,7 +245,6 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
 	mutex_init(&devlink->reporters_lock);
 	mutex_init(&devlink->linecards_lock);
 	refcount_set(&devlink->refcount, 1);
-	init_completion(&devlink->comp);
 
 	return devlink;
 
@@ -298,7 +290,7 @@ void devlink_free(struct devlink *devlink)
 
 	xa_erase(&devlinks, devlink->index);
 
-	kfree(devlink);
+	devlink_put(devlink);
 }
 EXPORT_SYMBOL_GPL(devlink_free);
 
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index c3977c69552a..7e77eebde3b9 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -12,7 +12,6 @@
 #include <net/net_namespace.h>
 
 #define DEVLINK_REGISTERED XA_MARK_1
-#define DEVLINK_UNREGISTERING XA_MARK_2
 
 #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
 	(__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
@@ -52,7 +51,6 @@ struct devlink {
 	struct lock_class_key lock_key;
 	u8 reload_failed:1;
 	refcount_t refcount;
-	struct completion comp;
 	struct rcu_head rcu;
 	struct notifier_block netdevice_nb;
 	char priv[] __aligned(NETDEV_ALIGN);
-- 
2.38.1

[RFC net-next 06/10] devlink: don't require setting features before registration

[Jakub Kicinski]

Requiring devlink_set_features() to be run before devlink is
registered is overzealous. devlink_set_features() itself is
a leftover from old workarounds which were trying to prevent
initiating reload before probe was complete.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/devlink/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/devlink/core.c b/net/devlink/core.c
index 413b92534ad6..f30fc167c8ad 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -131,8 +131,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
  */
 void devlink_set_features(struct devlink *devlink, u64 features)
 {
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	WARN_ON(features & DEVLINK_F_RELOAD &&
 		!devlink_reload_supported(devlink->ops));
 	devlink->features = features;
-- 
2.38.1

[RFC net-next 07/10] netdevsim: rename a label

[Jakub Kicinski]

err_dl_unregister should unregister the devlink instance.
Looks like renaming it was missed in one of the reshufflings.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index b962fc8e1397..d25f6e86d901 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1563,7 +1563,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	err = devlink_params_register(devlink, nsim_devlink_params,
 				      ARRAY_SIZE(nsim_devlink_params));
 	if (err)
-		goto err_dl_unregister;
+		goto err_resource_unregister;
 	nsim_devlink_set_params_init_values(nsim_dev, devlink);
 
 	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
@@ -1629,7 +1629,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 err_params_unregister:
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
-err_dl_unregister:
+err_resource_unregister:
 	devl_resources_unregister(devlink);
 err_vfc_free:
 	kfree(nsim_dev->vfconfigs);
-- 
2.38.1

[RFC net-next 08/10] netdevsim: move devlink registration under the instance lock

[Jakub Kicinski]

To prevent races with netdev code accessing free devlink instances
move the registration under the devlink instance lock.
Core now waits for the instance to be registered before accessing it.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/dev.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index d25f6e86d901..c9952a34c39a 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1566,10 +1566,14 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 		goto err_resource_unregister;
 	nsim_devlink_set_params_init_values(nsim_dev, devlink);
 
-	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
+	err = devl_register(devlink);
 	if (err)
 		goto err_params_unregister;
 
+	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
+	if (err)
+		goto err_dl_unregister;
+
 	err = nsim_dev_traps_init(devlink);
 	if (err)
 		goto err_dummy_region_exit;
@@ -1607,7 +1611,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
 	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
-	devlink_register(devlink);
 	return 0;
 
 err_hwstats_exit:
@@ -1626,6 +1629,8 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev_traps_exit(devlink);
 err_dummy_region_exit:
 	nsim_dev_dummy_region_exit(nsim_dev);
+err_dl_unregister:
+	devl_unregister(devlink);
 err_params_unregister:
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
@@ -1668,12 +1673,12 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
 	struct devlink *devlink = priv_to_devlink(nsim_dev);
 
-	devlink_unregister(devlink);
 	devl_lock(devlink);
 	nsim_dev_reload_destroy(nsim_dev);
 
 	nsim_bpf_dev_exit(nsim_dev);
 	nsim_dev_debugfs_exit(nsim_dev);
+	devl_unregister(devlink);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 	devl_resources_unregister(devlink);
-- 
2.38.1

[RFC net-next 09/10] devlink: allow registering parameters after the instance

[Jakub Kicinski]

It's most natural to register the instance first and then its
subobjects. Now that we can use the instance lock to protect
the atomicity of all init - it should also be safe.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/devlink/basic.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/devlink/basic.c b/net/devlink/basic.c
index 6b18e70a39fd..10b90a49aba0 100644
--- a/net/devlink/basic.c
+++ b/net/devlink/basic.c
@@ -5263,7 +5263,13 @@ static void devlink_param_notify(struct devlink *devlink,
 	WARN_ON(cmd != DEVLINK_CMD_PARAM_NEW && cmd != DEVLINK_CMD_PARAM_DEL &&
 		cmd != DEVLINK_CMD_PORT_PARAM_NEW &&
 		cmd != DEVLINK_CMD_PORT_PARAM_DEL);
-	ASSERT_DEVLINK_REGISTERED(devlink);
+
+	/* devlink_notify_register() / devlink_notify_unregister()
+	 * will replay the notifications if the params are added/removed
+	 * outside of the lifetime of the instance.
+	 */
+	if (!devl_is_alive(devlink))
+		return;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
@@ -10915,8 +10921,6 @@ int devlink_params_register(struct devlink *devlink,
 	const struct devlink_param *param = params;
 	int i, err;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	for (i = 0; i < params_count; i++, param++) {
 		err = devlink_param_register(devlink, param);
 		if (err)
@@ -10947,8 +10951,6 @@ void devlink_params_unregister(struct devlink *devlink,
 	const struct devlink_param *param = params;
 	int i;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	for (i = 0; i < params_count; i++, param++)
 		devlink_param_unregister(devlink, param);
 }
@@ -10968,8 +10970,6 @@ int devlink_param_register(struct devlink *devlink,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	WARN_ON(devlink_param_verify(param));
 	WARN_ON(devlink_param_find_by_name(&devlink->param_list, param->name));
 
@@ -10985,6 +10985,7 @@ int devlink_param_register(struct devlink *devlink,
 	param_item->param = param;
 
 	list_add_tail(&param_item->list, &devlink->param_list);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_param_register);
@@ -10999,11 +11000,10 @@ void devlink_param_unregister(struct devlink *devlink,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	param_item =
 		devlink_param_find_by_name(&devlink->param_list, param->name);
 	WARN_ON(!param_item);
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_DEL);
 	list_del(&param_item->list);
 	kfree(param_item);
 }
@@ -11063,8 +11063,6 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 {
 	struct devlink_param_item *param_item;
 
-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
-
 	param_item = devlink_param_find_by_id(&devlink->param_list, param_id);
 	if (!param_item)
 		return -EINVAL;
@@ -11078,6 +11076,8 @@ int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 	else
 		param_item->driverinit_value = init_val;
 	param_item->driverinit_value_valid = true;
+
+	devlink_param_notify(devlink, 0, param_item, DEVLINK_CMD_PARAM_NEW);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_param_driverinit_value_set);
-- 
2.38.1

[RFC net-next 10/10] netdevsim: register devlink instance before sub-objects

[Jakub Kicinski]

Move the devlink instance registration up so that all the sub-object
manipulation happens on a valid instance.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/dev.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index c9952a34c39a..738784fda117 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1556,23 +1556,23 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 		goto err_devlink_unlock;
 	}
 
-	err = nsim_dev_resources_register(devlink);
+	err = devl_register(devlink);
 	if (err)
 		goto err_vfc_free;
 
+	err = nsim_dev_resources_register(devlink);
+	if (err)
+		goto err_dl_unregister;
+
 	err = devlink_params_register(devlink, nsim_devlink_params,
 				      ARRAY_SIZE(nsim_devlink_params));
 	if (err)
 		goto err_resource_unregister;
 	nsim_devlink_set_params_init_values(nsim_dev, devlink);
 
-	err = devl_register(devlink);
-	if (err)
-		goto err_params_unregister;
-
 	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
 	if (err)
-		goto err_dl_unregister;
+		goto err_params_unregister;
 
 	err = nsim_dev_traps_init(devlink);
 	if (err)
@@ -1629,13 +1629,13 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev_traps_exit(devlink);
 err_dummy_region_exit:
 	nsim_dev_dummy_region_exit(nsim_dev);
-err_dl_unregister:
-	devl_unregister(devlink);
 err_params_unregister:
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 err_resource_unregister:
 	devl_resources_unregister(devlink);
+err_dl_unregister:
+	devl_unregister(devlink);
 err_vfc_free:
 	kfree(nsim_dev->vfconfigs);
 err_devlink_unlock:
@@ -1678,10 +1678,10 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 
 	nsim_bpf_dev_exit(nsim_dev);
 	nsim_dev_debugfs_exit(nsim_dev);
-	devl_unregister(devlink);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 	devl_resources_unregister(devlink);
+	devl_unregister(devlink);
 	kfree(nsim_dev->vfconfigs);
 	kfree(nsim_dev->fa_cookie);
 	devl_unlock(devlink);
-- 
2.38.1

Re: [RFC net-next 00/10] devlink: remove the wait-for-references on unregister

[Jacob Keller]

On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> This set is on top of the previous RFC.
> 
> Move the registration and unregistration of the devlink instances
> under their instance locks. Don't perform the netdev-style wait
> for all references when unregistering the instance.
> 

Could you explain the reasoning/benefits here? I'm sure some of this is
explained in each commit message as well but it would help to understand
the overall series.

> Jakub Kicinski (10):
>   devlink: bump the instance index directly when iterating
>   devlink: update the code in netns move to latest helpers
>   devlink: protect devlink->dev by the instance lock
>   devlink: always check if the devlink instance is registered
>   devlink: remove the registration guarantee of references
>   devlink: don't require setting features before registration
>   netdevsim: rename a label
>   netdevsim: move devlink registration under the instance lock
>   devlink: allow registering parameters after the instance
>   netdevsim: register devlink instance before sub-objects
> 
>  drivers/net/netdevsim/dev.c |  15 +++--
>  include/net/devlink.h       |   3 +
>  net/devlink/basic.c         |  64 ++++++++++++------
>  net/devlink/core.c          | 127 +++++++++++++++++-------------------
>  net/devlink/devl_internal.h |  20 ++----
>  net/devlink/netlink.c       |  19 ++++--
>  6 files changed, 136 insertions(+), 112 deletions(-)
> 

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jacob Keller]

On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> Always check under the instance lock whether the devlink instance
> is still / already registered.
> 

Ok. So now the reference ensures less about whats valid. It guarantees a
lock but doesn't ensure that the devlink remains registered unless you
acquire the lock and check that the devlink is alive under lock now?

> This is a no-op for the most part, as the unregistration path currently
> waits for all references. On the init path, however, we may temporarily
> open up a race with netdev code, if netdevs are registered before the
> devlink instance. This is temporary, the next change fixes it, and this
> commit has been split out for the ease of review.
> 

This means you're adding the problem here, but its fixed in next commit..?

> Note that in case of iterating over sub-objects which have their
> own lock (regions and line cards) we assume an implicit dependency
> between those objects existing and devlink unregistration.
> 

That seems reasonable.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/net/devlink.h |  1 +
>  net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
>  net/devlink/core.c    | 25 +++++++++++++++++++++----
>  net/devlink/netlink.c | 10 ++++++++--
>  4 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 6a2e4f21779f..36e013d3aa52 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
>  void devl_lock(struct devlink *devlink);
>  int devl_trylock(struct devlink *devlink);
>  void devl_unlock(struct devlink *devlink);
> +bool devl_is_alive(struct devlink *devlink);
>  void devl_assert_locked(struct devlink *devlink);
>  bool devl_lock_is_held(struct devlink *devlink);
>  
> diff --git a/net/devlink/basic.c b/net/devlink/basic.c
> index 5f33d74eef83..6b18e70a39fd 100644
> --- a/net/devlink/basic.c
> +++ b/net/devlink/basic.c
> @@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
>  		int idx = 0;
>  
>  		mutex_lock(&devlink->linecards_lock);
> +		if (!devl_is_alive(devlink))
> +			goto next_devlink;
> +
>  		list_for_each_entry(linecard, &devlink->linecard_list, list) {
>  			if (idx < dump->idx) {
>  				idx++;
> @@ -2151,6 +2154,7 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
>  			}
>  			idx++;
>  		}
> +next_devlink:
>  		mutex_unlock(&devlink->linecards_lock);
>  		devlink_put(devlink);
>  	}
> @@ -7809,6 +7813,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
>  		int idx = 0;
>  
>  		mutex_lock(&devlink->reporters_lock);
> +		if (!devl_is_alive(devlink)) {
> +			mutex_unlock(&devlink->reporters_lock);
> +			devlink_put(devlink);
> +			continue;
> +		}
> +
>  		list_for_each_entry(reporter, &devlink->reporter_list,
>  				    list) {
>  			if (idx < dump->idx) {
> @@ -7830,6 +7840,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
>  		mutex_unlock(&devlink->reporters_lock);
>  
>  		devl_lock(devlink);
> +		if (!devl_is_alive(devlink))
> +			goto next_devlink;
> +
>  		xa_for_each(&devlink->ports, port_index, port) {
>  			mutex_lock(&port->reporters_lock);
>  			list_for_each_entry(reporter, &port->reporter_list, list) {
> @@ -7853,6 +7866,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
>  			}
>  			mutex_unlock(&port->reporters_lock);
>  		}
> +next_devlink:
>  		devl_unlock(devlink);
>  		devlink_put(devlink);
>  	}
> @@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
>  		return;
>  
>  	devl_lock(devlink);
> -	__devlink_compat_running_version(devlink, buf, len);
> +	if (devl_is_alive(devlink))
> +		__devlink_compat_running_version(devlink, buf, len);
>  	devl_unlock(devlink);
>  }
>  
> @@ -12227,20 +12242,28 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
>  	struct devlink_flash_update_params params = {};
>  	int ret;
>  
> -	if (!devlink->ops->flash_update)
> -		return -EOPNOTSUPP;
> +	devl_lock(devlink);
> +	if (!devl_is_alive(devlink)) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	if (!devlink->ops->flash_update) {
> +		ret = -EOPNOTSUPP;
> +		goto out_unlock;
> +	}
>  
>  	ret = request_firmware(&params.fw, file_name, devlink->dev);
>  	if (ret)
> -		return ret;
> +		goto out_unlock;
>  
> -	devl_lock(devlink);
>  	devlink_flash_update_begin_notify(devlink);
>  	ret = devlink->ops->flash_update(devlink, &params, NULL);
>  	devlink_flash_update_end_notify(devlink);
> -	devl_unlock(devlink);
>  
>  	release_firmware(params.fw);
> +out_unlock:
> +	devl_unlock(devlink);
>  
>  	return ret;
>  }
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index d3b8336946fd..2abad8247597 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -67,6 +67,21 @@ void devl_unlock(struct devlink *devlink)
>  }
>  EXPORT_SYMBOL_GPL(devl_unlock);
>  
> +bool devl_is_alive(struct devlink *devlink)
> +{
> +	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> +}
> +EXPORT_SYMBOL_GPL(devl_is_alive);
> +
> +/**
> + * devlink_try_get() - try to obtain a reference on a devlink instance
> + * @devlink: instance to reference
> + *
> + * Obtain a reference on a devlink instance. A reference on a devlink instance
> + * only implies that it's safe to take the instance lock. It does not imply
> + * that the instance is registered, use devl_is_alive() after taking
> + * the instance lock to check registration status.
> + */
>  struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>  {
>  	if (refcount_inc_not_zero(&devlink->refcount))
> @@ -300,10 +315,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
>  	devlinks_xa_for_each_registered_get(net, index, devlink) {
>  		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
>  		devl_lock(devlink);
> -		err = devlink_reload(devlink, &init_net,
> -				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> -				     DEVLINK_RELOAD_LIMIT_UNSPEC,
> -				     &actions_performed, NULL);
> +		err = 0;
> +		if (devl_is_alive(devlink))
> +			err = devlink_reload(devlink, &init_net,
> +					     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
> +					     DEVLINK_RELOAD_LIMIT_UNSPEC,
> +					     &actions_performed, NULL);
>  		devl_unlock(devlink);
>  		devlink_put(devlink);
>  
> diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
> index b38df704be1c..773efaabb6ad 100644
> --- a/net/devlink/netlink.c
> +++ b/net/devlink/netlink.c
> @@ -98,7 +98,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
>  
>  	devlinks_xa_for_each_registered_get(net, index, devlink) {
>  		devl_lock(devlink);
> -		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
> +		if (devl_is_alive(devlink) &&
> +		    strcmp(devlink->dev->bus->name, busname) == 0 &&
>  		    strcmp(dev_name(devlink->dev), devname) == 0)
>  			return devlink;
>  		devl_unlock(devlink);
> @@ -210,7 +211,12 @@ int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
>  
>  	devlink_dump_for_each_instance_get(msg, dump, devlink) {
>  		devl_lock(devlink);
> -		err = cmd->dump_one(msg, devlink, cb);
> +
> +		if (devl_is_alive(devlink))
> +			err = cmd->dump_one(msg, devlink, cb);
> +		else
> +			err = 0;
> +
>  		devl_unlock(devlink);
>  		devlink_put(devlink);
>  

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jacob Keller]

On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> The objective of exposing the devlink instance locks to
> drivers was to let them use these locks to prevent user space
> from accessing the device before it's fully initialized.
> This is difficult because devlink_unregister() waits for all
> references to be released, meaning that devlink_unregister()
> can't itself be called under the instance lock.
> 

Sure.

> To avoid this issue devlink_register() was moved after subobject
> registration a while ago. Unfortunately the netdev paths get
> a hold of the devlink instances _before_ they are registered.
> Ideally netdev should wait for devlink init to finish (synchronizing
> on the instance lock). This can't work because we don't know if the
> instance will _ever_ be registered (in case of failures it may not).
> The other option of returning an error until devlink_register()
> is called is unappealing (user space would get a notification
> netdev exist but would have to wait arbitrary amount of time
> before accessing some of its attributes).
> 

Nice summary of the problems and options that we have tried already.

I think its also important as this can allow sub objects to be
registered after the devlink instance?

> Weaken the guarantees of the devlink references.
> 
> Holding a reference will now only guarantee that the memory
> of the object is around. Another way of looking at it is that
> the reference now protects the object not its "registered" status.
> Use devlink instance lock to synchronize unregistration.
> 

Right, this makes sense.

> This implies that releasing of the "main" reference of the devlink
> instance moves from devlink_unregister() to devlink_free().
> 

This makes sense and I think aligns more with how most references work
in practice. Good.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Code change seems straight forward enough. I had a minor question, but:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  include/net/devlink.h       |  2 ++
>  net/devlink/core.c          | 64 ++++++++++++++++---------------------
>  net/devlink/devl_internal.h |  2 --
>  3 files changed, 30 insertions(+), 38 deletions(-)
> 
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 36e013d3aa52..cc910612b3f4 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1648,6 +1648,8 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
>  	return devlink_alloc_ns(ops, priv_size, &init_net, dev);
>  }
>  void devlink_set_features(struct devlink *devlink, u64 features);
> +int devl_register(struct devlink *devlink);
> +void devl_unregister(struct devlink *devlink);
>  void devlink_register(struct devlink *devlink);
>  void devlink_unregister(struct devlink *devlink);
>  void devlink_free(struct devlink *devlink);
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index 2abad8247597..413b92534ad6 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -89,21 +89,10 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)
>  	return NULL;
>  }
>  
> -static void __devlink_put_rcu(struct rcu_head *head)
> -{
> -	struct devlink *devlink = container_of(head, struct devlink, rcu);
> -
> -	complete(&devlink->comp);
> -}
> -
>  void devlink_put(struct devlink *devlink)
>  {
>  	if (refcount_dec_and_test(&devlink->refcount))
> -		/* Make sure unregister operation that may await the completion
> -		 * is unblocked only after all users are after the end of
> -		 * RCU grace period.
> -		 */
> -		call_rcu(&devlink->rcu, __devlink_put_rcu);
> +		kfree_rcu(devlink, rcu);
>  }
>  
>  struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> @@ -116,13 +105,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
>  	if (!devlink)
>  		goto unlock;
>  
> -	/* In case devlink_unregister() was already called and "unregistering"
> -	 * mark was set, do not allow to get a devlink reference here.
> -	 * This prevents live-lock of devlink_unregister() wait for completion.
> -	 */
> -	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
> -		goto next;
> -
>  	if (!devlink_try_get(devlink))
>  		goto next;
>  	if (!net_eq(devlink_net(devlink), net)) {
> @@ -158,37 +140,48 @@ void devlink_set_features(struct devlink *devlink, u64 features)
>  EXPORT_SYMBOL_GPL(devlink_set_features);
>  
>  /**
> - *	devlink_register - Register devlink instance
> - *
> - *	@devlink: devlink
> + * devl_register - Register devlink instance
> + * @devlink: devlink
>   */
> -void devlink_register(struct devlink *devlink)
> +int devl_register(struct devlink *devlink)
>  {
>  	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
> -	/* Make sure that we are in .probe() routine */
> +	devl_assert_locked(devlink);
>  
>  	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>  	devlink_notify_register(devlink);
> +
> +	return 0;

Any particular reason to change this to int when it doesn't have a
failure case yet? Future patches I assume? You don't check the
devl_register return value.

> +}
> +EXPORT_SYMBOL_GPL(devl_register);
> +
> +void devlink_register(struct devlink *devlink)
> +{
> +	devl_lock(devlink);
> +	devl_register(devlink);
> +	devl_unlock(devlink);
>  }
>  EXPORT_SYMBOL_GPL(devlink_register);
>  
>  /**
> - *	devlink_unregister - Unregister devlink instance
> - *
> - *	@devlink: devlink
> + * devl_unregister - Unregister devlink instance
> + * @devlink: devlink
>   */
> -void devlink_unregister(struct devlink *devlink)
> +void devl_unregister(struct devlink *devlink)
>  {
>  	ASSERT_DEVLINK_REGISTERED(devlink);
> -	/* Make sure that we are in .remove() routine */
> -
> -	xa_set_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
> -	devlink_put(devlink);
> -	wait_for_completion(&devlink->comp);
> +	devl_assert_locked(devlink);
>  
>  	devlink_notify_unregister(devlink);
>  	xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> -	xa_clear_mark(&devlinks, devlink->index, DEVLINK_UNREGISTERING);
> +}
> +EXPORT_SYMBOL_GPL(devl_unregister);
> +
> +void devlink_unregister(struct devlink *devlink)
> +{
> +	devl_lock(devlink);
> +	devl_unregister(devlink);
> +	devl_unlock(devlink);
>  }
>  EXPORT_SYMBOL_GPL(devlink_unregister);
>  
> @@ -252,7 +245,6 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	mutex_init(&devlink->reporters_lock);
>  	mutex_init(&devlink->linecards_lock);
>  	refcount_set(&devlink->refcount, 1);
> -	init_completion(&devlink->comp);
>  
>  	return devlink;
>  
> @@ -298,7 +290,7 @@ void devlink_free(struct devlink *devlink)
>  
>  	xa_erase(&devlinks, devlink->index);
>  
> -	kfree(devlink);
> +	devlink_put(devlink);
>  }
>  EXPORT_SYMBOL_GPL(devlink_free);
>  
> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> index c3977c69552a..7e77eebde3b9 100644
> --- a/net/devlink/devl_internal.h
> +++ b/net/devlink/devl_internal.h
> @@ -12,7 +12,6 @@
>  #include <net/net_namespace.h>
>  
>  #define DEVLINK_REGISTERED XA_MARK_1
> -#define DEVLINK_UNREGISTERING XA_MARK_2
>  
>  #define DEVLINK_RELOAD_STATS_ARRAY_SIZE \
>  	(__DEVLINK_RELOAD_LIMIT_MAX * __DEVLINK_RELOAD_ACTION_MAX)
> @@ -52,7 +51,6 @@ struct devlink {
>  	struct lock_class_key lock_key;
>  	u8 reload_failed:1;
>  	refcount_t refcount;
> -	struct completion comp;
>  	struct rcu_head rcu;
>  	struct notifier_block netdevice_nb;
>  	char priv[] __aligned(NETDEV_ALIGN);

Re: [RFC net-next 07/10] netdevsim: rename a label

[Jacob Keller]

On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> err_dl_unregister should unregister the devlink instance.
> Looks like renaming it was missed in one of the reshufflings.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  drivers/net/netdevsim/dev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index b962fc8e1397..d25f6e86d901 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -1563,7 +1563,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  	err = devlink_params_register(devlink, nsim_devlink_params,
>  				      ARRAY_SIZE(nsim_devlink_params));
>  	if (err)
> -		goto err_dl_unregister;
> +		goto err_resource_unregister;
>  	nsim_devlink_set_params_init_values(nsim_dev, devlink);
>  
>  	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
> @@ -1629,7 +1629,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  err_params_unregister:
>  	devlink_params_unregister(devlink, nsim_devlink_params,
>  				  ARRAY_SIZE(nsim_devlink_params));
> -err_dl_unregister:
> +err_resource_unregister:
>  	devl_resources_unregister(devlink);
>  err_vfc_free:
>  	kfree(nsim_dev->vfconfigs);

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

On Mon, 19 Dec 2022 09:48:54 -0800 Jacob Keller wrote:
> On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> > Always check under the instance lock whether the devlink instance
> > is still / already registered.
> 
> Ok. So now the reference ensures less about whats valid. It guarantees a
> lock but doesn't ensure that the devlink remains registered unless you
> acquire the lock and check that the devlink is alive under lock now?

Correct.

> > This is a no-op for the most part, as the unregistration path currently
> > waits for all references. On the init path, however, we may temporarily
> > open up a race with netdev code, if netdevs are registered before the
> > devlink instance. This is temporary, the next change fixes it, and this
> > commit has been split out for the ease of review.
> >   
> 
> This means you're adding the problem here, but its fixed in next commit..?

Yes, I can squash when posting for applying, but TBH I think the clarity
of the changes outweighs the tiny and transient race.

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jakub Kicinski]

On Mon, 19 Dec 2022 09:56:26 -0800 Jacob Keller wrote:
> > -void devlink_register(struct devlink *devlink)
> > +int devl_register(struct devlink *devlink)
> >  {
> >  	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
> > -	/* Make sure that we are in .probe() routine */
> > +	devl_assert_locked(devlink);
> >  
> >  	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> >  	devlink_notify_register(devlink);
> > +
> > +	return 0;  
> 
> Any particular reason to change this to int when it doesn't have a
> failure case yet? Future patches I assume? You don't check the
> devl_register return value.

I was wondering if anyone would notice :)

Returning errors from the registration helper seems natural,
and if we don't have this ability it may impact our ability
to extend the core in the long run.
I was against making core functions void in the first place.
It's a good opportunity to change back.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jacob Keller]

On 12/19/2022 1:55 PM, Jakub Kicinski wrote:
> On Mon, 19 Dec 2022 09:48:54 -0800 Jacob Keller wrote:
>> On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
>>> Always check under the instance lock whether the devlink instance
>>> is still / already registered.
>>
>> Ok. So now the reference ensures less about whats valid. It guarantees a
>> lock but doesn't ensure that the devlink remains registered unless you
>> acquire the lock and check that the devlink is alive under lock now?
> 
> Correct.
> 
>>> This is a no-op for the most part, as the unregistration path currently
>>> waits for all references. On the init path, however, we may temporarily
>>> open up a race with netdev code, if netdevs are registered before the
>>> devlink instance. This is temporary, the next change fixes it, and this
>>> commit has been split out for the ease of review.
>>>   
>>
>> This means you're adding the problem here, but its fixed in next commit..?
> 
> Yes, I can squash when posting for applying, but TBH I think the clarity
> of the changes outweighs the tiny and transient race.

I would agree. The only reason I could think it to be a problem is if a
bisect lands precisely on this commit and you happen to hit this... what
are the side effects of the race here? If the side effects don't include
significant issues I think its fine.

Re: [RFC net-next 00/10] devlink: remove the wait-for-references on unregister

[Jakub Kicinski]

On Mon, 19 Dec 2022 09:38:09 -0800 Jacob Keller wrote:
> On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
> > This set is on top of the previous RFC.
> > 
> > Move the registration and unregistration of the devlink instances
> > under their instance locks. Don't perform the netdev-style wait
> > for all references when unregistering the instance.
> 
> Could you explain the reasoning/benefits here? I'm sure some of this is
> explained in each commit message as well but it would help to understand
> the overall series.

Fair point, I'll add this:

  Yang Yingliang reported [1] a use-after-free in devlink because netdev
  paths are able to acquire a reference on the devlink instances before
  they are registered.

[1]
https://lore.kernel.org/all/20221122121048.776643-1-yangyingliang@huawei.com/

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jacob Keller]

On 12/19/2022 2:02 PM, Jakub Kicinski wrote:
> On Mon, 19 Dec 2022 09:56:26 -0800 Jacob Keller wrote:
>>> -void devlink_register(struct devlink *devlink)
>>> +int devl_register(struct devlink *devlink)
>>>  {
>>>  	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
>>> -	/* Make sure that we are in .probe() routine */
>>> +	devl_assert_locked(devlink);
>>>  
>>>  	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>>>  	devlink_notify_register(devlink);
>>> +
>>> +	return 0;  
>>
>> Any particular reason to change this to int when it doesn't have a
>> failure case yet? Future patches I assume? You don't check the
>> devl_register return value.
> 
> I was wondering if anyone would notice :)
> 

I'm fine with it, but I would expect that devlink_register would want to
report it at least?

> Returning errors from the registration helper seems natural,
> and if we don't have this ability it may impact our ability
> to extend the core in the long run.
> I was against making core functions void in the first place.
> It's a good opportunity to change back.

Sure. I think its better to be able to report an error but wanted to
make sure its actually caught or at least logged if it occurs.

We can ofcourse change the function templates again since we don't
really guarantee API stability across versions, but it is more work for
backporting in the future.

Re: [RFC net-next 00/10] devlink: remove the wait-for-references on unregister

[Jacob Keller]

On 12/19/2022 2:10 PM, Jakub Kicinski wrote:
> On Mon, 19 Dec 2022 09:38:09 -0800 Jacob Keller wrote:
>> On 12/16/2022 5:19 PM, Jakub Kicinski wrote:
>>> This set is on top of the previous RFC.
>>>
>>> Move the registration and unregistration of the devlink instances
>>> under their instance locks. Don't perform the netdev-style wait
>>> for all references when unregistering the instance.
>>
>> Could you explain the reasoning/benefits here? I'm sure some of this is
>> explained in each commit message as well but it would help to understand
>> the overall series.
> 
> Fair point, I'll add this:
> 
>   Yang Yingliang reported [1] a use-after-free in devlink because netdev
>   paths are able to acquire a reference on the devlink instances before
>   they are registered.
> 
> [1]
> https://lore.kernel.org/all/20221122121048.776643-1-yangyingliang@huawei.com/

Great. I also think its better to allow some of the sub objects to be
setup either before or after registering, as some things might be
dynamic. This series lets us get there safely which is good.

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jakub Kicinski]

On Mon, 19 Dec 2022 14:14:18 -0800 Jacob Keller wrote:
> On 12/19/2022 2:02 PM, Jakub Kicinski wrote:
> > I was wondering if anyone would notice :)
> 
> I'm fine with it, but I would expect that devlink_register would want to
> report it at least?

New code should not use devlink_* functions, so probably not worth it.

> > Returning errors from the registration helper seems natural,
> > and if we don't have this ability it may impact our ability
> > to extend the core in the long run.
> > I was against making core functions void in the first place.
> > It's a good opportunity to change back.  
> 
> Sure. I think its better to be able to report an error but wanted to
> make sure its actually caught or at least logged if it occurs.
> 
> We can ofcourse change the function templates again since we don't
> really guarantee API stability across versions, but it is more work for
> backporting in the future.

Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote:
>We use a clever find_first() / find_after() scheme currently,
>which works nicely as xarray will write the "current" index
>into the variable we pass.
>
>We can't do the same thing during the "dump walk" because
>there we must not increment the index until we're sure
>that the instance has been fully dumped.

To be honest, this "we something" desctiption style makes things quite
hard to understand. Could you please rephrase it to actually talk
about the entities in code?


>
>Since we have a precedent and a requirement for manually futzing
>with the increment of the index, let's switch
>devlinks_xa_for_each_registered_get() to do the same thing.
>It removes some indirections.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> net/devlink/core.c          | 31 +++++++++----------------------
> net/devlink/devl_internal.h | 17 ++++-------------
> 2 files changed, 13 insertions(+), 35 deletions(-)
>
>diff --git a/net/devlink/core.c b/net/devlink/core.c
>index 371d6821315d..88c88b8053e2 100644
>--- a/net/devlink/core.c
>+++ b/net/devlink/core.c
>@@ -91,16 +91,13 @@ void devlink_put(struct devlink *devlink)
> 		call_rcu(&devlink->rcu, __devlink_put_rcu);
> }
> 
>-struct devlink *
>-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
>-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
>-					  unsigned long, xa_mark_t))
>+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> {
>-	struct devlink *devlink;
>+	struct devlink *devlink = NULL;
> 
> 	rcu_read_lock();
> retry:
>-	devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
>+	devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> 	if (!devlink)
> 		goto unlock;
> 
>@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> 	 * This prevents live-lock of devlink_unregister() wait for completion.
> 	 */
> 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
>-		goto retry;
>+		goto next;
> 
>-	/* For a possible retry, the xa_find_after() should be always used */
>-	xa_find_fn = xa_find_after;

Hmm. Any idea why xa_find_after()? implementation is different to
xa_find()?


> 	if (!devlink_try_get(devlink))
>-		goto retry;
>+		goto next;
> 	if (!net_eq(devlink_net(devlink), net)) {
> 		devlink_put(devlink);
>-		goto retry;
>+		goto next;
> 	}
> unlock:
> 	rcu_read_unlock();
> 	return devlink;
>-}
>-
>-struct devlink *
>-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp)
>-{
>-	return devlinks_xa_find_get(net, indexp, xa_find);
>-}
> 
>-struct devlink *
>-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp)
>-{
>-	return devlinks_xa_find_get(net, indexp, xa_find_after);
>+next:
>+	(*indexp)++;
>+	goto retry;
> }
> 
> /**
>diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>index 1d7ab11f2f7e..ef0369449592 100644
>--- a/net/devlink/devl_internal.h
>+++ b/net/devlink/devl_internal.h
>@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
>  * in loop body in order to release the reference.
>  */
> #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
>-	for (index = 0,							\
>-	     devlink = devlinks_xa_find_get_first(net, &index);	\
>-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
>-
>-struct devlink *
>-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
>-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
>-					  unsigned long, xa_mark_t));
>-struct devlink *
>-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
>-struct devlink *
>-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
>+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)

You don't need ()' in the 2nd for arg.


>+
>+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp);
> 
> /* Netlink */
> #define DEVLINK_NL_FLAG_NEED_PORT		BIT(0)
>@@ -133,7 +124,7 @@ struct devlink_gen_cmd {
>  */
> #define devlink_dump_for_each_instance_get(msg, dump, devlink)		\
> 	for (; (devlink = devlinks_xa_find_get(sock_net(msg->sk),	\
>-					       &dump->instance, xa_find)); \
>+					       &dump->instance));	\
> 	     dump->instance++, dump->idx = 0)
> 
> extern const struct genl_small_ops devlink_nl_ops[56];
>-- 
>2.38.1
>

Re: [RFC net-next 10/10] netdevsim: register devlink instance before sub-objects

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:53AM CET, kuba@kernel.org wrote:
>Move the devlink instance registration up so that all the sub-object
>manipulation happens on a valid instance.

I wonder, why don't you squash patch 8 to this one and make 1 move, to
the fina destination?

Re: [RFC net-next 02/10] devlink: update the code in netns move to latest helpers

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:45AM CET, kuba@kernel.org wrote:
>devlink_pernet_pre_exit() is the only obvious place which takes
>the instance lock without using the devl_ helpers. Update the code
>and move the error print after releasing the reference
>(having unlock and put together feels slightly idiomatic).

To be prudent, this should be 2 patches as it changes 2 separate things.
Anyway,

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
>Always check under the instance lock whether the devlink instance
>is still / already registered.
>
>This is a no-op for the most part, as the unregistration path currently
>waits for all references. On the init path, however, we may temporarily
>open up a race with netdev code, if netdevs are registered before the
>devlink instance. This is temporary, the next change fixes it, and this
>commit has been split out for the ease of review.
>
>Note that in case of iterating over sub-objects which have their
>own lock (regions and line cards) we assume an implicit dependency
>between those objects existing and devlink unregistration.

This would be probably very valuable to add as a comment inside the code
for the future reader mind sake.


>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> include/net/devlink.h |  1 +
> net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
> net/devlink/core.c    | 25 +++++++++++++++++++++----
> net/devlink/netlink.c | 10 ++++++++--
> 4 files changed, 59 insertions(+), 12 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 6a2e4f21779f..36e013d3aa52 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
> void devl_lock(struct devlink *devlink);
> int devl_trylock(struct devlink *devlink);
> void devl_unlock(struct devlink *devlink);
>+bool devl_is_alive(struct devlink *devlink);
> void devl_assert_locked(struct devlink *devlink);
> bool devl_lock_is_held(struct devlink *devlink);
> 
>diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>index 5f33d74eef83..6b18e70a39fd 100644
>--- a/net/devlink/basic.c
>+++ b/net/devlink/basic.c
>@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> 		int idx = 0;
> 
> 		mutex_lock(&devlink->linecards_lock);
>+		if (!devl_is_alive(devlink))
>+			goto next_devlink;
>+
> 		list_for_each_entry(linecard, &devlink->linecard_list, list) {
> 			if (idx < dump->idx) {
> 				idx++;
>@@ -2151,6 +2154,7 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> 			}
> 			idx++;
> 		}
>+next_devlink:
> 		mutex_unlock(&devlink->linecards_lock);
> 		devlink_put(devlink);
> 	}
>@@ -7809,6 +7813,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 		int idx = 0;
> 
> 		mutex_lock(&devlink->reporters_lock);
>+		if (!devl_is_alive(devlink)) {
>+			mutex_unlock(&devlink->reporters_lock);
>+			devlink_put(devlink);
>+			continue;
>+		}
>+
> 		list_for_each_entry(reporter, &devlink->reporter_list,
> 				    list) {
> 			if (idx < dump->idx) {
>@@ -7830,6 +7840,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 		mutex_unlock(&devlink->reporters_lock);
> 
> 		devl_lock(devlink);
>+		if (!devl_is_alive(devlink))
>+			goto next_devlink;
>+
> 		xa_for_each(&devlink->ports, port_index, port) {
> 			mutex_lock(&port->reporters_lock);
> 			list_for_each_entry(reporter, &port->reporter_list, list) {
>@@ -7853,6 +7866,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 			}
> 			mutex_unlock(&port->reporters_lock);
> 		}
>+next_devlink:
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 	}
>@@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
> 		return;
> 
> 	devl_lock(devlink);
>-	__devlink_compat_running_version(devlink, buf, len);
>+	if (devl_is_alive(devlink))
>+		__devlink_compat_running_version(devlink, buf, len);
> 	devl_unlock(devlink);
> }
> 
>@@ -12227,20 +12242,28 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
> 	struct devlink_flash_update_params params = {};
> 	int ret;
> 
>-	if (!devlink->ops->flash_update)
>-		return -EOPNOTSUPP;
>+	devl_lock(devlink);
>+	if (!devl_is_alive(devlink)) {
>+		ret = -ENODEV;
>+		goto out_unlock;
>+	}
>+
>+	if (!devlink->ops->flash_update) {
>+		ret = -EOPNOTSUPP;
>+		goto out_unlock;
>+	}
> 
> 	ret = request_firmware(&params.fw, file_name, devlink->dev);
> 	if (ret)
>-		return ret;
>+		goto out_unlock;
> 
>-	devl_lock(devlink);
> 	devlink_flash_update_begin_notify(devlink);
> 	ret = devlink->ops->flash_update(devlink, &params, NULL);
> 	devlink_flash_update_end_notify(devlink);
>-	devl_unlock(devlink);
> 
> 	release_firmware(params.fw);
>+out_unlock:
>+	devl_unlock(devlink);
> 
> 	return ret;
> }
>diff --git a/net/devlink/core.c b/net/devlink/core.c
>index d3b8336946fd..2abad8247597 100644
>--- a/net/devlink/core.c
>+++ b/net/devlink/core.c
>@@ -67,6 +67,21 @@ void devl_unlock(struct devlink *devlink)
> }
> EXPORT_SYMBOL_GPL(devl_unlock);
> 
>+bool devl_is_alive(struct devlink *devlink)

Why "alive"? To be consistent with the existing terminology, how about
to name it devl_is_registered()?

Also, "devl_" implicates that it should be called with devlink instance
lock held, so probably devlink_is_registered() would be better.


>+{
>+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>+}
>+EXPORT_SYMBOL_GPL(devl_is_alive);
>+
>+/**
>+ * devlink_try_get() - try to obtain a reference on a devlink instance
>+ * @devlink: instance to reference
>+ *
>+ * Obtain a reference on a devlink instance. A reference on a devlink instance
>+ * only implies that it's safe to take the instance lock. It does not imply
>+ * that the instance is registered, use devl_is_alive() after taking
>+ * the instance lock to check registration status.
>+ */

This comment is not related to the patch, should be added in a separate
one.


> struct devlink *__must_check devlink_try_get(struct devlink *devlink)
> {
> 	if (refcount_inc_not_zero(&devlink->refcount))
>@@ -300,10 +315,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
> 	devlinks_xa_for_each_registered_get(net, index, devlink) {
> 		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
> 		devl_lock(devlink);
>-		err = devlink_reload(devlink, &init_net,
>-				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
>-				     DEVLINK_RELOAD_LIMIT_UNSPEC,
>-				     &actions_performed, NULL);
>+		err = 0;
>+		if (devl_is_alive(devlink))
>+			err = devlink_reload(devlink, &init_net,
>+					     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
>+					     DEVLINK_RELOAD_LIMIT_UNSPEC,
>+					     &actions_performed, NULL);
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 
>diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>index b38df704be1c..773efaabb6ad 100644
>--- a/net/devlink/netlink.c
>+++ b/net/devlink/netlink.c
>@@ -98,7 +98,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
> 
> 	devlinks_xa_for_each_registered_get(net, index, devlink) {
> 		devl_lock(devlink);
>-		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
>+		if (devl_is_alive(devlink) &&
>+		    strcmp(devlink->dev->bus->name, busname) == 0 &&
> 		    strcmp(dev_name(devlink->dev), devname) == 0)
> 			return devlink;
> 		devl_unlock(devlink);
>@@ -210,7 +211,12 @@ int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
> 
> 	devlink_dump_for_each_instance_get(msg, dump, devlink) {
> 		devl_lock(devlink);
>-		err = cmd->dump_one(msg, devlink, cb);
>+
>+		if (devl_is_alive(devlink))
>+			err = cmd->dump_one(msg, devlink, cb);
>+		else
>+			err = 0;
>+
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 
>-- 
>2.38.1
>

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jiri Pirko]

Mon, Dec 19, 2022 at 11:02:10PM CET, kuba@kernel.org wrote:
>On Mon, 19 Dec 2022 09:56:26 -0800 Jacob Keller wrote:
>> > -void devlink_register(struct devlink *devlink)
>> > +int devl_register(struct devlink *devlink)
>> >  {
>> >  	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
>> > -	/* Make sure that we are in .probe() routine */
>> > +	devl_assert_locked(devlink);
>> >  
>> >  	xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>> >  	devlink_notify_register(devlink);
>> > +
>> > +	return 0;  
>> 
>> Any particular reason to change this to int when it doesn't have a
>> failure case yet? Future patches I assume? You don't check the
>> devl_register return value.
>
>I was wondering if anyone would notice :)
>
>Returning errors from the registration helper seems natural,
>and if we don't have this ability it may impact our ability
>to extend the core in the long run.
>I was against making core functions void in the first place.
>It's a good opportunity to change back.

devlink_register originally returned int. Leon changed that as part of
his work. I believe I expressed my negative feelings about that back
then. Sigh.

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:48AM CET, kuba@kernel.org wrote:
>

[...]


>Holding a reference will now only guarantee that the memory
>of the object is around. Another way of looking at it is that
>the reference now protects the object not its "registered" status.

This would help to understand what you are doing in patch 3. Perphaps it
would be fine to describe the goal in the cover letter?

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
>Always check under the instance lock whether the devlink instance
>is still / already registered.
>
>This is a no-op for the most part, as the unregistration path currently
>waits for all references. On the init path, however, we may temporarily
>open up a race with netdev code, if netdevs are registered before the
>devlink instance. This is temporary, the next change fixes it, and this
>commit has been split out for the ease of review.
>
>Note that in case of iterating over sub-objects which have their
>own lock (regions and line cards) we assume an implicit dependency
>between those objects existing and devlink unregistration.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> include/net/devlink.h |  1 +
> net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
> net/devlink/core.c    | 25 +++++++++++++++++++++----
> net/devlink/netlink.c | 10 ++++++++--
> 4 files changed, 59 insertions(+), 12 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 6a2e4f21779f..36e013d3aa52 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
> void devl_lock(struct devlink *devlink);
> int devl_trylock(struct devlink *devlink);
> void devl_unlock(struct devlink *devlink);
>+bool devl_is_alive(struct devlink *devlink);
> void devl_assert_locked(struct devlink *devlink);
> bool devl_lock_is_held(struct devlink *devlink);
> 
>diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>index 5f33d74eef83..6b18e70a39fd 100644
>--- a/net/devlink/basic.c
>+++ b/net/devlink/basic.c
>@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> 		int idx = 0;
> 
> 		mutex_lock(&devlink->linecards_lock);
>+		if (!devl_is_alive(devlink))
>+			goto next_devlink;


Thinking about this a bit more, things would be cleaner if reporters and
linecards are converted to rely on instance lock as well. I don't see a
good reason for a separate lock in both cases, really.

Also, we could introduce devlinks_xa_for_each_registered_get_lock()
iterator that would lock the instance as well right away to avoid
this devl_is_alive() dance on multiple places when you iterate devlinks.


>+
> 		list_for_each_entry(linecard, &devlink->linecard_list, list) {
> 			if (idx < dump->idx) {
> 				idx++;
>@@ -2151,6 +2154,7 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> 			}
> 			idx++;
> 		}
>+next_devlink:
> 		mutex_unlock(&devlink->linecards_lock);
> 		devlink_put(devlink);
> 	}
>@@ -7809,6 +7813,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 		int idx = 0;
> 
> 		mutex_lock(&devlink->reporters_lock);
>+		if (!devl_is_alive(devlink)) {
>+			mutex_unlock(&devlink->reporters_lock);
>+			devlink_put(devlink);
>+			continue;
>+		}
>+
> 		list_for_each_entry(reporter, &devlink->reporter_list,
> 				    list) {
> 			if (idx < dump->idx) {
>@@ -7830,6 +7840,9 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 		mutex_unlock(&devlink->reporters_lock);
> 
> 		devl_lock(devlink);
>+		if (!devl_is_alive(devlink))
>+			goto next_devlink;
>+
> 		xa_for_each(&devlink->ports, port_index, port) {
> 			mutex_lock(&port->reporters_lock);
> 			list_for_each_entry(reporter, &port->reporter_list, list) {
>@@ -7853,6 +7866,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct sk_buff *msg,
> 			}
> 			mutex_unlock(&port->reporters_lock);
> 		}
>+next_devlink:
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 	}
>@@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
> 		return;
> 
> 	devl_lock(devlink);

How about to have a helper, something like devl_lock_alive() (or
devl_lock_registered() with the naming scheme I suggest in the other
thread)? Then you can do:

	if (!devl_lock_alive(devlink))
		return;
	__devlink_compat_running_version(devlink, buf, len);
	devl_unlock(devlink);



>-	__devlink_compat_running_version(devlink, buf, len);
>+	if (devl_is_alive(devlink))
>+		__devlink_compat_running_version(devlink, buf, len);
> 	devl_unlock(devlink);
> }
> 
>@@ -12227,20 +12242,28 @@ int devlink_compat_flash_update(struct devlink *devlink, const char *file_name)
> 	struct devlink_flash_update_params params = {};
> 	int ret;
> 
>-	if (!devlink->ops->flash_update)
>-		return -EOPNOTSUPP;
>+	devl_lock(devlink);
>+	if (!devl_is_alive(devlink)) {
>+		ret = -ENODEV;
>+		goto out_unlock;
>+	}
>+
>+	if (!devlink->ops->flash_update) {
>+		ret = -EOPNOTSUPP;
>+		goto out_unlock;
>+	}
> 
> 	ret = request_firmware(&params.fw, file_name, devlink->dev);
> 	if (ret)
>-		return ret;
>+		goto out_unlock;
> 
>-	devl_lock(devlink);
> 	devlink_flash_update_begin_notify(devlink);
> 	ret = devlink->ops->flash_update(devlink, &params, NULL);
> 	devlink_flash_update_end_notify(devlink);
>-	devl_unlock(devlink);
> 
> 	release_firmware(params.fw);
>+out_unlock:
>+	devl_unlock(devlink);
> 
> 	return ret;
> }
>diff --git a/net/devlink/core.c b/net/devlink/core.c
>index d3b8336946fd..2abad8247597 100644
>--- a/net/devlink/core.c
>+++ b/net/devlink/core.c
>@@ -67,6 +67,21 @@ void devl_unlock(struct devlink *devlink)
> }
> EXPORT_SYMBOL_GPL(devl_unlock);
> 
>+bool devl_is_alive(struct devlink *devlink)
>+{
>+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>+}
>+EXPORT_SYMBOL_GPL(devl_is_alive);
>+
>+/**
>+ * devlink_try_get() - try to obtain a reference on a devlink instance
>+ * @devlink: instance to reference
>+ *
>+ * Obtain a reference on a devlink instance. A reference on a devlink instance
>+ * only implies that it's safe to take the instance lock. It does not imply
>+ * that the instance is registered, use devl_is_alive() after taking
>+ * the instance lock to check registration status.
>+ */
> struct devlink *__must_check devlink_try_get(struct devlink *devlink)
> {
> 	if (refcount_inc_not_zero(&devlink->refcount))
>@@ -300,10 +315,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
> 	devlinks_xa_for_each_registered_get(net, index, devlink) {
> 		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
> 		devl_lock(devlink);
>-		err = devlink_reload(devlink, &init_net,
>-				     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
>-				     DEVLINK_RELOAD_LIMIT_UNSPEC,
>-				     &actions_performed, NULL);
>+		err = 0;
>+		if (devl_is_alive(devlink))
>+			err = devlink_reload(devlink, &init_net,
>+					     DEVLINK_RELOAD_ACTION_DRIVER_REINIT,
>+					     DEVLINK_RELOAD_LIMIT_UNSPEC,
>+					     &actions_performed, NULL);
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 
>diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c
>index b38df704be1c..773efaabb6ad 100644
>--- a/net/devlink/netlink.c
>+++ b/net/devlink/netlink.c
>@@ -98,7 +98,8 @@ devlink_get_from_attrs_lock(struct net *net, struct nlattr **attrs)
> 
> 	devlinks_xa_for_each_registered_get(net, index, devlink) {
> 		devl_lock(devlink);
>-		if (strcmp(devlink->dev->bus->name, busname) == 0 &&
>+		if (devl_is_alive(devlink) &&
>+		    strcmp(devlink->dev->bus->name, busname) == 0 &&
> 		    strcmp(dev_name(devlink->dev), devname) == 0)
> 			return devlink;
> 		devl_unlock(devlink);
>@@ -210,7 +211,12 @@ int devlink_instance_iter_dump(struct sk_buff *msg, struct netlink_callback *cb)
> 
> 	devlink_dump_for_each_instance_get(msg, dump, devlink) {
> 		devl_lock(devlink);
>-		err = cmd->dump_one(msg, devlink, cb);
>+
>+		if (devl_is_alive(devlink))
>+			err = cmd->dump_one(msg, devlink, cb);
>+		else
>+			err = 0;
>+
> 		devl_unlock(devlink);
> 		devlink_put(devlink);
> 
>-- 
>2.38.1
>

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Mon, Jan 02, 2023 at 03:57:24PM CET, jiri@resnulli.us wrote:
>Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
>>Always check under the instance lock whether the devlink instance
>>is still / already registered.
>>
>>This is a no-op for the most part, as the unregistration path currently
>>waits for all references. On the init path, however, we may temporarily
>>open up a race with netdev code, if netdevs are registered before the
>>devlink instance. This is temporary, the next change fixes it, and this
>>commit has been split out for the ease of review.
>>
>>Note that in case of iterating over sub-objects which have their
>>own lock (regions and line cards) we assume an implicit dependency
>>between those objects existing and devlink unregistration.
>>
>>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>>---
>> include/net/devlink.h |  1 +
>> net/devlink/basic.c   | 35 +++++++++++++++++++++++++++++------
>> net/devlink/core.c    | 25 +++++++++++++++++++++----
>> net/devlink/netlink.c | 10 ++++++++--
>> 4 files changed, 59 insertions(+), 12 deletions(-)
>>
>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>index 6a2e4f21779f..36e013d3aa52 100644
>>--- a/include/net/devlink.h
>>+++ b/include/net/devlink.h
>>@@ -1626,6 +1626,7 @@ struct device *devlink_to_dev(const struct devlink *devlink);
>> void devl_lock(struct devlink *devlink);
>> int devl_trylock(struct devlink *devlink);
>> void devl_unlock(struct devlink *devlink);
>>+bool devl_is_alive(struct devlink *devlink);
>> void devl_assert_locked(struct devlink *devlink);
>> bool devl_lock_is_held(struct devlink *devlink);
>> 
>>diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>>index 5f33d74eef83..6b18e70a39fd 100644
>>--- a/net/devlink/basic.c
>>+++ b/net/devlink/basic.c
>>@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
>> 		int idx = 0;
>> 
>> 		mutex_lock(&devlink->linecards_lock);
>>+		if (!devl_is_alive(devlink))
>>+			goto next_devlink;
>
>
>Thinking about this a bit more, things would be cleaner if reporters and
>linecards are converted to rely on instance lock as well. I don't see a
>good reason for a separate lock in both cases, really.
>
>Also, we could introduce devlinks_xa_for_each_registered_get_lock()
>iterator that would lock the instance as well right away to avoid
>this devl_is_alive() dance on multiple places when you iterate devlinks.

devlinks_xa_find_get_locked()
would do the check&lock at the end.



>
>
>>+
>> 		list_for_each_entry(linecard, &devlink->linecard_list, list) {
>> 			if (idx < dump->idx) {
>> 				idx++;

[...]

Re: [RFC net-next 06/10] devlink: don't require setting features before registration

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:49AM CET, kuba@kernel.org wrote:
>Requiring devlink_set_features() to be run before devlink is
>registered is overzealous. devlink_set_features() itself is
>a leftover from old workarounds which were trying to prevent
>initiating reload before probe was complete.

Wouldn't it be better to remove this entirely? I don't think it is
needed anymore.


>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> net/devlink/core.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/net/devlink/core.c b/net/devlink/core.c
>index 413b92534ad6..f30fc167c8ad 100644
>--- a/net/devlink/core.c
>+++ b/net/devlink/core.c
>@@ -131,8 +131,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
>  */
> void devlink_set_features(struct devlink *devlink, u64 features)
> {
>-	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
>-
> 	WARN_ON(features & DEVLINK_F_RELOAD &&
> 		!devlink_reload_supported(devlink->ops));
> 	devlink->features = features;
>-- 
>2.38.1
>

Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jakub Kicinski]

On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
> Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote:
> >We use a clever find_first() / find_after() scheme currently,
> >which works nicely as xarray will write the "current" index
> >into the variable we pass.
> >
> >We can't do the same thing during the "dump walk" because
> >there we must not increment the index until we're sure
> >that the instance has been fully dumped.  
> 
> To be honest, this "we something" desctiption style makes things quite
> hard to understand. Could you please rephrase it to actually talk
> about the entities in code?

Could you be more specific? I'm probably misunderstanding but it sounds
to me like you're asking me to describe what I'm doing rather than
the background and motivation.

> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> >index 1d7ab11f2f7e..ef0369449592 100644
> >--- a/net/devlink/devl_internal.h
> >+++ b/net/devlink/devl_internal.h
> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
> >  * in loop body in order to release the reference.
> >  */
> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
> >-	for (index = 0,							\
> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
> >-
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t));
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)  
> 
> You don't need ()' in the 2nd for arg.

Pretty sure I was getting a compiler warning for assignment in 
a condition....

Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jakub Kicinski]

On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t))
> >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
> > {
> >-	struct devlink *devlink;
> >+	struct devlink *devlink = NULL;
> > 
> > 	rcu_read_lock();
> > retry:
> >-	devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> >+	devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED);
> > 	if (!devlink)
> > 		goto unlock;
> > 
> >@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> > 	 * This prevents live-lock of devlink_unregister() wait for completion.
> > 	 */
> > 	if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING))
> >-		goto retry;
> >+		goto next;
> > 
> >-	/* For a possible retry, the xa_find_after() should be always used */
> >-	xa_find_fn = xa_find_after;  
> 
> Hmm. Any idea why xa_find_after()? implementation is different to
> xa_find()?

I'm guessing it's because for _after the code needs to take special
care of skipping multi-index entries. If an entry spans 0-3 xa_find(0)
can return the same entry as xa_find(1). But xa_find_after(1) must
return an entry under index 4 or higher.

Since our use of the Xarray is very trivial with no range indexes,
it should not matter.

Let me CC Matthew, just in case. The question boils down to whether:

	for (index = 0; (entry = xa_find(net, &index));	index++)  

is a legal way of iterating over an Xarray without range indexes.

> > 	if (!devlink_try_get(devlink))
> >-		goto retry;
> >+		goto next;
> > 	if (!net_eq(devlink_net(devlink), net)) {
> > 		devlink_put(devlink);
> >-		goto retry;
> >+		goto next;
> > 	}
> > unlock:
> > 	rcu_read_unlock();
> > 	return devlink;
> >-}
> >-
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find);
> >-}
> > 
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp)
> >-{
> >-	return devlinks_xa_find_get(net, indexp, xa_find_after);
> >+next:
> >+	(*indexp)++;
> >+	goto retry;
> > }
> > 
> > /**
> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> >index 1d7ab11f2f7e..ef0369449592 100644
> >--- a/net/devlink/devl_internal.h
> >+++ b/net/devlink/devl_internal.h
> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
> >  * in loop body in order to release the reference.
> >  */
> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
> >-	for (index = 0,							\
> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
> >-
> >-struct devlink *
> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
> >-					  unsigned long, xa_mark_t));
> >-struct devlink *
> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
> >-struct devlink *
> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)  

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

On Mon, 2 Jan 2023 14:58:16 +0100 Jiri Pirko wrote:
> Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
> >Always check under the instance lock whether the devlink instance
> >is still / already registered.
> >
> >This is a no-op for the most part, as the unregistration path currently
> >waits for all references. On the init path, however, we may temporarily
> >open up a race with netdev code, if netdevs are registered before the
> >devlink instance. This is temporary, the next change fixes it, and this
> >commit has been split out for the ease of review.
> >
> >Note that in case of iterating over sub-objects which have their
> >own lock (regions and line cards) we assume an implicit dependency
> >between those objects existing and devlink unregistration.  
> 
> This would be probably very valuable to add as a comment inside the code
> for the future reader mind sake.

Where, tho?

I'm strongly against the pointlessly fine-grained locking going forward
so hopefully there won't be any more per-subobject locks added anyway.

> >+bool devl_is_alive(struct devlink *devlink)  
> 
> Why "alive"? To be consistent with the existing terminology, how about
> to name it devl_is_registered()?

I dislike the similarity to device_is_registered() which has very
different semantics. I prefer alive.

> Also, "devl_" implicates that it should be called with devlink instance
> lock held, so probably devlink_is_registered() would be better.

I'm guessing you realized this isn't correct later on.

> >+{
> >+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> >+}
> >+EXPORT_SYMBOL_GPL(devl_is_alive);
> >+
> >+/**
> >+ * devlink_try_get() - try to obtain a reference on a devlink instance
> >+ * @devlink: instance to reference
> >+ *
> >+ * Obtain a reference on a devlink instance. A reference on a devlink instance
> >+ * only implies that it's safe to take the instance lock. It does not imply
> >+ * that the instance is registered, use devl_is_alive() after taking
> >+ * the instance lock to check registration status.
> >+ */  
> 
> This comment is not related to the patch, should be added in a separate
> one.

The point of adding this comment is to say that one has to use
devl_is_alive() after accessing an instance by reference.
It is very much in the right patch.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

On Mon, 2 Jan 2023 15:57:24 +0100 Jiri Pirko wrote:
> Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
> >Always check under the instance lock whether the devlink instance
> >is still / already registered.
> >
> >This is a no-op for the most part, as the unregistration path currently
> >waits for all references. On the init path, however, we may temporarily
> >open up a race with netdev code, if netdevs are registered before the
> >devlink instance. This is temporary, the next change fixes it, and this
> >commit has been split out for the ease of review.
> >
> >Note that in case of iterating over sub-objects which have their
> >own lock (regions and line cards) we assume an implicit dependency
> >between those objects existing and devlink unregistration.

> >diff --git a/net/devlink/basic.c b/net/devlink/basic.c
> >index 5f33d74eef83..6b18e70a39fd 100644
> >--- a/net/devlink/basic.c
> >+++ b/net/devlink/basic.c
> >@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
> > 		int idx = 0;
> > 
> > 		mutex_lock(&devlink->linecards_lock);
> >+		if (!devl_is_alive(devlink))
> >+			goto next_devlink;  
> 
> Thinking about this a bit more, things would be cleaner if reporters and
> linecards are converted to rely on instance lock as well. I don't see a
> good reason for a separate lock in both cases, really.

We had discussion before, I'm pretty sure.
IIRC you said that mlx4's locking prevents us from using the instance
lock for regions.

> Also, we could introduce devlinks_xa_for_each_registered_get_lock()
> iterator that would lock the instance as well right away to avoid
> this devl_is_alive() dance on multiple places when you iterate devlinks.

That's what I started with, but the ability to factor our the
unlock/put on error paths made the callback approach much cleaner.
And after using the callback for all the dumps there's only a couple
places which would use devlinks_xa_for_each_registered_get_lock().

> >@@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
> > 		return;
> > 
> > 	devl_lock(devlink);  
> 
> How about to have a helper, something like devl_lock_alive() (or
> devl_lock_registered() with the naming scheme I suggest in the other
> thread)? Then you can do:
> 
> 	if (!devl_lock_alive(devlink))
> 		return;
> 	__devlink_compat_running_version(devlink, buf, len);
> 	devl_unlock(devlink);

I guess aesthetic preference.

If I had the cycles I'd make devlink_try_get() return a wrapped type

struct devlink_ref {
	struct devlink *devlink;
};

which one would have to pass to devl_lock_from_ref() or some such:

struct devlink *devl_lock_from_ref(struct devlink_ref dref)
{
	if (!dref.devlink)
		return NULL;
	devl_lock(dref.devlink);
	if (devl_lock_alive(dref.devlink))
		return dref.devlink;
	devl_unlock(dref.devlink);
	return NULL;
}

But the number of calls to devl_is_alive() is quite small after all
the cleanup, so I don't think the extra helpers are justified at this
point. "Normal coders" should not be exposed to any of the lifetime
details, not when coding the drivers, not when adding typical devlink
features/subobjects.

Re: [RFC net-next 05/10] devlink: remove the registration guarantee of references

[Jakub Kicinski]

On Mon, 2 Jan 2023 15:32:04 +0100 Jiri Pirko wrote:
> >Holding a reference will now only guarantee that the memory
> >of the object is around. Another way of looking at it is that
> >the reference now protects the object not its "registered" status.  
> 
> This would help to understand what you are doing in patch 3. Perphaps it
> would be fine to describe the goal in the cover letter?

Fair point, will do!

Re: [RFC net-next 06/10] devlink: don't require setting features before registration

[Jakub Kicinski]

On Mon, 2 Jan 2023 16:25:18 +0100 Jiri Pirko wrote:
> Sat, Dec 17, 2022 at 02:19:49AM CET, kuba@kernel.org wrote:
> >Requiring devlink_set_features() to be run before devlink is
> >registered is overzealous. devlink_set_features() itself is
> >a leftover from old workarounds which were trying to prevent
> >initiating reload before probe was complete.  
> 
> Wouldn't it be better to remove this entirely? I don't think it is
> needed anymore.

I think you're right. Since users don't have access to the instance
before it's registered - this flag can have no impact.

Re: [RFC net-next 10/10] netdevsim: register devlink instance before sub-objects

[Jakub Kicinski]

On Mon, 2 Jan 2023 14:34:42 +0100 Jiri Pirko wrote:
> Sat, Dec 17, 2022 at 02:19:53AM CET, kuba@kernel.org wrote:
> >Move the devlink instance registration up so that all the sub-object
> >manipulation happens on a valid instance.  
> 
> I wonder, why don't you squash patch 8 to this one and make 1 move, to
> the fina destination?

I found the squashed version a lot harder to review.

Re: [RFC net-next 06/10] devlink: don't require setting features before registration

[Jakub Kicinski]

On Mon, 2 Jan 2023 15:24:47 -0800 Jakub Kicinski wrote:
> On Mon, 2 Jan 2023 16:25:18 +0100 Jiri Pirko wrote:
> > Sat, Dec 17, 2022 at 02:19:49AM CET, kuba@kernel.org wrote:  
> > >Requiring devlink_set_features() to be run before devlink is
> > >registered is overzealous. devlink_set_features() itself is
> > >a leftover from old workarounds which were trying to prevent
> > >initiating reload before probe was complete.    
> > 
> > Wouldn't it be better to remove this entirely? I don't think it is
> > needed anymore.  
> 
> I think you're right. Since users don't have access to the instance
> before it's registered - this flag can have no impact.

Let's leave this for a separate follow up, mlx5 needs a bit more work.
It sets the feature conditionally.

Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jiri Pirko]

Mon, Jan 02, 2023 at 11:48:13PM CET, kuba@kernel.org wrote:
>On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote:
>> Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote:
>> >We use a clever find_first() / find_after() scheme currently,
>> >which works nicely as xarray will write the "current" index
>> >into the variable we pass.
>> >
>> >We can't do the same thing during the "dump walk" because
>> >there we must not increment the index until we're sure
>> >that the instance has been fully dumped.  
>> 
>> To be honest, this "we something" desctiption style makes things quite
>> hard to understand. Could you please rephrase it to actually talk
>> about the entities in code?
>
>Could you be more specific? I'm probably misunderstanding but it sounds
>to me like you're asking me to describe what I'm doing rather than
>the background and motivation.

"We" is us, you me and other people. It is weird to talk about the code
and what there as "we do", "we use" etc. It's quite confusing as the
reader it not able to distinguish if you are talking about code or
people (developers in this case). I'm just askin if it is possible
to make the descriptions easier to understand.


>
>> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
>> >index 1d7ab11f2f7e..ef0369449592 100644
>> >--- a/net/devlink/devl_internal.h
>> >+++ b/net/devlink/devl_internal.h
>> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family;
>> >  * in loop body in order to release the reference.
>> >  */
>> > #define devlinks_xa_for_each_registered_get(net, index, devlink)	\
>> >-	for (index = 0,							\
>> >-	     devlink = devlinks_xa_find_get_first(net, &index);	\
>> >-	     devlink; devlink = devlinks_xa_find_get_next(net, &index))
>> >-
>> >-struct devlink *
>> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp,
>> >-		     void * (*xa_find_fn)(struct xarray *, unsigned long *,
>> >-					  unsigned long, xa_mark_t));
>> >-struct devlink *
>> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp);
>> >-struct devlink *
>> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp);
>> >+	for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)  
>> 
>> You don't need ()' in the 2nd for arg.
>
>Pretty sure I was getting a compiler warning for assignment in 
>a condition....

I see.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Tue, Jan 03, 2023 at 12:05:14AM CET, kuba@kernel.org wrote:
>On Mon, 2 Jan 2023 14:58:16 +0100 Jiri Pirko wrote:
>> Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
>> >Always check under the instance lock whether the devlink instance
>> >is still / already registered.
>> >
>> >This is a no-op for the most part, as the unregistration path currently
>> >waits for all references. On the init path, however, we may temporarily
>> >open up a race with netdev code, if netdevs are registered before the
>> >devlink instance. This is temporary, the next change fixes it, and this
>> >commit has been split out for the ease of review.
>> >
>> >Note that in case of iterating over sub-objects which have their
>> >own lock (regions and line cards) we assume an implicit dependency
>> >between those objects existing and devlink unregistration.  
>> 
>> This would be probably very valuable to add as a comment inside the code
>> for the future reader mind sake.
>
>Where, tho?
>
>I'm strongly against the pointlessly fine-grained locking going forward
>so hopefully there won't be any more per-subobject locks added anyway.

Agreed. That is what I suggested in the other thread too.


>
>> >+bool devl_is_alive(struct devlink *devlink)  
>> 
>> Why "alive"? To be consistent with the existing terminology, how about
>> to name it devl_is_registered()?
>
>I dislike the similarity to device_is_registered() which has very
>different semantics. I prefer alive.

Interesting. Didn't occur to me to look into device.h when reading
devlink.c code. I mean, is device_register() behaviour in sync with
devlink_register?

Your alive() helper is checking "register mark". It's an odd and unneded
inconsistency in newly added code :/


>
>> Also, "devl_" implicates that it should be called with devlink instance
>> lock held, so probably devlink_is_registered() would be better.
>
>I'm guessing you realized this isn't correct later on.

From what I see, no need to hold instance mutex for xa mark checking,
alhough I understand why you want the helper to be called with the lock.
Perhaps assert and a little comment would make this clear?


>
>> >+{
>> >+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>> >+}
>> >+EXPORT_SYMBOL_GPL(devl_is_alive);
>> >+
>> >+/**
>> >+ * devlink_try_get() - try to obtain a reference on a devlink instance
>> >+ * @devlink: instance to reference
>> >+ *
>> >+ * Obtain a reference on a devlink instance. A reference on a devlink instance
>> >+ * only implies that it's safe to take the instance lock. It does not imply
>> >+ * that the instance is registered, use devl_is_alive() after taking
>> >+ * the instance lock to check registration status.
>> >+ */  
>> 
>> This comment is not related to the patch, should be added in a separate
>> one.
>
>The point of adding this comment is to say that one has to use
>devl_is_alive() after accessing an instance by reference.
>It is very much in the right patch.

Gotha! My mistake, sorry.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Tue, Jan 03, 2023 at 12:16:30AM CET, kuba@kernel.org wrote:
>On Mon, 2 Jan 2023 15:57:24 +0100 Jiri Pirko wrote:
>> Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:
>> >Always check under the instance lock whether the devlink instance
>> >is still / already registered.
>> >
>> >This is a no-op for the most part, as the unregistration path currently
>> >waits for all references. On the init path, however, we may temporarily
>> >open up a race with netdev code, if netdevs are registered before the
>> >devlink instance. This is temporary, the next change fixes it, and this
>> >commit has been split out for the ease of review.
>> >
>> >Note that in case of iterating over sub-objects which have their
>> >own lock (regions and line cards) we assume an implicit dependency
>> >between those objects existing and devlink unregistration.
>
>> >diff --git a/net/devlink/basic.c b/net/devlink/basic.c
>> >index 5f33d74eef83..6b18e70a39fd 100644
>> >--- a/net/devlink/basic.c
>> >+++ b/net/devlink/basic.c
>> >@@ -2130,6 +2130,9 @@ static int devlink_nl_cmd_linecard_get_dumpit(struct sk_buff *msg,
>> > 		int idx = 0;
>> > 
>> > 		mutex_lock(&devlink->linecards_lock);
>> >+		if (!devl_is_alive(devlink))
>> >+			goto next_devlink;  
>> 
>> Thinking about this a bit more, things would be cleaner if reporters and
>> linecards are converted to rely on instance lock as well. I don't see a
>> good reason for a separate lock in both cases, really.
>
>We had discussion before, I'm pretty sure.
>IIRC you said that mlx4's locking prevents us from using the instance
>lock for regions.

Yeah, let me check it out again. For the linecards, that could be done.
Let me take care of these.


>
>> Also, we could introduce devlinks_xa_for_each_registered_get_lock()
>> iterator that would lock the instance as well right away to avoid
>> this devl_is_alive() dance on multiple places when you iterate devlinks.
>
>That's what I started with, but the ability to factor our the
>unlock/put on error paths made the callback approach much cleaner.
>And after using the callback for all the dumps there's only a couple
>places which would use devlinks_xa_for_each_registered_get_lock().

I see. Okay.


>
>> >@@ -12218,7 +12232,8 @@ void devlink_compat_running_version(struct devlink *devlink,
>> > 		return;
>> > 
>> > 	devl_lock(devlink);  
>> 
>> How about to have a helper, something like devl_lock_alive() (or
>> devl_lock_registered() with the naming scheme I suggest in the other
>> thread)? Then you can do:
>> 
>> 	if (!devl_lock_alive(devlink))
>> 		return;
>> 	__devlink_compat_running_version(devlink, buf, len);
>> 	devl_unlock(devlink);
>
>I guess aesthetic preference.
>
>If I had the cycles I'd make devlink_try_get() return a wrapped type
>
>struct devlink_ref {
>	struct devlink *devlink;
>};
>
>which one would have to pass to devl_lock_from_ref() or some such:
>
>struct devlink *devl_lock_from_ref(struct devlink_ref dref)
>{
>	if (!dref.devlink)
>		return NULL;
>	devl_lock(dref.devlink);
>	if (devl_lock_alive(dref.devlink))
>		return dref.devlink;
>	devl_unlock(dref.devlink);
>	return NULL;
>}
>
>But the number of calls to devl_is_alive() is quite small after all
>the cleanup, so I don't think the extra helpers are justified at this
>point. "Normal coders" should not be exposed to any of the lifetime
>details, not when coding the drivers, not when adding typical devlink
>features/subobjects.

Fair point.

Re: [RFC net-next 06/10] devlink: don't require setting features before registration

[Jiri Pirko]

Tue, Jan 03, 2023 at 12:32:54AM CET, kuba@kernel.org wrote:
>On Mon, 2 Jan 2023 15:24:47 -0800 Jakub Kicinski wrote:
>> On Mon, 2 Jan 2023 16:25:18 +0100 Jiri Pirko wrote:
>> > Sat, Dec 17, 2022 at 02:19:49AM CET, kuba@kernel.org wrote:  
>> > >Requiring devlink_set_features() to be run before devlink is
>> > >registered is overzealous. devlink_set_features() itself is
>> > >a leftover from old workarounds which were trying to prevent
>> > >initiating reload before probe was complete.    
>> > 
>> > Wouldn't it be better to remove this entirely? I don't think it is
>> > needed anymore.  
>> 
>> I think you're right. Since users don't have access to the instance
>> before it's registered - this flag can have no impact.
>
>Let's leave this for a separate follow up, mlx5 needs a bit more work.
>It sets the feature conditionally.

Okay, let me take care of that.

Re: [RFC net-next 10/10] netdevsim: register devlink instance before sub-objects

[Jiri Pirko]

Tue, Jan 03, 2023 at 12:25:46AM CET, kuba@kernel.org wrote:
>On Mon, 2 Jan 2023 14:34:42 +0100 Jiri Pirko wrote:
>> Sat, Dec 17, 2022 at 02:19:53AM CET, kuba@kernel.org wrote:
>> >Move the devlink instance registration up so that all the sub-object
>> >manipulation happens on a valid instance.  
>> 
>> I wonder, why don't you squash patch 8 to this one and make 1 move, to
>> the fina destination?
>
>I found the squashed version a lot harder to review.

I'm puzzled. Both patches move calls to devl_register/unregister().
The first one moves it, the second one moves it a bit more. What's
making the squashed patch hard to review?

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Sat, Dec 17, 2022 at 02:19:47AM CET, kuba@kernel.org wrote:

[...]


>+bool devl_is_alive(struct devlink *devlink)
>+{
>+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
>+}
>+EXPORT_SYMBOL_GPL(devl_is_alive);

Why is this exported? Drivers should not use this, as you said.

Re: [RFC net-next 01/10] devlink: bump the instance index directly when iterating

[Jakub Kicinski]

On Tue, 3 Jan 2023 08:35:16 +0100 Jiri Pirko wrote:
> >> To be honest, this "we something" desctiption style makes things quite
> >> hard to understand. Could you please rephrase it to actually talk
> >> about the entities in code?  
> >
> >Could you be more specific? I'm probably misunderstanding but it sounds
> >to me like you're asking me to describe what I'm doing rather than
> >the background and motivation.  
> 
> "We" is us, you me and other people. It is weird to talk about the code
> and what there as "we do", "we use" etc. It's quite confusing as the
> reader it not able to distinguish if you are talking about code or
> people (developers in this case). I'm just askin if it is possible
> to make the descriptions easier to understand.

Actually thinking about it again, this patch is not strictly
necessary. I'll rewrite the commit message to just say it's a
simplification based on the fact we don't have multi-index entries.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

On Tue, 3 Jan 2023 10:26:12 +0100 Jiri Pirko wrote:
> >> Why "alive"? To be consistent with the existing terminology, how about
> >> to name it devl_is_registered()?  
> >
> >I dislike the similarity to device_is_registered() which has very
> >different semantics. I prefer alive.  
> 
> Interesting. Didn't occur to me to look into device.h when reading
> devlink.c code. I mean, is device_register() behaviour in sync with
> devlink_register?
> 
> Your alive() helper is checking "register mark". It's an odd and unneded
> inconsistency in newly added code :/

Alright.

> >> Also, "devl_" implicates that it should be called with devlink instance
> >> lock held, so probably devlink_is_registered() would be better.  
> >
> >I'm guessing you realized this isn't correct later on.  
> 
> From what I see, no need to hold instance mutex for xa mark checking,
> alhough I understand why you want the helper to be called with the lock.
> Perhaps assert and a little comment would make this clear?

I'll add the comment. The assert would have to OR holding the subobject
locks. Is that what you had in mind?

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jakub Kicinski]

On Tue, 3 Jan 2023 13:26:30 +0100 Jiri Pirko wrote:
> >+bool devl_is_alive(struct devlink *devlink)
> >+{
> >+	return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> >+}
> >+EXPORT_SYMBOL_GPL(devl_is_alive);  
> 
> Why is this exported? Drivers should not use this, as you said.

I'll make it a static inline in the internal header.

Re: [RFC net-next 10/10] netdevsim: register devlink instance before sub-objects

[Jakub Kicinski]

On Tue, 3 Jan 2023 10:51:14 +0100 Jiri Pirko wrote:
> >> I wonder, why don't you squash patch 8 to this one and make 1 move, to
> >> the fina destination?  
> >
> >I found the squashed version a lot harder to review.  
> 
> I'm puzzled. Both patches move calls to devl_register/unregister().
> The first one moves it, the second one moves it a bit more. What's
> making the squashed patch hard to review?

Ah, I thought you meant patch 7, sorry.
This one matters less, I'll squash.

Re: [RFC net-next 04/10] devlink: always check if the devlink instance is registered

[Jiri Pirko]

Wed, Jan 04, 2023 at 03:49:59AM CET, kuba@kernel.org wrote:
>On Tue, 3 Jan 2023 10:26:12 +0100 Jiri Pirko wrote:
>> >> Why "alive"? To be consistent with the existing terminology, how about
>> >> to name it devl_is_registered()?  
>> >
>> >I dislike the similarity to device_is_registered() which has very
>> >different semantics. I prefer alive.  
>> 
>> Interesting. Didn't occur to me to look into device.h when reading
>> devlink.c code. I mean, is device_register() behaviour in sync with
>> devlink_register?
>> 
>> Your alive() helper is checking "register mark". It's an odd and unneded
>> inconsistency in newly added code :/
>
>Alright.
>
>> >> Also, "devl_" implicates that it should be called with devlink instance
>> >> lock held, so probably devlink_is_registered() would be better.  
>> >
>> >I'm guessing you realized this isn't correct later on.  
>> 
>> From what I see, no need to hold instance mutex for xa mark checking,
>> alhough I understand why you want the helper to be called with the lock.
>> Perhaps assert and a little comment would make this clear?
>
>I'll add the comment. The assert would have to OR holding the subobject
>locks. Is that what you had in mind?

Ah right, the subobject locks. That will go away I'm sure. Yes, that is
what I had in mind. After that, we can put assert here.