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

[Minchan Kim]

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.