On Wed, Sep 22, 2021 at 11:58 AM Peter Xu <peterx@xxxxxxxxxx> wrote: > > On Wed, Sep 22, 2021 at 08:21:40PM +0200, David Hildenbrand wrote: > > On 22.09.21 19:51, Peter Xu wrote: > > > We forbid merging thps for uffd-wp enabled regions, by breaking the khugepaged > > > scanning right after we detected a uffd-wp armed pte (either present, or swap). > > > > > > It works, but it's less efficient, because those ptes only exist for VM_UFFD_WP > > > enabled VMAs. Checking against the vma flag would be more efficient, and good > > > enough. To be explicit, we could still be able to merge some thps for > > > VM_UFFD_WP regions before this patch as long as they have zero uffd-wp armed > > > ptes, however that's not a major target for thp collapse anyways. > > > > > > > Hm, are we sure there are no users that could benefit from the current > > handling? > > > > I'm thinking about long-term uffd-wp users that effectively end up wp-ing on > > only a small fraction of a gigantic vma, or always wp complete blocks in a > > certain granularity in the range of THP. > > Yes, that's a good question. > > > > > Databases come to mind ... > > One thing to mention is that this patch didn't forbid thp being used within a > uffd-wp-ed range. E.g., we still allow thp exist, we can uffd-wp a thp and > it'll split only until when the thp is written. > > While what this patch does is it stops khugepaged from proactively merging > those small pages into thps as long as VM_UFFD_WP|VM_UFFD_MINOR is set. It may > still affect some user, but it's not a complete disable on thp. > > > > > In the past, I played with the idea of using uffd-wp to protect access to > > logically unplugged memory regions part of virtio-mem devices in QEMU -- > > which would exactly do something as described above. But I'll most probably > > be using ordinary uffd once any users that might read such logically > > unplugged memory have been "fixed". > > Yes, even if you'd like to keep using uffd-wp that sounds like a very > reasonable scenario. > > > > > The change itself looks sane to me AFAIKT. > > So one major motivation of this patch of mine is to prepare for shmem, because > the old commit obviously only covered anonymous. > > But after a 2nd thought, I just noticed shmem shouldn't have a problem with > khugepaged merging at all! > > The thing is, when khugepaged is merging a shmem thp, unlike anonymous, it'll > not merge the ptes into a pmd, but it'll simply zap the ptes. It means all > uffd-wp tracking information won't be lost even if merging happened, those info > will still be kept inside pgtables using (the upcoming) pte markers. khugepqged does remove the pgtables. Please check out retract_page_tables(). The pmd will be cleared and the ptes will be freed otherwise the collapsed THP won't get PMD mapped by later access. > > When faulted, we'll just do small page mappings while it won't stop the shmem > thp from being mapped hugely in other mm, afaict. > > With that in mind, indeed I see this patch less necessary to be merged; so for > sparsely wr-protected vmas like virtio-mem we can still keep some of the ranges > mergeable, that sounds a good thing to keep it as-is. > > NACK myself for now: let's not lose that good property of both thp+uffd-wp so > easily, and I'll think more of it. > > (To Axel: my question to minor mode handling thp still stands, I think..) > > Thanks, > > -- > Peter Xu > >
