Re: [PATCH 5/6] posix-cpu-timers: Force next expiration recalc after early timer firing

[Frederic Weisbecker]

On Wed, Jun 16, 2021 at 11:42:53AM +0200, Peter Zijlstra wrote:
> On Fri, Jun 04, 2021 at 01:31:58PM +0200, Frederic Weisbecker wrote:
> > diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> > index 0b5715c8db04..d8325a906314 100644
> > --- a/kernel/time/posix-cpu-timers.c
> > +++ b/kernel/time/posix-cpu-timers.c
> > @@ -405,6 +405,21 @@ static int posix_cpu_timer_create(struct k_itimer *new_timer)
> >  	return 0;
> >  }
> >  
> > +static void __disarm_timer(struct k_itimer *timer, struct task_struct *p,
> > +			   u64 old_expires)
> > +{
> > +	int clkidx = CPUCLOCK_WHICH(timer->it_clock);
> > +	struct posix_cputimer_base *base;
> > +
> > +	if (CPUCLOCK_PERTHREAD(timer->it_clock))
> > +		base = p->posix_cputimers.bases + clkidx;
> > +	else
> > +		base = p->signal->posix_cputimers.bases + clkidx;
> > +
> > +	if (old_expires == base->nextevt)
> > +		base->nextevt = 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
> > @@ -415,24 +430,14 @@ static void disarm_timer(struct k_itimer *timer, struct task_struct *p)
> >  {
> >  	struct cpu_timer *ctmr = &timer->it.cpu;
> >  	u64 old_expires = cpu_timer_getexpires(ctmr);
> > -	struct posix_cputimer_base *base;
> >  	bool queued;
> > -	int clkidx;
> >  
> >  	queued = cpu_timer_dequeue(ctmr);
> >  	cpu_timer_setexpires(ctmr, 0);
> >  	if (!queued)
> >  		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 (old_expires == base->nextevt)
> > -		base->nextevt = 0;
> > +	__disarm_timer(timer, p, old_expires);
> >  }
> >  
> >  
> > @@ -686,8 +691,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,
> >  			u64 exp = bump_cpu_timer(timer, val);
> >  
> >  			if (val < exp) {
> > -				old_expires = exp - val;
> > -				old->it_value = ns_to_timespec64(old_expires);
> > +				old->it_value = ns_to_timespec64(exp - val);
> >  			} else {
> >  				old->it_value.tv_nsec = 1;
> >  				old->it_value.tv_sec = 0;
> > @@ -748,9 +752,28 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,
> >  		 * accumulate more time on this clock.
> >  		 */
> >  		cpu_timer_fire(timer);
> > +
> > +		sighand = lock_task_sighand(p, &flags);
> > +		if (sighand == NULL)
> > +			goto out;
> > +		if (!cpu_timer_queued(&timer->it.cpu)) {
> > +			/*
> > +			 * Disarm the previous timer to deactivate the tick
> > +			 * dependency and process wide cputime counter if
> > +			 * necessary.
> > +			 */
> > +			__disarm_timer(timer, p, old_expires);
> > +			/*
> > +			 * If the previous timer was deactivated, we might have
> > +			 * just started the process wide cputime counter. Make
> > +			 * sure we poke the tick to deactivate it then.
> > +			 */
> > +			if (!old_expires && !CPUCLOCK_PERTHREAD(timer->it_clock))
> > +				p->signal->posix_cputimers.bases[clkid].nextevt = 0;
> > +		}
> > +		unlock_task_sighand(p, &flags);
> >  	}
> 
> I'm thinking this is a better fix than patch #2. AFAICT you can now go
> back to unconditionally doing start, and then if we fire it early, we'll
> disarm the thing.
> 
> That would avoid the disconnect between the start condition and the fire
> condition.

Right but the drawback is that we unconditionally start the threadgroup
counter while initializing the timer to 0 (deactivated).

Then in the next tick at least one thread will need to lock the sighand
and re-evaluate the whole list.