Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 19 Jun 2021 12:53:34 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH 4/8] membarrier: Make the post-switch-mm barrier explicit
To:     Andy Lutomirski <luto@kernel.org>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Rik van Riel <riel@surriel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-mm@kvack.org,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        the arch/x86 maintainers <x86@kernel.org>
References: <cover.1623813516.git.luto@kernel.org>
        <f184d013a255a523116b692db4996c5db2569e86.1623813516.git.luto@kernel.org>
        <1623816595.myt8wbkcar.astroid@bobo.none>
        <YMmpxP+ANG5nIUcm@hirez.programming.kicks-ass.net>
        <617cb897-58b1-8266-ecec-ef210832e927@kernel.org>
        <1623893358.bbty474jyy.astroid@bobo.none>
        <58b949fb-663e-4675-8592-25933a3e361c@www.fastmail.com>
        <c3c7a1cf-1c87-42cc-b2d6-cc2df55e5b57@www.fastmail.com>
        <1623911501.q97zemobmw.astroid@bobo.none>
        <5efaca70-35a0-1ce5-98ff-651a5f153a0a@kernel.org>
In-Reply-To: <5efaca70-35a0-1ce5-98ff-651a5f153a0a@kernel.org>
MIME-Version: 1.0
Message-Id: <1624070824.uyhrzf8zc7.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Andy Lutomirski's message of June 18, 2021 9:49 am:
> On 6/16/21 11:51 PM, Nicholas Piggin wrote:
>> Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:
>>> On Wed, Jun 16, 2021, at 7:57 PM, Andy Lutomirski wrote:
>>>>
>>>>
>>>> On Wed, Jun 16, 2021, at 6:37 PM, Nicholas Piggin wrote:
>>>>> Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:
>>>>>> On 6/16/21 12:35 AM, Peter Zijlstra wrote:
>>>>>>> On Wed, Jun 16, 2021 at 02:19:49PM +1000, Nicholas Piggin wrote:
>>>>>>>> Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:
>>>>>>>>> membarrier() needs a barrier after any CPU changes mm.  There is =
currently
>>>>>>>>> a comment explaining why this barrier probably exists in all case=
s.  This
>>>>>>>>> is very fragile -- any change to the relevant parts of the schedu=
ler
>>>>>>>>> might get rid of these barriers, and it's not really clear to me =
that
>>>>>>>>> the barrier actually exists in all necessary cases.
>>>>>>>>
>>>>>>>> The comments and barriers in the mmdrop() hunks? I don't see what =
is=20
>>>>>>>> fragile or maybe-buggy about this. The barrier definitely exists.
>>>>>>>>
>>>>>>>> And any change can change anything, that doesn't make it fragile. =
My
>>>>>>>> lazy tlb refcounting change avoids the mmdrop in some cases, but i=
t
>>>>>>>> replaces it with smp_mb for example.
>>>>>>>
>>>>>>> I'm with Nick again, on this. You're adding extra barriers for no
>>>>>>> discernible reason, that's not generally encouraged, seeing how ext=
ra
>>>>>>> barriers is extra slow.
>>>>>>>
>>>>>>> Both mmdrop() itself, as well as the callsite have comments saying =
how
>>>>>>> membarrier relies on the implied barrier, what's fragile about that=
?
>>>>>>>
>>>>>>
>>>>>> My real motivation is that mmgrab() and mmdrop() don't actually need=
 to
>>>>>> be full barriers.  The current implementation has them being full
>>>>>> barriers, and the current implementation is quite slow.  So let's tr=
y
>>>>>> that commit message again:
>>>>>>
>>>>>> membarrier() needs a barrier after any CPU changes mm.  There is cur=
rently
>>>>>> a comment explaining why this barrier probably exists in all cases. =
The
>>>>>> logic is based on ensuring that the barrier exists on every control =
flow
>>>>>> path through the scheduler.  It also relies on mmgrab() and mmdrop()=
 being
>>>>>> full barriers.
>>>>>>
>>>>>> mmgrab() and mmdrop() would be better if they were not full barriers=
.  As a
>>>>>> trivial optimization, mmgrab() could use a relaxed atomic and mmdrop=
()
>>>>>> could use a release on architectures that have these operations.
>>>>>
>>>>> I'm not against the idea, I've looked at something similar before (no=
t
>>>>> for mmdrop but a different primitive). Also my lazy tlb shootdown ser=
ies=20
>>>>> could possibly take advantage of this, I might cherry pick it and tes=
t=20
>>>>> performance :)
>>>>>
>>>>> I don't think it belongs in this series though. Should go together wi=
th
>>>>> something that takes advantage of it.
>>>>
>>>> I=E2=80=99m going to see if I can get hazard pointers into shape quick=
ly.
>>>
>>> Here it is.  Not even boot tested!
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?=
h=3Dsched/lazymm&id=3Decc3992c36cb88087df9c537e2326efb51c95e31
>>>
>>> Nick, I think you can accomplish much the same thing as your patch by:
>>>
>>> #define for_each_possible_lazymm_cpu while (false)
>>=20
>> I'm not sure what you mean? For powerpc, other CPUs can be using the mm=20
>> as lazy at this point. I must be missing something.
>=20
> What I mean is: if you want to shoot down lazies instead of doing the
> hazard pointer trick to track them, you could do:
>=20
> #define for_each_possible_lazymm_cpu while (false)
>=20
> which would promise to the core code that you don't have any lazies left
> by the time exit_mmap() is done.  You might need a new hook in
> exit_mmap() depending on exactly how you implement the lazy shootdown.

Oh for configuring it away entirely. I'll have to see how it falls out,=20
I suspect we'd want to just no-op that entire function and avoid the 2=20
atomics if we are taking care of our lazy mms with shootdowns.

The more important thing would be the context switch fast path, but even=20
there, there's really no reason why the two approaches couldn't be made=20
to both work with some careful helper functions or structuring of the=20
code.

Thanks,
Nick