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: <6548b3b3-30c9-8f64-7d28-8a434e0a0b80@redhat.com>
Date:   Wed, 18 Jan 2023 10:43:47 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
To:     James Houghton <jthoughton@google.com>
Cc:     Peter Xu <peterx@redhat.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        David Rientjes <rientjes@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mina Almasry <almasrymina@google.com>,
        Zach O'Keefe <zokeefe@google.com>,
        Manish Mishra <manish.mishra@nutanix.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        "Dr . David Alan Gilbert" <dgilbert@redhat.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Yang Shi <shy828301@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20230105101844.1893104-1-jthoughton@google.com>
 <20230105101844.1893104-22-jthoughton@google.com> <Y78+/wleTEC3gyqu@x1n>
 <CADrL8HU-prbfx2xxXCi0RPznp5DB68-NjqqmdM+4aUeUUURhiA@mail.gmail.com>
 <Y8AnROAtMngKntnq@x1n>
 <CADrL8HWFfqCqbpmvv8BSpvvzJ9aEeBEN30bMLuWGancsfMXv2w@mail.gmail.com>
 <Y8BtJzBLTpw5IR+H@x1n>
 <CADrL8HUi-j4ais45Xq8Jpb6a7DsWiXrKNeJfsqBRMi1Lier8xA@mail.gmail.com>
 <Y8B8mW2zSWDDwp7G@x1n> <06423461-c543-56fe-cc63-cabda6871104@redhat.com>
 <CADrL8HUdg1Fr=tLEQRkDjeTzNzzSM6EPhvDgzURxSZSBMLgjoQ@mail.gmail.com>
Content-Language: en-US
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH 21/46] hugetlb: use struct hugetlb_pte for
 walk_hugetlb_range
In-Reply-To: <CADrL8HUdg1Fr=tLEQRkDjeTzNzzSM6EPhvDgzURxSZSBMLgjoQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18.01.23 00:11, James Houghton wrote:
> On Mon, Jan 16, 2023 at 2:17 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 12.01.23 22:33, Peter Xu wrote:
>>> On Thu, Jan 12, 2023 at 04:17:52PM -0500, James Houghton wrote:
>>>> I'll look into it, but doing it this way will use _mapcount, so we
>>>> won't be able to use the vmemmap optimization. I think even if we do
>>>> use Hugh's approach, refcount is still being kept on the head page, so
>>>> there's still an overflow risk there (but maybe I am
>>>> misunderstanding).
>>>
>>> Could you remind me what's the issue if using refcount on the small pages
>>> rather than the head (assuming vmemmap still can be disabled)?
>>
>> The THP-way of doing things is refcounting on the head page. All folios
>> use a single refcount on the head.
>>
>> There has to be a pretty good reason to do it differently.
> 
> Peter and I have discussed this a lot offline. There are two main problems here:
> 
> 1. Refcount overflow
> 
> Refcount is always kept on the head page (before and after this
> series). IIUC, this means that if THPs could be 1G in size, they too
> would be susceptible to the same potential overflow. How easy is the
> overflow? [1]

Right. You'd need 8k VMAs. With 2 MiB THP you'd need 4096k VMAs. So ~64 
processes with 64k VMAs. Not impossible to achieve if one really wants 
to break the system ...

Side note: a long long time ago, we used to have sub-page refcounts for 
THP. IIRC, that was even before we had sub-page mapcounts and was used 
to make COW decisions.

> 
> To deal with this, the best solution we've been able to come up with
> is to check if refcount is > INT_MAX/2 (similar to try_get_page()),
> and if it is, stop the operation (UFFDIO_CONTINUE or a page fault)
> from proceeding. In the UFFDIO_CONTINUE case, return ENOMEM. In the
> page fault cause, return VM_FAULT_SIGBUS (not VM_FAULT_OOM; we don't
> want to kill a random process).

You'd have to also make sure that fork() won't do the same. At least 
with uffd-wp, Peter also added page table copying during fork() for 
MAP_SHARED mappings, which would have to be handled.

Of course, one can just disallow fork() with any HGM right from the 
start and keep it all simpler to not open up a can of worms there.

