On Thu, Apr 14, 2022 at 3:16 AM Peter Zijlstra <pet...@infradead.org> wrote:
>
> On Wed, Apr 13, 2022 at 03:01:21PM -0700, Dan Williams wrote:
>
> > > That's not an obvious conclusion; lockdep has lots of funny annotations,
> > > subclasses is just one.
> > >
> > > I think the big new development in lockdep since that time with Alan
> > > Stern is that lockdep now has support for dynamic keys; that is lock
> > > keys in heap memory (as opposed to static storage).
> >
> > Ah, I was not aware of that, that should allow for deep cleanups of
> > this proposal.
>
> > > If you want lockdep validation for one (or more) dev->mutex instances,
> > > why not pull them out of the no_validate class and use the normal
> > > locking?
> >
> > Sounds perfect, just didn't know how to do that with my current
> > understanding of how to communicate this to lockdep.
> >
> > >
> > > This is all quite insane.
> >
> > Yes, certainly in comparison to your suggestion on the next patch.
> > That looks much more sane, and even better I think it allows for
> > optional lockdep validation without even needing to touch
> > include/linux/device.h.
>
> Right, so lockdep has:
>
> - classes, based off of lock_class_key address;
>
> * lock_class_key should be static storage; except now we also have:
>
> lockdep_{,un}register_key()
>
> which allows dynamic memory (aka. heap) to be used for classes,
> important to note that lockdep memory usage is still static storage
> because the memory allocators use locks too. So if you register too
> many dynamic keys, you'll run out of lockdep memory etc.. so be
> careful.
>
> * things like mutex_init() have a static lock_class_key per site
> and hence every lock initialized by the same code ends up in the
> same class by default.
>
> * can be trivially changed at any time, assuming the lock isn't held,
> using lockdep_set_class*() family.
>
> (extensively used all over the kernel, for example by the vfs to
> give each filesystem type their own locking order rules)
>
> * lockdep_set_no_validate_class() is a magical variant of
> lockdep_set_class() that sets a magical lock_class_key.
Golden, thanks Peter!
>
> * can be changed while held using lock_set_class()
One more sanity check... So in driver subsystems there are cases where
a device on busA hosts a topology on busB. When that happens there's a
need to set the lock class late in a driver since busA knows nothing
about the locking rules of busB. Since the device has a longer
lifetime than a driver when the driver exits it must set dev->mutex
back to the novalidate class, otherwise it sets up a use after free of
the static lock_class_key. I came up with this and it seems to work,
just want to make sure I'm properly using the lock_set_class() API and
it is ok to transition back and forth from the novalidate case:
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 990b6670222e..32673e1a736d 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -405,6 +405,29 @@ struct cxl_nvdimm_bridge
*cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd);
#define __mock static
#endif
+#ifdef CONFIG_PROVE_LOCKING
+static inline void cxl_lock_reset_class(void *_dev)
+{
+ struct device *dev = _dev;
+
+ lock_set_class(&dev->mutex.dep_map, "__lockdep_no_validate__",
+ &__lockdep_no_validate__, 0, _THIS_IP_);
+}
+
+static inline int cxl_lock_set_class(struct device *dev, const char *name,
+ struct lock_class_key *key)
+{
+ lock_set_class(&dev->mutex.dep_map, name, key, 0, _THIS_IP_);
+ return devm_add_action_or_reset(dev, cxl_lock_reset_class, dev);
+}
+#else
+static inline int cxl_lock_set_class(struct device *dev, const char *name,
+ struct lock_class_key *key)
+{
+ return 0;
+}
+#endif
+
#ifdef CONFIG_PROVE_CXL_LOCKING
enum cxl_lock_class {
CXL_ANON_LOCK,
> ( from a lockdep pov it unlocks the held stack,
> changes the class of your lock, and re-locks the
> held stack, now with a different class nesting ).
>
> Be carefule! It doesn't forget the old nesting order, so you
> can trivally generate cycles.
>
> - subclasses, basically distinct addresses inside above mentioned
> lock_class_key object, limited to 8. Normally used with
> *lock_nested() family of functions. Typically used to lock multiple
> instances of a single lock class where there is defined order between
> instances (see for instance: double_rq_lock()).
>
> - nest_lock; eg. mutex_lock_nest_lock(), which allows expressing things
> like: multiple locks of class A can be taken in any order, provided
> we hold lock B.
>
> With many of these, it's possible to get it wrong and annotate real
> deadlocks away, so be careful :-)
Noted.
