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, 19 Jan 2023 09:31:43 -0800
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     James Houghton <jthoughton@google.com>
Cc:     Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.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
Subject: Re: [PATCH 21/46] hugetlb: use struct hugetlb_pte for
 walk_hugetlb_range
Message-ID: <Y8l+f2wNp2gAjvYg@monkey>
References: <Y8BtJzBLTpw5IR+H@x1n>
 <CADrL8HUi-j4ais45Xq8Jpb6a7DsWiXrKNeJfsqBRMi1Lier8xA@mail.gmail.com>
 <Y8B8mW2zSWDDwp7G@x1n>
 <06423461-c543-56fe-cc63-cabda6871104@redhat.com>
 <CADrL8HUdg1Fr=tLEQRkDjeTzNzzSM6EPhvDgzURxSZSBMLgjoQ@mail.gmail.com>
 <6548b3b3-30c9-8f64-7d28-8a434e0a0b80@redhat.com>
 <Y8gRpEonhXgqfb41@x1n>
 <CADrL8HVGMTowH4trJhS+eM_EwZKoUgu7LmfwyTGyGRnNnwL3Zg@mail.gmail.com>
 <Y8hITxr/BBMuO6WX@monkey>
 <CADrL8HUggALQET-09Zw3BhFjZdw_G9+v6CU=qtGtK=KZ_DeAsw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CADrL8HUggALQET-09Zw3BhFjZdw_G9+v6CU=qtGtK=KZ_DeAsw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KUlpiM6I3g6OSXRgt08tm+IcxFhwsx2xtAIQLSbedXA8sjHU7aq6AVcFmncy?=
 =?us-ascii?Q?a8hB/hXrAmPMvz+j3RPUJf5WNfLnGxSzeUoPDJbeYCKR4vm2OhnWUOpm3bUm?=
 =?us-ascii?Q?s401H5PMIvBK8e9msAhdyWLCZqsTFUwaW+MOFrMWTnH99zR7XQfj5cvHySki?=
 =?us-ascii?Q?1BCbm69WexS6h+EOpdB/JEboSbcj6IGUpk1cygf6NJlAFua6L7S9kl2YiS2U?=
 =?us-ascii?Q?O4j9Qjtd/c7J3z8dWu61lQZpkDaCCXSxADWjy06ot+Vi2j4pCXaEXlTW2E90?=
 =?us-ascii?Q?DzW18tyK51JqtDa567ceXJ6zQ7YypxBSF1CGMcFzMvH9dQ/iA5mP7sanQQwU?=
 =?us-ascii?Q?VyJsTZnppJVwjcxcIKWbZZPb9huJfPV9pKOa0P2/hYNtN98OGDbaO68TSm2J?=
 =?us-ascii?Q?amH1urKLbuHIqkMh0uN0Jlz9xvB0PWw7OWcLXaLjfx5ddKdgn1mMbefgOL/N?=
 =?us-ascii?Q?foWjgUVMF80yhjVHv8rt+9os4QoInB3R9N17n1D8xo5aQY1yp3V6ULf0xyUt?=
 =?us-ascii?Q?kZekF3PLVT2PkIrQNfYxIDTCdF9hCWN4A+X9Cvpqhi3ZUkQZ1lCabcV/RF5L?=
 =?us-ascii?Q?jKGfOwqLsfyAjjwrxuP3h6li/9DzGpuh0APcPJyNMBIREgKu21eHCr4fdPRF?=
 =?us-ascii?Q?vgvdKxwZIX7/WtRpjdTPbNuZ0K/n89LHRPEDcjSuskCwIuL3TdkxxomK9/9L?=
 =?us-ascii?Q?U5jaIMyQNSajd1v2nCsc07/jPJmH1zvyS6eA6ww6GcEkvqlf7n0ip9lemDFn?=
 =?us-ascii?Q?iNM/jOxVKizNZjLFozSYyB+NmOyIVXt9+5jVvTXMIK9BORODfZw+yW471cB3?=
 =?us-ascii?Q?TclHH7V2raOiSeUBd1l+gjemCxbFQl8shjdRSrxpa0pu77UDiwO12WWxw198?=
 =?us-ascii?Q?n5EmHAhrzx2TgnuN2KmokXUNFKuKfMA7yW4au+Xu2LrUUCNL8Flx9Xqe1wHZ?=
 =?us-ascii?Q?+PYvxZ7Mz6Q2KXM5SfSZNMvEL1fmTfALfbTVMYskWxVlFghuWL5fcNO+Ghdx?=
 =?us-ascii?Q?eqnSNQEm/2I+F2ZzA5lHnkgwlvlSdMKCTBH9w3YZIoFT6fu3+PdwIk1Zjwar?=
 =?us-ascii?Q?11qgkozV9F+GYdIVwFSioEsuA+xtruaSGOF5YUDFq1A8Z4Nz4k8HlAoKnuwG?=
 =?us-ascii?Q?LalB2co9LN5HgjonhgY57VL6lE3CPqVvtQutnZWIjW2wJo6T3ikld+5PrqAN?=
 =?us-ascii?Q?CJwmJnuzIX3ZUCyGtD1j6YawcY8Xb24Wf/2hrUUwNwDpb4QOYRtm6gUBQdwy?=
 =?us-ascii?Q?xF6l+b8n2P6YWe3CfWhDljv7XguZe4tDdA7N3Ubz3/WXTMpiM9WmEqs0rvrO?=
 =?us-ascii?Q?58FG4uuP8FwOcmCwQoDuvVXC22xGTMCF4pDUN26/rWSHr6J4ECxT6He4sA4J?=
 =?us-ascii?Q?LzAB0uVnnmrW1ugM/NaC6J1wlmdsJvMzp7jOXNZw0Q5dAWI7Aujt36jDmSvQ?=
 =?us-ascii?Q?PsVRe9u3qaDGZ1kL7a+kgKsUFOWWUMeA15ksH8Kwns+/W/f+e33DuhQGaShF?=
 =?us-ascii?Q?mWNDFhYii/MsUORLW93eu54jPxIHLdjoNIjnirTytSr+4fmIzIvyzB+0ibj3?=
 =?us-ascii?Q?L6z0DrqvgwaWLvzo+Rl60D9SDiM3NsdSwxRPnQ72rFML3UroalaH2KlxC8r0?=
 =?us-ascii?Q?9w=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?GUbXtqgPsGU5bpyMEuYEGq+0okt35hojA373TjHEs8Mf6ltMxcAH04KBFmoS?=
 =?us-ascii?Q?nODmNqMbQ2Nfv8fN/Bnx4MS4eMRw5+mBS6AEU4eUejGpy5wehlD4+lsioIGC?=
 =?us-ascii?Q?vjd0y9JUhEwyyQbgWZghVXeFC01yxbEOekNKY0xgd/w39qZ9W8scDF/LZaZq?=
 =?us-ascii?Q?PBe7XWxOXHt4xYiCovnNn7OQ+31or5DRIo8u+QhudrFy7ahjND9VVmUQt2cN?=
 =?us-ascii?Q?9yKcLtYdim87lG9Weza0/yNJYE1+yoVWJ2HXGium0Cku3d1IO9F+clhp1qpT?=
 =?us-ascii?Q?pC4dKVfIlqUnyndwgnhW6ewOfDIGKDA8oanSJFeCBNVUSb8Laq1E0LwB0fnw?=
 =?us-ascii?Q?fH2Zv350uwHznOcUDBnTE6aDALqOvdDeVHnhT7SkiVu32ZwZXQfPbATVP4KC?=
 =?us-ascii?Q?ISvh0jTRFGOTVIiIJu7KxwmFYuh6EGPVscajB6HkO/XtPzrZWp6DqAsOqBpt?=
 =?us-ascii?Q?iS6FWbqI1PQMr/PViIE1idzL4IHDv82gmutgrVXCVvs7APurflIjVM07rH6o?=
 =?us-ascii?Q?YZCdVvrUwZxQfoKdQ+cHX62532cD2asOlV+1NX41SliG1bH40cpo+fAuYiMq?=
 =?us-ascii?Q?n6rpLMmhx9gMT+pNuhtIp6EE8bx9b7fewVMh4IyWki6h+G80ZCZI6bJaFf8z?=
 =?us-ascii?Q?aJcGLm84ZjTxIjeBb/NOINeQzCjxW8yPpv77ucUM0EFwHk0x8OkgX1HW87pU?=
 =?us-ascii?Q?OqLmwKUA2LUraBYsqyd+x2MjWsyEeq3EZyN370XcYHvJXC+PJG0wvt/NuuL+?=
 =?us-ascii?Q?NPdB48U2NziTc9BvWOIdQbTv+4iBBQPnklwVxTrTDxpVPXc6e7MYdOMyijMd?=
 =?us-ascii?Q?6xt50kuC02fYL5HgTX4Cixb72pckvWrFI7kwDGk24LxA1VHn1W7ZaKGfY+Qg?=
 =?us-ascii?Q?6r8ZQoIiM1dOGHpXvxz3T83R1T3Po5Y76dhYnR3Oet/JufP7ik+sVH0OImUd?=
 =?us-ascii?Q?BZZxJsAKVjDNMRqb7nz/vtrUA/eR10eP0Flr10tFF/kRsMrksy1ygshNseU0?=
 =?us-ascii?Q?LlnilHx13fzO3maQrd+dGxcgTg0JibhrcZ148QIt2ZmQLJG+yZt+hsFsotg1?=
 =?us-ascii?Q?Ry6Jkn3g7cM0pWxdo4iYzDLdGdhUx7gP0BDiA6teb0BQG2RoZLVuWRqrUAel?=
 =?us-ascii?Q?nExAx4O81dq+wof4NkF/XwyYuloLtro6HA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2304517f-5cd7-42d8-cc84-08dafa430a04
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 17:31:46.6997
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lEOaZAzJ+OUenIypCPWKpJb3Ldp2eEI710aqSJYn3F4jpbnulFiakMesQeOCM/JLS9qNYR9ornG/C4v387zlMA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR10MB6785
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.122.1
 definitions=2023-01-19_11,2023-01-19_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 adultscore=0 mlxscore=0 mlxlogscore=999 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2301190144
