Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Apr 07 2022 at 16:17, Steven Rostedt wrote:
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index fda13c9d1256..cc76ab0659f3 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -67,11 +67,12 @@ struct timer_list {
>  #define TIMER_DEFERRABLE	0x00080000
>  #define TIMER_PINNED		0x00100000
>  #define TIMER_IRQSAFE		0x00200000
> +#define TIMER_FREED		0x00400000
>  #define TIMER_INIT_FLAGS	(TIMER_DEFERRABLE | TIMER_PINNED | TIMER_IRQSAFE)
> -#define TIMER_ARRAYSHIFT	22
> -#define TIMER_ARRAYMASK		0xFFC00000
> +#define TIMER_ARRAYSHIFT	23
> +#define TIMER_ARRAYMASK		0xFF800000

#define LVL_BITS	6
#define LVL_SIZE	(1UL << LVL_BITS)

#if HZ > 100
# define LVL_DEPTH	9
# else
# define LVL_DEPTH	8
#endif

#define WHEEL_SIZE	(LVL_SIZE * LVL_DEPTH)

WHEEL_SIZE(HZ > 100) = (1 << 6) * 9 = 576
max_index(HZ > 100) = 575 = 0x23f

0x23f << ARRAY_SHIFT = 0x11f800000

Q: Does 0x11f800000 fit into 32bit?
A: No, and we are not making flags 64bit just because.

If you want a bit you have to steal it from the CPUMASK field, but you
could simply set timer->function to NULL, so you can spare extra checks,
no?

> @@ -966,6 +966,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, unsigned int option
>  
>  	BUG_ON(!timer->function);
>  
> +	WARN_ON(timer->flags & TIMER_FREED);

So this would become:

-  	BUG_ON(!timer->function);
+	if (WARN_ON(!timer->function))
+		return -EBROKEN;

You really want to return here and not proceed.

That needs a corresponding change in add_timer_on() plus:

-		fn = timer->function;
+		fn = READ_ONCE(timer->function);

in expire_timers().

> +int del_timer_free(struct timer_list *timer)
> +{
> +	struct timer_base *base;
> +	unsigned long flags;
> +	int ret;
> +
> +	debug_assert_init(timer);
> +
> +	for (;;) {
> +		ret = -1;
> +		base = lock_timer_base(timer, &flags);
> +
> +		if (base->running_timer != timer)
> +			ret = detach_if_pending(timer, base, true);
> +
> +		if (ret >= 0) {
> +			timer->flags |= TIMER_FREED;
> +			raw_spin_unlock_irqrestore(&base->lock, flags);
> +			break;
> +		}
> +
> +		raw_spin_unlock_irqrestore(&base->lock, flags);
> +
> +		del_timer_wait_running(timer);
> +		cpu_relax();
> +	}
> +
> +	return ret;
> +}

So this reimplements del_timer_sync() just slightly different for no
reason. Something like the uncompiled below.

Thanks,

        tglx
---
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -183,12 +183,17 @@ extern int timer_reduce(struct timer_lis
 extern void add_timer(struct timer_list *timer);
 
 extern int try_to_del_timer_sync(struct timer_list *timer);
+extern int __del_timer_sync(struct timer_list *timer, bool free);
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
-  extern int del_timer_sync(struct timer_list *timer);
-#else
-# define del_timer_sync(t)		del_timer(t)
-#endif
+static inline int del_timer_sync(struct timer_list *timer)
+{
+	return __del_timer_sync(timer, false);
+}
+
+static inline int del_timer_sync_free(struct timer_list *timer)
+{
+	return __del_timer_sync(timer, true);
+}
 
 #define del_singleshot_timer_sync(t) del_timer_sync(t)
 
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -964,7 +964,8 @@ static inline int
 	unsigned int idx = UINT_MAX;
 	int ret = 0;
 
-	BUG_ON(!timer->function);
+	if (WARN_ON(!timer->function))
+		return -EINVAL;
 
 	/*
 	 * This is a common optimization triggered by the networking code - if
@@ -1140,7 +1141,8 @@ EXPORT_SYMBOL(timer_reduce);
  */
 void add_timer(struct timer_list *timer)
 {
-	BUG_ON(timer_pending(timer));
+	if (WARN_ON(timer_pending(timer)))
+		return;
 	__mod_timer(timer, timer->expires, MOD_TIMER_NOTPENDING);
 }
 EXPORT_SYMBOL(add_timer);
@@ -1157,7 +1159,8 @@ void add_timer_on(struct timer_list *tim
 	struct timer_base *new_base, *base;
 	unsigned long flags;
 
-	BUG_ON(timer_pending(timer) || !timer->function);
+	if (WARN_ON(timer_pending(timer) || !timer->function))
+		return;
 
 	new_base = get_timer_cpu_base(timer->flags, cpu);
 
@@ -1213,14 +1216,7 @@ int del_timer(struct timer_list *timer)
 }
 EXPORT_SYMBOL(del_timer);
 
-/**
- * try_to_del_timer_sync - Try to deactivate a timer
- * @timer: timer to delete
- *
- * This function tries to deactivate a timer. Upon successful (ret >= 0)
- * exit the timer is not queued and the handler is not running on any CPU.
- */
-int try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer, boot free)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1232,11 +1228,25 @@ int try_to_del_timer_sync(struct timer_l
 
 	if (base->running_timer != timer)
 		ret = detach_if_pending(timer, base, true);
+	if (free)
+		timer->function = NULL;
 
 	raw_spin_unlock_irqrestore(&base->lock, flags);
 
 	return ret;
 }
+
+/**
+ * try_to_del_timer_sync - Try to deactivate a timer
+ * @timer: timer to delete
+ *
+ * This function tries to deactivate a timer. Upon successful (ret >= 0)
+ * exit the timer is not queued and the handler is not running on any CPU.
+ */
+int try_to_del_timer_sync(struct timer_list *timer)
+{
+	return __try_to_del_timer_sync(timer, false);
+}
 EXPORT_SYMBOL(try_to_del_timer_sync);
 
 #ifdef CONFIG_PREEMPT_RT
@@ -1312,7 +1322,6 @@ static inline void timer_sync_wait_runni
 static inline void del_timer_wait_running(struct timer_list *timer) { }
 #endif
 
-#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT_RT)
 /**
  * del_timer_sync - deactivate a timer and wait for the handler to finish.
  * @timer: the timer to be deactivated
@@ -1349,7 +1358,7 @@ static inline void del_timer_wait_runnin
  *
  * The function returns whether it has deactivated a pending timer or not.
  */
-int del_timer_sync(struct timer_list *timer)
+int __del_timer_sync(struct timer_list *timer, bool free)
 {
 	int ret;
 
@@ -1379,7 +1388,7 @@ int del_timer_sync(struct timer_list *ti
 		lockdep_assert_preemption_enabled();
 
 	do {
-		ret = try_to_del_timer_sync(timer);
+		ret = __try_to_del_timer_sync(timer, free);
 
 		if (unlikely(ret < 0)) {
 			del_timer_wait_running(timer);
@@ -1389,8 +1398,7 @@ int del_timer_sync(struct timer_list *ti
 
 	return ret;
 }
-EXPORT_SYMBOL(del_timer_sync);
-#endif
+EXPORT_SYMBOL(__del_timer_sync);
 
 static void call_timer_fn(struct timer_list *timer,
 			  void (*fn)(struct timer_list *),