Re: BUG: soft lockup in smp_call_function

[Hillf Danton <hdanton@sina.com>]

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);
 
--