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:   Thu, 17 Jun 2021 11:37:27 +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 <peterz@infradead.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Dave Hansen <dave.hansen@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, linux-mm@kvack.org,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        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>
In-Reply-To: <617cb897-58b1-8266-ecec-ef210832e927@kernel.org>
MIME-Version: 1.0
Message-Id: <1623893358.bbty474jyy.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 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 curre=
ntly
>>>> a comment explaining why this barrier probably exists in all cases.  T=
his
>>>> is very fragile -- any change to the relevant parts of the scheduler
>>>> 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 it
>>> replaces it with smp_mb for example.
>>=20
>> I'm with Nick again, on this. You're adding extra barriers for no
>> discernible reason, that's not generally encouraged, seeing how extra
>> barriers is extra slow.
>>=20
>> Both mmdrop() itself, as well as the callsite have comments saying how
>> membarrier relies on the implied barrier, what's fragile about that?
>>=20
>=20
> 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 try
> that commit message again:
>=20
> membarrier() needs a barrier after any CPU changes mm.  There is currentl=
y
> 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() bein=
g
> full barriers.
>=20
> 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 (not
for mmdrop but a different primitive). Also my lazy tlb shootdown series=20
could possibly take advantage of this, I might cherry pick it and test=20
performance :)

I don't think it belongs in this series though. Should go together with
something that takes advantage of it.

Thanks,
Nick