X-Proofpoint-ORIG-GUID: 5nwhaBKjaBC8s4wFYA0ujA7Omqe1yJS1
X-Proofpoint-GUID: 5nwhaBKjaBC8s4wFYA0ujA7Omqe1yJS1
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 01/19/23 08:57, James Houghton wrote:
> > > > > 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?
> > >
> > > This is actually how mapcount was treated in HGM RFC v1 (though not
> > > refcount); it is doable for both [2].
> >
> > My apologies for being late to the party :)
> >
> > When Peter first brought up the issue with ref/map_count overflows I was
> > thinking that we should use a scheme like David describes above.  As
> > James points out, this was the approach taken in the first RFC.
> >
> > > One caveat here: if a page is unmapped in small pieces, it is
> > > difficult to know if the page is legitimately completely unmapped (we
> > > would have to check all the PTEs in the page table).
> >
> > Are we allowing unmapping of small (non-huge page sized) areas with HGM?
> > We must be if you are concerned with it.  What API would cause this?
> > I just do not remember this discussion.
> 
> There was some discussion about allowing MADV_DONTNEED on
> less-than-hugepage pieces [3] (it actually motivated the switch from
> UFFD_FEATURE_MINOR_HUGETLBFS_HGM to MADV_SPLIT). It isn't implemented
> in this series, but it could be implemented in the future.

