lkml.org 
[lkml]   [2023]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 0/4] smaps / mm/gup: fix gup_can_follow_protnone fallout
On 28.07.23 19:30, David Hildenbrand wrote:
> On 28.07.23 18:18, Linus Torvalds wrote:
>> On Thu, 27 Jul 2023 at 14:28, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> This is my proposal on how to handle the fallout of 474098edac26
>>> ("mm/gup: replace FOLL_NUMA by gup_can_follow_protnone()") where I
>>> accidentially missed that follow_page() and smaps implicitly kept the
>>> FOLL_NUMA flag clear by *not* setting it if FOLL_FORCE is absent, to
>>> not trigger faults on PROT_NONE-mapped PTEs.
>>
>> Ugh.
>
> I was hoping for that reaction, with the assumption that we would get
> something cleaner :)
>
>>
>> I hate how it uses FOLL_FORCE that is inherently scary.
>
> I hate FOLL_FORCE, but I hate FOLL_NUMA even more, because to me it
> is FOLL_FORCE in disguise (currently and before 474098edac26, if
> FOLL_FORCE is set, FOLL_NUMA won't be set and the other way around).
>
>>
>> Why do we have that "gup_can_follow_protnone()" logic AT ALL?
>
> That's what I was hoping for.
>
>>
>> Couldn't we just get rid of that disgusting thing, and just say that
>> GUP (and follow_page()) always just ignores NUMA hinting, and always
>> just follows protnone?
>>
>> We literally used to have this:
>>
>> if (!(gup_flags & FOLL_FORCE))
>> gup_flags |= FOLL_NUMA;
>>
>> ie we *always* set FOLL_NUMA for any sane situation. FOLL_FORCE should
>> be the rare crazy case.
>
> Yes, but my point would be that we now spell that "rare crazy case"
> out for follow_page().
>
> If you're talking about patch #1, I agree, therefore patch #3 to
> avoid all that nasty FOLL_FORCE handling in GUP callers.
>
> But yeah, if we can avoid all that, great.
>
>>
>> The original reason for not setting FOLL_NUMA all the time is
>> documented in commit 0b9d705297b2 ("mm: numa: Support NUMA hinting
>> page faults from gup/gup_fast") from way back in 2012:
>>
>> * If FOLL_FORCE and FOLL_NUMA are both set, handle_mm_fault
>> * would be called on PROT_NONE ranges. We must never invoke
>> * handle_mm_fault on PROT_NONE ranges or the NUMA hinting
>> * page faults would unprotect the PROT_NONE ranges if
>> * _PAGE_NUMA and _PAGE_PROTNONE are sharing the same pte/pmd
>> * bitflag. So to avoid that, don't set FOLL_NUMA if
>> * FOLL_FORCE is set.
>
>
> In handle_mm_fault(), we never call do_numa_page() if
> !vma_is_accessible(). Same for do_huge_pmd_numa_page().
>
> So, if we would ever end up triggering a page fault on
> mprotect(PROT_NONE) ranges (i.e., via FOLL_FORCE), we
> would simply do nothing.
>
> At least that's the hope, I'll take a closer look just to make
> sure we're good on all call paths.
>
>>
>> but I don't think the original reason for this is *true* any more.
>>
>> Because then two years later in 2014, in commit c46a7c817e66 ("x86:
>> define _PAGE_NUMA by reusing software bits on the PMD and PTE levels")
>> Mel made the code able to distinguish between PROT_NONE and NUMA
>> pages, and he changed the comment above too.
>
> CCing Mel.
>
> I remember that pte_protnone() can only distinguished between
> NUMA vs. actual mprotect(PROT_NONE) by looking at the VMA -- vma_is_accessible().
>
> Indeed, include/linux/pgtable.h:
>
> /*
> * Technically a PTE can be PROTNONE even when not doing NUMA balancing but
> * the only case the kernel cares is for NUMA balancing and is only ever set
> * when the VMA is accessible. For PROT_NONE VMAs, the PTEs are not marked
> * _PAGE_PROTNONE so by default, implement the helper as "always no". It
> * is the responsibility of the caller to distinguish between PROT_NONE
> * protections and NUMA hinting fault protections.
> */
>
>>
>> But I get the very very strong feeling that instead of changing the
>> comment, he should have actually removed the comment and the code.
>>
>> So I get the strong feeling that all these FOLL_NUMA games should just
>> go away. You removed the FOLL_NUMA bit, but you replaced it with using
>> FOLL_FORCE.
>>
>> So rather than make this all even more opaque and make it even harder
>> to figure out why we have that code in the first place, I think it
>> should all just be removed.
>
> At least to me, spelling FOLL_FORCE in follow_page() now out is much
> less opaque then getting that implied by lack of FOLL_NUMA.
>
>>
>> The original reason for FOLL_NUMA simply does not exist any more. We
>> know exactly when a page is marked for NUMA faulting, and we should
>> simply *ignore* it for GUP and follow_page().
>>
>> I think we should treat a NUMA-faulting page as just being present
>> (and not NUMA-fault it).
>>
>> Am I missing something?
>
> There was the case for "FOLL_PIN represents application behavior and
> should trigger NUMA faults", but I guess that can be ignored.


Re-reading commit 0b9d705297b2 ("mm: numa: Support NUMA hinting page
faults from gup/gup_fast"), it actually does spell out an important case
that we should handle:

"KVM secondary MMU page faults will trigger the NUMA hinting page
faults through gup_fast -> get_user_pages -> follow_page ->
handle_mm_fault."

That is still the case today (and important). Not triggering NUMA
hinting faults would degrade KVM.

Hmm. So three alternatives I see:

1) Use FOLL_FORCE in follow_page() to unconditionally disable protnone
checks. Alternatively, have an internal FOLL_NO_PROTNONE flag if we
don't like that.

2) Revert the commit and reintroduce unconditional FOLL_NUMA without
FOLL_FORCE.

3) Have a FOLL_NUMA that callers like KVM can pass.

Thoughts?

--
Cheers,

David / dhildenb

\
 
 \ /
  Last update: 2023-07-28 21:43    [W:0.155 / U:2.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site