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;
Message-ID: <818a2511-5eed-7c29-b52f-1cab2bd40434@redhat.com>
Date:   Thu, 27 Jul 2023 22:43:27 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] smaps: Fix the abnormal memory statistics obtained
 through /proc/pid/smaps
Content-Language: en-US
To:     Peter Xu <peterx@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
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>
 <ZMK+jSDgOmJKySTr@x1n> <30e58727-0a6a-4461-e9b1-f64d6eea026c@redhat.com>
 <ZMLT4aL9V61Bl5TG@x1n>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <ZMLT4aL9V61Bl5TG@x1n>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 27.07.23 22:30, Peter Xu wrote:
> On Thu, Jul 27, 2023 at 09:17:45PM +0200, David Hildenbrand wrote:
>> On 27.07.23 20:59, Peter Xu wrote:
>>> 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?
>>>
>>
>> For example in follow_page()
>>
>> if (vma_is_secretmem(vma))
>> 	return NULL;
>>
>> if (WARN_ON_ONCE(foll_flags & FOLL_PIN))
>> 	return NULL;
>>
>>
>> Maybe you can elaborate why you think we should *not* be using
>> vm_normal_page_pmd() and instead some arbitrary GUP internal helper? I don't
>> get it.
> 
> Because the old code was written like that?

And it's not 2014 anymore. Nowadays we do have the right helper in place.

[...]

>> FOLL_NUMA naming was nowadays wrong to begin with (not to mention, confusing
>> a we learned). There are other reasons why we have PROT_NONE -- mprotect(),
>> for example.
> 
> It doesn't really violate with the name, IMHO - protnone can be either numa
> hint or PROT_NONE for real. As long as we return NULL for a FOLL_NUMA
> request we're achieving the goal we want - we guarantee a NUMA balancing to
> trigger with when FOLL_NUMA provided.  It doesn't need to guarantee
> anything else, afaiu.  The final check relies in vma_is_accessible() in the
> fault paths anyway.  So I don't blame the old name that much.

IMHO, the name FOLL_NUMA made sense when it still was called pte_numa.

> 
>>
>> We could have a flag that goes the other way around: FOLL_IGNORE_PROTNONE
>> ... which surprisingly then ends up being exactly what FOLL_FORCE means
>> without FOLL_WRITE, and what this patch does.
>>
>> Does that make sense to you?
>>
>>
>>>
>>> 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.
>>
>> smaps needs to be changed in any case IMHO. And I'm absolutely not in favor
>> of revicing FOLL_NUMA.
> 
> As stated above, to me FOLL_NUMA is all fine and clear.  If you think
> having a flag back for protnone is worthwhile no matter as-is (FOLL_NUMA)
> or with reverted meaning, then that sounds all fine to me. Maybe the old
> name at least makes old developers know what's that.
> 
> I don't have a strong opinion on names though; mostly never had.

I'll avoid new FOLL_ flags first and post my proposal. If many people 
are unhappy with that approach, we can revert the commit and call it a day.

Thanks!

-- 
Cheers,

David / dhildenb