Re: [PATCH v9 3/3] mm/madvise: introduce process_madvise() syscall: an external memory hinting API

[Minchan Kim <minchan@kernel.org>]

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), &iov, &iter);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = do_process_madvise(pidfd, &iter, 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 <minchan@kernel.org>
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 <minchan@kernel.org>
---
 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), &iov, &iter);
+	else
+#endif
+		ret = import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack),
+				&iov, &iter);
+	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(&iter);
+
+	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);
+	}
+
+	if (ret == 0)
+		ret = total_len - iov_iter_count(&iter);
 
 	mmput(mm);
+	return ret;
+
 release_task:
 	put_task_struct(task);
 put_pid:
 	put_pid(pid);
-	return ret;
-}
-
-static inline int madv_import_iovec(int type, const struct iovec __user *uvec, size_t 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
-
-	return import_iovec(type, uvec, nr_segs, fast_segs, iov, i);
-}
-
-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];
-	struct iovec *iov = iovstack;
-	struct iov_iter iter;
-
-	ret = madv_import_iovec(READ, vec, vlen, ARRAY_SIZE(iovstack), &iov, &iter);
-	if (ret < 0)
-		return ret;
-
-	ret = do_process_madvise(pidfd, &iter, behavior, flags);
+free_iov:
 	kfree(iov);
+out:
 	return ret;
 }
-- 
2.28.0.681.g6f77f65b4e-goog