OK, so we do not actually create HGM mappings until a uffd operation is
done at a less than huge page size granularity.  MADV_SPLIT just says
that HGM mappings are 'possible' for this vma.  Hopefully, my understanding
is correct.

I was concerned about things like the page fault path, but in that case
we have already 'entered HGM mode' via a uffd operation.

Both David and Peter have asked whether eliminating intermediate mapping
levels would be a simplification.  I trust your response that it would
not help much in the current design/implementation.  But, it did get me
thinking about something else.

Perhaps we have discussed this before, and perhaps it does not meet all
user needs, but one way possibly simplify this is:

- 'Enable HGM' via MADV_SPLIT.  Must be done at huge page (hstate)
  granularity.
- MADV_SPLIT implicitly unmaps everything with in the range.
- MADV_SPLIT says all mappings for this vma will now be done at a base
  (4K) page size granularity.  vma would be marked some way.
- I think this eliminates the need for hugetlb_pte's as we KNOW the
  mapping size.
- We still use huge pages to back 4K mappings, and we still have to deal
  with the ref/map_count issues.
- Code touching hugetlb page tables would KNOW the mapping size up front.

Again, apologies if we talked about and previously dismissed this type
of approach.

> > When I was thinking about this I was a bit concerned about having enough
> > information to know exactly when to inc or dec counts.  I was actually
> > worried about knowing to do the increment.  I don't recall how it was
> > done in the first RFC, but from a high level it would need to be done
> > when the first hstate level PTE is allocated/added to the page table.
> > Right?  My concern was with all the places where we could 'error out'
> > after allocating the PTE, but before initializing it.  I was just thinking
> > that we might need to scan the page table or keep metadata for better
> > or easier accounting.
> 
> The only two places where we can *create* a high-granularity page
> table are: __mcopy_atomic_hugetlb (UFFDIO_CONTINUE) and
> copy_hugetlb_page_range. RFC v1 did not properly deal with the cases
> where we error out. To correctly handle these cases, we basically have
> to do the pagecache lookup before touching the page table.
> 
> 1. For __mcopy_atomic_hugetlb, we can lookup the page before doing the
> PT walk/alloc. If PT walk tells us to inc the page ref/mapcount, we do
> so immediately. We can easily pass the page into
> hugetlb_mcopy_atomic_pte() (via 'pagep') .
> 
> 2. For copy_hugetlb_page_range() for VM_MAYSHARE, we can also do the
> lookup before we do the page table walk. I'm not sure how to support
> non-shared HGM mappings with this scheme (in this series, we also
> don't support non-shared; we return -EINVAL).
> NB: The only case where high-granularity mappings for !VM_MAYSHARE
> VMAs would come up is as a result of hwpoison.
> 
> So we can avoid keeping additional metadata for what this series is
> trying to accomplish, but if the above isn't acceptable, then I/we can
> try to come up with a scheme that would be acceptable.

Ok, I was thinking we had to deal with other code paths such as page
fault.  But, now I understand that is not the case with this design.

> There is also the possibility that the scheme implemented in this
> version of the series is acceptable (i.e., the page_mapcount() API
> difference, which results in slightly modified page migration behavior
> and smaps output, is ok... assuming we have the refcount overflow
> check).
> 
> >
> > I think Peter mentioned it elsewhere, we should come up with a workable
> > scheme for HGM ref/map counting.  This can be done somewhat independently.
> 
> FWIW, what makes the most sense to me right now is to implement the
> THP-like scheme and mark HGM as mutually exclusive with the vmemmap
> optimization. We can later come up with a scheme that lets us retain
> compatibility. (Is that what you mean by "this can be done somewhat
> independently", Mike?)

Sort of, I was only saying that getting the ref/map counting right seems
like a task than can be independently worked.  Using the THP-like scheme
is good.
-- 
Mike Kravetz