Messages in this thread | | | Date | Fri, 28 Jul 2023 21:40:54 +0200 | From | David Hildenbrand <> | Subject | Re: [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
| |