Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Jul 2023 14:59:25 -0400
From:   Peter Xu <peterx@redhat.com>
To:     David Hildenbrand <david@redhat.com>
Cc:     liubo <liubo254@huawei.com>, akpm@linux-foundation.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        hughd@google.com, willy@infradead.org
Subject: Re: [PATCH] smaps: Fix the abnormal memory statistics obtained
 through /proc/pid/smaps
Message-ID: <ZMK+jSDgOmJKySTr@x1n>
References: <20230726073409.631838-1-liubo254@huawei.com>
 <CADFyXm5nkgZjVMj3iJhqQnyA1AOmqZ-AKdaWyUD=UvZsOEOcPg@mail.gmail.com>
 <ZMJt+VWzIG4GAjeb@x1n>
 <f49c2a51-4dd8-784b-57fa-34fb397db2b7@redhat.com>
 <ZMKJjDaqZ7FW0jfe@x1n>
 <5a2c9ae4-50f5-3301-3b50-f57026e1f8e8@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <5a2c9ae4-50f5-3301-3b50-f57026e1f8e8@redhat.com>
Precedence: bulk

On Thu, Jul 27, 2023 at 07:27:02PM +0200, David Hildenbrand wrote:
> > > 
> > > 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.

What's the sanity checks that you're referring to?

> 
> > 
> > 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);

This seems to be slightly against your other solution though for smaps,
where we want to avoid abusing FOLL_FORCE.. isn't it..

Why read only?  That'll always attach FOLL_FORCE to all follow page call
sites indeed for now, but just curious - logically "I want to fetch the
page even if protnone" is orthogonal to do with write permission here to
me.

I still worry about further abuse of FOLL_FORCE, I believe you also worry
that so you proposed the other way for the smaps issue.

Do you think we can just revive FOLL_NUMA?  That'll be very clear to me
from that aspect that we do still have valid use cases for it.

The very least is if with above we should really document FOLL_FORCE - we
should mention NUMA effects.  But that's ... really confusing. Thinking
about that I personally prefer a revival of FOLL_NUMA, then smaps issue all
go away.

Thanks,

-- 
Peter Xu