Message-ID:
 <MW4PR12MB68755A569FE893DC72D86616B9112@MW4PR12MB6875.namprd12.prod.outlook.com>
Date: Tue, 23 Apr 2024 19:32:43 +0000
From: Shivansh Vij <shivanshvij@...look.com>
To: Ryan Roberts <ryan.roberts@....com>, David Hildenbrand <david@...hat.com>,
	Mike Rapoport <rppt@...ux.ibm.com>
CC: Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>,
	Joey Gouly <joey.gouly@....com>, Ard Biesheuvel <ardb@...nel.org>, Mark
 Rutland <mark.rutland@....com>, Anshuman Khandual
	<anshuman.khandual@....com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kselftest@...r.kernel.org"
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v1 0/5] arm64/mm: uffd write-protect and soft-dirty
 tracking

Hey All,
 
>On 19/04/2024 18:12, David Hildenbrand wrote:
>> On 19.04.24 18:30, Mike Rapoport wrote:
>>> On Fri, Apr 19, 2024 at 11:45:14AM +0200, David Hildenbrand wrote:
>>>> On 19.04.24 10:33, Shivansh Vij wrote:
>>>>>> On 19/04/2024 08:43, Ryan Roberts wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> This series adds uffd write-protect and soft-dirty tracking support for
>>>>>>> arm64. I
>>>>>>> consider the soft-dirty support (patches 3 and 4) as RFC - see rationale
>>>>>>> below.
>>>>>>>
>>>>>>> That said, these are the last 2 SW bits and we may want to keep 1 bit in
>>>>>>> reserve
>>>>>>> for future use. soft-dirty is only used for CRIU to my knowledge, and it is
>>>>>>> thought that their use case could be solved with the more generic uffd-wp. So
>>>>>>> unless somebody makes a clear case for the inclusion of soft-dirty
>>>>>>> support, we
>>>>>>> are probably better off dropping patches 3 and 4 and keeping bit 63 for
>>>>>>> future
>>>>>>> use. Although note that the most recent attempt to add soft-dirty for
>>>>>>> arm64 was
>>>>>>> last month [1] so I'd like to give Shivansh Vij the opportunity to make the
>>>>>>> case.
>>>>>
>>>>> Appreciate the opportunity to provide input here.
>>>>>
>>>>> I picked option one (dirty tracking in arm) because it seems to be the
>>>>> simplest way to move forward, whereas it would be a relatively heavy
>>>>> effort to add uffd-wp support to CRIU.
>>>>>
>>>>>  From a performance perspective I am also a little worried that uffd
>>>>> will be slower than just tracking the dirty bits asynchronously with
>>>>> sw dirty, but maybe that's not as much of a concern with the addition
>>>>> of uffd-wp async.
>>>>>
>>>>> With all this being said, I'll defer to the wisdom of the crowd about
>>>>> which approach makes more sense - after all, with this patch we should
>>>>> get uffd-wp support on arm so at least there will be _a_ way forward
>>>>> for CRIU (albeit one requiring slightly more work).
>>>>
>>>> Ccing Mike and Peter. In 2017, Mike gave a presentation "Memory tracking for
>>>> iterative container migration"[1] at LPC
>>>>
>>>> Some key points are still true I think:
>>>> (1) More flexible and robust than soft-dirty
>>>> (2) May obsolete soft-dirty
>>>>
>>>> We further recently added a new UFFD_FEATURE_WP_ASYNC feature as part of
>>>> [2], because getting soft-dirty return reliable results in some cases turned
>>>> out rather hard to fix.
>
>But it sounds like the current soft-dirty semantic is sufficient for CRIU on
>other arches? If I understood correctly from my brief scan of the linked post,
>the problem is that soft-dirty can sometimes provide false-positives? So could
>result in uneccessary copy, but never lost data?

This is how I've always understood it as well.

