Message-ID: <CAKOZueupb10vmm-bmL0j_b__qsC9ZrzhzHgpGhwPVUrfX0X-Og@mail.gmail.com>
Date:   Wed, 22 May 2019 06:16:35 -0700
From:   Daniel Colascione <dancol@...gle.com>
To:     Christian Brauner <christian@...uner.io>
Cc:     Minchan Kim <minchan@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>, Michal Hocko <mhocko@...e.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Tim Murray <timmurray@...gle.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Sonny Rao <sonnyrao@...gle.com>,
        Brian Geffon <bgeffon@...gle.com>, Jann Horn <jannh@...gle.com>
Subject: Re: [RFC 0/7] introduce memory hinting API for external process

On Wed, May 22, 2019 at 1:22 AM Christian Brauner <christian@...uner.io> wrote:
>
> On Wed, May 22, 2019 at 7:12 AM Daniel Colascione <dancol@...gle.com> wrote:
> >
> > On Tue, May 21, 2019 at 4:39 AM Christian Brauner <christian@...uner.io> wrote:
> > >
> > > On Tue, May 21, 2019 at 01:30:29PM +0200, Christian Brauner wrote:
> > > > On Tue, May 21, 2019 at 08:05:52PM +0900, Minchan Kim wrote:
> > > > > On Tue, May 21, 2019 at 10:42:00AM +0200, Christian Brauner wrote:
> > > > > > On Mon, May 20, 2019 at 12:52:47PM +0900, Minchan Kim wrote:
> > > > > > > - Background
> > > > > > >
> > > > > > > The Android terminology used for forking a new process and starting an app
> > > > > > > from scratch is a cold start, while resuming an existing app is a hot start.
> > > > > > > While we continually try to improve the performance of cold starts, hot
> > > > > > > starts will always be significantly less power hungry as well as faster so
> > > > > > > we are trying to make hot start more likely than cold start.
> > > > > > >
> > > > > > > To increase hot start, Android userspace manages the order that apps should
> > > > > > > be killed in a process called ActivityManagerService. ActivityManagerService
> > > > > > > tracks every Android app or service that the user could be interacting with
> > > > > > > at any time and translates that into a ranked list for lmkd(low memory
> > > > > > > killer daemon). They are likely to be killed by lmkd if the system has to
> > > > > > > reclaim memory. In that sense they are similar to entries in any other cache.
> > > > > > > Those apps are kept alive for opportunistic performance improvements but
> > > > > > > those performance improvements will vary based on the memory requirements of
> > > > > > > individual workloads.
> > > > > > >
> > > > > > > - Problem
> > > > > > >
> > > > > > > Naturally, cached apps were dominant consumers of memory on the system.
> > > > > > > However, they were not significant consumers of swap even though they are
> > > > > > > good candidate for swap. Under investigation, swapping out only begins
> > > > > > > once the low zone watermark is hit and kswapd wakes up, but the overall
> > > > > > > allocation rate in the system might trip lmkd thresholds and cause a cached
> > > > > > > process to be killed(we measured performance swapping out vs. zapping the
> > > > > > > memory by killing a process. Unsurprisingly, zapping is 10x times faster
> > > > > > > even though we use zram which is much faster than real storage) so kill
> > > > > > > from lmkd will often satisfy the high zone watermark, resulting in very
> > > > > > > few pages actually being moved to swap.
> > > > > > >
> > > > > > > - Approach
> > > > > > >
> > > > > > > The approach we chose was to use a new interface to allow userspace to
> > > > > > > proactively reclaim entire processes by leveraging platform information.
> > > > > > > This allowed us to bypass the inaccuracy of the kernel’s LRUs for pages
> > > > > > > that are known to be cold from userspace and to avoid races with lmkd
> > > > > > > by reclaiming apps as soon as they entered the cached state. Additionally,
> > > > > > > it could provide many chances for platform to use much information to
> > > > > > > optimize memory efficiency.
> > > > > > >
> > > > > > > IMHO we should spell it out that this patchset complements MADV_WONTNEED
> > > > > > > and MADV_FREE by adding non-destructive ways to gain some free memory
> > > > > > > space. MADV_COLD is similar to MADV_WONTNEED in a way that it hints the
> > > > > > > kernel that memory region is not currently needed and should be reclaimed
> > > > > > > immediately; MADV_COOL is similar to MADV_FREE in a way that it hints the
> > > > > > > kernel that memory region is not currently needed and should be reclaimed
> > > > > > > when memory pressure rises.
> > > > > > >
> > > > > > > To achieve the goal, the patchset introduce two new options for madvise.
> > > > > > > One is MADV_COOL which will deactive activated pages and the other is
> > > > > > > MADV_COLD which will reclaim private pages instantly. These new options
> > > > > > > complement MADV_DONTNEED and MADV_FREE by adding non-destructive ways to
> > > > > > > gain some free memory space. MADV_COLD is similar to MADV_DONTNEED in a way
> > > > > > > that it hints the kernel that memory region is not currently needed and
> > > > > > > should be reclaimed immediately; MADV_COOL is similar to MADV_FREE in a way
> > > > > > > that it hints the kernel that memory region is not currently needed and
> > > > > > > should be reclaimed when memory pressure rises.
> > > > > > >
> > > > > > > This approach is similar in spirit to madvise(MADV_WONTNEED), but the
> > > > > > > information required to make the reclaim decision is not known to the app.
> > > > > > > Instead, it is known to a centralized userspace daemon, and that daemon
> > > > > > > must be able to initiate reclaim on its own without any app involvement.
> > > > > > > To solve the concern, this patch introduces new syscall -
> > > > > > >
> > > > > > >         struct pr_madvise_param {
> > > > > > >                 int size;
> > > > > > >                 const struct iovec *vec;
> > > > > > >         }
> > > > > > >
> > > > > > >         int process_madvise(int pidfd, ssize_t nr_elem, int *behavior,
> > > > > > >                                 struct pr_madvise_param *restuls,
> > > > > > >                                 struct pr_madvise_param *ranges,
> > > > > > >                                 unsigned long flags);
> > > > > > >
> > > > > > > The syscall get pidfd to give hints to external process and provides
> > > > > > > pair of result/ranges vector arguments so that it could give several
> > > > > > > hints to each address range all at once.
> > > > > > >
> > > > > > > I guess others have different ideas about the naming of syscall and options
> > > > > > > so feel free to suggest better naming.
> > > > > >
> > > > > > Yes, all new syscalls making use of pidfds should be named
> > > > > > pidfd_<action>. So please make this pidfd_madvise.
> > > > >
> > > > > I don't have any particular preference but just wondering why pidfd is
> > > > > so special to have it as prefix of system call name.
> > > >
> > > > It's a whole new API to address processes. We already have
> > > > clone(CLONE_PIDFD) and pidfd_send_signal() as you have seen since you
> > > > exported pidfd_to_pid(). And we're going to have pidfd_open(). Your
> > > > syscall works only with pidfds so it's tied to this api as well so it
> > > > should follow the naming scheme. This also makes life easier for
> > > > userspace and is consistent.
> > >
> > > This is at least my reasoning. I'm not going to make this a whole big
> > > pedantic argument. If people have really strong feelings about not using
> > > this prefix then fine. But if syscalls can be grouped together and have
> > > consistent naming this is always a big plus.
> >
> > My hope has been that pidfd use becomes normalized enough that
> > prefixing "pidfd_" to pidfd-accepting system calls becomes redundant.
> > We write write(), not fd_write(), right? :-) pidfd_open() makes sense
> > because the primary purpose of this system call is to operate on a
> > pidfd, but I think process_madvise() is fine.
>
> This madvise syscall just operates on pidfds. It would make sense to
> name it process_madvise() if were to operate both on pid_t and int pidfd.

The name of the function ought to encode its purpose, not its
signature. The system call under discussion operates on processes and
so should be called "process_madvise". That this system call happens
to accept a pidfd to identify the process on which it operates is not
its most interesting aspect of the system call. The argument type
isn't important enough to spotlight in the permanent name of an API.
Pidfds are novel now, but they won't be novel in the future.

> Giving specific names to system calls won't stop it from becoming
> normalized.

We could name system calls with `cat /dev/urandom | xxd` and they'd
still get used. It doesn't follow that all names are equally good.

Powered by blists - more mailing lists