Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 28 Jul 2023 09:18:45 -0700 | Subject | Re: [PATCH v1 0/4] smaps / mm/gup: fix gup_can_follow_protnone fallout |
| |
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 hate how it uses FOLL_FORCE that is inherently scary.
Why do we have that "gup_can_follow_protnone()" logic AT ALL?
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.
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.
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.
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.
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?
Linus
| |