Currently, functions like synchronize_srcu() do not have lockdep
annotations resembling those of other write-side locking primitives.
Such annotations might look as follows:
lock_acquire();
lock_release();
Such annotations would tell lockdep that synchronize_srcu() acts like
an empty critical section that waits for other (read-side) critical
sections to finish. This would definitely catch some deadlock, but
as pointed out by Paul Mckenney [1], this could also introduce false
positives because of irq-safe/unsafe detection. Of course, there are
tricks could help with this:
might_sleep(); // Existing statement in __synchronize_srcu().
if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
local_irq_disable();
lock_acquire();
lock_release();
local_irq_enable();
}
But it would be better for lockdep to provide a separate annonation for
functions like synchronize_srcu(), so that people won't need to repeat
the ugly tricks above.
Therefore introduce lock_sync(), which is simply an lock+unlock
pair with no irq safe/unsafe deadlock check. This works because the
to-be-annontated functions do not create real critical sections, and
there is therefore no way that irq can create extra dependencies.
[1]: https://lore.kernel.org/lkml/20180412021233.ewncg5jjuzjw3x62@tardis/
Signed-off-by: Boqun Feng <[email protected]>
Acked-by: Waiman Long <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Signed-off-by: Boqun Feng <[email protected]>
---
include/linux/lockdep.h | 5 +++++
kernel/locking/lockdep.c | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 1023f349af71..14d9dbedc6c1 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -268,6 +268,10 @@ extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
extern void lock_release(struct lockdep_map *lock, unsigned long ip);
+extern void lock_sync(struct lockdep_map *lock, unsigned int subclass,
+ int read, int check, struct lockdep_map *nest_lock,
+ unsigned long ip);
+
/* lock_is_held_type() returns */
#define LOCK_STATE_UNKNOWN -1
#define LOCK_STATE_NOT_HELD 0
@@ -554,6 +558,7 @@ do { \
#define lock_map_acquire_read(l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_)
#define lock_map_acquire_tryread(l) lock_acquire_shared_recursive(l, 0, 1, NULL, _THIS_IP_)
#define lock_map_release(l) lock_release(l, _THIS_IP_)
+#define lock_map_sync(l) lock_sync(l, 0, 0, 1, NULL, _THIS_IP_)
#ifdef CONFIG_PROVE_LOCKING
# define might_lock(lock) \
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 50d4863974e7..36430cf8e407 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -5693,6 +5693,40 @@ void lock_release(struct lockdep_map *lock, unsigned long ip)
}
EXPORT_SYMBOL_GPL(lock_release);
+/*
+ * lock_sync() - A special annotation for synchronize_{s,}rcu()-like API.
+ *
+ * No actual critical section is created by the APIs annotated with this: these
+ * APIs are used to wait for one or multiple critical sections (on other CPUs
+ * or threads), and it means that calling these APIs inside these critical
+ * sections is potential deadlock.
+ *
+ * This annotation acts as an acqurie+release anontation pair with hardirqoff
+ * being 1. Since there's no critical section, no interrupt can create extra
+ * dependencies "inside" the annotation, hardirqoff == 1 allows us to avoid
+ * false positives.
+ */
+void lock_sync(struct lockdep_map *lock, unsigned subclass, int read,
+ int check, struct lockdep_map *nest_lock, unsigned long ip)
+{
+ unsigned long flags;
+
+ if (unlikely(!lockdep_enabled()))
+ return;
+
+ raw_local_irq_save(flags);
+ check_flags(flags);
+
+ lockdep_recursion_inc();
+ __lock_acquire(lock, subclass, 0, read, check, 1, nest_lock, ip, 0, 0);
+
+ if (__lock_release(lock, ip))
+ check_chain_key(current);
+ lockdep_recursion_finish();
+ raw_local_irq_restore(flags);
+}
+EXPORT_SYMBOL_GPL(lock_sync);
+
noinstr int lock_is_held_type(const struct lockdep_map *lock, int read)
{
unsigned long flags;
--
2.39.2
On Mon, Mar 20, 2023 at 10:06:14AM -0700, Davidlohr Bueso wrote:
> On Thu, 16 Mar 2023, Boqun Feng wrote:
>
> > +/*
> > + * lock_sync() - A special annotation for synchronize_{s,}rcu()-like API.
> > + *
> > + * No actual critical section is created by the APIs annotated with this: these
> > + * APIs are used to wait for one or multiple critical sections (on other CPUs
> > + * or threads), and it means that calling these APIs inside these critical
> > + * sections is potential deadlock.
> > + *
> > + * This annotation acts as an acqurie+release anontation pair with hardirqoff
> ^acquire
Good eye! Applied locally.
Regards,
Boqun
On Thu, 16 Mar 2023, Boqun Feng wrote:
>+/*
>+ * lock_sync() - A special annotation for synchronize_{s,}rcu()-like API.
>+ *
>+ * No actual critical section is created by the APIs annotated with this: these
>+ * APIs are used to wait for one or multiple critical sections (on other CPUs
>+ * or threads), and it means that calling these APIs inside these critical
>+ * sections is potential deadlock.
>+ *
>+ * This annotation acts as an acqurie+release anontation pair with hardirqoff
^acquire