Is it reasonable, to have more than one (or a handful) of VMAs mapping a 
huge page via a HGM? Restricting it to a single one, would make handling 
   much easier.

If there is ever demand for more HGM mappings, that whole problem (and 
complexity) could be dealt with later. ... but I assume it will already 
be a requirement for VMs (e.g., under QEMU) that share memory with other 
processes (virtiofsd and friends?)


> 
> (So David, I think this answers your question. Refcount should be
> handled just like THPs.)
> 
> 2. page_mapcount() API differences
> 
> In this series, page_mapcount() returns the total number of page table
> references for the compound page. For example, if you have a
> PTE-mapped 2M page (with no other mappings), page_mapcount() for each
> 4K page will be 512. This is not the same as a THP: page_mapcount()
> would return 1 for each page. Because of the difference in
> page_mapcount(), we have 4 problems:

IMHO, it would actually be great to just be able to remove the sub-page 
mapcounts for THP and make it all simpler.

Right now, the sub-page mapcount is mostly required for making COW 
decisions, but only for accounting purposes IIRC (NR_ANON_THPS, 
NR_SHMEM_PMDMAPPED, NR_FILE_PMDMAPPED) and mlock handling IIRC. See 
page_remove_rmap().

If we can avoid that complexity right from the start for hugetlb, great, ..

> 
> i. Smaps uses page_mapcount() >= 2 to determine if hugetlb memory is
> "private_hugetlb" or "shared_hugetlb".
> ii. Migration with MPOL_MF_MOVE will check page_mapcount() to see if
> the hugepage is shared or not. Pages that would otherwise be migrated
> now require MPOL_MF_MOVE_ALL to be migrated.
> [Really both of the above are checking how many VMAs are mapping our hugepage.]
> iii. CoW. This isn't a problem right now because CoW is only possible
> with MAP_PRIVATE VMAs and HGM can only be enabled for MAP_SHARED VMAs.
> iv. The hwpoison handling code will check if it successfully unmapped
> the poisoned page. This isn't a problem right now, as hwpoison will
> unmap all the mappings for the hugepage, not just the 4K where the
> poison was found.
> 
> Doing it this way allows HGM to remain compatible with the hugetlb
> vmemmap optimization. None of the above problems strike me as
> particularly major, but it's unclear to me how important it is to have
> page_mapcount() have a consistent meaning for hugetlb vs non-hugetlb.

See below, maybe we should tackle HGM from a different direction.

> 
> The other way page_mapcount() (let's say the "THP-like way") could be
> done is like this: increment compound mapcount if we're mapping a
> hugetlb page normally (e.g., 1G page with a PUD). If we're mapping at
> high-granularity, increment the mapcount for each 4K page that is
> getting mapped (e.g., PMD within a 1G page: increment the mapcount for
> the 512 pages that are now mapped). This yields the same
> page_mapcount() API we had before, but we lose the hugetlb vmemmap
> optimization.
> 
> We could introduce an API like hugetlb_vma_mapcount() that would, for
> hugetlb, give us the number of VMAs that map a hugepage, but I don't
> think people would like this.
> 
> I'm curious what others think (Mike, Matthew?). I'm guessing the
> THP-like way is probably what most people would want, though it would
> be a real shame to lose the vmemmap optimization.

Heh, not me :) Having a single mapcount is certainly much cleaner. ... 
and if we're dealing with refcount overflows already, mapcount overflows 
are not an issue.


I wonder if the following crazy idea has already been discussed: treat 
the whole mapping as a single large logical mapping. One reference and 
one mapping, no matter how the individual parts are mapped into the 
assigned page table sub-tree.

Because for hugetlb with MAP_SHARED, we know that the complete assigned 
sub-tree of page tables can only map the given hugetlb page, no 
fragments of something else. That's very different to THP in private 
mappings ...

So as soon as the first piece gets mapped, we increment 
refcount+mapcount. Other pieces in the same subtree don't do that.

Once the last piece is unmapped (or simpler: once the complete subtree 
of page tables is gone), we decrement refcount+mapcount. Might require 
some brain power to do this tracking, but I wouldn't call it impossible 
right from the start.

Would such a design violate other design aspects that are important?

-- 
Thanks,

David / dhildenb