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

[Steven Rostedt]

On Fri, 08 Apr 2022 12:37:40 +0200
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> 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.

And this is why it's a RFC patch. ;-)

I figured shrinking that mask was going to break something, but I
wanted to know if this approach was sound before digging further into
making it correct.

> 
> 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?


That could work. Thanks.

> 
> > @@ -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.

I mention exactly this later in the thread with my discussions with
Guenter. But it was for the flag check, and not the NULL function.

> 
> That needs a corresponding change in add_timer_on() plus:
> 
> -		fn = timer->function;
> +		fn = READ_ONCE(timer->function);
> 
> in expire_timers().

OK.

> 
> > +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.

Yeah, I was going to try a helper function for both, but wasn't sure I
would mess up something subtle, so for the RFC patch, I figured I just
implement it completely.

> 
> Thanks,
> 
>         tglx
> ---
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h

I'll take a look at the below later today.

Thanks for the review.

-- Steve

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