Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Apr 2023 15:28:30 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Victor Hassan <victor@allwinnertech.com>, fweisbec@gmail.com,
        mingo@kernel.org, jindong.yue@nxp.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] tick/broadcast: Do not set oneshot_mask except
 was_periodic was true
Message-ID: <ZD1Jfr7TGq/4gabB@localhost.localdomain>
References: <20230412003425.11323-1-victor@allwinnertech.com>
 <87sfd0yi4g.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87sfd0yi4g.ffs@tglx>
Precedence: bulk

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.