On Wed, Jun 24, 2020 at 01:00:14PM -0700, David Rientjes wrote:
> On Mon, 22 Jun 2020, Minchan Kim wrote:
>
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 551ed816eefe..23abca3f93fa 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -17,6 +17,7 @@
> > #include <linux/falloc.h>
> > #include <linux/fadvise.h>
> > #include <linux/sched.h>
> > +#include <linux/sched/mm.h>
> > #include <linux/ksm.h>
> > #include <linux/fs.h>
> > #include <linux/file.h>
> > @@ -995,6 +996,18 @@ madvise_behavior_valid(int behavior)
> > }
> > }
> >
> > +static bool
> > +process_madvise_behavior_valid(int behavior)
> > +{
> > + switch (behavior) {
> > + case MADV_COLD:
> > + case MADV_PAGEOUT:
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > /*
> > * The madvise(2) system call.
> > *
> > @@ -1042,6 +1055,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.
> > *
> > * return values:
> > * zero - success
> > @@ -1176,3 +1194,106 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > {
> > return do_madvise(current, current->mm, start, len_in, behavior);
> > }
> > +
> > +static int process_madvise_vec(struct task_struct *target_task,
> > + 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(target_task, 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)
> > +{
> > + ssize_t ret;
> > + struct pid *pid;
> > + struct task_struct *task;
> > + struct mm_struct *mm;
> > + size_t total_len = iov_iter_count(iter);
> > +
> > + if (flags != 0)
> > + return -EINVAL;
> > +
> > + pid = pidfd_get_pid(pidfd);
> > + if (IS_ERR(pid))
> > + return PTR_ERR(pid);
> > +
> > + task = get_pid_task(pid, PIDTYPE_PID);
> > + if (!task) {
> > + ret = -ESRCH;
> > + goto put_pid;
> > + }
> > +
> > + if (task->mm != current->mm &&
> > + !process_madvise_behavior_valid(behavior)) {
> > + ret = -EINVAL;
> > + goto release_task;
> > + }
> > +
> > + mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> > + if (IS_ERR_OR_NULL(mm)) {
> > + ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> > + goto release_task;
> > + }
> >
>
> mm is always task->mm right? I'm wondering if it would be better to find
> the mm directly in process_madvise_vec() rather than passing it into the
> function. I'm not sure why we'd pass both task and mm here.
That's because of hint Jann provided in the past version.
https://lore.kernel.org/linux-api/CAG48ez27=pwm5m_N_988xT1huO7g7h6arTQL44zev6TD-h-7Tg@xxxxxxxxxxxxxx/
Thanks for the review, David.
