| Message ID | 20220701095926.1191660-3-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: devlink: devl_* cosmetic fixes | expand |
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?
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) 2) lock/unlock for devlink->lock is done here w/o helpers as well 3) there is really no gain of using helper here. 4) rtnl_mutex is probably not good example, it has a lot of ancient history behind it.
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"
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"
On Mon, 4 Jul 2022 08:19:17 +0200 Jiri Pirko wrote: > 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. As I said - my understanding is that we want to discourage (driver) authors from wrapping locks in lock/unlock helpers. Which was very fashionable at some point (IDK why, but it seem to have mostly gone away?). If the helper already exists I think consistency wins and we should use it.
Tue, Jul 05, 2022 at 04:58:39AM CEST, kuba@kernel.org wrote: >On Mon, 4 Jul 2022 08:19:17 +0200 Jiri Pirko wrote: >> 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. > >As I said - my understanding is that we want to discourage (driver) >authors from wrapping locks in lock/unlock helpers. Which was very >fashionable at some point (IDK why, but it seem to have mostly gone >away?). > >If the helper already exists I think consistency wins and we should use >it. Okay, will send a patch to convert devlink.c to use these helpers. Thanks!
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); if (WARN_ON(devlink_port->devlink_rate)) return -EBUSY; @@ -10224,7 +10224,7 @@ void devl_rate_leaf_destroy(struct devlink_port *devlink_port) { struct devlink_rate *devlink_rate = devlink_port->devlink_rate; - devl_assert_locked(devlink_port->devlink); + lockdep_assert_held(&devlink_port->devlink->lock); if (!devlink_rate) return; @@ -10270,7 +10270,7 @@ void devl_rate_nodes_destroy(struct devlink *devlink) static struct devlink_rate *devlink_rate, *tmp; const struct devlink_ops *ops = devlink->ops; - devl_assert_locked(devlink); + lockdep_assert_held(&devlink->lock); list_for_each_entry(devlink_rate, &devlink->rate_list, list) { if (!devlink_rate->parent)