>
>>>>>
>>>>> We might still have to optimize that approach for some very sparse large
>>>>> VMAs, but that should be solvable.
>>>>>
>>>>   "The major defect of this approach of dirty tracking is we need to
>>>>   populate the pgtables when tracking starts. Soft-dirty doesn't do it
>>>>   like that. It's unwanted in the case where the range of memory to track
>>>>   is huge and unpopulated (e.g., tracking updates on a 10G file with
>>>>   mmap() on top, without having any page cache installed yet). One way to
>>>>   improve this is to allow pte markers exist for larger than PTE level
>>>>   for PMD+. That will not change the interface if to implemented, so we
>>>>   can leave that for later.")[3]
>>>>
>>>>
>>>> If we can avoid adding soft-dirty on arm64 that would be great. This will
>>>> require work on the CRIU side. One downside of uffd-wp is that it is
>>>> currently not as avilable on architectures as soft-dirty.
>>>
>>> Using uffd-wp instead of soft-dirty in CRIU will require quite some work on
>>> CRIU side and probably on the kernel side too.
>>>
>>> And as of now we'll anyway have to maintain soft-dirty because powerpc and
>>> s390 don't have uffd-wp.
>>>
>>> With UFFD_FEATURE_WP_ASYNC the concern that uffd-wp will be slower than
>>> soft-dirty probably doesn't exist, but we won't know for sure until
>>> somebody will try.
>>>
>>> But there were other limitations, the most prominent was checkpointing an
>>> application that uses uffd. If CRIU is to use uffd-wp for tracking of the
>>> dirty pages, there should be some support for multiple uffd contexts for a
>>> VMA and that's surely a lot of work.
>>
>> Is it even already supported to checkpoint an application that is using uffd?
>> Hard to believe, what if the monitor is running in a completely different
>> process than the one being checkpointed?
>
>Shivansh, do you speak for CRIU? Are you able to comment on whether CRIU
>supports checkpointing an app that uses uffd?

I do not speak for CRIU - I'm just a user (and hopefully a future contributor), but not a maintainer or owner. I can however comment on whether CRIU supports checkpointing an app that uses UFFD - it doesn't. Looking through both the implementation of CRIU (specifically how they restore memory [1]), and at recently filed Github issues [2], it's pretty clear that CRIU doesn't support processes using UFFD - that they do not currently have plans to [3].

[1] https://github.com/checkpoint-restore/criu/blob/criu-2.x-stable/criu/mem.c#L683
[2] https://github.com/checkpoint-restore/criu/issues/2021
[3] https://github.com/checkpoint-restore/criu/issues/2021#issuecomment-1346971967

>>
>> Further ... isn't CRIU already using uffd in some cases? ...documentation
>> mentions [1] that it is used for "lazy (or post-copy) restore in CRIU". At least
>> if the documentation is correct and its actually implemented.
>>
>
>Shivansh, same question - do you know the current CRIU status/plans for using
>uffd-wp instead of soft-dirty? If CRIU doesn't currently implement it and has no
>current plans to, how can we guage interest in making a plan?
>

While I cannot gauge whether the maintainers or main contributors of CRIU plan on using uffd-wp instead of soft-dirty in the future, I can tell you that there is no currently open issue to track that work, and whenever anyone in the past has asked about ARM64 pre-dump support to CRIU (which is the feature that uses soft-dirty/would use uffd-wp), they've always just said it's not supported - but that they do want the feature [4]. 

So in summary, they want the feature, but no one is working on implementing it (either with soft-dirty or with uffd-wp). 

I doubt that CRIU would have any issues with adding the feature via soft-dirty (since, as shown in [4] they're interested in it), but as for using uffd-wp they definitely haven't shown any interest thus far. Based on the fact that it would be a very significant amount of work and it would really only be for ARM64 support (which they're already fine without), I'd be very surprised if they were interested in pursuing it.

[4] https://github.com/checkpoint-restore/criu/issues/1859#issuecomment-1972674047

>>
>>>
>>>> But I'll throw in another idea: do we really need soft-dirty and uffd-wp to
>>>> exist at the same time in the same process (or the VMA?). In theory, we
>
>My instinct is that MUXing a PTE bit like this will lead to some subtle problems
>that won't appear on arches that support either one or both of the features
>independently and unconditionally. Surely better to limit ourselves to either
>"arm64 will only support uffd-wp" or "arm64 will support both uffd-wp and
>soft-dirty". That way, we could move ahead with reviewing/merging the uffd-wp
>support asynchronously to deciding whether we want to support soft-dirty.
>

My personal preference is having both approaches supported - especially in the context of CRIU since I doubt they'll be willing to rewrite all of the dumping and restore logic just for ARM64 support. 

Powered by blists - more mailing lists