Re: [RFC PATCH 0/8] mm/madvise: support process_madvise(MADV_DONTNEED)

[Nadav Amit]

> On Oct 4, 2021, at 10:58 AM, David Hildenbrand <david@xxxxxxxxxx> wrote:
> 
>>> 
>>> Thanks for the pointer.
>>> 
>>> And my question would be if something like DAMON would actually be what you want.
>> I looked into DAMON and even with the proposed future extensions it sounds
>> as a different approach with certain benefits but with many limitations.
>> The major limitation of DAMON is that you need to predefine the logic you
>> want for reclamation into the kernel. You can add programability through
>> some API or even eBPF, but it would never be as easy or as versatile as
>> what user manager can achieve. We already have pretty much all the
>> facilities to do so from userspace, and the missing parts (at least for
>> basic userspace manager) are almost already there. In contrast, see how
>> many iterations are needed for the basic DAMON implementation.
> 
> I can see what you're saying when looking at optimizing a hand full of special applications. I yet fail to see how something like that could work as a full replacement for in kernel swapping. I'm happy to learn.

I am not arguing it is a full replacement, at least at this stage.

> 
>> The second, also big, difference is that DAMON looks only on reclamation.
>> If you want a custom prefetch scheme or different I/O stack for backing
>> storage, you cannot have such one.
> 
> I do wonder if it could be extended for prefetching. But I am absolutely not a DAMON expert.
> 
> [...]

These are 2 different approaches. One, is to provide some logic
for the kernel (DAMON). The other is to provide userspace full
control over paging operations (with caveats). Obviously, due to
the caveats, the kernel paging mechanism behaves as a backup.

> 
>>> 
>>> You raise a very excellent point (and it should have been part of your initial sales pitch): how does it differ to process_vm_writev().
>>> 
>>> I can say that it differs in a way that you can break applications in more extreme ways. Let me give you two examples:
>>> 
>>> 1. longterm pinnings: you raised this yourself; this can break an application silently and there is barely a safe way your tooling could handle it.
>>> 
>>> 2. pagemap: applications can depend on the populated(present |swap) information in the pagemap for correctness. For example, there was recently a discussion to use pagemap information to speed up live migration of VMs, by skipping migration of !populated pages. There is currently no way your tooling can fake that. In comparison, ordinary swapping in the kernel can handle it.
>> I understand (1). As for (2): the scenario that you mention sound
>> very specific, and one can argue that ignoring UFFD-registered
>> regions in such a case is either (1) wrong or (2) should trigger
>> some UFFD event.
>>> 
>>> Is it easy to break an application with process_vm_writev()? Yes. When talking about dynamic debugging, it's expected that you break the target already -- or the target is already broken. Is it easier to break an application with process_madvise(MADV_DONTNEED)? I'd say yes, especially when implementing something way beyond debugging as you describe.
>> If you do not know what you are doing, you can easily break anything.
>> Note that there are other APIs that can break your application even
>> worse, specifically ptrace().
>>> I'm giving you "a hard time" for the reason Michal raised: we discussed this in the past already at least two times IIRC and "it is a free ticket to all sorts of hard to debug problem" in our opinion; especially when we mess around in other process address spaces besides for debugging.
>>> 
>>> I'm not the person to ack/nack this, I'm just asking the questions :)
>> I see your points and I try to look for a path of least resistance.
>> I thought that process_madvise() is a nice interface to hook into.
> 
> It would be the right interface -- iff the operation wouldn't have a bad smell to it. We don't really want applications to mess around in the page table layout of some other process: however, that is exactly what you require. By unlocking that interface for that use case we agree that what you are proposing is a "sane use case", but  ...
> 
>> But if you are concerned it will be misused, how about adding instead
>> an IOCTL that will zap pages but only in UFFD-registered regions?
>> A separate IOCTL for this matter have an advantage of being more
>> tailored for UFFD, not to notify UFFD upon “remove” and to be less
>> likely to be misused.
> 
> ... that won't change the fact that with your user-space swapping approach that requires this interface we can break some applications silently, and that's really the major concern I have.
> 
> I mean, there are more cases where you can just harm the target application I think, for example if the target application uses SOFTDIRTY tracking.
> 
> 
> To judge if this is a sane use case we want to support, it would help a lot if there would be actual code+evaluation when actually implementing some of these advanced policies. Because you raise a lot of interesting points in your reply to Michal to back your use case, and naive me thinks "this sounds interesting but ... aren't we losing a lot of flexibility+features when doing this in user space? Does anyone actually want to do it like that?".
> 
> Again, I'm not the person to ack/nack this, I'm just questioning if the use case that requires this interface is actually something that will get used later in real life because it has real advantages, or if it's a pure research project that will get abandoned at some point and we ended up exposing an interface we really didn't want to expose so far (especially, because all other requests so far were bogus).

I do want to release the code, but it is really
incomplete/immature at this point. I would not that there additional
use cases, such as workloads that have discardable cache (or memoization
data), which want a central/another entity to discard the data when
there is memory pressure. (You can think about it as a userspace
shrinker).

Anyhow, as a path of least resistance, I think I would do the
following:

1. Wait for the other madvise related patches to be applied.
2. Simplify the patches, specifically removing the data structure
   changes based on Kirill feedback.
3. Defer the enablement of the MADV_DONTNEED until I can show
   code/performance numbers.

Regards,
Nadav