Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jiri Pirko <jiri@resnulli.us>]

Sat, Jul 02, 2022 at 09:29:46PM CEST, kuba@kernel.org wrote:
>On Sat, 2 Jul 2022 17:58:06 +0200 Jiri Pirko wrote:
>> Fri, Jul 01, 2022 at 06:33:16PM CEST, kuba@kernel.org wrote:
>> >On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:  
>> >> In devlink.c there is direct access to whole struct devlink so there is
>> >> no need to use helper. So obey the customs and work with lock directly
>> >> avoiding helpers which might obfuscate things a bit.  
>> >  
>> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> >> index 25b481dd1709..a7477addbd59 100644
>> >> --- a/net/core/devlink.c
>> >> +++ b/net/core/devlink.c
>> >> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
>> >>  	struct devlink *devlink = devlink_port->devlink;
>> >>  	struct devlink_rate *devlink_rate;
>> >>  
>> >> -	devl_assert_locked(devlink_port->devlink);
>> >> +	lockdep_assert_held(&devlink_port->devlink->lock);  
>> >
>> >I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
>> >in rtnetlink.c?  
>> 
>> Well:
>> 
>> 1) it's been a long time policy not to use helpers for locks if not
>>    needed. There reason is that the reader has easier job in seeing what
>>    the code is doing. And here, it is not needed to use helper (we can
>>    access the containing struct)
>
>AFAIU the policy is not to _create_ helpers for locks for no good
>reason. If the helper already exists it's better to consistently use
>it.
>
>> 2) lock/unlock for devlink->lock is done here w/o helpers as well
>
>Existing code, I didn't want to cause major code churn until the
>transition is finished.
>
>> 3) there is really no gain of using helper here.
>
>Shorter, easier to type and remember, especially if the author is
>already using the exported assert in the driver.
>
>> 4) rtnl_mutex is probably not good example, it has a lot of ancient
>>    history behind it.
>
>It's our main lock so we know it best. Do you have other examples?
>
>Look, I don't really care, I just want to make sure we document the
>rules of engagement clearly for everyone to see and uniformly enforce. 
>So we either need to bash out exactly what we want (and I think our
>views differ) or you should switch the commit message to say "I feel
>like" rather than referring to "customs" 😁

Jakub, I don't really care. If you say we should do it differently, I
will do it differently. I just want the use to be consistent. From the
earlier reactions of DaveM on such helpers, I got an impression we don't
want them if possible. If this is no longer true, I'm fine with it. Just
tell me what I should do.

Thanks!