Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Thomas Hellstrom <thellstrom@vmware.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        =?iso-8859-1?Q?Thomas_Hellstr=F6m_=28VMware=29?= 
        <thomas_os@shipmail.org>
CC:     "Kirill A. Shutemov" <kirill@shutemov.name>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        Matthew Wilcox <willy@infradead.org>,
        Will Deacon <will.deacon@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Rik van Riel <riel@surriel.com>,
        Minchan Kim <minchan@kernel.org>,
        Michal Hocko <mhocko@suse.com>,
        Huang Ying <ying.huang@intel.com>,
        =?iso-8859-1?Q?J=E9r=F4me_Glisse?= <jglisse@redhat.com>
Subject: Re: [PATCH v4 3/9] mm: pagewalk: Don't split transhuge pmds when a
 pmd_entry is present
Thread-Topic: [PATCH v4 3/9] mm: pagewalk: Don't split transhuge pmds when a
 pmd_entry is present
Thread-Index: AQHVfbjttG4Vh+cm+UmzThOYz5Zjow==
Date:   Wed, 9 Oct 2019 18:52:34 +0000
Message-ID: <MN2PR05MB6141B981C2CAB4955D59747EA1950@MN2PR05MB6141.namprd05.prod.outlook.com>
References: <20191008091508.2682-1-thomas_os@shipmail.org>
 <20191008091508.2682-4-thomas_os@shipmail.org>
 <20191009152737.p42w7w456zklxz72@box>
 <CAHk-=wh4waroKr-Xtcv+5pTxBcHxGEj-g73eQvXVawML_C0EXw@mail.gmail.com>
 <03d85a6a-e24a-82f4-93b8-86584b463471@shipmail.org>
 <CAHk-=whhdRSqjX5wy1LzFYnOG58UztpifkNvbxBcTVbT3Mzv4g@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ea50289d-6ab6-4c85-2ac9-08d74ce9d8ca
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Oct 2019 18:52:34.2494
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: KZW7/Ls3/MMTeBBBMtwlKRlY+7EgTZRhgmSmwWc+0iHgT3Z6qb4NzArk79ldLmHvCNYdpqcZf9AC+a+rhwhU4A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR05MB6192
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,=0A=
=0A=
On 10/9/19 7:17 PM, Linus Torvalds wrote:=0A=
> On Wed, Oct 9, 2019 at 10:03 AM Thomas Hellstr=F6m (VMware)=0A=
> <thomas_os@shipmail.org> wrote:=0A=
>> Nope, it handles the hugepages by ignoring them, since they should be=0A=
>> read-only, but if pmd_entry() was called with something else than a=0A=
>> hugepage, then it requests the fallback, but never a split.=0A=
> But  PAGE_WALK_FALLBACK _is_ a split.=0A=
>=0A=
> Oh, except you did this=0A=
>=0A=
> -               split_huge_pmd(walk->vma, pmd, addr);=0A=
> +               if (!ops->pmd_entry)=0A=
> +                       split_huge_pmd(walk->vma, pmd, addr);=0A=
>=0A=
>=0A=
> so it avoids the split.=0A=
>=0A=
> No, that's unacceptable. And makes no sense anyway. If it doesn't=0A=
> split the pmd, then it shouldn't walk the pte's - because they don't=0A=
> exist. And if it's not a hugepmd, then the split is a no-op, so the=0A=
> test makes no sense.=0A=
>=0A=
> I hadn't noticed that part of the patch. That simply can't be right. I=0A=
> don't think you've tested this, because you never actually have=0A=
> hugepages, do you?=0A=
>=0A=
> You didn't notice or realize that split_huge_pmd() just does that=0A=
>=0A=
>                 if (is_swap_pmd(*____pmd) || pmd_trans_huge(*____pmd)   \=
=0A=
>                                         || pmd_devmap(*____pmd))        \=
=0A=
>=0A=
> thing and doesn't do anythign at all if it's not huge.=0A=
>=0A=
> So no. That code makes no sense at all, and I didn't realize how=0A=
> senseless it was, becasue I stupidly missed that "make the split=0A=
> conditional" - which is insane and wrong - and I thought that you=0A=
> wanted PAGE_WALK_FALLBACK to split a pmd and fall back to per-pte=0A=
> entries, which is what the name implies.=0A=
>=0A=
> But that's not what you wanted at all.=0A=
>=0A=
> Just get rid of PAGE_WALK_FALLBACK entirely then, and make the rule be=0A=
> that a zero return value just means "split and do ptes". Which is what=0A=
> you want (see above why "split" simply is wrong, and isn't an issue=0A=
> for you anyway.=0A=
>=0A=
> That won't change any existing cases, since even if they do have a=0A=
> zero return value, they don't have a pte_entry() callback, so they=0A=
> won't do that "split and do ptes" anyway.=0A=
>=0A=
>              Linus=0A=
>=0A=
Hmm, so we have the following cases we need to handle when returning=0A=
from the pmd_entry() handler.=0A=
=0A=
1) Huge pmd was handled - Returns 0 and continues.=0A=
2) A pmd is otherwise unstable, typically someone just zapped a huge=0A=
pmd. Returns PAGE_WALK_FALLBACK, gets caught in the pmd_trans_unstable()=0A=
test and retries.=0A=
3) A pte directory - Returns PAGE_WALK_FALLBACK, falls through, avoids=0A=
the split and continues to the next level. Yeah that split avoidance=0A=
test is indeed made unnecessary by the preceding pmd_trans_unstable() test.=
=0A=
=0A=
-               split_huge_pmd(walk->vma, pmd, addr);=0A=
+               if (!ops->pmd_entry)=0A=
+                       split_huge_pmd(walk->vma, pmd, addr);=0A=
=0A=
But as the commit message says, PAGE_WALK_FALLBACK is necessary to have=0A=
a virtual address range being handled once and only once. Therefore we=0A=
must distinguish between 1) and 2) since 2) must be retried until it's=0A=
handled correctly.=0A=
=0A=
So we need the PAGE_WALK_FALLBACK. And if we instead were to combine 1)=0A=
and 3) in a single return value and use, for example PAGE_WALK_RETRY for=0A=
2)  the following could happen.=0A=
=0A=
a) we handle the huge pmd and return 0 from pte_entry().=0A=
b) another process splits it.=0A=
c) we fall through to the pte level and handle the same address range=0A=
again...=0A=
=0A=
So to summarize, yes the test in the code you cite is unnecessary. But=0A=
if we want to guarantee a virtual address range being handled once and=0A=
only once we need the PAGE_WALK_FALLBACK, (perhaps renamed). If not, we=0A=
can do without it similar to your original patch.=0A=
=0A=
Thanks,=0A=
=0A=
/Thomas=0A=
=0A=
=0A=
=0A=