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

[Jiri Pirko <jiri@resnulli.us>]

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.