linux-kernel - Re: [PATCH v2] tick/broadcast: Do not set oneshot_mask except was

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZD1Jfr7TGq/4gabB@localhost.localdomain>
Date:   Mon, 17 Apr 2023 15:28:30 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Victor Hassan <victor@...winnertech.com>, fweisbec@...il.com,
        mingo@...nel.org, jindong.yue@....com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tick/broadcast: Do not set oneshot_mask except
 was_periodic was true

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.