Message-ID: <a456a41d-c089-a639-b223-4412bad82e8d@redhat.com>
Date:   Mon, 4 Oct 2021 19:58:29 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Xu <peterx@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Minchan Kim <minchan@...nel.org>,
        Colin Cross <ccross@...gle.com>,
        Suren Baghdasarya <surenb@...gle.com>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>
Subject: Re: [RFC PATCH 0/8] mm/madvise: support
 process_madvise(MADV_DONTNEED)

>>
>> 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.

> 
> 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.

[...]

>>
>> 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).

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists