linux-kernel - Re: [PATCH v3 3/4] mm: FLEXIBLE

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <a7289b0f-466e-1eaa-dae6-3a4e55b46528@arm.com>
Date:   Tue, 18 Jul 2023 11:36:09 +0100
From:   Ryan Roberts <ryan.roberts@....com>
To:     Hugh Dickins <hughd@...gle.com>
Cc:     Yu Zhao <yuzhao@...gle.com>, Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Yin Fengwei <fengwei.yin@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Anshuman Khandual <anshuman.khandual@....com>,
        Yang Shi <shy828301@...il.com>,
        "Huang, Ying" <ying.huang@...el.com>, Zi Yan <ziy@...dia.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH v3 3/4] mm: FLEXIBLE_THP for improved performance

On 18/07/2023 00:37, Hugh Dickins wrote:
> On Mon, 17 Jul 2023, Ryan Roberts wrote:
> 
>>>>>> +static int alloc_anon_folio(struct vm_fault *vmf, struct folio **folio)
>>>>>> +{
>>>>>> +       int i;
>>>>>> +       gfp_t gfp;
>>>>>> +       pte_t *pte;
>>>>>> +       unsigned long addr;
>>>>>> +       struct vm_area_struct *vma = vmf->vma;
>>>>>> +       int prefer = anon_folio_order(vma);
>>>>>> +       int orders[] = {
>>>>>> +               prefer,
>>>>>> +               prefer > PAGE_ALLOC_COSTLY_ORDER ? PAGE_ALLOC_COSTLY_ORDER : 0,
>>>>>> +               0,
>>>>>> +       };
>>>>>> +
>>>>>> +       *folio = NULL;
>>>>>> +
>>>>>> +       if (vmf_orig_pte_uffd_wp(vmf))
>>>>>> +               goto fallback;
>>>>>> +
>>>>>> +       for (i = 0; orders[i]; i++) {
>>>>>> +               addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << orders[i]);
>>>>>> +               if (addr >= vma->vm_start &&
>>>>>> +                   addr + (PAGE_SIZE << orders[i]) <= vma->vm_end)
>>>>>> +                       break;
>>>>>> +       }
>>>>>> +
>>>>>> +       if (!orders[i])
>>>>>> +               goto fallback;
>>>>>> +
>>>>>> +       pte = pte_offset_map(vmf->pmd, vmf->address & PMD_MASK);
>>>>>> +       if (!pte)
>>>>>> +               return -EAGAIN;
>>>>>
>>>>> It would be a bug if this happens. So probably -EINVAL?
>>>>
>>>> Not sure what you mean? Hugh Dickins' series that went into v6.5-rc1 makes it
>>>> possible for pte_offset_map() to fail (if I understood correctly) and we have to
>>>> handle this. The intent is that we will return from the fault without making any
>>>> change, then we will refault and try again.
>>>
>>> Thanks for checking that -- it's very relevant. One detail is that
>>> that series doesn't affect anon. IOW, collapsing PTEs into a PMD can't
>>> happen while we are holding mmap_lock for read here, and therefore,
>>> the race that could cause pte_offset_map() on shmem/file PTEs to fail
>>> doesn't apply here.
>>
>> But Hugh's patches have changed do_anonymous_page() to handle failure from
>> pte_offset_map_lock(). So I was just following that pattern. If this really
>> can't happen, then I'd rather WARN/BUG on it, and simplify alloc_anon_folio()'s
>> prototype to just return a `struct folio *` (and if it's null that means ENOMEM).
>>
>> Hugh, perhaps you can comment?
> 
> I agree with your use of -EAGAIN there: I find it better to allow for the
> possibility, than to go to great effort persuading that it's impossible;
> especially because what's possible tomorrow may differ from today.
> 
> And notice that, before my changes, there used to be a pmd_trans_unstable()
> check above, implying that it is possible for it to fail (for more reasons
> than corruption causing pmd_bad()) - one scenario would be that the
> pte_alloc() above succeeded *because* someone else had managed to insert
> a huge pmd there already (maybe we have MMF_DISABLE_THP but they did not).
> 
> But I see from later mail that Yu Zhao now agrees with your -EAGAIN too,
> so we are all on the same folio.

Thanks for the explanation. I think we are all now agreed that the error case
needs handling and -EAGAIN is the correct code.

> 
> Hugh
> 
> p.s. while giving opinions, I'm one of those against using "THP" for
> large but not pmd-mappable folios; and was glad to see Matthew arguing
> the same way when considering THP_SWPOUT in another thread today.

Honestly, I don't have an opinion either way on this (probably because I don't
have the full context and history of THP like many of you do). So given there is
a fair bit of opposition to FLEXIBLE_THP, I'll change it back to
LARGE_ANON_FOLIO (and move it out of the THP sub-menu) in the next revision.