* Re: [PATCH 1/2] open: add close_range()
From: Christian Brauner @ 2019-05-21 13:23 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-ia64, linux-sh, linux-kernel, dhowells, linux-kselftest,
sparclinux, shuah, linux-arch, linux-s390, miklos, x86, torvalds,
linux-mips, linux-xtensa, tkjos, arnd, jannh, linux-m68k, viro,
tglx, ldv, linux-arm-kernel, linux-parisc, linux-api, oleg,
linux-alpha, linux-fsdevel, linuxppc-dev
In-Reply-To: <87h89o9cng.fsf@oldenburg2.str.redhat.com>
On Tue, May 21, 2019 at 03:10:11PM +0200, Florian Weimer wrote:
> * Christian Brauner:
>
> >> Solaris has an fdwalk function:
> >>
> >> <https://docs.oracle.com/cd/E88353_01/html/E37843/closefrom-3c.html>
> >>
> >> So a different way to implement this would expose a nextfd system call
> >
> > Meh. If nextfd() then I would like it to be able to:
> > - get the nextfd(fd) >= fd
> > - get highest open fd e.g. nextfd(-1)
>
> The highest open descriptor isn't istering for fdwalk because nextfd
> would just fail.
Sure. I was thinking about other usecases. For example, sometimes in
userspace you want to do the following:
save_fd = dup(fd, <well-known-number-at-the-end-of-the-range);
close_range(3, (save_fd - 1));
Which brings me to another point. So even if we don't do close_range() I
would like libc to maybe give us something like close_range() for such
scenarios.
>
> > But then I wonder if nextfd() needs to be a syscall and isn't just
> > either:
> > fcntl(fd, F_GET_NEXT)?
> > or
> > prctl(PR_GET_NEXT)?
>
> I think the fcntl route is a bit iffy because you might need it to get
> the *first* valid descriptor.
>
> >> to userspace, so that we can use that to implement both fdwalk and
> >> closefrom. But maybe fdwalk is just too obscure, given the existence of
> >> /proc.
> >
> > Yeah we probably don't need fdwalk.
>
> Agreed. Just wanted to bring it up for completeness. I certainly don't
> want to derail the implementation of close_range.
>
> Thanks,
> Florian
^ permalink raw reply
* Re: [PATCH 1/2] open: add close_range()
From: Christian Brauner @ 2019-05-21 13:18 UTC (permalink / raw)
To: Florian Weimer
Cc: viro, linux-kernel, linux-fsdevel, linux-api, jannh, oleg, tglx,
torvalds, arnd, shuah, dhowells, tkjos, ldv, miklos, linux-alpha,
linux-arm-kernel, linux-ia64, linux-m68k, linux-mips,
linux-parisc, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-xtensa, linux-arch, linux-kselftest, x86
In-Reply-To: <87h89o9cng.fsf@oldenburg2.str.redhat.com>
On Tue, May 21, 2019 at 03:10:11PM +0200, Florian Weimer wrote:
> * Christian Brauner:
>
> >> Solaris has an fdwalk function:
> >>
> >> <https://docs.oracle.com/cd/E88353_01/html/E37843/closefrom-3c.html>
> >>
> >> So a different way to implement this would expose a nextfd system call
> >
> > Meh. If nextfd() then I would like it to be able to:
> > - get the nextfd(fd) >= fd
> > - get highest open fd e.g. nextfd(-1)
>
> The highest open descriptor isn't istering for fdwalk because nextfd
> would just fail.
>
> > But then I wonder if nextfd() needs to be a syscall and isn't just
> > either:
> > fcntl(fd, F_GET_NEXT)?
> > or
> > prctl(PR_GET_NEXT)?
>
> I think the fcntl route is a bit iffy because you might need it to get
> the *first* valid descriptor.
Oh, how would that be difficult? Maybe I'm missing context.
Couldn't you just do
fcntl(0, F_GET_NEXT)
>
> >> to userspace, so that we can use that to implement both fdwalk and
> >> closefrom. But maybe fdwalk is just too obscure, given the existence of
> >> /proc.
> >
> > Yeah we probably don't need fdwalk.
>
> Agreed. Just wanted to bring it up for completeness. I certainly don't
> want to derail the implementation of close_range.
No, that's perfectly fine. I mean, you clearly need this and are one of
the major stakeholders. For example, Rust (probably also Python) will
call down into libc and not use the syscall directly. They kinda do this
with getfdtable<sm> rn already.
So what you say makes sense for libc has some relevance for the other
tools as well.
Christian
^ permalink raw reply
* Re: [PATCH 1/2] open: add close_range()
From: Florian Weimer @ 2019-05-21 13:10 UTC (permalink / raw)
To: Christian Brauner
Cc: viro, linux-kernel, linux-fsdevel, linux-api, jannh, oleg, tglx,
torvalds, arnd, shuah, dhowells, tkjos, ldv, miklos, linux-alpha,
linux-arm-kernel, linux-ia64, linux-m68k, linux-mips,
linux-parisc, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-xtensa, linux-arch, linux-kselftest, x86
In-Reply-To: <20190521130438.q3u4wvve7p6md6cm@brauner.io>
* Christian Brauner:
>> Solaris has an fdwalk function:
>>
>> <https://docs.oracle.com/cd/E88353_01/html/E37843/closefrom-3c.html>
>>
>> So a different way to implement this would expose a nextfd system call
>
> Meh. If nextfd() then I would like it to be able to:
> - get the nextfd(fd) >= fd
> - get highest open fd e.g. nextfd(-1)
The highest open descriptor isn't istering for fdwalk because nextfd
would just fail.
> But then I wonder if nextfd() needs to be a syscall and isn't just
> either:
> fcntl(fd, F_GET_NEXT)?
> or
> prctl(PR_GET_NEXT)?
I think the fcntl route is a bit iffy because you might need it to get
the *first* valid descriptor.
>> to userspace, so that we can use that to implement both fdwalk and
>> closefrom. But maybe fdwalk is just too obscure, given the existence of
>> /proc.
>
> Yeah we probably don't need fdwalk.
Agreed. Just wanted to bring it up for completeness. I certainly don't
want to derail the implementation of close_range.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH 1/2] open: add close_range()
From: Christian Brauner @ 2019-05-21 13:04 UTC (permalink / raw)
To: Florian Weimer
Cc: viro, linux-kernel, linux-fsdevel, linux-api, jannh, oleg, tglx,
torvalds, arnd, shuah, dhowells, tkjos, ldv, miklos, linux-alpha,
linux-arm-kernel, linux-ia64, linux-m68k, linux-mips,
linux-parisc, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-xtensa, linux-arch, linux-kselftest, x86
In-Reply-To: <87tvdoau12.fsf@oldenburg2.str.redhat.com>
On Tue, May 21, 2019 at 02:09:29PM +0200, Florian Weimer wrote:
> * Christian Brauner:
>
> > +/**
> > + * __close_range() - Close all file descriptors in a given range.
> > + *
> > + * @fd: starting file descriptor to close
> > + * @max_fd: last file descriptor to close
> > + *
> > + * This closes a range of file descriptors. All file descriptors
> > + * from @fd up to and including @max_fd are closed.
> > + */
> > +int __close_range(struct files_struct *files, unsigned fd, unsigned max_fd)
> > +{
> > + unsigned int cur_max;
> > +
> > + if (fd > max_fd)
> > + return -EINVAL;
> > +
> > + rcu_read_lock();
> > + cur_max = files_fdtable(files)->max_fds;
> > + rcu_read_unlock();
> > +
> > + /* cap to last valid index into fdtable */
> > + if (max_fd >= cur_max)
> > + max_fd = cur_max - 1;
> > +
> > + while (fd <= max_fd)
> > + __close_fd(files, fd++);
> > +
> > + return 0;
> > +}
>
> This seems rather drastic. How long does this block in kernel mode?
> Maybe it's okay as long as the maximum possible value for cur_max stays
> around 4 million or so.
That's probably valid concern when you reach very high numbers though I
wonder how relevant this is in practice.
Also, you would only be blocking yourself I imagine, i.e. you can't DOS
another task with this unless your multi-threaded.
>
> Solaris has an fdwalk function:
>
> <https://docs.oracle.com/cd/E88353_01/html/E37843/closefrom-3c.html>
>
> So a different way to implement this would expose a nextfd system call
Meh. If nextfd() then I would like it to be able to:
- get the nextfd(fd) >= fd
- get highest open fd e.g. nextfd(-1)
But then I wonder if nextfd() needs to be a syscall and isn't just
either:
fcntl(fd, F_GET_NEXT)?
or
prctl(PR_GET_NEXT)?
Technically, one could also do:
fd_range(unsigned fd, unsigend end_fd, unsigned flags);
fd_range(3, 50, FD_RANGE_CLOSE);
/* return highest fd within the range [3, 50] */
fd_range(3, 50, FD_RANGE_NEXT);
/* return highest fd */
fd_range(3, UINT_MAX, FD_RANGE_NEXT);
This syscall could also reasonably be extended.
> to userspace, so that we can use that to implement both fdwalk and
> closefrom. But maybe fdwalk is just too obscure, given the existence of
> /proc.
Yeah we probably don't need fdwalk.
>
> I'll happily implement closefrom on top of close_range in glibc (plus
> fallback for older kernels based on /proc—with an abort in case that
> doesn't work because the RLIMIT_NOFILE hack is unreliable
> unfortunately).
>
> Thanks,
> Florian
^ permalink raw reply
* Re: [RFC 0/7] introduce memory hinting API for external process
From: Shakeel Butt @ 2019-05-21 12:56 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Tim Murray, Minchan Kim, Andrew Morton, LKML, linux-mm,
Michal Hocko, Johannes Weiner, Joel Fernandes, Suren Baghdasaryan,
Daniel Colascione, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <1754d0ef-6756-d88b-f728-17b1fe5d5b07@arm.com>
On Mon, May 20, 2019 at 7:55 PM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
>
> On 05/20/2019 10:29 PM, Tim Murray wrote:
> > On Sun, May 19, 2019 at 11:37 PM Anshuman Khandual
> > <anshuman.khandual@arm.com> wrote:
> >>
> >> Or Is the objective here is reduce the number of processes which get killed by
> >> lmkd by triggering swapping for the unused memory (user hinted) sooner so that
> >> they dont get picked by lmkd. Under utilization for zram hardware is a concern
> >> here as well ?
> >
> > The objective is to avoid some instances of memory pressure by
> > proactively swapping pages that userspace knows to be cold before
> > those pages reach the end of the LRUs, which in turn can prevent some
> > apps from being killed by lmk/lmkd. As soon as Android userspace knows
> > that an application is not being used and is only resident to improve
> > performance if the user returns to that app, we can kick off
> > process_madvise on that process's pages (or some portion of those
> > pages) in a power-efficient way to reduce memory pressure long before
> > the system hits the free page watermark. This allows the system more
> > time to put pages into zram versus waiting for the watermark to
> > trigger kswapd, which decreases the likelihood that later memory
> > allocations will cause enough pressure to trigger a kill of one of
> > these apps.
>
> So this opens up bit of LRU management to user space hints. Also because the app
> in itself wont know about the memory situation of the entire system, new system
> call needs to be called from an external process.
>
> >
> >> Swapping out memory into zram wont increase the latency for a hot start ? Or
> >> is it because as it will prevent a fresh cold start which anyway will be slower
> >> than a slow hot start. Just being curious.
> >
> > First, not all swapped pages will be reloaded immediately once an app
> > is resumed. We've found that an app's working set post-process_madvise
> > is significantly smaller than what an app allocates when it first
> > launches (see the delta between pswpin and pswpout in Minchan's
> > results). Presumably because of this, faulting to fetch from zram does
>
> pswpin 417613 1392647 975034 233.00
> pswpout 1274224 2661731 1387507 108.00
>
> IIUC the swap-in ratio is way higher in comparison to that of swap out. Is that
> always the case ? Or it tend to swap out from an active area of the working set
> which faulted back again.
>
> > not seem to introduce a noticeable hot start penalty, not does it
> > cause an increase in performance problems later in the app's
> > lifecycle. I've measured with and without process_madvise, and the
> > differences are within our noise bounds. Second, because we're not
>
> That is assuming that post process_madvise() working set for the application is
> always smaller. There is another challenge. The external process should ideally
> have the knowledge of active areas of the working set for an application in
> question for it to invoke process_madvise() correctly to prevent such scenarios.
>
> > preemptively evicting file pages and only making them more likely to
> > be evicted when there's already memory pressure, we avoid the case
> > where we process_madvise an app then immediately return to the app and
> > reload all file pages in the working set even though there was no
> > intervening memory pressure. Our initial version of this work evicted
>
> That would be the worst case scenario which should be avoided. Memory pressure
> must be a parameter before actually doing the swap out. But pages if know to be
> inactive/cold can be marked high priority to be swapped out.
>
> > file pages preemptively and did cause a noticeable slowdown (~15%) for
> > that case; this patch set avoids that slowdown. Finally, the benefit
> > from avoiding cold starts is huge. The performance improvement from
> > having a hot start instead of a cold start ranges from 3x for very
> > small apps to 50x+ for larger apps like high-fidelity games.
>
> Is there any other real world scenario apart from this app based ecosystem where
> user hinted LRU management might be helpful ? Just being curious. Thanks for the
> detailed explanation. I will continue looking into this series.
Chrome OS is another real world use-case for this user hinted LRU
management approach by proactively reclaiming reclaim from tabs not
accessed by the user for some time.
^ permalink raw reply
* Re: [RFC 0/7] introduce memory hinting API for external process
From: Shakeel Butt @ 2019-05-21 12:53 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Michal Hocko, Johannes Weiner,
Tim Murray, Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190520035254.57579-1-minchan@kernel.org>
On Sun, May 19, 2019 at 8:53 PM Minchan Kim <minchan@kernel.org> 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.
It is not clear what exactly is the problem from the above para. IMO
low usage of swap is not the problem but rather global memory pressure
and the reactive response to it is the problem. Killing apps over swap
is preferred as you have noted zapping frees memory faster but it
indirectly increases cold start. Also swapping on allocation causes
latency issues for the app. So, a proactive mechanism is needed to
keep global pressure away and indirectly reduces cold starts and alloc
stalls.
>
> - 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.
I think it would be good to have clear reasoning on why "reclaim from
userspace" approach is taken. Android runtime clearly has more
accurate stale/cold information at the app/process level and can
positively influence kernel's reclaim decisions. So, "reclaim from
userspace" approach makes total sense for Android. I envision that
Chrome OS would be another very obvious user of this approach. There
can be tens of tabs which the user have not touched for sometime.
Chrome OS can proactively reclaim memory from such tabs.
>
> IMHO we should spell it out that this patchset complements MADV_WONTNEED
MADV_DONTNEED? same at couple of places below.
> 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.
>
> - Experiment
>
> We did bunch of testing with several hundreds of real users, not artificial
> benchmark on android. We saw about 17% cold start decreasement without any
> significant battery/app startup latency issues. And with artificial benchmark
> which launches and switching apps, we saw average 7% app launching improvement,
> 18% less lmkd kill and good stat from vmstat.
>
> A is vanilla and B is process_madvise.
>
>
> A B delta ratio(%)
> allocstall_dma 0 0 0 0.00
> allocstall_movable 1464 457 -1007 -69.00
> allocstall_normal 263210 190763 -72447 -28.00
> allocstall_total 264674 191220 -73454 -28.00
> compact_daemon_wake 26912 25294 -1618 -7.00
> compact_fail 17885 14151 -3734 -21.00
> compact_free_scanned 4204766409 3835994922 -368771487 -9.00
> compact_isolated 3446484 2967618 -478866 -14.00
> compact_migrate_scanned 1621336411 1324695710 -296640701 -19.00
> compact_stall 19387 15343 -4044 -21.00
> compact_success 1502 1192 -310 -21.00
> kswapd_high_wmark_hit_quickly 234 184 -50 -22.00
> kswapd_inodesteal 221635 233093 11458 5.00
> kswapd_low_wmark_hit_quickly 66065 54009 -12056 -19.00
> nr_dirtied 259934 296476 36542 14.00
> nr_vmscan_immediate_reclaim 2587 2356 -231 -9.00
> nr_vmscan_write 1274232 2661733 1387501 108.00
> nr_written 1514060 2937560 1423500 94.00
> pageoutrun 67561 55133 -12428 -19.00
> pgactivate 2335060 1984882 -350178 -15.00
> pgalloc_dma 13743011 14096463 353452 2.00
> pgalloc_movable 0 0 0 0.00
> pgalloc_normal 18742440 16802065 -1940375 -11.00
> pgalloc_total 32485451 30898528 -1586923 -5.00
> pgdeactivate 4262210 2930670 -1331540 -32.00
> pgfault 30812334 31085065 272731 0.00
> pgfree 33553970 31765164 -1788806 -6.00
> pginodesteal 33411 15084 -18327 -55.00
> pglazyfreed 0 0 0 0.00
> pgmajfault 551312 1508299 956987 173.00
> pgmigrate_fail 43927 29330 -14597 -34.00
> pgmigrate_success 1399851 1203922 -195929 -14.00
> pgpgin 24141776 19032156 -5109620 -22.00
> pgpgout 959344 1103316 143972 15.00
> pgpgoutclean 4639732 3765868 -873864 -19.00
> pgrefill 4884560 3006938 -1877622 -39.00
> pgrotated 37828 25897 -11931 -32.00
> pgscan_direct 1456037 957567 -498470 -35.00
> pgscan_direct_throttle 0 0 0 0.00
> pgscan_kswapd 6667767 5047360 -1620407 -25.00
> pgscan_total 8123804 6004927 -2118877 -27.00
> pgskip_dma 0 0 0 0.00
> pgskip_movable 0 0 0 0.00
> pgskip_normal 14907 25382 10475 70.00
> pgskip_total 14907 25382 10475 70.00
> pgsteal_direct 1118986 690215 -428771 -39.00
> pgsteal_kswapd 4750223 3657107 -1093116 -24.00
> pgsteal_total 5869209 4347322 -1521887 -26.00
> pswpin 417613 1392647 975034 233.00
> pswpout 1274224 2661731 1387507 108.00
> slabs_scanned 13686905 10807200 -2879705 -22.00
> workingset_activate 668966 569444 -99522 -15.00
> workingset_nodereclaim 38957 32621 -6336 -17.00
> workingset_refault 2816795 2179782 -637013 -23.00
> workingset_restore 294320 168601 -125719 -43.00
>
> pgmajfault is increased by 173% because swapin is increased by 200% by
> process_madvise hint. However, swap read based on zram is much cheaper
> than file IO in performance point of view and app hot start by swapin is
> also cheaper than cold start from the beginning of app which needs many IO
> from storage and initialization steps.
>
> This patchset is against on next-20190517.
>
> Minchan Kim (7):
> mm: introduce MADV_COOL
> mm: change PAGEREF_RECLAIM_CLEAN with PAGE_REFRECLAIM
> mm: introduce MADV_COLD
> mm: factor out madvise's core functionality
> mm: introduce external memory hinting API
> mm: extend process_madvise syscall to support vector arrary
> mm: madvise support MADV_ANONYMOUS_FILTER and MADV_FILE_FILTER
>
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> include/linux/page-flags.h | 1 +
> include/linux/page_idle.h | 15 +
> include/linux/proc_fs.h | 1 +
> include/linux/swap.h | 2 +
> include/linux/syscalls.h | 2 +
> include/uapi/asm-generic/mman-common.h | 12 +
> include/uapi/asm-generic/unistd.h | 2 +
> kernel/signal.c | 2 +-
> kernel/sys_ni.c | 1 +
> mm/madvise.c | 600 +++++++++++++++++++++----
> mm/swap.c | 43 ++
> mm/vmscan.c | 80 +++-
> 14 files changed, 680 insertions(+), 83 deletions(-)
>
> --
> 2.21.0.1020.gf2820cf01a-goog
>
^ permalink raw reply
* Re: [PATCH 1/2] open: add close_range()
From: Florian Weimer @ 2019-05-21 12:09 UTC (permalink / raw)
To: Christian Brauner
Cc: viro, linux-kernel, linux-fsdevel, linux-api, jannh, oleg, tglx,
torvalds, arnd, shuah, dhowells, tkjos, ldv, miklos, linux-alpha,
linux-arm-kernel, linux-ia64, linux-m68k, linux-mips,
linux-parisc, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-xtensa, linux-arch, linux-kselftest, x86
In-Reply-To: <20190521113448.20654-1-christian@brauner.io>
* Christian Brauner:
> +/**
> + * __close_range() - Close all file descriptors in a given range.
> + *
> + * @fd: starting file descriptor to close
> + * @max_fd: last file descriptor to close
> + *
> + * This closes a range of file descriptors. All file descriptors
> + * from @fd up to and including @max_fd are closed.
> + */
> +int __close_range(struct files_struct *files, unsigned fd, unsigned max_fd)
> +{
> + unsigned int cur_max;
> +
> + if (fd > max_fd)
> + return -EINVAL;
> +
> + rcu_read_lock();
> + cur_max = files_fdtable(files)->max_fds;
> + rcu_read_unlock();
> +
> + /* cap to last valid index into fdtable */
> + if (max_fd >= cur_max)
> + max_fd = cur_max - 1;
> +
> + while (fd <= max_fd)
> + __close_fd(files, fd++);
> +
> + return 0;
> +}
This seems rather drastic. How long does this block in kernel mode?
Maybe it's okay as long as the maximum possible value for cur_max stays
around 4 million or so.
Solaris has an fdwalk function:
<https://docs.oracle.com/cd/E88353_01/html/E37843/closefrom-3c.html>
So a different way to implement this would expose a nextfd system call
to userspace, so that we can use that to implement both fdwalk and
closefrom. But maybe fdwalk is just too obscure, given the existence of
/proc.
I'll happily implement closefrom on top of close_range in glibc (plus
fallback for older kernels based on /proc—with an abort in case that
doesn't work because the RLIMIT_NOFILE hack is unreliable
unfortunately).
Thanks,
Florian
^ permalink raw reply
* [PATCH 2/2] tests: add close_range() tests
From: Christian Brauner @ 2019-05-21 11:34 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: jannh, fweimer, oleg, tglx, torvalds, arnd, shuah, dhowells,
tkjos, ldv, miklos, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-xtensa, linux-arch, linux-kselftest,
x86, Christian Brauner
In-Reply-To: <20190521113448.20654-1-christian@brauner.io>
This adds basic tests for the new close_range() syscall.
- test that no invalid flags can be passed
- test that a range of file descriptors is correctly closed
- test that a range of file descriptors is correctly closed if there there
are already closed file descriptors in the range
- test that max_fd is correctly capped to the current fdtable maximum
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dmitry V. Levin <ldv@altlinux.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: linux-api@vger.kernel.org
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/core/.gitignore | 1 +
tools/testing/selftests/core/Makefile | 6 +
.../testing/selftests/core/close_range_test.c | 128 ++++++++++++++++++
4 files changed, 136 insertions(+)
create mode 100644 tools/testing/selftests/core/.gitignore
create mode 100644 tools/testing/selftests/core/Makefile
create mode 100644 tools/testing/selftests/core/close_range_test.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 9781ca79794a..06e57fabbff9 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -4,6 +4,7 @@ TARGETS += bpf
TARGETS += breakpoints
TARGETS += capabilities
TARGETS += cgroup
+TARGETS += core
TARGETS += cpufreq
TARGETS += cpu-hotplug
TARGETS += drivers/dma-buf
diff --git a/tools/testing/selftests/core/.gitignore b/tools/testing/selftests/core/.gitignore
new file mode 100644
index 000000000000..6e6712ce5817
--- /dev/null
+++ b/tools/testing/selftests/core/.gitignore
@@ -0,0 +1 @@
+close_range_test
diff --git a/tools/testing/selftests/core/Makefile b/tools/testing/selftests/core/Makefile
new file mode 100644
index 000000000000..de3ae68aa345
--- /dev/null
+++ b/tools/testing/selftests/core/Makefile
@@ -0,0 +1,6 @@
+CFLAGS += -g -I../../../../usr/include/ -I../../../../include
+
+TEST_GEN_PROGS := close_range_test
+
+include ../lib.mk
+
diff --git a/tools/testing/selftests/core/close_range_test.c b/tools/testing/selftests/core/close_range_test.c
new file mode 100644
index 000000000000..ab10cd205ab9
--- /dev/null
+++ b/tools/testing/selftests/core/close_range_test.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/kernel.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <unistd.h>
+
+#include "../kselftest.h"
+
+static inline int sys_close_range(unsigned int fd, unsigned int max_fd,
+ unsigned int flags)
+{
+ return syscall(__NR_close_range, fd, max_fd, flags);
+}
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#endif
+
+int main(int argc, char **argv)
+{
+ const char *test_name = "close_range";
+ int i, ret;
+ int open_fds[100];
+ int fd_max, fd_mid, fd_min;
+
+ ksft_set_plan(7);
+
+ for (i = 0; i < ARRAY_SIZE(open_fds); i++) {
+ int fd;
+
+ fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ if (errno == ENOENT)
+ ksft_exit_skip(
+ "%s test: skipping test since /dev/null does not exist\n",
+ test_name);
+
+ ksft_exit_fail_msg(
+ "%s test: %s - failed to open /dev/null\n",
+ strerror(errno), test_name);
+ }
+
+ open_fds[i] = fd;
+ }
+
+ fd_min = open_fds[0];
+ fd_max = open_fds[99];
+
+ ret = sys_close_range(fd_min, fd_max, 1);
+ if (!ret)
+ ksft_exit_fail_msg(
+ "%s test: managed to pass invalid flag value\n",
+ test_name);
+ ksft_test_result_pass("do not allow invalid flag values for close_range()\n");
+
+ fd_mid = open_fds[50];
+ ret = sys_close_range(fd_min, fd_mid, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 4 to 50\n",
+ test_name);
+ ksft_test_result_pass("close_range() from %d to %d\n", fd_min, fd_mid);
+
+ for (i = 0; i <= 50; i++) {
+ ret = fcntl(open_fds[i], F_GETFL);
+ if (ret >= 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 4 to 50\n",
+ test_name);
+ }
+ ksft_test_result_pass("fcntl() verify closed range from %d to %d\n", fd_min, fd_mid);
+
+ /* create a couple of gaps */
+ close(57);
+ close(78);
+ close(81);
+ close(82);
+ close(84);
+ close(90);
+
+ fd_mid = open_fds[51];
+ /* Choose slightly lower limit and leave some fds for a later test */
+ fd_max = open_fds[92];
+ ret = sys_close_range(fd_mid, fd_max, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 51 to 100\n",
+ test_name);
+ ksft_test_result_pass("close_range() from %d to %d\n", fd_mid, fd_max);
+
+ for (i = 51; i <= 92; i++) {
+ ret = fcntl(open_fds[i], F_GETFL);
+ if (ret >= 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 51 to 100\n",
+ test_name);
+ }
+ ksft_test_result_pass("fcntl() verify closed range from %d to %d\n", fd_mid, fd_max);
+
+ fd_mid = open_fds[93];
+ fd_max = open_fds[99];
+ /* test that the kernel caps and still closes all fds */
+ ret = sys_close_range(fd_mid, UINT_MAX, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 51 to 100\n",
+ test_name);
+ ksft_test_result_pass("close_range() from %d to %d\n", fd_mid, fd_max);
+
+ for (i = 93; i < 100; i++) {
+ ret = fcntl(open_fds[i], F_GETFL);
+ if (ret >= 0)
+ ksft_exit_fail_msg(
+ "%s test: Failed to close range of file descriptors from 51 to 100\n",
+ test_name);
+ }
+ ksft_test_result_pass("fcntl() verify closed range from %d to %d\n", fd_mid, fd_max);
+
+ return ksft_exit_pass();
+}
--
2.21.0
^ permalink raw reply related
* [PATCH 1/2] open: add close_range()
From: Christian Brauner @ 2019-05-21 11:34 UTC (permalink / raw)
To: viro, linux-kernel, linux-fsdevel, linux-api
Cc: jannh, fweimer, oleg, tglx, torvalds, arnd, shuah, dhowells,
tkjos, ldv, miklos, linux-alpha, linux-arm-kernel, linux-ia64,
linux-m68k, linux-mips, linux-parisc, linuxppc-dev, linux-s390,
linux-sh, sparclinux, linux-xtensa, linux-arch, linux-kselftest,
x86, Christian Brauner
This adds the close_range() syscall. It allows to efficiently close a range
of file descriptors up to all file descriptors of a calling task.
The syscall came up in a recent discussion around the new mount API and
making new file descriptor types cloexec by default. During this
discussion, Al suggested the close_range() syscall (cf. [1]). Note, a
syscall in this manner has been requested by various people over time.
First, it helps to close all file descriptors of an exec()ing task. This
can be done safely via (quoting Al's example from [1] verbatim):
/* that exec is sensitive */
unshare(CLONE_FILES);
/* we don't want anything past stderr here */
close_range(3, ~0U);
execve(....);
The code snippet above is one way of working around the problem that file
descriptors are not cloexec by default. This is aggravated by the fact that
we can't just switch them over without massively regressing userspace. For
a whole class of programs having an in-kernel method of closing all file
descriptors is very helpful (e.g. demons, service managers, programming
language standard libraries, container managers etc.).
(Please note, unshare(CLONE_FILES) should only be needed if the calling
task is multi-threaded and shares the file descriptor table with another
thread in which case two threads could race with one thread allocating
file descriptors and the other one closing them via close_range(). For the
general case close_range() before the execve() is sufficient.)
Second, it allows userspace to avoid implementing closing all file
descriptors by parsing through /proc/<pid>/fd/* and calling close() on each
file descriptor. From looking at various large(ish) userspace code bases
this or similar patterns are very common in:
- service managers (cf. [4])
- libcs (cf. [6])
- container runtimes (cf. [5])
- programming language runtimes/standard libraries
- Python (cf. [2])
- Rust (cf. [7], [8])
As Dmitry pointed out there's even a long-standing glibc bug about missing
kernel support for this task (cf. [3]).
In addition, the syscall will also work for tasks that do not have procfs
mounted and on kernels that do not have procfs support compiled in. In such
situations the only way to make sure that all file descriptors are closed
is to call close() on each file descriptor up to UINT_MAX or RLIMIT_NOFILE,
OPEN_MAX trickery (cf. comment [8] on Rust).
The performance is striking. For good measure, comparing the following
simple close_all_fds() userspace implementation that is essentially just
glibc's version in [6]:
static int close_all_fds(void)
{
DIR *dir;
struct dirent *direntp;
dir = opendir("/proc/self/fd");
if (!dir)
return -1;
while ((direntp = readdir(dir))) {
int fd;
if (strcmp(direntp->d_name, ".") == 0)
continue;
if (strcmp(direntp->d_name, "..") == 0)
continue;
fd = atoi(direntp->d_name);
if (fd == 0 || fd == 1 || fd == 2)
continue;
close(fd);
}
closedir(dir); /* cannot fail */
return 0;
}
to close_range() yields:
1. closing 4 open files:
- close_all_fds(): ~280 us
- close_range(): ~24 us
2. closing 1000 open files:
- close_all_fds(): ~5000 us
- close_range(): ~800 us
close_range() is designed to allow for some flexibility. Specifically, it
does not simply always close all open file descriptors of a task. Instead,
callers can specify an upper bound.
This is e.g. useful for scenarios where specific file descriptors are
created with well-known numbers that are supposed to be excluded from
getting closed.
For extra paranoia close_range() comes with a flags argument. This can e.g.
be used to implement extension. Once can imagine userspace wanting to stop
at the first error instead of ignoring errors under certain circumstances.
There might be other valid ideas in the future. In any case, a flag
argument doesn't hurt and keeps us on the safe side.
>From an implementation side this is kept rather dumb. It saw some input
from David and Jann but all nonsense is obviously my own!
- Errors to close file descriptors are currently ignored. (Could be changed
by setting a flag in the future if needed.)
- __close_range() is a rather simplistic wrapper around __close_fd().
My reasoning behind this is based on the nature of how __close_fd() needs
to release an fd. But maybe I misunderstood specifics:
We take the files_lock and rcu-dereference the fdtable of the calling
task, we find the entry in the fdtable, get the file and need to release
files_lock before calling filp_close().
In the meantime the fdtable might have been altered so we can't just
retake the spinlock and keep the old rcu-reference of the fdtable
around. Instead we need to grab a fresh reference to the fdtable.
If my reasoning is correct then there's really no point in fancyfying
__close_range(): We just need to rcu-dereference the fdtable of the
calling task once to cap the max_fd value correctly and then go on
calling __close_fd() in a loop.
/* References */
[1]: https://lore.kernel.org/lkml/20190516165021.GD17978@ZenIV.linux.org.uk/
[2]: https://github.com/python/cpython/blob/9e4f2f3a6b8ee995c365e86d976937c141d867f8/Modules/_posixsubprocess.c#L220
[3]: https://sourceware.org/bugzilla/show_bug.cgi?id=10353#c7
[4]: https://github.com/systemd/systemd/blob/5238e9575906297608ff802a27e2ff9effa3b338/src/basic/fd-util.c#L217
[5]: https://github.com/lxc/lxc/blob/ddf4b77e11a4d08f09b7b9cd13e593f8c047edc5/src/lxc/start.c#L236
[6]: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/grantpt.c;h=2030e07fa6e652aac32c775b8c6e005844c3c4eb;hb=HEAD#l17
Note that this is an internal implementation that is not exported.
Currently, libc seems to not provide an exported version of this
because of missing kernel support to do this.
[7]: https://github.com/rust-lang/rust/issues/12148
[8]: https://github.com/rust-lang/rust/blob/5f47c0613ed4eb46fca3633c1297364c09e5e451/src/libstd/sys/unix/process2.rs#L303-L308
Rust's solution is slightly different but is equally unperformant.
Rust calls getdtablesize() which is a glibc library function that
simply returns the current RLIMIT_NOFILE or OPEN_MAX values. Rust then
goes on to call close() on each fd. That's obviously overkill for most
tasks. Rarely, tasks - especially non-demons - hit RLIMIT_NOFILE or
OPEN_MAX.
Let's be nice and assume an unprivileged user with RLIMIT_NOFILE set
to 1024. Even in this case, there's a very high chance that in the
common case Rust is calling the close() syscall 1021 times pointlessly
if the task just has 0, 1, and 2 open.
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jann Horn <jannh@google.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Dmitry V. Levin <ldv@altlinux.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: linux-api@vger.kernel.org
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd32.h | 2 ++
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/file.c | 30 +++++++++++++++++++++
fs/open.c | 20 ++++++++++++++
include/linux/fdtable.h | 2 ++
include/linux/syscalls.h | 2 ++
include/uapi/asm-generic/unistd.h | 4 ++-
22 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 9e7704e44f6d..b55d93af8096 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -473,3 +473,4 @@
541 common fsconfig sys_fsconfig
542 common fsmount sys_fsmount
543 common fspick sys_fspick
+545 common close_range sys_close_range
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index aaf479a9e92d..0125c97c75dd 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -447,3 +447,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index c39e90600bb3..9a3270d29b42 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -886,6 +886,8 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig)
__SYSCALL(__NR_fsmount, sys_fsmount)
#define __NR_fspick 433
__SYSCALL(__NR_fspick, sys_fspick)
+#define __NR_close_range 435
+__SYSCALL(__NR_close_range, sys_close_range)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index e01df3f2f80d..1a90b464e96f 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -354,3 +354,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 7e3d0734b2f3..2dee2050f9ef 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -433,3 +433,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 26339e417695..923ef69e5a76 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -439,3 +439,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 0e2dd68ade57..967ed9de51cd 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -372,3 +372,4 @@
431 n32 fsconfig sys_fsconfig
432 n32 fsmount sys_fsmount
433 n32 fspick sys_fspick
+435 n32 close_range sys_close_range
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 5eebfa0d155c..71de731102b1 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -348,3 +348,4 @@
431 n64 fsconfig sys_fsconfig
432 n64 fsmount sys_fsmount
433 n64 fspick sys_fspick
+435 n64 close_range sys_close_range
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 3cc1374e02d0..5a325ab29f88 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -421,3 +421,4 @@
431 o32 fsconfig sys_fsconfig
432 o32 fsmount sys_fsmount
433 o32 fspick sys_fspick
+435 o32 close_range sys_close_range
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index c9e377d59232..dcc0a0879139 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -430,3 +430,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 103655d84b4b..ba2c1f078cbd 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -515,3 +515,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index e822b2964a83..d7c9043d2902 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -436,3 +436,4 @@
431 common fsconfig sys_fsconfig sys_fsconfig
432 common fsmount sys_fsmount sys_fsmount
433 common fspick sys_fspick sys_fspick
+435 common close_range sys_close_range sys_close_range
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 016a727d4357..9b5e6bf0ce32 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -436,3 +436,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index e047480b1605..8c674a1e0072 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -479,3 +479,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ad968b7bac72..7f7a89a96707 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -438,3 +438,4 @@
431 i386 fsconfig sys_fsconfig __ia32_sys_fsconfig
432 i386 fsmount sys_fsmount __ia32_sys_fsmount
433 i386 fspick sys_fspick __ia32_sys_fspick
+435 i386 close_range sys_close_range __ia32_sys_close_range
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index b4e6f9e6204a..0f7d47ae921c 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -355,6 +355,7 @@
431 common fsconfig __x64_sys_fsconfig
432 common fsmount __x64_sys_fsmount
433 common fspick __x64_sys_fspick
+435 common close_range __x64_sys_close_range
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 5fa0ee1c8e00..b489532265d0 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -404,3 +404,4 @@
431 common fsconfig sys_fsconfig
432 common fsmount sys_fsmount
433 common fspick sys_fspick
+435 common close_range sys_close_range
diff --git a/fs/file.c b/fs/file.c
index 3da91a112bab..3680977a685a 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -641,6 +641,36 @@ int __close_fd(struct files_struct *files, unsigned fd)
}
EXPORT_SYMBOL(__close_fd); /* for ksys_close() */
+/**
+ * __close_range() - Close all file descriptors in a given range.
+ *
+ * @fd: starting file descriptor to close
+ * @max_fd: last file descriptor to close
+ *
+ * This closes a range of file descriptors. All file descriptors
+ * from @fd up to and including @max_fd are closed.
+ */
+int __close_range(struct files_struct *files, unsigned fd, unsigned max_fd)
+{
+ unsigned int cur_max;
+
+ if (fd > max_fd)
+ return -EINVAL;
+
+ rcu_read_lock();
+ cur_max = files_fdtable(files)->max_fds;
+ rcu_read_unlock();
+
+ /* cap to last valid index into fdtable */
+ if (max_fd >= cur_max)
+ max_fd = cur_max - 1;
+
+ while (fd <= max_fd)
+ __close_fd(files, fd++);
+
+ return 0;
+}
+
/*
* variant of __close_fd that gets a ref on the file for later fput
*/
diff --git a/fs/open.c b/fs/open.c
index 9c7d724a6f67..c7baaee7aa47 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1174,6 +1174,26 @@ SYSCALL_DEFINE1(close, unsigned int, fd)
return retval;
}
+/**
+ * close_range() - Close all file descriptors in a given range.
+ *
+ * @fd: starting file descriptor to close
+ * @max_fd: last file descriptor to close
+ * @flags: reserved for future extensions
+ *
+ * This closes a range of file descriptors. All file descriptors
+ * from @fd up to and including @max_fd are closed.
+ * Currently, errors to close a given file descriptor are ignored.
+ */
+SYSCALL_DEFINE3(close_range, unsigned int, fd, unsigned int, max_fd,
+ unsigned int, flags)
+{
+ if (flags)
+ return -EINVAL;
+
+ return __close_range(current->files, fd, max_fd);
+}
+
/*
* This routine simulates a hangup on the tty, to arrange that users
* are given clean terminals at login time.
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index f07c55ea0c22..fcd07181a365 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -121,6 +121,8 @@ extern void __fd_install(struct files_struct *files,
unsigned int fd, struct file *file);
extern int __close_fd(struct files_struct *files,
unsigned int fd);
+extern int __close_range(struct files_struct *files, unsigned int fd,
+ unsigned int max_fd);
extern int __close_fd_get_file(unsigned int fd, struct file **res);
extern struct kmem_cache *files_cachep;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e2870fe1be5b..c0189e223255 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -441,6 +441,8 @@ asmlinkage long sys_fchown(unsigned int fd, uid_t user, gid_t group);
asmlinkage long sys_openat(int dfd, const char __user *filename, int flags,
umode_t mode);
asmlinkage long sys_close(unsigned int fd);
+asmlinkage long sys_close_range(unsigned int fd, unsigned int max_fd,
+ unsigned int flags);
asmlinkage long sys_vhangup(void);
/* fs/pipe.c */
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index a87904daf103..3f36c8745d24 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -844,9 +844,11 @@ __SYSCALL(__NR_fsconfig, sys_fsconfig)
__SYSCALL(__NR_fsmount, sys_fsmount)
#define __NR_fspick 433
__SYSCALL(__NR_fspick, sys_fspick)
+#define __NR_close_range 435
+__SYSCALL(__NR_close_range, sys_close_range)
#undef __NR_syscalls
-#define __NR_syscalls 434
+#define __NR_syscalls 436
/*
* 32 bit systems traditionally used different
--
2.21.0
^ permalink raw reply related
* Re: [RFC 6/7] mm: extend process_madvise syscall to support vector arrary
From: Michal Hocko @ 2019-05-21 10:37 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521102613.GC219653@google.com>
On Tue 21-05-19 19:26:13, Minchan Kim wrote:
> On Tue, May 21, 2019 at 08:24:21AM +0200, Michal Hocko wrote:
> > On Tue 21-05-19 11:48:20, Minchan Kim wrote:
> > > On Mon, May 20, 2019 at 11:22:58AM +0200, Michal Hocko wrote:
> > > > [Cc linux-api]
> > > >
> > > > On Mon 20-05-19 12:52:53, Minchan Kim wrote:
> > > > > Currently, process_madvise syscall works for only one address range
> > > > > so user should call the syscall several times to give hints to
> > > > > multiple address range.
> > > >
> > > > Is that a problem? How big of a problem? Any numbers?
> > >
> > > We easily have 2000+ vma so it's not trivial overhead. I will come up
> > > with number in the description at respin.
> >
> > Does this really have to be a fast operation? I would expect the monitor
> > is by no means a fast path. The system call overhead is not what it used
> > to be, sigh, but still for something that is not a hot path it should be
> > tolerable, especially when the whole operation is quite expensive on its
> > own (wrt. the syscall entry/exit).
>
> What's different with process_vm_[readv|writev] and vmsplice?
> If the range needed to be covered is a lot, vector operation makes senese
> to me.
I am not saying that the vector API is wrong. All I am trying to say is
that the benefit is not really clear so far. If you want to push it
through then you should better get some supporting data.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 5/7] mm: introduce external memory hinting API
From: Minchan Kim @ 2019-05-21 10:32 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521061743.GC32329@dhcp22.suse.cz>
On Tue, May 21, 2019 at 08:17:43AM +0200, Michal Hocko wrote:
> On Tue 21-05-19 11:41:07, Minchan Kim wrote:
> > On Mon, May 20, 2019 at 11:18:29AM +0200, Michal Hocko wrote:
> > > [Cc linux-api]
> > >
> > > On Mon 20-05-19 12:52:52, Minchan Kim wrote:
> > > > There is some usecase that centralized userspace daemon want to give
> > > > a memory hint like MADV_[COOL|COLD] to other process. Android's
> > > > ActivityManagerService is one of them.
> > > >
> > > > It's 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 the centralized userspace daemon(ActivityManagerService),
> > > > and that daemon must be able to initiate reclaim on its own without
> > > > any app involvement.
> > >
> > > Could you expand some more about how this all works? How does the
> > > centralized daemon track respective ranges? How does it synchronize
> > > against parallel modification of the address space etc.
> >
> > Currently, we don't track each address ranges because we have two
> > policies at this moment:
> >
> > deactive file pages and reclaim anonymous pages of the app.
> >
> > Since the daemon has a ability to let background apps resume(IOW, process
> > will be run by the daemon) and both hints are non-disruptive stabilty point
> > of view, we are okay for the race.
>
> Fair enough but the API should consider future usecases where this might
> be a problem. So we should really think about those potential scenarios
> now. If we are ok with that, fine, but then we should be explicit and
> document it that way. Essentially say that any sort of synchronization
> is supposed to be done by monitor. This will make the API less usable
> but maybe that is enough.
Okay, I will add more about that in the description.
>
> > > > To solve the issue, this patch introduces new syscall process_madvise(2)
> > > > which works based on pidfd so it could give a hint to the exeternal
> > > > process.
> > > >
> > > > int process_madvise(int pidfd, void *addr, size_t length, int advise);
> > >
> > > OK, this makes some sense from the API point of view. When we have
> > > discussed that at LSFMM I was contemplating about something like that
> > > except the fd would be a VMA fd rather than the process. We could extend
> > > and reuse /proc/<pid>/map_files interface which doesn't support the
> > > anonymous memory right now.
> > >
> > > I am not saying this would be a better interface but I wanted to mention
> > > it here for a further discussion. One slight advantage would be that
> > > you know the exact object that you are operating on because you have a
> > > fd for the VMA and we would have a more straightforward way to reject
> > > operation if the underlying object has changed (e.g. unmapped and reused
> > > for a different mapping).
> >
> > I agree your point. If I didn't miss something, such kinds of vma level
> > modify notification doesn't work even file mapped vma at this moment.
> > For anonymous vma, I think we could use userfaultfd, pontentially.
> > It would be great if someone want to do with disruptive hints like
> > MADV_DONTNEED.
> >
> > I'd like to see it further enhancement after landing address range based
> > operation via limiting hints process_madvise supports to non-disruptive
> > only(e.g., MADV_[COOL|COLD]) so that we could catch up the usercase/workload
> > when someone want to extend the API.
>
> So do you think we want both interfaces (process_madvise and madvisefd)?
What I have in mind is to extend process_madvise later like this
struct pr_madvise_param {
int size; /* the size of this structure */
union {
const struct iovec __user *vec; /* address range array */
int fd; /* supported from 6.0 */
}
}
with introducing new hint Or-able PR_MADV_RANGE_FD, so that process_madvise
can go with fd instead of address range.
^ permalink raw reply
* Re: [RFC 6/7] mm: extend process_madvise syscall to support vector arrary
From: Minchan Kim @ 2019-05-21 10:26 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521062421.GD32329@dhcp22.suse.cz>
On Tue, May 21, 2019 at 08:24:21AM +0200, Michal Hocko wrote:
> On Tue 21-05-19 11:48:20, Minchan Kim wrote:
> > On Mon, May 20, 2019 at 11:22:58AM +0200, Michal Hocko wrote:
> > > [Cc linux-api]
> > >
> > > On Mon 20-05-19 12:52:53, Minchan Kim wrote:
> > > > Currently, process_madvise syscall works for only one address range
> > > > so user should call the syscall several times to give hints to
> > > > multiple address range.
> > >
> > > Is that a problem? How big of a problem? Any numbers?
> >
> > We easily have 2000+ vma so it's not trivial overhead. I will come up
> > with number in the description at respin.
>
> Does this really have to be a fast operation? I would expect the monitor
> is by no means a fast path. The system call overhead is not what it used
> to be, sigh, but still for something that is not a hot path it should be
> tolerable, especially when the whole operation is quite expensive on its
> own (wrt. the syscall entry/exit).
What's different with process_vm_[readv|writev] and vmsplice?
If the range needed to be covered is a lot, vector operation makes senese
to me.
>
> I am not saying we do not need a multiplexing API, I am just not sure
> we need it right away. Btw. there was some demand for other MM syscalls
> to provide a multiplexing API (e.g. mprotect), maybe it would be better
> to handle those in one go?
That's the exactly what Daniel Colascione suggested from internal
review. That would be a interesting approach if we could aggregate
all of system call in one go.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [RFC 1/7] mm: introduce MADV_COOL
From: Michal Hocko @ 2019-05-21 10:05 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521091134.GA219653@google.com>
On Tue 21-05-19 18:11:34, Minchan Kim wrote:
> On Tue, May 21, 2019 at 08:04:43AM +0200, Michal Hocko wrote:
> > On Tue 21-05-19 07:54:19, Minchan Kim wrote:
> > > On Mon, May 20, 2019 at 10:16:21AM +0200, Michal Hocko wrote:
> > [...]
> > > > > Internally, it works via deactivating memory from active list to
> > > > > inactive's head so when the memory pressure happens, they will be
> > > > > reclaimed earlier than other active pages unless there is no
> > > > > access until the time.
> > > >
> > > > Could you elaborate about the decision to move to the head rather than
> > > > tail? What should happen to inactive pages? Should we move them to the
> > > > tail? Your implementation seems to ignore those completely. Why?
> > >
> > > Normally, inactive LRU could have used-once pages without any mapping
> > > to user's address space. Such pages would be better candicate to
> > > reclaim when the memory pressure happens. With deactivating only
> > > active LRU pages of the process to the head of inactive LRU, we will
> > > keep them in RAM longer than used-once pages and could have more chance
> > > to be activated once the process is resumed.
> >
> > You are making some assumptions here. You have an explicit call what is
> > cold now you are assuming something is even colder. Is this assumption a
> > general enough to make people depend on it? Not that we wouldn't be able
> > to change to logic later but that will always be risky - especially in
> > the area when somebody want to make a user space driven memory
> > management.
>
> Think about MADV_FREE. It moves those pages into inactive file LRU's head.
> See the get_scan_count which makes forceful scanning of inactive file LRU
> if it has enough size based on the memory pressure.
> The reason is it's likely to have used-once pages in inactive file LRU,
> generally. Those pages has been top-priority candidate to be reclaimed
> for a long time.
OK, fair enough. Being consistent with MADV_FREE is reasonable. I just
forgot we do rotate like this there.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 3/7] mm: introduce MADV_COLD
From: Minchan Kim @ 2019-05-21 9:13 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521060820.GB32329@dhcp22.suse.cz>
On Tue, May 21, 2019 at 08:08:20AM +0200, Michal Hocko wrote:
> On Tue 21-05-19 08:00:38, Minchan Kim wrote:
> > On Mon, May 20, 2019 at 10:27:03AM +0200, Michal Hocko wrote:
> > > [Cc linux-api]
> > >
> > > On Mon 20-05-19 12:52:50, Minchan Kim wrote:
> > > > When a process expects no accesses to a certain memory range
> > > > for a long time, it could hint kernel that the pages can be
> > > > reclaimed instantly but data should be preserved for future use.
> > > > This could reduce workingset eviction so it ends up increasing
> > > > performance.
> > > >
> > > > This patch introduces the new MADV_COLD hint to madvise(2)
> > > > syscall. MADV_COLD can be used by a process to mark a memory range
> > > > as not expected to be used for a long time. The hint can help
> > > > kernel in deciding which pages to evict proactively.
> > >
> > > As mentioned in other email this looks like a non-destructive
> > > MADV_DONTNEED alternative.
> > >
> > > > Internally, it works via reclaiming memory in process context
> > > > the syscall is called. If the page is dirty but backing storage
> > > > is not synchronous device, the written page will be rotate back
> > > > into LRU's tail once the write is done so they will reclaim easily
> > > > when memory pressure happens. If backing storage is
> > > > synchrnous device(e.g., zram), hte page will be reclaimed instantly.
> > >
> > > Why do we special case async backing storage? Please always try to
> > > explain _why_ the decision is made.
> >
> > I didn't make any decesion. ;-) That's how current reclaim works to
> > avoid latency of freeing page in interrupt context. I had a patchset
> > to resolve the concern a few years ago but got distracted.
>
> Please articulate that in the changelog then. Or even do not go into
> implementation details and stick with - reuse the current reclaim
> implementation. If you call out some of the specific details you are
> risking people will start depending on them. The fact that this reuses
> the currect reclaim logic is enough from the review point of view
> because we know that there is no additional special casing to worry
> about.
I should have clarified. I will remove those lines in respin.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [RFC 1/7] mm: introduce MADV_COOL
From: Minchan Kim @ 2019-05-21 9:11 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521060443.GA32329@dhcp22.suse.cz>
On Tue, May 21, 2019 at 08:04:43AM +0200, Michal Hocko wrote:
> On Tue 21-05-19 07:54:19, Minchan Kim wrote:
> > On Mon, May 20, 2019 at 10:16:21AM +0200, Michal Hocko wrote:
> [...]
> > > > Internally, it works via deactivating memory from active list to
> > > > inactive's head so when the memory pressure happens, they will be
> > > > reclaimed earlier than other active pages unless there is no
> > > > access until the time.
> > >
> > > Could you elaborate about the decision to move to the head rather than
> > > tail? What should happen to inactive pages? Should we move them to the
> > > tail? Your implementation seems to ignore those completely. Why?
> >
> > Normally, inactive LRU could have used-once pages without any mapping
> > to user's address space. Such pages would be better candicate to
> > reclaim when the memory pressure happens. With deactivating only
> > active LRU pages of the process to the head of inactive LRU, we will
> > keep them in RAM longer than used-once pages and could have more chance
> > to be activated once the process is resumed.
>
> You are making some assumptions here. You have an explicit call what is
> cold now you are assuming something is even colder. Is this assumption a
> general enough to make people depend on it? Not that we wouldn't be able
> to change to logic later but that will always be risky - especially in
> the area when somebody want to make a user space driven memory
> management.
Think about MADV_FREE. It moves those pages into inactive file LRU's head.
See the get_scan_count which makes forceful scanning of inactive file LRU
if it has enough size based on the memory pressure.
The reason is it's likely to have used-once pages in inactive file LRU,
generally. Those pages has been top-priority candidate to be reclaimed
for a long time.
Only parts I am aware of moving pages into tail of inactive LRU are places
writeback is done for pages VM already decide to reclaim by LRU aging or
destructive operation like invalidating but couldn't completed. It's
really strong hints with no doubt.
>
> > > What should happen for shared pages? In other words do we want to allow
> > > less privileged process to control evicting of shared pages with a more
> > > privileged one? E.g. think of all sorts of side channel attacks. Maybe
> > > we want to do the same thing as for mincore where write access is
> > > required.
> >
> > It doesn't work with shared pages(ie, page_mapcount > 1). I will add it
> > in the description.
>
> OK, this is good for the starter. It makes the implementation simpler
> and we can add shared mappings coverage later.
>
> Although I would argue that touching only writeable mappings should be
> reasonably safe.
>
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 2/7] mm: Extend copy_vma()
From: Kirill Tkhai @ 2019-05-21 8:48 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: akpm, dan.j.williams, mhocko, keith.busch, kirill.shutemov,
alexander.h.duyck, ira.weiny, andreyknvl, arunks, vbabka, cl,
riel, keescook, hannes, npiggin, mathieu.desnoyers, shakeelb,
guro, aarcange, hughd, jglisse, mgorman, daniel.m.jordan, jannh,
kilobyte, linux-api, linux-kernel, linux-mm
In-Reply-To: <20190521081821.fbngbxk7lzwrb7md@box>
Hi, Kirill,
On 21.05.2019 11:18, Kirill A. Shutemov wrote:
> On Mon, May 20, 2019 at 05:00:12PM +0300, Kirill Tkhai wrote:
>> This prepares the function to copy a vma between
>> two processes. Two new arguments are introduced.
>
> This kind of changes requires a lot more explanation in commit message,
> describing all possible corner cases> For instance, I would really like to see a story on why logic around
> need_rmap_locks is safe after the change.
Let me fast answer on the below question firstly, and later I'll write
wide explanations, since this requires much more time.
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>> include/linux/mm.h | 4 ++--
>> mm/mmap.c | 33 ++++++++++++++++++++++++---------
>> mm/mremap.c | 4 ++--
>> 3 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 0e8834ac32b7..afe07e4a76f8 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -2329,8 +2329,8 @@ extern void __vma_link_rb(struct mm_struct *, struct vm_area_struct *,
>> struct rb_node **, struct rb_node *);
>> extern void unlink_file_vma(struct vm_area_struct *);
>> extern struct vm_area_struct *copy_vma(struct vm_area_struct **,
>> - unsigned long addr, unsigned long len, pgoff_t pgoff,
>> - bool *need_rmap_locks);
>> + struct mm_struct *, unsigned long addr, unsigned long len,
>> + pgoff_t pgoff, bool *need_rmap_locks, bool clear_flags_ctx);
>> extern void exit_mmap(struct mm_struct *);
>>
>> static inline int check_data_rlimit(unsigned long rlim,
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 57803a0a3a5c..99778e724ad1 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -3195,19 +3195,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
>> }
>>
>> /*
>> - * Copy the vma structure to a new location in the same mm,
>> - * prior to moving page table entries, to effect an mremap move.
>> + * Copy the vma structure to new location in the same vma
>> + * prior to moving page table entries, to effect an mremap move;
>> */
>> struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>> - unsigned long addr, unsigned long len, pgoff_t pgoff,
>> - bool *need_rmap_locks)
>> + struct mm_struct *mm, unsigned long addr,
>> + unsigned long len, pgoff_t pgoff,
>> + bool *need_rmap_locks, bool clear_flags_ctx)
>> {
>> struct vm_area_struct *vma = *vmap;
>> unsigned long vma_start = vma->vm_start;
>> - struct mm_struct *mm = vma->vm_mm;
>> + struct vm_userfaultfd_ctx uctx;
>> struct vm_area_struct *new_vma, *prev;
>> struct rb_node **rb_link, *rb_parent;
>> bool faulted_in_anon_vma = true;
>> + unsigned long flags;
>>
>> /*
>> * If anonymous vma has not yet been faulted, update new pgoff
>> @@ -3220,15 +3222,25 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>>
>> if (find_vma_links(mm, addr, addr + len, &prev, &rb_link, &rb_parent))
>> return NULL; /* should never get here */
>> - new_vma = vma_merge(mm, prev, addr, addr + len, vma->vm_flags,
>> - vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
>> - vma->vm_userfaultfd_ctx);
>> +
>> + uctx = vma->vm_userfaultfd_ctx;
>> + flags = vma->vm_flags;
>> + if (clear_flags_ctx) {
>> + uctx = NULL_VM_UFFD_CTX;
>> + flags &= ~(VM_UFFD_MISSING | VM_UFFD_WP | VM_MERGEABLE |
>> + VM_LOCKED | VM_LOCKONFAULT | VM_WIPEONFORK |
>> + VM_DONTCOPY);
>> + }
>
> Why is the new logic required? No justification given.
Ditto.
>> +
>> + new_vma = vma_merge(mm, prev, addr, addr + len, flags, vma->anon_vma,
>> + vma->vm_file, pgoff, vma_policy(vma), uctx);
>> if (new_vma) {
>> /*
>> * Source vma may have been merged into new_vma
>> */
>> if (unlikely(vma_start >= new_vma->vm_start &&
>> - vma_start < new_vma->vm_end)) {
>> + vma_start < new_vma->vm_end) &&
>> + vma->vm_mm == mm) {
>
> How can vma_merge() succeed if vma->vm_mm != mm?
We don't use vma as an argument of vma_merge(). We use vma as a source of
vma->anon_vma, vma->vm_file and vma_policy().
We search some new_vma in mm with the same characteristics as vma has in vma->vm_mm.
In case of success vma_merge() returns it for us. For example, it may success, when
vma->vm_mm is mm_struct of forked process, while mm is mm_struct of its parent.
[...]
Kirill
^ permalink raw reply
* Re: [PATCH v2 1/7] mm: Add process_vm_mmap() syscall declaration
From: Kirill Tkhai @ 2019-05-21 8:29 UTC (permalink / raw)
To: Ira Weiny
Cc: akpm, dan.j.williams, mhocko, keith.busch, kirill.shutemov,
alexander.h.duyck, andreyknvl, arunks, vbabka, cl, riel, keescook,
hannes, npiggin, mathieu.desnoyers, shakeelb, guro, aarcange,
hughd, jglisse, mgorman, daniel.m.jordan, jannh, kilobyte,
linux-api, linux-kernel, linux-mm
In-Reply-To: <20190521002827.GA30518@iweiny-DESK2.sc.intel.com>
Hi, Ira,
On 21.05.2019 03:28, Ira Weiny wrote:
> On Mon, May 20, 2019 at 05:00:07PM +0300, Kirill Tkhai wrote:
>> Similar to process_vm_readv() and process_vm_writev(),
>> add declarations of a new syscall, which will allow
>> to map memory from or to another process.
>
> Shouldn't this be the last patch in the series so that the syscall is actually
> implemented first?
It looks like there is no dependencies in the last patch to declarations made
in the first patch, so we really can move it.
I'll make this after there are accumulated some commentaries about the logic
to reduce number of patch series.
[...]
Thanks,
Kirill
^ permalink raw reply
* Re: [PATCH v2 2/7] mm: Extend copy_vma()
From: Kirill A. Shutemov @ 2019-05-21 8:18 UTC (permalink / raw)
To: Kirill Tkhai
Cc: akpm, dan.j.williams, mhocko, keith.busch, kirill.shutemov,
alexander.h.duyck, ira.weiny, andreyknvl, arunks, vbabka, cl,
riel, keescook, hannes, npiggin, mathieu.desnoyers, shakeelb,
guro, aarcange, hughd, jglisse, mgorman, daniel.m.jordan, jannh,
kilobyte, linux-api, linux-kernel, linux-mm
In-Reply-To: <155836081252.2441.9024100415314519956.stgit@localhost.localdomain>
On Mon, May 20, 2019 at 05:00:12PM +0300, Kirill Tkhai wrote:
> This prepares the function to copy a vma between
> two processes. Two new arguments are introduced.
This kind of changes requires a lot more explanation in commit message,
describing all possible corner cases.
For instance, I would really like to see a story on why logic around
need_rmap_locks is safe after the change.
>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
> include/linux/mm.h | 4 ++--
> mm/mmap.c | 33 ++++++++++++++++++++++++---------
> mm/mremap.c | 4 ++--
> 3 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0e8834ac32b7..afe07e4a76f8 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2329,8 +2329,8 @@ extern void __vma_link_rb(struct mm_struct *, struct vm_area_struct *,
> struct rb_node **, struct rb_node *);
> extern void unlink_file_vma(struct vm_area_struct *);
> extern struct vm_area_struct *copy_vma(struct vm_area_struct **,
> - unsigned long addr, unsigned long len, pgoff_t pgoff,
> - bool *need_rmap_locks);
> + struct mm_struct *, unsigned long addr, unsigned long len,
> + pgoff_t pgoff, bool *need_rmap_locks, bool clear_flags_ctx);
> extern void exit_mmap(struct mm_struct *);
>
> static inline int check_data_rlimit(unsigned long rlim,
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 57803a0a3a5c..99778e724ad1 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3195,19 +3195,21 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
> }
>
> /*
> - * Copy the vma structure to a new location in the same mm,
> - * prior to moving page table entries, to effect an mremap move.
> + * Copy the vma structure to new location in the same vma
> + * prior to moving page table entries, to effect an mremap move;
> */
> struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> - unsigned long addr, unsigned long len, pgoff_t pgoff,
> - bool *need_rmap_locks)
> + struct mm_struct *mm, unsigned long addr,
> + unsigned long len, pgoff_t pgoff,
> + bool *need_rmap_locks, bool clear_flags_ctx)
> {
> struct vm_area_struct *vma = *vmap;
> unsigned long vma_start = vma->vm_start;
> - struct mm_struct *mm = vma->vm_mm;
> + struct vm_userfaultfd_ctx uctx;
> struct vm_area_struct *new_vma, *prev;
> struct rb_node **rb_link, *rb_parent;
> bool faulted_in_anon_vma = true;
> + unsigned long flags;
>
> /*
> * If anonymous vma has not yet been faulted, update new pgoff
> @@ -3220,15 +3222,25 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>
> if (find_vma_links(mm, addr, addr + len, &prev, &rb_link, &rb_parent))
> return NULL; /* should never get here */
> - new_vma = vma_merge(mm, prev, addr, addr + len, vma->vm_flags,
> - vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> - vma->vm_userfaultfd_ctx);
> +
> + uctx = vma->vm_userfaultfd_ctx;
> + flags = vma->vm_flags;
> + if (clear_flags_ctx) {
> + uctx = NULL_VM_UFFD_CTX;
> + flags &= ~(VM_UFFD_MISSING | VM_UFFD_WP | VM_MERGEABLE |
> + VM_LOCKED | VM_LOCKONFAULT | VM_WIPEONFORK |
> + VM_DONTCOPY);
> + }
Why is the new logic required? No justification given.
> +
> + new_vma = vma_merge(mm, prev, addr, addr + len, flags, vma->anon_vma,
> + vma->vm_file, pgoff, vma_policy(vma), uctx);
> if (new_vma) {
> /*
> * Source vma may have been merged into new_vma
> */
> if (unlikely(vma_start >= new_vma->vm_start &&
> - vma_start < new_vma->vm_end)) {
> + vma_start < new_vma->vm_end) &&
> + vma->vm_mm == mm) {
How can vma_merge() succeed if vma->vm_mm != mm?
> /*
> * The only way we can get a vma_merge with
> * self during an mremap is if the vma hasn't
> @@ -3249,6 +3261,9 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> new_vma = vm_area_dup(vma);
> if (!new_vma)
> goto out;
> + new_vma->vm_mm = mm;
> + new_vma->vm_flags = flags;
> + new_vma->vm_userfaultfd_ctx = uctx;
> new_vma->vm_start = addr;
> new_vma->vm_end = addr + len;
> new_vma->vm_pgoff = pgoff;
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 37b5b2ad91be..9a96cfc28675 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -352,8 +352,8 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> return err;
>
> new_pgoff = vma->vm_pgoff + ((old_addr - vma->vm_start) >> PAGE_SHIFT);
> - new_vma = copy_vma(&vma, new_addr, new_len, new_pgoff,
> - &need_rmap_locks);
> + new_vma = copy_vma(&vma, mm, new_addr, new_len, new_pgoff,
> + &need_rmap_locks, false);
> if (!new_vma)
> return -ENOMEM;
>
>
^ permalink raw reply
* Re: [RFC 0/7] introduce memory hinting API for external process
From: Michal Hocko @ 2019-05-21 6:34 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Minchan Kim, Andrew Morton, LKML, linux-mm, Johannes Weiner,
Tim Murray, Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521014452.GA6738@bombadil.infradead.org>
[linux-api]
On Mon 20-05-19 18:44:52, Matthew Wilcox wrote:
> On Mon, May 20, 2019 at 12:52:47PM +0900, Minchan Kim wrote:
> > 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.
>
> Do we tear down page tables for these ranges? That seems like a good
> way of reclaiming potentially a substantial amount of memory.
I do not think we can in general because this is a non-destructive
operation. So at least we cannot tear down anonymous ptes (they will
turn into swap entries).
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 0/7] introduce memory hinting API for external process
From: Michal Hocko @ 2019-05-21 6:32 UTC (permalink / raw)
To: Minchan Kim
Cc: Johannes Weiner, Andrew Morton, LKML, linux-mm, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521043950.GJ10039@google.com>
[Cc linux-api]
On Tue 21-05-19 13:39:50, Minchan Kim wrote:
> On Mon, May 20, 2019 at 12:46:05PM -0400, Johannes Weiner wrote:
> > On Mon, May 20, 2019 at 12:52:47PM +0900, Minchan Kim wrote:
> > > - 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.
> >
> > I agree with this approach and the semantics. But these names are very
> > vague and extremely easy to confuse since they're so similar.
> >
> > MADV_COLD could be a good name, but for deactivating pages, not
> > reclaiming them - marking memory "cold" on the LRU for later reclaim.
> >
> > For the immediate reclaim one, I think there is a better option too:
> > In virtual memory speak, putting a page into secondary storage (or
> > ensuring it's already there), and then freeing its in-memory copy, is
> > called "paging out". And that's what this flag is supposed to do. So
> > how about MADV_PAGEOUT?
> >
> > With that, we'd have:
> >
> > MADV_FREE: Mark data invalid, free memory when needed
> > MADV_DONTNEED: Mark data invalid, free memory immediately
> >
> > MADV_COLD: Data is not used for a while, free memory when needed
> > MADV_PAGEOUT: Data is not used for a while, free memory immediately
> >
> > What do you think?
>
> There are several suggestions until now. Thanks, Folks!
>
> For deactivating:
>
> - MADV_COOL
> - MADV_RECLAIM_LAZY
> - MADV_DEACTIVATE
> - MADV_COLD
> - MADV_FREE_PRESERVE
>
>
> For reclaiming:
>
> - MADV_COLD
> - MADV_RECLAIM_NOW
> - MADV_RECLAIMING
> - MADV_PAGEOUT
> - MADV_DONTNEED_PRESERVE
>
> It seems everybody doesn't like MADV_COLD so want to go with other.
> For consisteny of view with other existing hints of madvise, -preserve
> postfix suits well. However, originally, I don't like the naming FREE
> vs DONTNEED from the beginning. They were easily confused.
> I prefer PAGEOUT to RECLAIM since it's more likely to be nuance to
> represent reclaim with memory pressure and is supposed to paged-in
> if someone need it later. So, it imply PRESERVE.
> If there is not strong against it, I want to go with MADV_COLD and
> MADV_PAGEOUT.
>
> Other opinion?
I do not really care strongly. I am pretty sure we will have a lot of
suggestions because people tend to be good at arguing about that...
Anyway, unlike DONTNEED/FREE we do not have any other OS to implement
these features, right? So we shouldn't be tight to existing names.
On the other hand I kinda like the reference to the existing names but
DEACTIVATE/PAGEOUT seem a good fit to me as well. Unless there is way
much better name suggested I would go with one of those. Up to you.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 7/7] mm: madvise support MADV_ANONYMOUS_FILTER and MADV_FILE_FILTER
From: Michal Hocko @ 2019-05-21 6:26 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521025533.GH10039@google.com>
On Tue 21-05-19 11:55:33, Minchan Kim wrote:
> On Mon, May 20, 2019 at 11:28:01AM +0200, Michal Hocko wrote:
> > [cc linux-api]
> >
> > On Mon 20-05-19 12:52:54, Minchan Kim wrote:
> > > System could have much faster swap device like zRAM. In that case, swapping
> > > is extremely cheaper than file-IO on the low-end storage.
> > > In this configuration, userspace could handle different strategy for each
> > > kinds of vma. IOW, they want to reclaim anonymous pages by MADV_COLD
> > > while it keeps file-backed pages in inactive LRU by MADV_COOL because
> > > file IO is more expensive in this case so want to keep them in memory
> > > until memory pressure happens.
> > >
> > > To support such strategy easier, this patch introduces
> > > MADV_ANONYMOUS_FILTER and MADV_FILE_FILTER options in madvise(2) like
> > > that /proc/<pid>/clear_refs already has supported same filters.
> > > They are filters could be Ored with other existing hints using top two bits
> > > of (int behavior).
> >
> > madvise operates on top of ranges and it is quite trivial to do the
> > filtering from the userspace so why do we need any additional filtering?
> >
> > > Once either of them is set, the hint could affect only the interested vma
> > > either anonymous or file-backed.
> > >
> > > With that, user could call a process_madvise syscall simply with a entire
> > > range(0x0 - 0xFFFFFFFFFFFFFFFF) but either of MADV_ANONYMOUS_FILTER and
> > > MADV_FILE_FILTER so there is no need to call the syscall range by range.
> >
> > OK, so here is the reason you want that. The immediate question is why
> > cannot the monitor do the filtering from the userspace. Slightly more
> > work, all right, but less of an API to expose and that itself is a
> > strong argument against.
>
> What I should do if we don't have such filter option is to enumerate all of
> vma via /proc/<pid>/maps and then parse every ranges and inode from string,
> which would be painful for 2000+ vmas.
Painful is not an argument to add a new user API. If the existing API
suits the purpose then it should be used. If it is not usable, we can
think of a different way.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 6/7] mm: extend process_madvise syscall to support vector arrary
From: Michal Hocko @ 2019-05-21 6:24 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521024820.GG10039@google.com>
On Tue 21-05-19 11:48:20, Minchan Kim wrote:
> On Mon, May 20, 2019 at 11:22:58AM +0200, Michal Hocko wrote:
> > [Cc linux-api]
> >
> > On Mon 20-05-19 12:52:53, Minchan Kim wrote:
> > > Currently, process_madvise syscall works for only one address range
> > > so user should call the syscall several times to give hints to
> > > multiple address range.
> >
> > Is that a problem? How big of a problem? Any numbers?
>
> We easily have 2000+ vma so it's not trivial overhead. I will come up
> with number in the description at respin.
Does this really have to be a fast operation? I would expect the monitor
is by no means a fast path. The system call overhead is not what it used
to be, sigh, but still for something that is not a hot path it should be
tolerable, especially when the whole operation is quite expensive on its
own (wrt. the syscall entry/exit).
I am not saying we do not need a multiplexing API, I am just not sure
we need it right away. Btw. there was some demand for other MM syscalls
to provide a multiplexing API (e.g. mprotect), maybe it would be better
to handle those in one go?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 5/7] mm: introduce external memory hinting API
From: Michal Hocko @ 2019-05-21 6:17 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190521024107.GF10039@google.com>
On Tue 21-05-19 11:41:07, Minchan Kim wrote:
> On Mon, May 20, 2019 at 11:18:29AM +0200, Michal Hocko wrote:
> > [Cc linux-api]
> >
> > On Mon 20-05-19 12:52:52, Minchan Kim wrote:
> > > There is some usecase that centralized userspace daemon want to give
> > > a memory hint like MADV_[COOL|COLD] to other process. Android's
> > > ActivityManagerService is one of them.
> > >
> > > It's 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 the centralized userspace daemon(ActivityManagerService),
> > > and that daemon must be able to initiate reclaim on its own without
> > > any app involvement.
> >
> > Could you expand some more about how this all works? How does the
> > centralized daemon track respective ranges? How does it synchronize
> > against parallel modification of the address space etc.
>
> Currently, we don't track each address ranges because we have two
> policies at this moment:
>
> deactive file pages and reclaim anonymous pages of the app.
>
> Since the daemon has a ability to let background apps resume(IOW, process
> will be run by the daemon) and both hints are non-disruptive stabilty point
> of view, we are okay for the race.
Fair enough but the API should consider future usecases where this might
be a problem. So we should really think about those potential scenarios
now. If we are ok with that, fine, but then we should be explicit and
document it that way. Essentially say that any sort of synchronization
is supposed to be done by monitor. This will make the API less usable
but maybe that is enough.
> > > To solve the issue, this patch introduces new syscall process_madvise(2)
> > > which works based on pidfd so it could give a hint to the exeternal
> > > process.
> > >
> > > int process_madvise(int pidfd, void *addr, size_t length, int advise);
> >
> > OK, this makes some sense from the API point of view. When we have
> > discussed that at LSFMM I was contemplating about something like that
> > except the fd would be a VMA fd rather than the process. We could extend
> > and reuse /proc/<pid>/map_files interface which doesn't support the
> > anonymous memory right now.
> >
> > I am not saying this would be a better interface but I wanted to mention
> > it here for a further discussion. One slight advantage would be that
> > you know the exact object that you are operating on because you have a
> > fd for the VMA and we would have a more straightforward way to reject
> > operation if the underlying object has changed (e.g. unmapped and reused
> > for a different mapping).
>
> I agree your point. If I didn't miss something, such kinds of vma level
> modify notification doesn't work even file mapped vma at this moment.
> For anonymous vma, I think we could use userfaultfd, pontentially.
> It would be great if someone want to do with disruptive hints like
> MADV_DONTNEED.
>
> I'd like to see it further enhancement after landing address range based
> operation via limiting hints process_madvise supports to non-disruptive
> only(e.g., MADV_[COOL|COLD]) so that we could catch up the usercase/workload
> when someone want to extend the API.
So do you think we want both interfaces (process_madvise and madvisefd)?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 3/7] mm: introduce MADV_COLD
From: Michal Hocko @ 2019-05-21 6:08 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190520230038.GD10039@google.com>
On Tue 21-05-19 08:00:38, Minchan Kim wrote:
> On Mon, May 20, 2019 at 10:27:03AM +0200, Michal Hocko wrote:
> > [Cc linux-api]
> >
> > On Mon 20-05-19 12:52:50, Minchan Kim wrote:
> > > When a process expects no accesses to a certain memory range
> > > for a long time, it could hint kernel that the pages can be
> > > reclaimed instantly but data should be preserved for future use.
> > > This could reduce workingset eviction so it ends up increasing
> > > performance.
> > >
> > > This patch introduces the new MADV_COLD hint to madvise(2)
> > > syscall. MADV_COLD can be used by a process to mark a memory range
> > > as not expected to be used for a long time. The hint can help
> > > kernel in deciding which pages to evict proactively.
> >
> > As mentioned in other email this looks like a non-destructive
> > MADV_DONTNEED alternative.
> >
> > > Internally, it works via reclaiming memory in process context
> > > the syscall is called. If the page is dirty but backing storage
> > > is not synchronous device, the written page will be rotate back
> > > into LRU's tail once the write is done so they will reclaim easily
> > > when memory pressure happens. If backing storage is
> > > synchrnous device(e.g., zram), hte page will be reclaimed instantly.
> >
> > Why do we special case async backing storage? Please always try to
> > explain _why_ the decision is made.
>
> I didn't make any decesion. ;-) That's how current reclaim works to
> avoid latency of freeing page in interrupt context. I had a patchset
> to resolve the concern a few years ago but got distracted.
Please articulate that in the changelog then. Or even do not go into
implementation details and stick with - reuse the current reclaim
implementation. If you call out some of the specific details you are
risking people will start depending on them. The fact that this reuses
the currect reclaim logic is enough from the review point of view
because we know that there is no additional special casing to worry
about.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC 1/7] mm: introduce MADV_COOL
From: Michal Hocko @ 2019-05-21 6:04 UTC (permalink / raw)
To: Minchan Kim
Cc: Andrew Morton, LKML, linux-mm, Johannes Weiner, Tim Murray,
Joel Fernandes, Suren Baghdasaryan, Daniel Colascione,
Shakeel Butt, Sonny Rao, Brian Geffon, linux-api
In-Reply-To: <20190520225419.GA10039@google.com>
On Tue 21-05-19 07:54:19, Minchan Kim wrote:
> On Mon, May 20, 2019 at 10:16:21AM +0200, Michal Hocko wrote:
[...]
> > > Internally, it works via deactivating memory from active list to
> > > inactive's head so when the memory pressure happens, they will be
> > > reclaimed earlier than other active pages unless there is no
> > > access until the time.
> >
> > Could you elaborate about the decision to move to the head rather than
> > tail? What should happen to inactive pages? Should we move them to the
> > tail? Your implementation seems to ignore those completely. Why?
>
> Normally, inactive LRU could have used-once pages without any mapping
> to user's address space. Such pages would be better candicate to
> reclaim when the memory pressure happens. With deactivating only
> active LRU pages of the process to the head of inactive LRU, we will
> keep them in RAM longer than used-once pages and could have more chance
> to be activated once the process is resumed.
You are making some assumptions here. You have an explicit call what is
cold now you are assuming something is even colder. Is this assumption a
general enough to make people depend on it? Not that we wouldn't be able
to change to logic later but that will always be risky - especially in
the area when somebody want to make a user space driven memory
management.
> > What should happen for shared pages? In other words do we want to allow
> > less privileged process to control evicting of shared pages with a more
> > privileged one? E.g. think of all sorts of side channel attacks. Maybe
> > we want to do the same thing as for mincore where write access is
> > required.
>
> It doesn't work with shared pages(ie, page_mapcount > 1). I will add it
> in the description.
OK, this is good for the starter. It makes the implementation simpler
and we can add shared mappings coverage later.
Although I would argue that touching only writeable mappings should be
reasonably safe.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox