Messages in this thread Patch in this message | | | Date | Thu, 27 Jul 2023 19:27:02 +0200 | From | David Hildenbrand <> | Subject | Re: [PATCH] smaps: Fix the abnormal memory statistics obtained through /proc/pid/smaps |
| |
>> >> This was wrong from the very start. If we're not in GUP, we shouldn't call >> GUP functions. > > My understanding is !GET && !PIN is also called gup.. otherwise we don't > need GET and it can just be always implied.
That's not the point. The point is that _arbitrary_ code shouldn't call into GUP internal helper functions, where they bypass, for example, any sanity checks.
> > The other proof is try_grab_page() doesn't fail hard on !GET && !PIN. So I > don't know whether that's "wrong" to be used.. >
To me, that is arbitrary code using a GUP internal helper and, therefore, wrong.
> Back to the topic: I'd say either of the patches look good to solve the > problem. If p2pdma pages are mapped as PFNMAP/MIXEDMAP (?), I guess > vm_normal_page_pmd() proposed here will also work on it, so nothing I see > wrong on 2nd one yet. > > It looks nicer indeed to not have FOLL_FORCE here, but it also makes me > just wonder whether we should document NUMA behavior for FOLL_* somewhere, > because we have an implication right now on !FOLL_FORCE over NUMA, which is > not obvious to me..
Yes, we probably should. For get_use_pages() and friends that behavior was always like that and it makes sense: usually it represent application behavior.
> > And to look more over that aspect, see follow_page(): previously we can > follow a page for protnone (as it never applies FOLL_NUMA) but now it won't > (it never applies FOLL_FORCE, either, so it seems "accidentally" implies > FOLL_NUMA now). Not sure whether it's intended, though..
That was certainly an oversight, thanks for spotting that. That patch was not supposed to change semantics:
diff --git a/mm/gup.c b/mm/gup.c index 76d222ccc3ff..ac926e19ff72 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -851,6 +851,13 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address, if (WARN_ON_ONCE(foll_flags & FOLL_PIN)) return NULL;
+ /* + * In contrast to get_user_pages() and friends, we don't want to + * fail if the PTE is PROT_NONE: see gup_can_follow_protnone(). + */ + if (!(foll_flags & FOLL_WRITE)) + foll_flags |= FOLL_FORCE; + page = follow_page_mask(vma, address, foll_flags, &ctx); if (ctx.pgmap) put_dev_pagemap(ctx.pgmap);
-- Cheers,
David / dhildenb
| |