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

[Jiri Pirko <jiri@resnulli.us>]

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

[...]