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

Thomas Gleixner
2022-04-08T10:37:47Z

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 *),