From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:14
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.
Instead the devlink instance refcount will only ensure that
the memory of the instance is not freed. All places which acquire
access to devlink instances via a reference must check that the
instance is still registered under the instance lock.
This fixes the problem of the netdev code accessing devlink
instances before they are registered.
RFC: https://lore.kernel.org/all/20221217011953.152487-1-kuba@kernel.org/
- rewrite the cover letter
- rewrite the commit message for patch 1
- un-export and rename devl_is_alive
- squash the netdevsim patches
Jakub Kicinski (9):
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
devlink: allow registering parameters after the instance
netdevsim: rename a label
netdevsim: move devlink registration under the instance lock
drivers/net/netdevsim/dev.c | 15 +++--
include/net/devlink.h | 2 +
net/devlink/core.c | 121 ++++++++++++++++--------------------
net/devlink/devl_internal.h | 28 ++++-----
net/devlink/leftover.c | 64 ++++++++++++-------
net/devlink/netlink.c | 19 ++++--
6 files changed, 137 insertions(+), 112 deletions(-)
--
2.38.1
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:16
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).
Reviewed-by: Jiri Pirko <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
net/devlink/core.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:16
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().
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
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(-)
@@ -83,21 +83,10 @@ struct devlink *__must_check devlink_try_get(struct devlink *devlink)returnNULL;}-staticvoid__devlink_put_rcu(structrcu_head*head)-{-structdevlink*devlink=container_of(head,structdevlink,rcu);--complete(&devlink->comp);-}-voiddevlink_put(structdevlink*devlink){if(refcount_dec_and_test(&devlink->refcount))-/* Make sure unregister operation that may await the completion-*isunblockedonlyafterallusersareaftertheendof-*RCUgraceperiod.-*/-call_rcu(&devlink->rcu,__devlink_put_rcu);+kfree_rcu(devlink,rcu);}structdevlink*devlinks_xa_find_get(structnet*net,unsignedlong*indexp)
@@ -110,13 +99,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)if(!devlink)gotounlock;-/* In case devlink_unregister() was already called and "unregistering"-*markwasset,donotallowtogetadevlinkreferencehere.-*Thispreventslive-lockofdevlink_unregister()waitforcompletion.-*/-if(xa_get_mark(&devlinks,*indexp,DEVLINK_UNREGISTERING))-gotonext;-if(!devlink_try_get(devlink))gotonext;if(!net_eq(devlink_net(devlink),net)){
@@ -152,37 +134,48 @@ void devlink_set_features(struct devlink *devlink, u64 features)EXPORT_SYMBOL_GPL(devlink_set_features);/**-*devlink_register-Registerdevlinkinstance-*-*@devlink:devlink+*devl_register-Registerdevlinkinstance+*@devlink:devlink*/-voiddevlink_register(structdevlink*devlink)+intdevl_register(structdevlink*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);++return0;+}+EXPORT_SYMBOL_GPL(devl_register);++voiddevlink_register(structdevlink*devlink)+{+devl_lock(devlink);+devl_register(devlink);+devl_unlock(devlink);}EXPORT_SYMBOL_GPL(devlink_register);/**-*devlink_unregister-Unregisterdevlinkinstance-*-*@devlink:devlink+*devl_unregister-Unregisterdevlinkinstance+*@devlink:devlink*/-voiddevlink_unregister(structdevlink*devlink)+voiddevl_unregister(structdevlink*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);++voiddevlink_unregister(structdevlink*devlink)+{+devl_lock(devlink);+devl_unregister(devlink);+devl_unlock(devlink);}EXPORT_SYMBOL_GPL(devlink_unregister);
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:18
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>
---
net/devlink/core.c | 19 +++++++++++++++----
net/devlink/devl_internal.h | 8 ++++++++
net/devlink/leftover.c | 35 +++++++++++++++++++++++++++++------
net/devlink/netlink.c | 10 ++++++++--
4 files changed, 60 insertions(+), 12 deletions(-)
@@ -86,6 +86,14 @@ extern struct genl_family devlink_nl_family;structdevlink*devlinks_xa_find_get(structnet*net,unsignedlong*indexp);+staticinlinebooldevl_is_registered(structdevlink*devlink)+{+/* To prevent races the caller must hold the instance lock+*oranotherlocktakenduringunregistration.+*/+returnxa_get_mark(&devlinks,devlink->index,DEVLINK_REGISTERED);+}+/* Netlink */#define DEVLINK_NL_FLAG_NEED_PORT BIT(0)#define DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT BIT(1)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:19
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(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:21
xa_find_after() is designed to handle multi-index entries correctly.
If a xarray has two entries one which spans indexes 0-3 and one at
index 4 xa_find_after(0) will return the entry at index 4.
Having to juggle the two callbacks, however, is unnecessary in case
of the devlink xarray, as there is 1:1 relationship with indexes.
Always use xa_find() and increment the index manually.
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(-)
@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp,*Thispreventslive-lockofdevlink_unregister()waitforcompletion.*/if(xa_get_mark(&devlinks,*indexp,DEVLINK_UNREGISTERING))-gotoretry;+gotonext;-/* For a possible retry, the xa_find_after() should be always used */-xa_find_fn=xa_find_after;if(!devlink_try_get(devlink))-gotoretry;+gotonext;if(!net_eq(devlink_net(devlink),net)){devlink_put(devlink);-gotoretry;+gotonext;}unlock:rcu_read_unlock();returndevlink;-}--structdevlink*-devlinks_xa_find_get_first(structnet*net,unsignedlong*indexp)-{-returndevlinks_xa_find_get(net,indexp,xa_find);-}-structdevlink*-devlinks_xa_find_get_next(structnet*net,unsignedlong*indexp)-{-returndevlinks_xa_find_get(net,indexp,xa_find_after);+next:+(*indexp)++;+gotoretry;}/**
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:22
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/devl_internal.h | 3 ++-
net/devlink/leftover.c | 7 +++----
net/devlink/netlink.c | 9 ++++++---
3 files changed, 11 insertions(+), 8 deletions(-)
@@ -6314,12 +6314,10 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb,start_offset=state->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))returnPTR_ERR(devlink);-devl_lock(devlink);-if(!attrs[DEVLINK_ATTR_REGION_NAME]){NL_SET_ERR_MSG(cb->extack,"No region name provided");err=-EINVAL;
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:24
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/leftover.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:35
err_dl_unregister should unregister the devlink instance.
Looks like renaming it was missed in one of the reshufflings.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
drivers/net/netdevsim/dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 06:34:39
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(-)
Fri, Jan 06, 2023 at 07:33:54AM CET, kuba@kernel.org wrote:
xa_find_after() is designed to handle multi-index entries correctly.
If a xarray has two entries one which spans indexes 0-3 and one at
index 4 xa_find_after(0) will return the entry at index 4.
Having to juggle the two callbacks, however, is unnecessary in case
of the devlink xarray, as there is 1:1 relationship with indexes.
Always use xa_find() and increment the index manually.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Fri, Jan 06, 2023 at 07:33:56AM CET, kuba@kernel.org wrote:
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>
Fri, Jan 06, 2023 at 07:33:57AM CET, kuba@kernel.org wrote:
quoted hunk
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>
---
net/devlink/core.c | 19 +++++++++++++++----
net/devlink/devl_internal.h | 8 ++++++++
net/devlink/leftover.c | 35 +++++++++++++++++++++++++++++------
net/devlink/netlink.c | 10 ++++++++--
4 files changed, 60 insertions(+), 12 deletions(-)
}
EXPORT_SYMBOL_GPL(devl_unlock);
+/**
+ * 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_registered() 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 +309,12 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp);
+static inline bool devl_is_registered(struct devlink *devlink)
+{
+ /* To prevent races the caller must hold the instance lock
+ * or another lock taken during unregistration.
+ */
+ return xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
+}
+
/* Netlink */
#define DEVLINK_NL_FLAG_NEED_PORT BIT(0)
#define DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT BIT(1)
Fri, Jan 06, 2023 at 07:33:58AM CET, kuba@kernel.org 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.
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().
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Fri, Jan 06, 2023 at 07:33:59AM 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.
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
I have a patch prepared to remove this entirely. Until then, this is
fine.
Reviewed-by: Jiri Pirko <redacted>
Fri, Jan 06, 2023 at 07:34:01AM CET, kuba@kernel.org wrote:
err_dl_unregister should unregister the devlink instance.
Looks like renaming it was missed in one of the reshufflings.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Fri, Jan 06, 2023 at 07:34:00AM CET, kuba@kernel.org wrote:
quoted hunk
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/leftover.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
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_registered(devlink))
+ return;
This helper would be nice to use on other places as well.
Like devlink_trap_group_notify(), devlink_trap_notify() and others. I
will take care of that in a follow-up.
Reviewed-by: Jiri Pirko <redacted>
Hello:
This series was applied to netdev/net-next.git (master)
by David S. Miller [off-list ref]:
On Thu, 5 Jan 2023 22:33:53 -0800 you wrote:
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.
Instead the devlink instance refcount will only ensure that
the memory of the instance is not freed. All places which acquire
access to devlink instances via a reference must check that the
instance is still registered under the instance lock.
[...]
Fri, Jan 06, 2023 at 07:34:02AM CET, kuba@kernel.org wrote:
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.
This sentense sounds a bit confusing to me. "to be registered" does not
sound correct.
"Core now waits for the instance lock to be unlocked before executing
access the instance" perhaps would be more accurate?
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
One way or another. Code looks fine:
Reviewed-by: Jiri Pirko <redacted>
Fri, Jan 06, 2023 at 07:33:53AM CET, kuba@kernel.org wrote:
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.
Instead the devlink instance refcount will only ensure that
the memory of the instance is not freed. All places which acquire
access to devlink instances via a reference must check that the
instance is still registered under the instance lock.
This fixes the problem of the netdev code accessing devlink
instances before they are registered.
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2023-01-06 17:06:35
On 1/5/2023 10:33 PM, Jakub Kicinski wrote:
quoted hunk
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>
---
net/devlink/core.c | 19 +++++++++++++++----
net/devlink/devl_internal.h | 8 ++++++++
net/devlink/leftover.c | 35 +++++++++++++++++++++++++++++------
net/devlink/netlink.c | 10 ++++++++--
4 files changed, 60 insertions(+), 12 deletions(-)
@@ -86,6 +86,14 @@ extern struct genl_family devlink_nl_family;structdevlink*devlinks_xa_find_get(structnet*net,unsignedlong*indexp);+staticinlinebooldevl_is_registered(structdevlink*devlink)+{+/* To prevent races the caller must hold the instance lock+*oranotherlocktakenduringunregistration.+*/
Why not just lockdep_assert here on the instance lock? I guess this
comment implies that another lock could be used instead but it seems
weird to allow that? I guess because of things like the linecards_lock
as opposed to the instance lock?
Thanks,
Jake
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2023-01-06 17:08:15
On 1/5/2023 10:33 PM, Jakub Kicinski wrote:
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.
Instead the devlink instance refcount will only ensure that
the memory of the instance is not freed. All places which acquire
access to devlink instances via a reference must check that the
instance is still registered under the instance lock.
This fixes the problem of the netdev code accessing devlink
instances before they are registered.
RFC: https://lore.kernel.org/all/20221217011953.152487-1-kuba@kernel.org/
- rewrite the cover letter
- rewrite the commit message for patch 1
- un-export and rename devl_is_alive
- squash the netdevsim patches
Jakub Kicinski (9):
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
devlink: allow registering parameters after the instance
netdevsim: rename a label
netdevsim: move devlink registration under the instance lock
drivers/net/netdevsim/dev.c | 15 +++--
include/net/devlink.h | 2 +
net/devlink/core.c | 121 ++++++++++++++++--------------------
net/devlink/devl_internal.h | 28 ++++-----
net/devlink/leftover.c | 64 ++++++++++++-------
net/devlink/netlink.c | 19 ++++--
6 files changed, 137 insertions(+), 112 deletions(-)
The whole series looks good to me. It looks like Jiri also has some
planned followups that will clean some of this up even more, which is great.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-06 21:20:25
On Fri, 6 Jan 2023 09:03:18 -0800 Jacob Keller wrote:
quoted
+static inline bool devl_is_registered(struct devlink *devlink)
+{
+ /* To prevent races the caller must hold the instance lock
+ * or another lock taken during unregistration.
+ */
Why not just lockdep_assert here on the instance lock? I guess this
comment implies that another lock could be used instead but it seems
weird to allow that? I guess because of things like the linecards_lock
as opposed to the instance lock?
Yup, as discussed on the RFC - removing the regions lock specifically
is quite tricky.
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_registered(devlink))
+ return;
This helper would be nice to use on other places as well.
Like devlink_trap_group_notify(), devlink_trap_notify() and others. I
will take care of that in a follow-up.
Alternatively we could reorder back to registering sub-objects
after the instance and not have to worry about re-sending
notifications :S
Fri, Jan 06, 2023 at 10:19:34PM CET, kuba@kernel.org wrote:
On Fri, 6 Jan 2023 09:03:18 -0800 Jacob Keller wrote:
quoted
quoted
+static inline bool devl_is_registered(struct devlink *devlink)
+{
+ /* To prevent races the caller must hold the instance lock
+ * or another lock taken during unregistration.
+ */
Why not just lockdep_assert here on the instance lock? I guess this
comment implies that another lock could be used instead but it seems
weird to allow that? I guess because of things like the linecards_lock
as opposed to the instance lock?
Yup, as discussed on the RFC - removing the regions lock specifically
is quite tricky.
I will submit in a jiff. Will add the assert here as a part
of the patchset.
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_registered(devlink))
+ return;
This helper would be nice to use on other places as well.
Like devlink_trap_group_notify(), devlink_trap_notify() and others. I
will take care of that in a follow-up.
Alternatively we could reorder back to registering sub-objects
after the instance and not have to worry about re-sending
notifications :S
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_registered(devlink))
+ return;
This helper would be nice to use on other places as well.
Like devlink_trap_group_notify(), devlink_trap_notify() and others. I
will take care of that in a follow-up.
Alternatively we could reorder back to registering sub-objects
after the instance and not have to worry about re-sending
notifications :S
I did find it convenient to be able to do both pre and post-registering,
but of the two I'd definitely prefer doing it post-registering, as that
makes it easier to handle/allow more dynamic sub-objects.
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_registered(devlink))
+ return;
This helper would be nice to use on other places as well.
Like devlink_trap_group_notify(), devlink_trap_notify() and others. I
will take care of that in a follow-up.
Alternatively we could reorder back to registering sub-objects
after the instance and not have to worry about re-sending
notifications :S
I did find it convenient to be able to do both pre and post-registering,
but of the two I'd definitely prefer doing it post-registering, as that
makes it easier to handle/allow more dynamic sub-objects.
I'm confused. You want to register objects after instance register?
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-10 20:22:30
On Tue, 10 Jan 2023 17:35:35 +0100 Jiri Pirko wrote:
quoted
I did find it convenient to be able to do both pre and post-registering,
but of the two I'd definitely prefer doing it post-registering, as that
makes it easier to handle/allow more dynamic sub-objects.
I'm confused. You want to register objects after instance register?
Tue, Jan 10, 2023 at 09:22:22PM CET, kuba@kernel.org wrote:
On Tue, 10 Jan 2023 17:35:35 +0100 Jiri Pirko wrote:
quoted
quoted
I did find it convenient to be able to do both pre and post-registering,
but of the two I'd definitely prefer doing it post-registering, as that
makes it easier to handle/allow more dynamic sub-objects.
I'm confused. You want to register objects after instance register?
Fri, Jan 06, 2023 at 07:34:00AM CET, kuba@kernel.org wrote:
quoted hunk
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/leftover.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
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_registered(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);
Hmm, params list is not protected by any lock. The protection it used
was that it is static after registration. You changed it but didn't add
the lock. All param register/unregister functions need to be renamed to
devl_* and assert instance lock. I will fix this.
[..]
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-11 16:46:02
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2023-01-11 21:29:23
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-12 07:07:52
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Thanks
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
From: Jakub Kicinski <kuba@kernel.org> Date: 2023-01-12 19:27:06
On Thu, 12 Jan 2023 09:07:43 +0200 Leon Romanovsky wrote:
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
To be clear my preference would be to always construct the three from
the root. Register the main instance, then sub-objects. I mean - you
tried forcing the opposite order and it only succeeded in 90-something
percent of cases. There's always special cases.
I don't understand your concern about user experience here. We have
notifications for each sub-object. Plus I think drivers should hold
the instance lock throughout the probe routine. I don't see a scenario
in which registering the main instance first would lead to retry/sleep
hacks in user space, do you? I'm talking about devlink and the subobjs
we have specifically.
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-12 20:06:54
On Thu, Jan 12, 2023 at 03:59:53PM +0100, Jiri Pirko wrote:
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
quoted
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
"reload" is triggered by me explicitly and I will get success/fail result
at the end. There is no much meaning in subobject notifications during
that operation.
Thanks
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-12 20:40:39
On Thu, Jan 12, 2023 at 11:20:21AM -0800, Jakub Kicinski wrote:
On Thu, 12 Jan 2023 09:07:43 +0200 Leon Romanovsky wrote:
quoted
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
To be clear my preference would be to always construct the three from
the root. Register the main instance, then sub-objects. I mean - you
tried forcing the opposite order and it only succeeded in 90-something
percent of cases. There's always special cases.
I don't understand your concern about user experience here. We have
notifications for each sub-object. Plus I think drivers should hold
the instance lock throughout the probe routine. I don't see a scenario
in which registering the main instance first would lead to retry/sleep
hacks in user space, do you? I'm talking about devlink and the subobjs
we have specifically.
The term "dynamic object addition" means for me what driver authors will
be able to add objects anytime in lifetime of the driver. I'm pretty sure
that once you allow that, we will see zoo here. Over time, you will get
everything from .probe() to workqueues. The latter caused me to write
about retry/sleep hacks.
If you success to force everyone to add objects in .probe() only, it
will be very close to what I tried to achieve.
Thanks
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2023-01-12 22:44:56
On 1/12/2023 12:09 PM, Leon Romanovsky wrote:
On Thu, Jan 12, 2023 at 11:20:21AM -0800, Jakub Kicinski wrote:
quoted
On Thu, 12 Jan 2023 09:07:43 +0200 Leon Romanovsky wrote:
quoted
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
To be clear my preference would be to always construct the three from
the root. Register the main instance, then sub-objects. I mean - you
tried forcing the opposite order and it only succeeded in 90-something
percent of cases. There's always special cases.
Right. I think its easier to simply require devlink to be registered first.
quoted
I don't understand your concern about user experience here. We have
notifications for each sub-object. Plus I think drivers should hold
the instance lock throughout the probe routine. I don't see a scenario
in which registering the main instance first would lead to retry/sleep
hacks in user space, do you? I'm talking about devlink and the subobjs
we have specifically.
The term "dynamic object addition" means for me what driver authors will
be able to add objects anytime in lifetime of the driver. I'm pretty sure
that once you allow that, we will see zoo here. Over time, you will get
everything from .probe() to workqueues. The latter caused me to write
about retry/sleep hacks.
If you success to force everyone to add objects in .probe() only, it
will be very close to what I tried to achieve.
Thanks
Yea. I was initially thinking of something like that, but I've convinced
myself that its a bad idea. The only "dynamic" objects (added after the
initialization phase of devlink) should be those which are triggered via
user space request (i.e. "devlink port add").
Thanks,
Jake
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-13 07:00:32
On Thu, Jan 12, 2023 at 02:44:43PM -0800, Jacob Keller wrote:
On 1/12/2023 12:09 PM, Leon Romanovsky wrote:
quoted
On Thu, Jan 12, 2023 at 11:20:21AM -0800, Jakub Kicinski wrote:
quoted
On Thu, 12 Jan 2023 09:07:43 +0200 Leon Romanovsky wrote:
quoted
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
To be clear my preference would be to always construct the three from
the root. Register the main instance, then sub-objects. I mean - you
tried forcing the opposite order and it only succeeded in 90-something
percent of cases. There's always special cases.
Back then, we had only one special case - netdevsim. I still think that
all recent complexity that was brought to the devlink could be avoided
if we would change netdevsim to behave as HW driver (remove sysfs).
Right. I think its easier to simply require devlink to be registered first.
devlink_register() is no more than a fancy way to say to the world: "I'm
ready to accept commands". Right now, when the need_lock flag is removed
from all devlink commands, we can place devlink_register() at any place.
quoted
quoted
I don't understand your concern about user experience here. We have
notifications for each sub-object. Plus I think drivers should hold
the instance lock throughout the probe routine. I don't see a scenario
in which registering the main instance first would lead to retry/sleep
hacks in user space, do you? I'm talking about devlink and the subobjs
we have specifically.
The term "dynamic object addition" means for me what driver authors will
be able to add objects anytime in lifetime of the driver. I'm pretty sure
that once you allow that, we will see zoo here. Over time, you will get
everything from .probe() to workqueues. The latter caused me to write
about retry/sleep hacks.
If you success to force everyone to add objects in .probe() only, it
will be very close to what I tried to achieve.
Thanks
Yea. I was initially thinking of something like that, but I've convinced
myself that its a bad idea. The only "dynamic" objects (added after the
initialization phase of devlink) should be those which are triggered via
user space request (i.e. "devlink port add").
Thu, Jan 12, 2023 at 08:58:58PM CET, leon@kernel.org wrote:
On Thu, Jan 12, 2023 at 03:59:53PM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
quoted
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
"reload" is triggered by me explicitly and I will get success/fail result
at the end. There is no much meaning in subobject notifications during
that operation.
Definitelly not. User would trigger reload, however another entity
(systemd for example) would listen to the notifications and react
if necessary.
Fri, Jan 13, 2023 at 07:45:46AM CET, leon@kernel.org wrote:
On Thu, Jan 12, 2023 at 02:44:43PM -0800, Jacob Keller wrote:
quoted
On 1/12/2023 12:09 PM, Leon Romanovsky wrote:
quoted
On Thu, Jan 12, 2023 at 11:20:21AM -0800, Jakub Kicinski wrote:
quoted
On Thu, 12 Jan 2023 09:07:43 +0200 Leon Romanovsky wrote:
quoted
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
To be clear my preference would be to always construct the three from
the root. Register the main instance, then sub-objects. I mean - you
tried forcing the opposite order and it only succeeded in 90-something
percent of cases. There's always special cases.
Back then, we had only one special case - netdevsim. I still think that
all recent complexity that was brought to the devlink could be avoided
if we would change netdevsim to behave as HW driver (remove sysfs).
quoted
Right. I think its easier to simply require devlink to be registered first.
devlink_register() is no more than a fancy way to say to the world: "I'm
ready to accept commands". Right now, when the need_lock flag is removed
from all devlink commands, we can place devlink_register() at any place.
quoted
quoted
quoted
I don't understand your concern about user experience here. We have
notifications for each sub-object. Plus I think drivers should hold
the instance lock throughout the probe routine. I don't see a scenario
in which registering the main instance first would lead to retry/sleep
hacks in user space, do you? I'm talking about devlink and the subobjs
we have specifically.
The term "dynamic object addition" means for me what driver authors will
be able to add objects anytime in lifetime of the driver. I'm pretty sure
that once you allow that, we will see zoo here. Over time, you will get
everything from .probe() to workqueues. The latter caused me to write
about retry/sleep hacks.
If you success to force everyone to add objects in .probe() only, it
will be very close to what I tried to achieve.
Thanks
Yea. I was initially thinking of something like that, but I've convinced
myself that its a bad idea. The only "dynamic" objects (added after the
initialization phase of devlink) should be those which are triggered via
user space request (i.e. "devlink port add").
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-15 08:36:09
On Fri, Jan 13, 2023 at 08:50:33AM +0100, Jiri Pirko wrote:
Thu, Jan 12, 2023 at 08:58:58PM CET, leon@kernel.org wrote:
quoted
On Thu, Jan 12, 2023 at 03:59:53PM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
quoted
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
"reload" is triggered by me explicitly and I will get success/fail result
at the end. There is no much meaning in subobject notifications during
that operation.
Definitelly not. User would trigger reload, however another entity
(systemd for example) would listen to the notifications and react
if necessary.
Listen yes, however it is not clear if notification sequence should
mimic fini/init flow.
Thanks
Sun, Jan 15, 2023 at 09:35:57AM CET, leon@kernel.org wrote:
On Fri, Jan 13, 2023 at 08:50:33AM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:58:58PM CET, leon@kernel.org wrote:
quoted
On Thu, Jan 12, 2023 at 03:59:53PM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
quoted
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
"reload" is triggered by me explicitly and I will get success/fail result
at the end. There is no much meaning in subobject notifications during
that operation.
Definitelly not. User would trigger reload, however another entity
(systemd for example) would listen to the notifications and react
if necessary.
Listen yes, however it is not clear if notification sequence should
mimic fini/init flow.
Well, it makes sense to me. Why do you think it should not?
From: Leon Romanovsky <leon@kernel.org> Date: 2023-01-16 11:25:20
On Mon, Jan 16, 2023 at 11:33:05AM +0100, Jiri Pirko wrote:
Sun, Jan 15, 2023 at 09:35:57AM CET, leon@kernel.org wrote:
quoted
On Fri, Jan 13, 2023 at 08:50:33AM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:58:58PM CET, leon@kernel.org wrote:
quoted
On Thu, Jan 12, 2023 at 03:59:53PM +0100, Jiri Pirko wrote:
quoted
Thu, Jan 12, 2023 at 08:07:43AM CET, leon@kernel.org wrote:
quoted
On Wed, Jan 11, 2023 at 01:29:03PM -0800, Jacob Keller wrote:
quoted
On 1/11/2023 8:45 AM, Jakub Kicinski wrote:
quoted
On Wed, 11 Jan 2023 10:32:13 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
I'm confused. You want to register objects after instance register?
+1, I think it's an anti-pattern.
Could you elaborate a bit please?
Mixing registering sub-objects before and after the instance is a bit
of an anti-pattern. Easy to introduce bugs during reload and reset /
error recovery. I thought that's what you were saying as well.
I was thinking of a case where an object is dynamic and might get added
based on events occurring after the devlink was registered.
But the more I think about it the less that makes sense. What events
would cause a whole subobject to be registerd which we wouldn't already
know about during initialization of devlink?
We do need some dynamic support because situations like "add port" will
add a port and then the ports subresources after the main devlink, but I
think that is already supported well and we'd add the port sub-resources
at the same time as the port.
But thinking more on this, there isn't really another good example since
we'd register things like health reporters, regions, resources, etc all
during initialization. Each of these sub objects may have dynamic
portions (ex: region captures, health events, etc) but the need for the
object should be known about during init time if its supported by the
device driver.
As a user, I don't want to see any late dynamic object addition which is
not triggered by me explicitly. As it doesn't make any sense to add
various delays per-vendor/kernel in configuration scripts just because
not everything is ready. Users need predictability, lazy addition of
objects adds chaos instead.
Agree with Jakub, it is anti-pattern.
Yeah, but, we have reload. And during reload, instance is still
registered yet the subobject disappear and reappear. So that would be
inconsistent with the init/fini flow.
Perhaps during reload we should emulate complete fini/init notification
flow to the user?
"reload" is triggered by me explicitly and I will get success/fail result
at the end. There is no much meaning in subobject notifications during
that operation.
Definitelly not. User would trigger reload, however another entity
(systemd for example) would listen to the notifications and react
if necessary.
Listen yes, however it is not clear if notification sequence should
mimic fini/init flow.
Well, it makes sense to me. Why do you think it should not?
After all this years, I still don't understand the mandate of devlink
reload. It doesn't load/unload driver completely and as such not really
performs probe/remove sequences. There is no requirement from the driver
to do anything even close to fini/init too.
Sometimes, devlink reload behaves as fini/init, but not always.
This is why I'm not sure.
Thanks