Re: [PATCH 3/6] posix-cpu-timers: Force next_expiration recalc after timer deletion

[Peter Zijlstra]

On Fri, Jun 04, 2021 at 01:31:56PM +0200, Frederic Weisbecker wrote:
> A timer deletion only dequeues the timer but it doesn't shutdown
> the related costly process wide cputimer counter and the tick dependency.
> 
> The following code snippet keeps this overhead around for one week after
> the timer deletion:
> 
> 	void trigger_process_counter(void)
> 	{
> 		timer_t id;
> 		struct itimerspec val = { };
> 
> 		val.it_value.tv_sec = 604800;
> 		timer_create(CLOCK_PROCESS_CPUTIME_ID, NULL, &id);
> 		timer_settime(id, 0, &val, NULL);
> 		timer_delete(id);
> 	}
> 
> Make sure the next target's tick recalculates the nearest expiration and
> clears the process wide counter and tick dependency if necessary.

> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 132fd56fb1cd..bb1f862c785e 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -405,6 +405,33 @@ static int posix_cpu_timer_create(struct k_itimer *new_timer)
>  	return 0;
>  }
>  
> +/*
> + * Dequeue the timer and reset the base if it was its earliest expiration.
> + * It makes sure the next tick recalculates the base next expiration so we
> + * don't keep the costly process wide cputime counter around for a random
> + * amount of time, along with the tick dependency.
> + */
> +static void disarm_timer(struct k_itimer *timer, struct task_struct *p)
> +{
> +	struct cpu_timer *ctmr = &timer->it.cpu;
> +	struct posix_cputimer_base *base;
> +	int clkidx;
> +
> +	if (!cpu_timer_dequeue(ctmr))
> +		return;
> +
> +	clkidx = CPUCLOCK_WHICH(timer->it_clock);
> +
> +	if (CPUCLOCK_PERTHREAD(timer->it_clock))
> +		base = p->posix_cputimers.bases + clkidx;
> +	else
> +		base = p->signal->posix_cputimers.bases + clkidx;
> +
> +	if (cpu_timer_getexpires(ctmr) == base->nextevt)
> +		base->nextevt = 0;
> +}

OK, so check_process_timers() unconditionally recomputes ->nextevt in
collect_posix_cputimers() provided ->timers_active. It also clears
->timers_active if it finds none are left. This recompute is before all
actual consumers of ->nextevt, with one exception.

This will loose the update of ->nextevt in arm_timer(), if one were to
happen between this and check_process_timers(), but afaict that has no
ill effect. Still that might warrant a comment somewhere.

Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>