Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Tue, Nov 17, 2020 at 12:06:48PM -0800, Linus Torvalds wrote: > On Mon, Nov 16, 2020 at 7:51 AM Minchan Kim wrote: > > > > Let me send a patch with your SoB if you don't mind. > > Eric, can you ack this SoB and I'll apply it to me tree? > > Or is it already queued up somewhere else? Andrew, picked it up. https://ozlabs.org/~akpm/mmots/broken-out/mm-madvise-fix-memory-leak-from-process_madvise.patch
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Mon, Nov 16, 2020 at 7:51 AM Minchan Kim wrote: > > Let me send a patch with your SoB if you don't mind. Eric, can you ack this SoB and I'll apply it to me tree? Or is it already queued up somewhere else? Linus
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Mon, Nov 16, 2020 at 10:02:42AM +0100, Eric Dumazet wrote:
< snip >
> > From 02d63c6b3f61a1085f4eab80f5171bd2627b5ab0 Mon Sep 17 00:00:00 2001
> > From: Minchan Kim
> > Date: Mon, 21 Sep 2020 09:31:25 -0700
> > Subject: [PATCH] mm: do not use helper functions for process_madvise
> >
> > This patch removes helper functions process_madvise_vec,
> > do_process_madvise and madv_import_iovec and use them inline.
> >
> > Signed-off-by: Minchan Kim
> > ---
> > mm/madvise.c | 97 +++-
> > 1 file changed, 43 insertions(+), 54 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index ae266dfede8a..aa8bc65dbdb6 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1166,37 +1166,40 @@ SYSCALL_DEFINE3(madvise, unsigned long, start,
> > size_t, len_in, int, behavior)
> > return do_madvise(current->mm, start, len_in, behavior);
> > }
> >
> > -static int process_madvise_vec(struct mm_struct *mm, struct iov_iter
> > *iter, int behavior)
> > -{
> > - struct iovec iovec;
> > - int ret = 0;
> > -
> > - while (iov_iter_count(iter)) {
> > - iovec = iov_iter_iovec(iter);
> > - ret = do_madvise(mm, (unsigned long)iovec.iov_base,
> > iovec.iov_len, behavior);
> > - if (ret < 0)
> > - break;
> > - iov_iter_advance(iter, iovec.iov_len);
> > - }
> > -
> > - return ret;
> > -}
> > -
> > -static ssize_t do_process_madvise(int pidfd, struct iov_iter *iter,
> > - int behavior, unsigned int flags)
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *,
> > vec,
> > + size_t, vlen, int, behavior, unsigned int, flags)
> > {
> > ssize_t ret;
> > + struct iovec iovstack[UIO_FASTIOV], iovec;
> > + struct iovec *iov = iovstack;
> > + struct iov_iter iter;
> > struct pid *pid;
> > struct task_struct *task;
> > struct mm_struct *mm;
> > - size_t total_len = iov_iter_count(iter);
> > + size_t total_len;
> >
> > - if (flags != 0)
> > - return -EINVAL;
> > + if (flags != 0) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > +#ifdef CONFIG_COMPAT
> > + if (in_compat_syscall())
> > + ret = compat_import_iovec(READ,
> > + (struct compat_iovec __user *)vec, vlen,
> > + ARRAY_SIZE(iovstack), , );
> > + else
> > +#endif
> > + ret = import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack),
> > + , );
> > + if (ret < 0)
> > + goto out;
> >
> > pid = pidfd_get_pid(pidfd);
> > - if (IS_ERR(pid))
> > - return PTR_ERR(pid);
> > + if (IS_ERR(pid)) {
> > + ret = PTR_ERR(pid);
> > + goto free_iov;
> > + }
> >
> > task = get_pid_task(pid, PIDTYPE_PID);
> > if (!task) {
> > @@ -1216,43 +1219,29 @@ static ssize_t do_process_madvise(int pidfd, struct
> > iov_iter *iter,
> > goto release_task;
> > }
> >
> > - ret = process_madvise_vec(mm, iter, behavior);
> > - if (ret >= 0)
> > - ret = total_len - iov_iter_count(iter);
> > + total_len = iov_iter_count();
> > +
> > + while (iov_iter_count()) {
> > + iovec = iov_iter_iovec();
> > + ret = do_madvise(mm, (unsigned long)iovec.iov_base,
> > + iovec.iov_len, behavior);
> > + if (ret < 0)
> > + break;
> > + iov_iter_advance(, iovec.iov_len);
> > + }
> > +
> > + if (ret == 0)
> > + ret = total_len - iov_iter_count();
> >
> > mmput(mm);
> > + return ret;
>
> This "return ret;" seems quite wrong...
/me slaps self.
>
> I will send the following :
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index
> 416a56b8e757bf3465ab13cea51e0751ade2c745..cc9224a59e9fa07e41f9b4ad2e58b9c97889299b
> 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1231,7 +1231,6 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const
> struct iovec __user *, vec,
> ret = total_len - iov_iter_count();
>
> mmput(mm);
> - return ret;
>
> release_task:
> put_task_struct(task);
>
>
>
>
> > +
> > release_task:
> > put_task_struct(task);
> > put_pid:
> > put_pid(pid);
> > - return ret;
> > -}
Thanks, Eric!
Let me send a patch with your SoB if you don't mind.
>From 0f37d5295324721ee19a3ad40fe58e3002cd9934 Mon Sep 17 00:00:00 2001
From: Eric Dumazet
Date: Mon, 16 Nov 2020 07:34:02 -0800
Subject: [PATCH] mm/madvise: fix memory leak from process_madvise
The eary return in process_madvise will produce memory leak.
Fix it.
Fixes: ecb8ac8b1f14 ("mm/madvise: introduce process_madvise() syscall: an
external memory hinting API")
Signed-off-by: Eric Dumazet
Signed-off-by: Minchan Kim
---
mm/madvise.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On 9/21/20 7:55 PM, Minchan Kim wrote:
> On Mon, Sep 21, 2020 at 07:56:33AM +0100, Christoph Hellwig wrote:
>> On Mon, Aug 31, 2020 at 05:06:33PM -0700, Minchan Kim wrote:
>>> There is usecase that System Management Software(SMS) want to give a
>>> memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the
>>> case of Android, it is the ActivityManagerService.
>>>
>>> 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.
>>>
>>> To solve the issue, this patch introduces a new syscall process_madvise(2).
>>> It uses pidfd of an external process to give the hint. It also supports
>>> vector address range because Android app has thousands of vmas due to
>>> zygote so it's totally waste of CPU and power if we should call the
>>> syscall one by one for each vma.(With testing 2000-vma syscall vs
>>> 1-vector syscall, it showed 15% performance improvement. I think it
>>> would be bigger in real practice because the testing ran very cache
>>> friendly environment).
>>
>> I'm really not sure this syscall is a good idea. If you want central
>> control you should implement an IPC mechanisms that allows your
>> supervisor daemon to tell the application to perform the madvice
>> instead of forcing the behavior on it.
>
> There was dicussion about the approach. There were several issues.
> One of them was the target app was already freezed and we wanted
> to run the syscall in caller's context, not callee.
>
>>
>>> /*
>>> * The madvise(2) system call.
>>> *
>>> @@ -1036,6 +1049,11 @@ madvise_behavior_valid(int behavior)
>>> * MADV_DONTDUMP - the application wants to prevent pages in the given
>>> range
>>> * from being included in its core dump.
>>> * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
>>> + * MADV_COLD - the application is not expected to use this memory soon,
>>> + * deactivate pages in this range so that they can be reclaimed
>>> + * easily if memory pressure hanppens.
>>> + * MADV_PAGEOUT - the application is not expected to use this memory soon,
>>> + * page out the pages in this range immediately.
>>
>> This should really go into a separate patch, as it has nothing to do
>> with the new syscall.
>
> Technically, right but I expected it's not worth to have separate patch.
>
>>
>>> +static int process_madvise_vec(struct mm_struct *mm, struct iov_iter
>>> *iter, int behavior)
>>> +{
>>> + struct iovec iovec;
>>> + int ret = 0;
>>> +
>>> + while (iov_iter_count(iter)) {
>>> + iovec = iov_iter_iovec(iter);
>>> + ret = do_madvise(mm, (unsigned long)iovec.iov_base,
>>> iovec.iov_len, behavior);
>>> + if (ret < 0)
>>> + break;
>>> + iov_iter_advance(iter, iovec.iov_len);
>>> + }
>>> +
>>> + return ret;
>>
>> Please avoid the entirely pointless overly long line.
>>
>>> +static inline int madv_import_iovec(int type, const struct iovec __user
>>> *uvec, unsigned int nr_segs,
>>> + unsigned int fast_segs, struct iovec **iov, struct iov_iter *i)
>>> +{
>>> +#ifdef CONFIG_COMPAT
>>> + if (in_compat_syscall())
>>> + return compat_import_iovec(type, (struct compat_iovec __user
>>> *)uvec, nr_segs,
>>> + fast_segs, iov, i);
>>> +#endif
>>
>> More of the same.
>>
>>> +SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *,
>>> vec,
>>> + unsigned long, vlen, int, behavior, unsigned int, flags)
>>> +{
>>> + ssize_t ret;
>>> + struct iovec iovstack[UIO_FASTIOV];
>>> + struct iovec *iov = iovstack;
>>> + struct iov_iter iter;
>>> +
>>> + ret = madv_import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), ,
>>> );
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ret = do_process_madvise(pidfd, , behavior, flags);
>>> + kfree(iov);
>>> + return ret;
>>
>> Even more here. But more importantly there seems to be absolutely
>> no reason for the madv_import_iovec and do_process_madvise helpers
>> that both are tiny and have this even smaller function as the only
>> caller.
>
> Fair enough.
>
>
> Andrew, could you fold this patch?
> Thank you.
>
> From 02d63c6b3f61a1085f4eab80f5171bd2627b5ab0 Mon Sep 17 00:00:00 2001
> From: Minchan Kim
> Date: Mon, 21 Sep 2020 09:31:25 -0700
> Subject: [PATCH] mm: do not use helper functions for process_madvise
>
> This patch removes helper functions process_madvise_vec,
> do_process_madvise and madv_import_iovec and use them inline.
>
> Signed-off-by: Minchan Kim
> ---
> mm/madvise.c | 97 +++-
> 1 file changed, 43 insertions(+), 54 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index ae266dfede8a..aa8bc65dbdb6 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
>
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Mon, Sep 21, 2020 at 07:56:33AM +0100, Christoph Hellwig wrote:
> On Mon, Aug 31, 2020 at 05:06:33PM -0700, Minchan Kim wrote:
> > There is usecase that System Management Software(SMS) want to give a
> > memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the
> > case of Android, it is the ActivityManagerService.
> >
> > 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.
> >
> > To solve the issue, this patch introduces a new syscall process_madvise(2).
> > It uses pidfd of an external process to give the hint. It also supports
> > vector address range because Android app has thousands of vmas due to
> > zygote so it's totally waste of CPU and power if we should call the
> > syscall one by one for each vma.(With testing 2000-vma syscall vs
> > 1-vector syscall, it showed 15% performance improvement. I think it
> > would be bigger in real practice because the testing ran very cache
> > friendly environment).
>
> I'm really not sure this syscall is a good idea. If you want central
> control you should implement an IPC mechanisms that allows your
> supervisor daemon to tell the application to perform the madvice
> instead of forcing the behavior on it.
There was dicussion about the approach. There were several issues.
One of them was the target app was already freezed and we wanted
to run the syscall in caller's context, not callee.
>
> > /*
> > * The madvise(2) system call.
> > *
> > @@ -1036,6 +1049,11 @@ madvise_behavior_valid(int behavior)
> > * MADV_DONTDUMP - the application wants to prevent pages in the given
> > range
> > * from being included in its core dump.
> > * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > + * MADV_COLD - the application is not expected to use this memory soon,
> > + * deactivate pages in this range so that they can be reclaimed
> > + * easily if memory pressure hanppens.
> > + * MADV_PAGEOUT - the application is not expected to use this memory soon,
> > + * page out the pages in this range immediately.
>
> This should really go into a separate patch, as it has nothing to do
> with the new syscall.
Technically, right but I expected it's not worth to have separate patch.
>
> > +static int process_madvise_vec(struct mm_struct *mm, struct iov_iter
> > *iter, int behavior)
> > +{
> > + struct iovec iovec;
> > + int ret = 0;
> > +
> > + while (iov_iter_count(iter)) {
> > + iovec = iov_iter_iovec(iter);
> > + ret = do_madvise(mm, (unsigned long)iovec.iov_base,
> > iovec.iov_len, behavior);
> > + if (ret < 0)
> > + break;
> > + iov_iter_advance(iter, iovec.iov_len);
> > + }
> > +
> > + return ret;
>
> Please avoid the entirely pointless overly long line.
>
> > +static inline int madv_import_iovec(int type, const struct iovec __user
> > *uvec, unsigned int nr_segs,
> > + unsigned int fast_segs, struct iovec **iov, struct iov_iter *i)
> > +{
> > +#ifdef CONFIG_COMPAT
> > + if (in_compat_syscall())
> > + return compat_import_iovec(type, (struct compat_iovec __user
> > *)uvec, nr_segs,
> > + fast_segs, iov, i);
> > +#endif
>
> More of the same.
>
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *,
> > vec,
> > + unsigned long, vlen, int, behavior, unsigned int, flags)
> > +{
> > + ssize_t ret;
> > + struct iovec iovstack[UIO_FASTIOV];
> > + struct iovec *iov = iovstack;
> > + struct iov_iter iter;
> > +
> > + ret = madv_import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), ,
> > );
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = do_process_madvise(pidfd, , behavior, flags);
> > + kfree(iov);
> > + return ret;
>
> Even more here. But more importantly there seems to be absolutely
> no reason for the madv_import_iovec and do_process_madvise helpers
> that both are tiny and have this even smaller function as the only
> caller.
Fair enough.
Andrew, could you fold this patch?
Thank you.
>From 02d63c6b3f61a1085f4eab80f5171bd2627b5ab0 Mon Sep 17 00:00:00 2001
From: Minchan Kim
Date: Mon, 21 Sep 2020 09:31:25 -0700
Subject: [PATCH] mm: do not use helper functions for process_madvise
This patch removes helper functions process_madvise_vec,
do_process_madvise and madv_import_iovec and use them inline.
Signed-off-by: Minchan Kim
---
mm/madvise.c | 97 +++-
1 file changed, 43 insertions(+), 54 deletions(-)
diff --git a/mm/madvise.c b/mm/madvise.c
index ae266dfede8a..aa8bc65dbdb6 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1166,37 +1166,40 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t,
len_in, int, behavior)
return
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Mon 21-09-20 07:56:33, Christoph Hellwig wrote: > On Mon, Aug 31, 2020 at 05:06:33PM -0700, Minchan Kim wrote: > > There is usecase that System Management Software(SMS) want to give a > > memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the > > case of Android, it is the ActivityManagerService. > > > > 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. > > > > To solve the issue, this patch introduces a new syscall process_madvise(2). > > It uses pidfd of an external process to give the hint. It also supports > > vector address range because Android app has thousands of vmas due to > > zygote so it's totally waste of CPU and power if we should call the > > syscall one by one for each vma.(With testing 2000-vma syscall vs > > 1-vector syscall, it showed 15% performance improvement. I think it > > would be bigger in real practice because the testing ran very cache > > friendly environment). > > I'm really not sure this syscall is a good idea. If you want central > control you should implement an IPC mechanisms that allows your > supervisor daemon to tell the application to perform the madvice > instead of forcing the behavior on it. Even though I am not entirely happy about the interface [1]. As it seems I am in minority in my concern I backed off and decided to not block this work because I do not see the problem with the functionality itself. And I find it very useful for userspace driven memory management people are asking for a long time. This functionality shouldn't be much different from the standard memory reclaim. It has some limitations (e.g. it can only handle mapped memory) but allows to pro-actively swap out or reclaim disk based memory based on a specific knowlege of the workload. Kernel is not able to do the same. [1] http://lkml.kernel.org/r/20200117115225.gv19...@dhcp22.suse.cz -- Michal Hocko SUSE Labs
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Mon, Aug 31, 2020 at 05:06:33PM -0700, Minchan Kim wrote:
> There is usecase that System Management Software(SMS) want to give a
> memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the
> case of Android, it is the ActivityManagerService.
>
> 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.
>
> To solve the issue, this patch introduces a new syscall process_madvise(2).
> It uses pidfd of an external process to give the hint. It also supports
> vector address range because Android app has thousands of vmas due to
> zygote so it's totally waste of CPU and power if we should call the
> syscall one by one for each vma.(With testing 2000-vma syscall vs
> 1-vector syscall, it showed 15% performance improvement. I think it
> would be bigger in real practice because the testing ran very cache
> friendly environment).
I'm really not sure this syscall is a good idea. If you want central
control you should implement an IPC mechanisms that allows your
supervisor daemon to tell the application to perform the madvice
instead of forcing the behavior on it.
> /*
> * The madvise(2) system call.
> *
> @@ -1036,6 +1049,11 @@ madvise_behavior_valid(int behavior)
> * MADV_DONTDUMP - the application wants to prevent pages in the given range
> * from being included in its core dump.
> * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> + * MADV_COLD - the application is not expected to use this memory soon,
> + * deactivate pages in this range so that they can be reclaimed
> + * easily if memory pressure hanppens.
> + * MADV_PAGEOUT - the application is not expected to use this memory soon,
> + * page out the pages in this range immediately.
This should really go into a separate patch, as it has nothing to do
with the new syscall.
> +static int process_madvise_vec(struct mm_struct *mm, struct iov_iter *iter,
> int behavior)
> +{
> + struct iovec iovec;
> + int ret = 0;
> +
> + while (iov_iter_count(iter)) {
> + iovec = iov_iter_iovec(iter);
> + ret = do_madvise(mm, (unsigned long)iovec.iov_base,
> iovec.iov_len, behavior);
> + if (ret < 0)
> + break;
> + iov_iter_advance(iter, iovec.iov_len);
> + }
> +
> + return ret;
Please avoid the entirely pointless overly long line.
> +static inline int madv_import_iovec(int type, const struct iovec __user
> *uvec, unsigned int nr_segs,
> + unsigned int fast_segs, struct iovec **iov, struct iov_iter *i)
> +{
> +#ifdef CONFIG_COMPAT
> + if (in_compat_syscall())
> + return compat_import_iovec(type, (struct compat_iovec __user
> *)uvec, nr_segs,
> + fast_segs, iov, i);
> +#endif
More of the same.
> +SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *,
> vec,
> + unsigned long, vlen, int, behavior, unsigned int, flags)
> +{
> + ssize_t ret;
> + struct iovec iovstack[UIO_FASTIOV];
> + struct iovec *iov = iovstack;
> + struct iov_iter iter;
> +
> + ret = madv_import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), ,
> );
> + if (ret < 0)
> + return ret;
> +
> + ret = do_process_madvise(pidfd, , behavior, flags);
> + kfree(iov);
> + return ret;
Even more here. But more importantly there seems to be absolutely
no reason for the madv_import_iovec and do_process_madvise helpers
that both are tiny and have this even smaller function as the only
caller.
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Thu, Sep 03, 2020 at 10:59:49AM -0700, Minchan Kim wrote: > On Thu, Sep 03, 2020 at 07:34:58PM +0200, Florian Weimer wrote: > > * Minchan Kim: > > > > > On Tue, Sep 01, 2020 at 08:46:02PM +0200, Florian Weimer wrote: > > >> * Minchan Kim: > > >> > > >> > ssize_t process_madvise(int pidfd, const struct iovec *iovec, > > >> > unsigned long vlen, int advice, unsigned int flags); > > >> > > >> size_t for vlen provides a clearer hint regarding the type of special > > >> treatment needed for ILP32 here (zero extension, not changing the type > > >> to long long). > > >> > > > > > > All existing system calls using iove in Linux uses unsigned long so > > > I want to be consistent with them unless process_madvise need something > > > speicial. > > > > Userspace uses int, following POSIX (where applicable). There is no > > consistency to be had here. > > Okay, I changed it with size_t. Maybe some context helps. We had the discussion about syscall conventions during LPC both in the KernelSummit and in the glibc toolchain session and one of wishlist conventions from libc was to always use size_t and not unsigned long for sizes. I know this has been a little frustrating having to change types and so on for the syscall quite a bit but I'm going to start drafting an updated version of our howto for adding syscalls now so things like this are more transparent going forward. I just hadn't gotten around to it right after Plumbers. Christian
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Thu, Sep 03, 2020 at 07:34:58PM +0200, Florian Weimer wrote:
> * Minchan Kim:
>
> > On Tue, Sep 01, 2020 at 08:46:02PM +0200, Florian Weimer wrote:
> >> * Minchan Kim:
> >>
> >> > ssize_t process_madvise(int pidfd, const struct iovec *iovec,
> >> > unsigned long vlen, int advice, unsigned int flags);
> >>
> >> size_t for vlen provides a clearer hint regarding the type of special
> >> treatment needed for ILP32 here (zero extension, not changing the type
> >> to long long).
> >>
> >
> > All existing system calls using iove in Linux uses unsigned long so
> > I want to be consistent with them unless process_madvise need something
> > speicial.
>
> Userspace uses int, following POSIX (where applicable). There is no
> consistency to be had here.
Okay, I changed it with size_t.
>From c9f079a2008fd05a38d3f0fd69c2c0e586e8ee7a Mon Sep 17 00:00:00 2001
From: Minchan Kim
Date: Mon, 31 Aug 2020 15:36:30 -0700
Subject: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an
external memory hinting API
There is usecase that System Management Software(SMS) want to give a
memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the
case of Android, it is the ActivityManagerService.
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.
To solve the issue, this patch introduces a new syscall process_madvise(2).
It uses pidfd of an external process to give the hint. It also supports
vector address range because Android app has thousands of vmas due to
zygote so it's totally waste of CPU and power if we should call the
syscall one by one for each vma.(With testing 2000-vma syscall vs
1-vector syscall, it showed 15% performance improvement. I think it
would be bigger in real practice because the testing ran very cache
friendly environment).
Another potential use case for the vector range is to amortize the cost
ofTLB shootdowns for multiple ranges when using MADV_DONTNEED; this
could benefit users like TCP receive zerocopy and malloc implementations.
In future, we could find more usecases for other advises so let's make it
happens as API since we introduce a new syscall at this moment. With
that, existing madvise(2) user could replace it with process_madvise(2)
with their own pid if they want to have batch address ranges support
feature.
ince it could affect other process's address range, only privileged
process(PTRACE_MODE_ATTACH_FSCREDS) or something else(e.g., being the
same UID) gives it the right to ptrace the process could use it
successfully. The flag argument is reserved for future use if we need to
extend the API.
I think supporting all hints madvise has/will supported/support to
process_madvise is rather risky. Because we are not sure all hints
make sense from external process and implementation for the hint may
rely on the caller being in the current context so it could be
error-prone. Thus, I just limited hints as MADV_[COLD|PAGEOUT] in this
patch.
If someone want to add other hints, we could hear the usecase and
review it for each hint. It's safer for maintenance rather than
introducing a buggy syscall but hard to fix it later.
So finally, the API is as follows,
ssize_t process_madvise(int pidfd, const struct iovec *iovec,
size_t vlen, int advice, unsigned int flags);
DESCRIPTION
The process_madvise() system call is used to give advice or directions
to the kernel about the address ranges from external process as well as
local process. It provides the advice to address ranges of process
described by iovec and vlen. The goal of such advice is to improve system
or application performance.
The pidfd selects the process referred to by the PID file descriptor
specified in pidfd. (See pidofd_open(2) for further information)
The pointer iovec points to an array of iovec structures, defined in
as:
struct iovec {
void *iov_base; /* starting address */
size_t iov_len; /* number of bytes to be advised */
};
The iovec describes address ranges beginning at address(iov_base)
and with size length of bytes(iov_len).
The vlen represents the number of elements in iovec.
The advice is indicated in the advice argument, which is one of the
following at this moment if the target process specified by pidfd is
external.
MADV_COLD
MADV_PAGEOUT
Permission to provide a hint to external process is governed by a
ptrace access mode PTRACE_MODE_ATTACH_FSCREDS check; see ptrace(2).
The process_madvise supports every advice madvise(2) has if
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
* Minchan Kim: > On Tue, Sep 01, 2020 at 08:46:02PM +0200, Florian Weimer wrote: >> * Minchan Kim: >> >> > ssize_t process_madvise(int pidfd, const struct iovec *iovec, >> > unsigned long vlen, int advice, unsigned int flags); >> >> size_t for vlen provides a clearer hint regarding the type of special >> treatment needed for ILP32 here (zero extension, not changing the type >> to long long). >> > > All existing system calls using iove in Linux uses unsigned long so > I want to be consistent with them unless process_madvise need something > speicial. Userspace uses int, following POSIX (where applicable). There is no consistency to be had here.
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
On Tue, Sep 01, 2020 at 08:46:02PM +0200, Florian Weimer wrote: > * Minchan Kim: > > > ssize_t process_madvise(int pidfd, const struct iovec *iovec, > > unsigned long vlen, int advice, unsigned int flags); > > size_t for vlen provides a clearer hint regarding the type of special > treatment needed for ILP32 here (zero extension, not changing the type > to long long). > All existing system calls using iove in Linux uses unsigned long so I want to be consistent with them unless process_madvise need something speicial.
Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
* Minchan Kim: > ssize_t process_madvise(int pidfd, const struct iovec *iovec, > unsigned long vlen, int advice, unsigned int flags); size_t for vlen provides a clearer hint regarding the type of special treatment needed for ILP32 here (zero extension, not changing the type to long long).
[PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API
There is usecase that System Management Software(SMS) want to give a
memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the
case of Android, it is the ActivityManagerService.
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.
To solve the issue, this patch introduces a new syscall process_madvise(2).
It uses pidfd of an external process to give the hint. It also supports
vector address range because Android app has thousands of vmas due to
zygote so it's totally waste of CPU and power if we should call the
syscall one by one for each vma.(With testing 2000-vma syscall vs
1-vector syscall, it showed 15% performance improvement. I think it
would be bigger in real practice because the testing ran very cache
friendly environment).
Another potential use case for the vector range is to amortize the cost
ofTLB shootdowns for multiple ranges when using MADV_DONTNEED; this
could benefit users like TCP receive zerocopy and malloc implementations.
In future, we could find more usecases for other advises so let's make it
happens as API since we introduce a new syscall at this moment. With
that, existing madvise(2) user could replace it with process_madvise(2)
with their own pid if they want to have batch address ranges support
feature.
ince it could affect other process's address range, only privileged
process(PTRACE_MODE_ATTACH_FSCREDS) or something else(e.g., being the
same UID) gives it the right to ptrace the process could use it
successfully. The flag argument is reserved for future use if we need to
extend the API.
I think supporting all hints madvise has/will supported/support to
process_madvise is rather risky. Because we are not sure all hints
make sense from external process and implementation for the hint may
rely on the caller being in the current context so it could be
error-prone. Thus, I just limited hints as MADV_[COLD|PAGEOUT] in this
patch.
If someone want to add other hints, we could hear the usecase and
review it for each hint. It's safer for maintenance rather than
introducing a buggy syscall but hard to fix it later.
So finally, the API is as follows,
ssize_t process_madvise(int pidfd, const struct iovec *iovec,
unsigned long vlen, int advice, unsigned int flags);
DESCRIPTION
The process_madvise() system call is used to give advice or directions
to the kernel about the address ranges from external process as well as
local process. It provides the advice to address ranges of process
described by iovec and vlen. The goal of such advice is to improve system
or application performance.
The pidfd selects the process referred to by the PID file descriptor
specified in pidfd. (See pidofd_open(2) for further information)
The pointer iovec points to an array of iovec structures, defined in
as:
struct iovec {
void *iov_base; /* starting address */
size_t iov_len; /* number of bytes to be advised */
};
The iovec describes address ranges beginning at address(iov_base)
and with size length of bytes(iov_len).
The vlen represents the number of elements in iovec.
The advice is indicated in the advice argument, which is one of the
following at this moment if the target process specified by pidfd is
external.
MADV_COLD
MADV_PAGEOUT
Permission to provide a hint to external process is governed by a
ptrace access mode PTRACE_MODE_ATTACH_FSCREDS check; see ptrace(2).
The process_madvise supports every advice madvise(2) has if target
process is in same thread group with calling process so user could
use process_madvise(2) to extend existing madvise(2) to support
vector address ranges.
RETURN VALUE
On success, process_madvise() returns the number of bytes advised.
This return value may be less than the total number of requested
bytes, if an error occurred. The caller should check return value
to determine whether a partial advice occurred.
FAQ:
Q.1 - Why does any external entity have better knowledge?
Quote from Sandeep
"For Android, every application (including the special SystemServer)
are forked from Zygote. The reason of course is to share as many
libraries and classes between the two as possible to benefit from the
preloading during boot.
After applications start, (almost) all of the APIs end up calling into
this SystemServer process over IPC (binder) and back to the
application.
In a fully running system, the SystemServer monitors every single
process periodically to calculate their PSS / RSS and also decides
which process is "important" to the user for interactivity.
So, because of how these processes start _and_ the fact
