On Wed, 13 Sep 2023 23:30:23 +0900 Tetsuo Handa wrote:
> On 2023/09/13 20:07, Hillf Danton wrote:
> >
> > cpu1 cpu4 (see below)
> > ==== ====
> > drm_crtc_vblank_off __run_hrtimer
> > spin_lock_irq(&dev->event_lock);
> > ...
> > drm_handle_vblank
> > hrtimer_cancel spin_lock_irqsave(&dev->event_lock, irqflags);
> >
> >
> > Deadlock should have been reported instead provided the lockdep_map in
> > struct timer_list were added also to hrtimer, so it is highly appreciated
> > if Tetsuo or Thomas adds it before 6.8 or 6.10.
>
> Not me. ;-)
>
> Since hrtimer_cancel() retries forever until lock_hrtimer_base() succeeds,
> we want to add a lockdep annotation into hrtimer_cancel() so that we can
> detect this type of deadlock?
Yes, you are right.
The diff below is my two cents (only for thoughts).
--- x/include/linux/timer.h
+++ y/include/linux/timer.h
@@ -124,6 +124,9 @@ struct hrtimer {
u8 is_rel;
u8 is_soft;
u8 is_hard;
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+#endif
};
/**
@@ -369,33 +372,65 @@ static inline void hrtimer_cancel_wait_r
/* Exported timer functions: */
/* Initialize timers: */
-extern void hrtimer_init(struct hrtimer *timer, clockid_t which_clock,
- enum hrtimer_mode mode);
-extern void hrtimer_init_sleeper(struct hrtimer_sleeper *sl, clockid_t clock_id,
- enum hrtimer_mode mode);
+extern void hrtimer_init_key(struct hrtimer *timer, clockid_t which_clock,
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key);
+extern void hrtimer_init_sleeper_key(struct hrtimer_sleeper *sl, clockid_t clock_id,
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key);
+#ifdef CONFIG_LOCKDEP
+#define hrtimer_init(t, c, m) \
+ do { \
+ static struct lock_class_key __key; \
+ hrtimer_init_key(t, c, m, #t, &__key); \
+ } while (0)
+
+#define hrtimer_init_sleeper(s, c, m) \
+ do { \
+ static struct lock_class_key __key; \
+ hrtimer_init_sleeper_key(s, c, m, #s, &__key); \
+ } while (0)
+#else
+#define hrtimer_init(t, c, m) \
+ hrtimer_init_key(t, c, m, NULL, NULL)
+
+#define hrtimer_init_sleeper(s, c, m) \
+ hrtimer_init_sleeper_key(s, c, m, NULL, NULL)
+#endif
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
-extern void hrtimer_init_on_stack(struct hrtimer *timer, clockid_t which_clock,
- enum hrtimer_mode mode);
-extern void hrtimer_init_sleeper_on_stack(struct hrtimer_sleeper *sl,
+extern void hrtimer_init_on_stack_key(struct hrtimer *timer, clockid_t which_clock,
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key);
+extern void hrtimer_init_sleeper_on_stack_key(struct hrtimer_sleeper *sl,
clockid_t clock_id,
- enum hrtimer_mode mode);
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key);
+#ifdef CONFIG_LOCKDEP
+ #define hrtimer_init_on_stack(t, c, m) \
+ do { \
+ static struct lock_class_key __key; \
+ hrtimer_init_on_stack_key(t, c, m, #t, &__key); \
+ } while (0)
+ #define hrtimer_init_sleeper_on_stack(s, c, m) \
+ do { \
+ static struct lock_class_key __key; \
+ hrtimer_init_sleeper_on_stack_key(s, c, m, #s, &__key); \
+ } while (0)
+#else
+ #define hrtimer_init_on_stack(t, c, m) \
+ hrtimer_init_on_stack_key(t, c, m, NULL, NULL)
+ #define hrtimer_init_sleeper_on_stack(s, c, m) \
+ hrtimer_init_sleeper_on_stack_key(s, c, m, NULL, NULL)
+#endif
extern void destroy_hrtimer_on_stack(struct hrtimer *timer);
#else
-static inline void hrtimer_init_on_stack(struct hrtimer *timer,
- clockid_t which_clock,
- enum hrtimer_mode mode)
-{
- hrtimer_init(timer, which_clock, mode);
-}
+#define hrtimer_init_on_stack(t, c, m) \
+ hrtimer_init(t, c, m)
-static inline void hrtimer_init_sleeper_on_stack(struct hrtimer_sleeper *sl,
- clockid_t clock_id,
- enum hrtimer_mode mode)
-{
- hrtimer_init_sleeper(sl, clock_id, mode);
-}
+#define hrtimer_init_sleeper_on_stack(s, c, m) \
+ hrtimer_init_sleeper(s, c, m)
static inline void destroy_hrtimer_on_stack(struct hrtimer *timer) { }
#endif
--- x/kernel/time/hrtimer.c
+++ y/kernel/time/hrtimer.c
@@ -428,22 +428,26 @@ static inline void debug_hrtimer_deactiv
static void __hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
enum hrtimer_mode mode);
-void hrtimer_init_on_stack(struct hrtimer *timer, clockid_t clock_id,
- enum hrtimer_mode mode)
+void hrtimer_init_on_stack_key(struct hrtimer *timer, clockid_t clock_id,
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key)
{
debug_object_init_on_stack(timer, &hrtimer_debug_descr);
__hrtimer_init(timer, clock_id, mode);
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
EXPORT_SYMBOL_GPL(hrtimer_init_on_stack);
static void __hrtimer_init_sleeper(struct hrtimer_sleeper *sl,
clockid_t clock_id, enum hrtimer_mode mode);
-void hrtimer_init_sleeper_on_stack(struct hrtimer_sleeper *sl,
- clockid_t clock_id, enum hrtimer_mode mode)
+void hrtimer_init_sleeper_on_stack_key(struct hrtimer_sleeper *sl,
+ clockid_t clock_id, enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key)
{
debug_object_init_on_stack(&sl->timer, &hrtimer_debug_descr);
__hrtimer_init_sleeper(sl, clock_id, mode);
+ lockdep_init_map(&sl->timer.lockdep_map, name, key, 0);
}
EXPORT_SYMBOL_GPL(hrtimer_init_sleeper_on_stack);
@@ -1439,6 +1443,8 @@ int hrtimer_cancel(struct hrtimer *timer
{
int ret;
+ lock_map_acquire(&timer->lockdep_map);
+ lock_map_release(&timer->lockdep_map);
do {
ret = hrtimer_try_to_cancel(timer);
@@ -1586,11 +1592,12 @@ static void __hrtimer_init(struct hrtime
* but the PINNED bit is ignored as pinning happens
* when the hrtimer is started
*/
-void hrtimer_init(struct hrtimer *timer, clockid_t clock_id,
- enum hrtimer_mode mode)
+void hrtimer_init_key(struct hrtimer *timer, clockid_t clock_id, enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key)
{
debug_init(timer, clock_id, mode);
__hrtimer_init(timer, clock_id, mode);
+ lockdep_init_map(&timer->lockdep_map, name, key, 0);
}
EXPORT_SYMBOL_GPL(hrtimer_init);
@@ -1647,6 +1654,11 @@ static void __run_hrtimer(struct hrtimer
enum hrtimer_restart (*fn)(struct hrtimer *);
bool expires_in_hardirq;
int restart;
+#ifdef CONFIG_LOCKDEP
+ struct lockdep_map lockdep_map;
+
+ lockdep_copy_map(&lockdep_map, &timer->lockdep_map);
+#endif
lockdep_assert_held(&cpu_base->lock);
@@ -1682,7 +1694,9 @@ static void __run_hrtimer(struct hrtimer
trace_hrtimer_expire_entry(timer, now);
expires_in_hardirq = lockdep_hrtimer_enter(timer);
+ lock_map_acquire(&lockdep_map);
restart = fn(timer);
+ lock_map_release(&lockdep_map);
lockdep_hrtimer_exit(expires_in_hardirq);
trace_hrtimer_expire_exit(timer);
@@ -2004,12 +2018,13 @@ static void __hrtimer_init_sleeper(struc
* @clock_id: the clock to be used
* @mode: timer mode abs/rel
*/
-void hrtimer_init_sleeper(struct hrtimer_sleeper *sl, clockid_t clock_id,
- enum hrtimer_mode mode)
+void hrtimer_init_sleeper_key(struct hrtimer_sleeper *sl, clockid_t clock_id,
+ enum hrtimer_mode mode,
+ const char *name, struct lock_class_key *key)
{
debug_init(&sl->timer, clock_id, mode);
__hrtimer_init_sleeper(sl, clock_id, mode);
-
+ lockdep_init_map(&sl->timer.lockdep_map, name, key, 0);
}
EXPORT_SYMBOL_GPL(hrtimer_init_sleeper);
--
