Re: [PATCH v2] tick/broadcast: Do not set oneshot_mask except was_periodic was true

[Frederic Weisbecker]

Le Sat, Apr 15, 2023 at 11:01:51PM +0200, Thomas Gleixner a écrit :
> > * CPU 1 stop its tick, next event is in one hour. It calls
> >   tick_broadcast_enter() and goes to sleep.
> 
> So there is already a broadcast device installed, right?

Yes

> 
> > * CPU 1 gets an interrupt that enqueues a new timer expiring in the next jiffy
> >   (note it's not yet actually programmed in the tick device)
> > * CPU 1 call tick_broadcast_exit().
> > * CPU 0 registers new broadcast device and sets CPU 1 in tick_broadcast_oneshot_mask
> 
> This lacks an explanation why CPU0 sets CPU1 in that mask. It does not
> _set_ it explicitely, only implicitely by ORing the periodic broadcast
> cpumask over.
> 
> Now the question is why is CPU1 set in the periodic broadcast mask when
> the CPU already switched over to NOHZ mode?
> 
> That needs to be explained too.

I probably got confused with that tick_broadcast_mask, so it's only set for
periodic broadcast? Should it be renamed to tick_periodic_broadcast_mask to
disambiguate my eternal confusion?

> 
> > * CPU 0 runs the broadcast callback, sees that the next timer for CPU 1
> >   is in one hour (because the recently enqueued timer for CPU 1 hasn't been programmed
> >   yet), so it programs the broadcast to that 1 hour deadline.
> > * CPU 1 runs tick_nohz_idle_stop_tick() which eventually writes and program
> >   dev->next_event to next jiffy
> > * CPU 1 runs into cpuidle_enter_state(), and tick_broadcast_enter() is ignored because
> >   the CPU is already in tick_broadcast_oneshot_mask, so the dev->next_event
> >   change isn't propagated to broadcast.
> > * CPU 1 goes to sleep for 1 hour.
> 
> Also please use tabular style to explain the parallel events as
> explained in the documentation.

Yeah my bad I asked Victor to integrate that scenario that popped out
of me misunderstanding that code. Not even mentioning the form.

Now to review your proposal.

Thanks.