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;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Tue, 20 Jun 2023 18:10:09 +1000
Message-Id: <CTHBPQOQXTFE.29F2F08A55JGP@wheely>
Subject: Re: +
 lazy-tlb-fix-hotplug-exit-race-with-mmu_lazy_tlb_shootdown.patch added to
 mm-hotfixes-unstable branch
From:   "Nicholas Piggin" <npiggin@gmail.com>
To:     "Thomas Gleixner" <tglx@linutronix.de>,
        <linux-kernel@vger.kernel.org>, <mm-commits@vger.kernel.org>,
        <torvalds@linux-foundation.org>, <peterz@infradead.org>,
        <akpm@linux-foundation.org>
References: <20230525205253.E2FAEC433EF@smtp.kernel.org>
 <87v8fv86bh.ffs@tglx> <CTH8ZNDRMLEZ.2WS18IS2E38T7@wheely>
 <87jzvyprsw.ffs@tglx>
In-Reply-To: <87jzvyprsw.ffs@tglx>
Precedence: bulk

On Tue Jun 20, 2023 at 4:32 PM AEST, Thomas Gleixner wrote:
> On Tue, Jun 20 2023 at 16:02, Nicholas Piggin wrote:
> > On Sun Jun 11, 2023 at 5:29 AM AEST, Thomas Gleixner wrote:
> >> /*
> >>  * Invoked on the outgoing CPU in context of the CPU hotplug thread
> >>  * after ensuring that there are no user space tasks left on the CPU.
> >>  *
> >>  * If there is a lazy mm in use on the hotplug thread, drop it and
> >>  * switch to init_mm.
> >>  *
> >>  * The reference count on init_mm is dropped in finish_cpu().
> >>  */
> >> static void sched_force_init_mm(void)
> >> {
> >>
> >> No?
> >
> > It could be done in many places. Peter touched it last and it's
> > been in the tree since prehistoric times.
>
> That's an argument for slapping it into some randomly chosen place and
> be done with it, right?

Ah, not exactly but I did misremember, I did have to change where I
added it so it does turn out to be more arbitrary than I thought.

If it goes in wait empty then than state is no longer wait empty, it's
wait empty and switch mm. I can put it there, should I also rename the
state?

> >> > +/*
> >> > + * After the CPU is offline, double check that it was previously sw=
itched to
> >> > + * init_mm. This call can be removed because the condition is caugh=
t in
> >> > + * finish_cpu() as well.
> >>
> >> So why adding it in the first place?
> >>
> >> The changelog mumbles something about reducing churn, but I fail to se=
e
> >> that reduction. This adds 10 lines of pointless code and comments for
> >> zero value.
> >
> > Not sure what you're talking about. The patch didn't add it. Removing i=
t
> > requires removing it from all archs, which is the churn.
>
> Sure. That's left as an exercise for others, right?

No, I'm telling you why I left the function in. Did not want to gate a
fix behind herding the arch cats. I will send the trivial patch to arch
trees after it's upstream. This is how such API changes are typically
done.

Thanks,
Nick