[PATCH v3 0/5] introduce memory hinting API for external process

[PATCH v3 0/5] introduce memory hinting API for external process

[Minchan Kim]

Now, we have MADV_PAGEOUT and MADV_COLD as madvise hinting API. With that,
application could give hints to kernel what memory range are preferred to be
reclaimed. However, in some platform(e.g., Android), the information
required to make the hinting decision is not known to the app.
Instead, it is known to a centralized userspace daemon(e.g., ActivityManagerService),
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 - process_madvise(2).
Bascially, it's same with madvise(2) syscall but it has some differences.

1. It needs pidfd of target process to provide the hint
2. It supports only MADV_{COLD|PAGEOUT|MERGEABLE|UNMEREABLE} at this moment.
   Other hints in madvise will be opened when there are explicit requests from
   community to prevent unexpected bugs we couldn't support.
3. Only privileged processes can do something for other process's address
   space.

For more detail of the new API, please see "mm: introduce external memory hinting API"
description in this patchset.

Minchan Kim (3):
  mm: factor out madvise's core functionality
  mm: introduce external memory hinting API
  mm: support both pid and pidfd for process_madvise

Oleksandr Natalenko (2):
  mm/madvise: employ mmget_still_valid for write lock
  mm/madvise: allow KSM hints for remote API

* from v2 - https://lore.kernel.org/linux-mm/20200116235953.163318-1-minchan@kernel.org/
  * check signal callee and caller to bail out - Kirill Tkhai
  * put more clarification for justification of new API

* from v1 - https://lore.kernel.org/linux-mm/20200110213433.94739-1-minchan@kernel.org/
  * fix syscall number - SeongJae
  * use get_pid_task - Kirill Tkhai
  * extend API to support pid as well as pidfd - Kirill Tkhai

 arch/alpha/kernel/syscalls/syscall.tbl      |   1 +
 arch/arm/tools/syscall.tbl                  |   1 +
 arch/arm64/include/asm/unistd.h             |   2 +-
 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/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 +
 include/linux/pid.h                         |   1 +
 include/linux/syscalls.h                    |   3 +
 include/uapi/asm-generic/unistd.h           |   5 +-
 kernel/exit.c                               |  17 --
 kernel/pid.c                                |  17 ++
 kernel/sys_ni.c                             |   1 +
 mm/madvise.c                                | 277 ++++++++++++++------
 24 files changed, 238 insertions(+), 102 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[PATCH v3 1/5] mm: factor out madvise's core functionality

[Minchan Kim]

This patch factor out madvise's core functionality so that upcoming
patch can reuse it without duplication. It shouldn't change any behavior.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 194 +++++++++++++++++++++++++++++----------------------
 1 file changed, 111 insertions(+), 83 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index bcdb6a042787..0c901de531e4 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -35,6 +35,7 @@
 struct madvise_walk_private {
 	struct mmu_gather *tlb;
 	bool pageout;
+	struct task_struct *task;
 };
 
 /*
@@ -306,12 +307,13 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
 	bool pageout = private->pageout;
 	struct mm_struct *mm = tlb->mm;
 	struct vm_area_struct *vma = walk->vma;
+	struct task_struct *task = private->task;
 	pte_t *orig_pte, *pte, ptent;
 	spinlock_t *ptl;
 	struct page *page = NULL;
 	LIST_HEAD(page_list);
 
-	if (fatal_signal_pending(current))
+	if (fatal_signal_pending(task))
 		return -EINTR;
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
@@ -469,12 +471,14 @@ static const struct mm_walk_ops cold_walk_ops = {
 };
 
 static void madvise_cold_page_range(struct mmu_gather *tlb,
+			     struct task_struct *task,
 			     struct vm_area_struct *vma,
 			     unsigned long addr, unsigned long end)
 {
 	struct madvise_walk_private walk_private = {
 		.pageout = false,
 		.tlb = tlb,
+		.task = task,
 	};
 
 	tlb_start_vma(tlb, vma);
@@ -482,7 +486,7 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
 	tlb_end_vma(tlb, vma);
 }
 
-static long madvise_cold(struct vm_area_struct *vma,
+static long madvise_cold(struct task_struct *task, struct vm_area_struct *vma,
 			struct vm_area_struct **prev,
 			unsigned long start_addr, unsigned long end_addr)
 {
@@ -495,19 +499,21 @@ static long madvise_cold(struct vm_area_struct *vma,
 
 	lru_add_drain();
 	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
-	madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
+	madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
 	tlb_finish_mmu(&tlb, start_addr, end_addr);
 
 	return 0;
 }
 
 static void madvise_pageout_page_range(struct mmu_gather *tlb,
+			     struct task_struct *task,
 			     struct vm_area_struct *vma,
 			     unsigned long addr, unsigned long end)
 {
 	struct madvise_walk_private walk_private = {
 		.pageout = true,
 		.tlb = tlb,
+		.task = task,
 	};
 
 	tlb_start_vma(tlb, vma);
@@ -531,9 +537,9 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
 		inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
 }
 
-static long madvise_pageout(struct vm_area_struct *vma,
-			struct vm_area_struct **prev,
-			unsigned long start_addr, unsigned long end_addr)
+static long madvise_pageout(struct task_struct *task,
+		struct vm_area_struct *vma, struct vm_area_struct **prev,
+		unsigned long start_addr, unsigned long end_addr)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct mmu_gather tlb;
@@ -547,7 +553,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
 
 	lru_add_drain();
 	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
-	madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
+	madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
 	tlb_finish_mmu(&tlb, start_addr, end_addr);
 
 	return 0;
@@ -751,7 +757,8 @@ static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
 	return 0;
 }
 
-static long madvise_dontneed_free(struct vm_area_struct *vma,
+static long madvise_dontneed_free(struct mm_struct *mm,
+				  struct vm_area_struct *vma,
 				  struct vm_area_struct **prev,
 				  unsigned long start, unsigned long end,
 				  int behavior)
@@ -763,8 +770,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
 	if (!userfaultfd_remove(vma, start, end)) {
 		*prev = NULL; /* mmap_sem has been dropped, prev is stale */
 
-		down_read(&current->mm->mmap_sem);
-		vma = find_vma(current->mm, start);
+		down_read(&mm->mmap_sem);
+		vma = find_vma(mm, start);
 		if (!vma)
 			return -ENOMEM;
 		if (start < vma->vm_start) {
@@ -811,7 +818,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
  * Application wants to free up the pages and associated backing store.
  * This is effectively punching a hole into the middle of a file.
  */
-static long madvise_remove(struct vm_area_struct *vma,
+static long madvise_remove(struct mm_struct *mm,
+				struct vm_area_struct *vma,
 				struct vm_area_struct **prev,
 				unsigned long start, unsigned long end)
 {
@@ -845,13 +853,13 @@ static long madvise_remove(struct vm_area_struct *vma,
 	get_file(f);
 	if (userfaultfd_remove(vma, start, end)) {
 		/* mmap_sem was not released by userfaultfd_remove() */
-		up_read(&current->mm->mmap_sem);
+		up_read(&mm->mmap_sem);
 	}
 	error = vfs_fallocate(f,
 				FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
 				offset, end - start);
 	fput(f);
-	down_read(&current->mm->mmap_sem);
+	down_read(&mm->mmap_sem);
 	return error;
 }
 
@@ -925,21 +933,23 @@ static int madvise_inject_error(int behavior,
 #endif
 
 static long
-madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
+madvise_vma(struct task_struct *task, struct mm_struct *mm,
+		struct vm_area_struct *vma, struct vm_area_struct **prev,
 		unsigned long start, unsigned long end, int behavior)
 {
 	switch (behavior) {
 	case MADV_REMOVE:
-		return madvise_remove(vma, prev, start, end);
+		return madvise_remove(mm, vma, prev, start, end);
 	case MADV_WILLNEED:
 		return madvise_willneed(vma, prev, start, end);
 	case MADV_COLD:
-		return madvise_cold(vma, prev, start, end);
+		return madvise_cold(task, vma, prev, start, end);
 	case MADV_PAGEOUT:
-		return madvise_pageout(vma, prev, start, end);
+		return madvise_pageout(task, vma, prev, start, end);
 	case MADV_FREE:
 	case MADV_DONTNEED:
-		return madvise_dontneed_free(vma, prev, start, end, behavior);
+		return madvise_dontneed_free(mm, vma, prev, start,
+						end, behavior);
 	default:
 		return madvise_behavior(vma, prev, start, end, behavior);
 	}
@@ -984,67 +994,19 @@ madvise_behavior_valid(int behavior)
 }
 
 /*
- * The madvise(2) system call.
+ * madvise_common - request behavior hint to address range of the target process
  *
- * Applications can use madvise() to advise the kernel how it should
- * handle paging I/O in this VM area.  The idea is to help the kernel
- * use appropriate read-ahead and caching techniques.  The information
- * provided is advisory only, and can be safely disregarded by the
- * kernel without affecting the correct operation of the application.
+ * @task: task_struct got behavior hint, not giving the hint
+ * @mm: mm_struct got behavior hint, not giving the hint
+ * @start: base address of the hinted range
+ * @len_in: length of the hinted range
+ * @behavior: requested hint
  *
- * behavior values:
- *  MADV_NORMAL - the default behavior is to read clusters.  This
- *		results in some read-ahead and read-behind.
- *  MADV_RANDOM - the system should read the minimum amount of data
- *		on any access, since it is unlikely that the appli-
- *		cation will need more than what it asks for.
- *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
- *		once, so they can be aggressively read ahead, and
- *		can be freed soon after they are accessed.
- *  MADV_WILLNEED - the application is notifying the system to read
- *		some pages ahead.
- *  MADV_DONTNEED - the application is finished with the given range,
- *		so the kernel can free resources associated with it.
- *  MADV_FREE - the application marks pages in the given range as lazy free,
- *		where actual purges are postponed until memory pressure happens.
- *  MADV_REMOVE - the application wants to free up the given range of
- *		pages and associated backing store.
- *  MADV_DONTFORK - omit this area from child's address space when forking:
- *		typically, to avoid COWing pages pinned by get_user_pages().
- *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
- *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
- *              range after a fork.
- *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
- *  MADV_HWPOISON - trigger memory error handler as if the given memory range
- *		were corrupted by unrecoverable hardware memory failure.
- *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
- *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
- *		this area with pages of identical content from other such areas.
- *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
- *  MADV_HUGEPAGE - the application wants to back the given range by transparent
- *		huge pages in the future. Existing pages might be coalesced and
- *		new pages might be allocated as THP.
- *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
- *		transparent huge pages so the existing pages will not be
- *		coalesced into THP and new pages will not be allocated as THP.
- *  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.
- *
- * return values:
- *  zero    - success
- *  -EINVAL - start + len < 0, start is not page-aligned,
- *		"behavior" is not a valid value, or application
- *		is attempting to release locked or shared pages,
- *		or the specified address range includes file, Huge TLB,
- *		MAP_SHARED or VMPFNMAP range.
- *  -ENOMEM - addresses in the specified range are not currently
- *		mapped, or are outside the AS of the process.
- *  -EIO    - an I/O error occurred while paging in data.
- *  -EBADF  - map exists, but area maps something that isn't a file.
- *  -EAGAIN - a kernel resource was temporarily unavailable.
+ * @task could be a zombie leader if it calls sys_exit so accessing mm_struct
+ * via task->mm is prohibited. Please use @mm instead of task->mm.
  */
-SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
+static int madvise_common(struct task_struct *task, struct mm_struct *mm,
+			unsigned long start, size_t len_in, int behavior)
 {
 	unsigned long end, tmp;
 	struct vm_area_struct *vma, *prev;
@@ -1082,10 +1044,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 
 	write = madvise_need_mmap_write(behavior);
 	if (write) {
-		if (down_write_killable(&current->mm->mmap_sem))
+		if (down_write_killable(&mm->mmap_sem))
 			return -EINTR;
 	} else {
-		down_read(&current->mm->mmap_sem);
+		down_read(&mm->mmap_sem);
 	}
 
 	/*
@@ -1093,7 +1055,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	 * ranges, just ignore them, but return -ENOMEM at the end.
 	 * - different from the way of handling in mlock etc.
 	 */
-	vma = find_vma_prev(current->mm, start, &prev);
+	vma = find_vma_prev(mm, start, &prev);
 	if (vma && start > vma->vm_start)
 		prev = vma;
 
@@ -1118,7 +1080,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 			tmp = end;
 
 		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
-		error = madvise_vma(vma, &prev, start, tmp, behavior);
+		error = madvise_vma(task, mm, vma, &prev, start, tmp, behavior);
 		if (error)
 			goto out;
 		start = tmp;
@@ -1130,14 +1092,80 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 		if (prev)
 			vma = prev->vm_next;
 		else	/* madvise_remove dropped mmap_sem */
-			vma = find_vma(current->mm, start);
+			vma = find_vma(mm, start);
 	}
 out:
 	blk_finish_plug(&plug);
 	if (write)
-		up_write(&current->mm->mmap_sem);
+		up_write(&mm->mmap_sem);
 	else
-		up_read(&current->mm->mmap_sem);
+		up_read(&mm->mmap_sem);
 
 	return error;
 }
+
+/*
+ * The madvise(2) system call.
+ *
+ * Applications can use madvise() to advise the kernel how it should
+ * handle paging I/O in this VM area.  The idea is to help the kernel
+ * use appropriate read-ahead and caching techniques.  The information
+ * provided is advisory only, and can be safely disregarded by the
+ * kernel without affecting the correct operation of the application.
+ *
+ * behavior values:
+ *  MADV_NORMAL - the default behavior is to read clusters.  This
+ *		results in some read-ahead and read-behind.
+ *  MADV_RANDOM - the system should read the minimum amount of data
+ *		on any access, since it is unlikely that the appli-
+ *		cation will need more than what it asks for.
+ *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
+ *		once, so they can be aggressively read ahead, and
+ *		can be freed soon after they are accessed.
+ *  MADV_WILLNEED - the application is notifying the system to read
+ *		some pages ahead.
+ *  MADV_DONTNEED - the application is finished with the given range,
+ *		so the kernel can free resources associated with it.
+ *  MADV_FREE - the application marks pages in the given range as lazy free,
+ *		where actual purges are postponed until memory pressure happens.
+ *  MADV_REMOVE - the application wants to free up the given range of
+ *		pages and associated backing store.
+ *  MADV_DONTFORK - omit this area from child's address space when forking:
+ *		typically, to avoid COWing pages pinned by get_user_pages().
+ *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
+ *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
+ *              range after a fork.
+ *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
+ *  MADV_HWPOISON - trigger memory error handler as if the given memory range
+ *		were corrupted by unrecoverable hardware memory failure.
+ *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
+ *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
+ *		this area with pages of identical content from other such areas.
+ *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
+ *  MADV_HUGEPAGE - the application wants to back the given range by transparent
+ *		huge pages in the future. Existing pages might be coalesced and
+ *		new pages might be allocated as THP.
+ *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
+ *		transparent huge pages so the existing pages will not be
+ *		coalesced into THP and new pages will not be allocated as THP.
+ *  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.
+ *
+ * return values:
+ *  zero    - success
+ *  -EINVAL - start + len < 0, start is not page-aligned,
+ *		"behavior" is not a valid value, or application
+ *		is attempting to release locked or shared pages,
+ *		or the specified address range includes file, Huge TLB,
+ *		MAP_SHARED or VMPFNMAP range.
+ *  -ENOMEM - addresses in the specified range are not currently
+ *		mapped, or are outside the AS of the process.
+ *  -EIO    - an I/O error occurred while paging in data.
+ *  -EBADF  - map exists, but area maps something that isn't a file.
+ *  -EAGAIN - a kernel resource was temporarily unavailable.
+ */
+SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
+{
+	return madvise_common(current, current->mm, start, len_in, behavior);
+}
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v3 1/5] mm: factor out madvise's core functionality

[Alexander Duyck]

On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> wrote:
>
> This patch factor out madvise's core functionality so that upcoming
> patch can reuse it without duplication. It shouldn't change any behavior.
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

There is a lot to unpack here. I really feel like this description
doesn't do the changes below any service. Really I feel like this
patch should probably be broken up over a few patches to make it
easier to review.

You have the moving of the function form the syscall madvise to the
function madvise_common. Then you have various function arguments that
are being added throughout.

Also this patchset needs to be rebased. It looks like there is already
a patch that is making madvise accessible:
https://lore.kernel.org/io-uring/20200110154739.2119-3-axboe@kernel.dk/

> ---
>  mm/madvise.c | 194 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 111 insertions(+), 83 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index bcdb6a042787..0c901de531e4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -35,6 +35,7 @@
>  struct madvise_walk_private {
>         struct mmu_gather *tlb;
>         bool pageout;
> +       struct task_struct *task;
>  };
>
>  /*
> @@ -306,12 +307,13 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>         bool pageout = private->pageout;
>         struct mm_struct *mm = tlb->mm;
>         struct vm_area_struct *vma = walk->vma;
> +       struct task_struct *task = private->task;
>         pte_t *orig_pte, *pte, ptent;
>         spinlock_t *ptl;
>         struct page *page = NULL;
>         LIST_HEAD(page_list);
>
> -       if (fatal_signal_pending(current))
> +       if (fatal_signal_pending(task))
>                 return -EINTR;
>
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE

Is this the only spot in this function that uses the task? If so why
bother with adding the argument above and not just call private->task
instead of current?

> @@ -469,12 +471,14 @@ static const struct mm_walk_ops cold_walk_ops = {
>  };
>
>  static void madvise_cold_page_range(struct mmu_gather *tlb,
> +                            struct task_struct *task,
>                              struct vm_area_struct *vma,
>                              unsigned long addr, unsigned long end)
>  {
>         struct madvise_walk_private walk_private = {
>                 .pageout = false,
>                 .tlb = tlb,
> +               .task = task,
>         };
>
>         tlb_start_vma(tlb, vma);
> @@ -482,7 +486,7 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
>         tlb_end_vma(tlb, vma);
>  }
>
> -static long madvise_cold(struct vm_area_struct *vma,
> +static long madvise_cold(struct task_struct *task, struct vm_area_struct *vma,
>                         struct vm_area_struct **prev,
>                         unsigned long start_addr, unsigned long end_addr)
>  {
> @@ -495,19 +499,21 @@ static long madvise_cold(struct vm_area_struct *vma,
>
>         lru_add_drain();
>         tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> -       madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
> +       madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
>         tlb_finish_mmu(&tlb, start_addr, end_addr);
>
>         return 0;
>  }
>

Is there any specific reason for adding the task in the middle of the
list of arguments instead of just placing it at the end? It makes it a
bit harder to review when arguments are added in the middle of the
argument list, or at least that is my opinion.

>  static void madvise_pageout_page_range(struct mmu_gather *tlb,
> +                            struct task_struct *task,
>                              struct vm_area_struct *vma,
>                              unsigned long addr, unsigned long end)
>  {
>         struct madvise_walk_private walk_private = {
>                 .pageout = true,
>                 .tlb = tlb,
> +               .task = task,
>         };
>
>         tlb_start_vma(tlb, vma);
> @@ -531,9 +537,9 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
>                 inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
>  }
>
> -static long madvise_pageout(struct vm_area_struct *vma,
> -                       struct vm_area_struct **prev,
> -                       unsigned long start_addr, unsigned long end_addr)
> +static long madvise_pageout(struct task_struct *task,
> +               struct vm_area_struct *vma, struct vm_area_struct **prev,
> +               unsigned long start_addr, unsigned long end_addr)
>  {
>         struct mm_struct *mm = vma->vm_mm;
>         struct mmu_gather tlb;
> @@ -547,7 +553,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
>
>         lru_add_drain();
>         tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> -       madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
> +       madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
>         tlb_finish_mmu(&tlb, start_addr, end_addr);
>
>         return 0;
> @@ -751,7 +757,8 @@ static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>         return 0;
>  }
>

Okay so we were adding a task_struct pointer as an argument up to this
point and now we are switching to adding an mm_struct pointer here. I
would split these two off as separate patches and explain in the patch
description why you are doing that instead of just passing the task
here as well and then accessing task->mm.


> -static long madvise_dontneed_free(struct vm_area_struct *vma,
> +static long madvise_dontneed_free(struct mm_struct *mm,
> +                                 struct vm_area_struct *vma,
>                                   struct vm_area_struct **prev,
>                                   unsigned long start, unsigned long end,
>                                   int behavior)
> @@ -763,8 +770,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
>         if (!userfaultfd_remove(vma, start, end)) {
>                 *prev = NULL; /* mmap_sem has been dropped, prev is stale */
>
> -               down_read(&current->mm->mmap_sem);
> -               vma = find_vma(current->mm, start);
> +               down_read(&mm->mmap_sem);
> +               vma = find_vma(mm, start);
>                 if (!vma)
>                         return -ENOMEM;
>                 if (start < vma->vm_start) {
> @@ -811,7 +818,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
>   * Application wants to free up the pages and associated backing store.
>   * This is effectively punching a hole into the middle of a file.
>   */
> -static long madvise_remove(struct vm_area_struct *vma,
> +static long madvise_remove(struct mm_struct *mm,
> +                               struct vm_area_struct *vma,
>                                 struct vm_area_struct **prev,
>                                 unsigned long start, unsigned long end)
>  {
> @@ -845,13 +853,13 @@ static long madvise_remove(struct vm_area_struct *vma,
>         get_file(f);
>         if (userfaultfd_remove(vma, start, end)) {
>                 /* mmap_sem was not released by userfaultfd_remove() */
> -               up_read(&current->mm->mmap_sem);
> +               up_read(&mm->mmap_sem);
>         }
>         error = vfs_fallocate(f,
>                                 FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
>                                 offset, end - start);
>         fput(f);
> -       down_read(&current->mm->mmap_sem);
> +       down_read(&mm->mmap_sem);
>         return error;
>  }
>
> @@ -925,21 +933,23 @@ static int madvise_inject_error(int behavior,
>  #endif
>
>  static long
> -madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
> +madvise_vma(struct task_struct *task, struct mm_struct *mm,
> +               struct vm_area_struct *vma, struct vm_area_struct **prev,
>                 unsigned long start, unsigned long end, int behavior)
>  {
>         switch (behavior) {
>         case MADV_REMOVE:
> -               return madvise_remove(vma, prev, start, end);
> +               return madvise_remove(mm, vma, prev, start, end);
>         case MADV_WILLNEED:
>                 return madvise_willneed(vma, prev, start, end);
>         case MADV_COLD:
> -               return madvise_cold(vma, prev, start, end);
> +               return madvise_cold(task, vma, prev, start, end);
>         case MADV_PAGEOUT:
> -               return madvise_pageout(vma, prev, start, end);
> +               return madvise_pageout(task, vma, prev, start, end);
>         case MADV_FREE:
>         case MADV_DONTNEED:
> -               return madvise_dontneed_free(vma, prev, start, end, behavior);
> +               return madvise_dontneed_free(mm, vma, prev, start,
> +                                               end, behavior);
>         default:
>                 return madvise_behavior(vma, prev, start, end, behavior);
>         }

Here you added both task and mm. Is ti truly necessary to have both?
Would it be possible to make use of either task->mm or mm->owner in
order to make it so that you only had to carry one of these?

> @@ -984,67 +994,19 @@ madvise_behavior_valid(int behavior)
>  }
>
>  /*
> - * The madvise(2) system call.
> + * madvise_common - request behavior hint to address range of the target process
>   *
> - * Applications can use madvise() to advise the kernel how it should
> - * handle paging I/O in this VM area.  The idea is to help the kernel
> - * use appropriate read-ahead and caching techniques.  The information
> - * provided is advisory only, and can be safely disregarded by the
> - * kernel without affecting the correct operation of the application.
> + * @task: task_struct got behavior hint, not giving the hint
> + * @mm: mm_struct got behavior hint, not giving the hint
> + * @start: base address of the hinted range
> + * @len_in: length of the hinted range
> + * @behavior: requested hint
>   *
> - * behavior values:
> - *  MADV_NORMAL - the default behavior is to read clusters.  This
> - *             results in some read-ahead and read-behind.
> - *  MADV_RANDOM - the system should read the minimum amount of data
> - *             on any access, since it is unlikely that the appli-
> - *             cation will need more than what it asks for.
> - *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> - *             once, so they can be aggressively read ahead, and
> - *             can be freed soon after they are accessed.
> - *  MADV_WILLNEED - the application is notifying the system to read
> - *             some pages ahead.
> - *  MADV_DONTNEED - the application is finished with the given range,
> - *             so the kernel can free resources associated with it.
> - *  MADV_FREE - the application marks pages in the given range as lazy free,
> - *             where actual purges are postponed until memory pressure happens.
> - *  MADV_REMOVE - the application wants to free up the given range of
> - *             pages and associated backing store.
> - *  MADV_DONTFORK - omit this area from child's address space when forking:
> - *             typically, to avoid COWing pages pinned by get_user_pages().
> - *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> - *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> - *              range after a fork.
> - *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> - *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> - *             were corrupted by unrecoverable hardware memory failure.
> - *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> - *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> - *             this area with pages of identical content from other such areas.
> - *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> - *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> - *             huge pages in the future. Existing pages might be coalesced and
> - *             new pages might be allocated as THP.
> - *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> - *             transparent huge pages so the existing pages will not be
> - *             coalesced into THP and new pages will not be allocated as THP.
> - *  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.
> - *
> - * return values:
> - *  zero    - success
> - *  -EINVAL - start + len < 0, start is not page-aligned,
> - *             "behavior" is not a valid value, or application
> - *             is attempting to release locked or shared pages,
> - *             or the specified address range includes file, Huge TLB,
> - *             MAP_SHARED or VMPFNMAP range.
> - *  -ENOMEM - addresses in the specified range are not currently
> - *             mapped, or are outside the AS of the process.
> - *  -EIO    - an I/O error occurred while paging in data.
> - *  -EBADF  - map exists, but area maps something that isn't a file.
> - *  -EAGAIN - a kernel resource was temporarily unavailable.
> + * @task could be a zombie leader if it calls sys_exit so accessing mm_struct
> + * via task->mm is prohibited. Please use @mm instead of task->mm.
>   */
> -SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> +static int madvise_common(struct task_struct *task, struct mm_struct *mm,
> +                       unsigned long start, size_t len_in, int behavior)
>  {
>         unsigned long end, tmp;
>         struct vm_area_struct *vma, *prev;

So I would save this piece and move it to a separate patch. All of the
noise from the comment move would be greatly reduced and make it
easier to review the rest of this.

> @@ -1082,10 +1044,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>
>         write = madvise_need_mmap_write(behavior);
>         if (write) {
> -               if (down_write_killable(&current->mm->mmap_sem))
> +               if (down_write_killable(&mm->mmap_sem))
>                         return -EINTR;
>         } else {
> -               down_read(&current->mm->mmap_sem);
> +               down_read(&mm->mmap_sem);
>         }
>
>         /*
> @@ -1093,7 +1055,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>          * ranges, just ignore them, but return -ENOMEM at the end.
>          * - different from the way of handling in mlock etc.
>          */
> -       vma = find_vma_prev(current->mm, start, &prev);
> +       vma = find_vma_prev(mm, start, &prev);
>         if (vma && start > vma->vm_start)
>                 prev = vma;
>
> @@ -1118,7 +1080,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>                         tmp = end;
>
>                 /* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> -               error = madvise_vma(vma, &prev, start, tmp, behavior);
> +               error = madvise_vma(task, mm, vma, &prev, start, tmp, behavior);
>                 if (error)
>                         goto out;
>                 start = tmp;
> @@ -1130,14 +1092,80 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>                 if (prev)
>                         vma = prev->vm_next;
>                 else    /* madvise_remove dropped mmap_sem */
> -                       vma = find_vma(current->mm, start);
> +                       vma = find_vma(mm, start);
>         }
>  out:
>         blk_finish_plug(&plug);
>         if (write)
> -               up_write(&current->mm->mmap_sem);
> +               up_write(&mm->mmap_sem);
>         else
> -               up_read(&current->mm->mmap_sem);
> +               up_read(&mm->mmap_sem);
>
>         return error;
>  }

Okay so the rest of this is just more of the task/mm being added as
separate arguments.

> +
> +/*
> + * The madvise(2) system call.
> + *
> + * Applications can use madvise() to advise the kernel how it should
> + * handle paging I/O in this VM area.  The idea is to help the kernel
> + * use appropriate read-ahead and caching techniques.  The information
> + * provided is advisory only, and can be safely disregarded by the
> + * kernel without affecting the correct operation of the application.
> + *
> + * behavior values:
> + *  MADV_NORMAL - the default behavior is to read clusters.  This
> + *             results in some read-ahead and read-behind.
> + *  MADV_RANDOM - the system should read the minimum amount of data
> + *             on any access, since it is unlikely that the appli-
> + *             cation will need more than what it asks for.
> + *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> + *             once, so they can be aggressively read ahead, and
> + *             can be freed soon after they are accessed.
> + *  MADV_WILLNEED - the application is notifying the system to read
> + *             some pages ahead.
> + *  MADV_DONTNEED - the application is finished with the given range,
> + *             so the kernel can free resources associated with it.
> + *  MADV_FREE - the application marks pages in the given range as lazy free,
> + *             where actual purges are postponed until memory pressure happens.
> + *  MADV_REMOVE - the application wants to free up the given range of
> + *             pages and associated backing store.
> + *  MADV_DONTFORK - omit this area from child's address space when forking:
> + *             typically, to avoid COWing pages pinned by get_user_pages().
> + *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> + *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> + *              range after a fork.
> + *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> + *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> + *             were corrupted by unrecoverable hardware memory failure.
> + *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> + *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> + *             this area with pages of identical content from other such areas.
> + *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> + *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> + *             huge pages in the future. Existing pages might be coalesced and
> + *             new pages might be allocated as THP.
> + *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> + *             transparent huge pages so the existing pages will not be
> + *             coalesced into THP and new pages will not be allocated as THP.
> + *  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.
> + *
> + * return values:
> + *  zero    - success
> + *  -EINVAL - start + len < 0, start is not page-aligned,
> + *             "behavior" is not a valid value, or application
> + *             is attempting to release locked or shared pages,
> + *             or the specified address range includes file, Huge TLB,
> + *             MAP_SHARED or VMPFNMAP range.
> + *  -ENOMEM - addresses in the specified range are not currently
> + *             mapped, or are outside the AS of the process.
> + *  -EIO    - an I/O error occurred while paging in data.
> + *  -EBADF  - map exists, but area maps something that isn't a file.
> + *  -EAGAIN - a kernel resource was temporarily unavailable.
> + */
> +SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> +{
> +       return madvise_common(current, current->mm, start, len_in, behavior);
> +}
> --
> 2.25.0.341.g760bfbb309-goog
>
>

Re: [PATCH v3 1/5] mm: factor out madvise's core functionality

[Minchan Kim]

Hi Alexander,

On Mon, Feb 10, 2020 at 03:00:41PM -0800, Alexander Duyck wrote:
> On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> wrote:
> >
> > This patch factor out madvise's core functionality so that upcoming
> > patch can reuse it without duplication. It shouldn't change any behavior.
> >
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> There is a lot to unpack here. I really feel like this description
> doesn't do the changes below any service. Really I feel like this
> patch should probably be broken up over a few patches to make it
> easier to review.

Once I read up below, yes, I agree with you. I will try to make it
more clear as you suggested.

> 
> You have the moving of the function form the syscall madvise to the
> function madvise_common. Then you have various function arguments that
> are being added throughout.
> 
> Also this patchset needs to be rebased. It looks like there is already
> a patch that is making madvise accessible:
> https://lore.kernel.org/io-uring/20200110154739.2119-3-axboe@kernel.dk/

Thanks for heads up!

> 
> > ---
> >  mm/madvise.c | 194 +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 111 insertions(+), 83 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index bcdb6a042787..0c901de531e4 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -35,6 +35,7 @@
> >  struct madvise_walk_private {
> >         struct mmu_gather *tlb;
> >         bool pageout;
> > +       struct task_struct *task;
> >  };
> >
> >  /*
> > @@ -306,12 +307,13 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >         bool pageout = private->pageout;
> >         struct mm_struct *mm = tlb->mm;
> >         struct vm_area_struct *vma = walk->vma;
> > +       struct task_struct *task = private->task;
> >         pte_t *orig_pte, *pte, ptent;
> >         spinlock_t *ptl;
> >         struct page *page = NULL;
> >         LIST_HEAD(page_list);
> >
> > -       if (fatal_signal_pending(current))
> > +       if (fatal_signal_pending(task))
> >                 return -EINTR;
> >
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> 
> Is this the only spot in this function that uses the task? If so why
> bother with adding the argument above and not just call private->task
> instead of current?

I just wanted to check target process signal initially but afterwards
realized we need to check caller process as well as target's one.
Anyway, it would be more clear not to handle it in this patchset to make
it "non-behavior change" for easier review.

> 
> > @@ -469,12 +471,14 @@ static const struct mm_walk_ops cold_walk_ops = {
> >  };
> >
> >  static void madvise_cold_page_range(struct mmu_gather *tlb,
> > +                            struct task_struct *task,
> >                              struct vm_area_struct *vma,
> >                              unsigned long addr, unsigned long end)
> >  {
> >         struct madvise_walk_private walk_private = {
> >                 .pageout = false,
> >                 .tlb = tlb,
> > +               .task = task,
> >         };
> >
> >         tlb_start_vma(tlb, vma);
> > @@ -482,7 +486,7 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
> >         tlb_end_vma(tlb, vma);
> >  }
> >
> > -static long madvise_cold(struct vm_area_struct *vma,
> > +static long madvise_cold(struct task_struct *task, struct vm_area_struct *vma,
> >                         struct vm_area_struct **prev,
> >                         unsigned long start_addr, unsigned long end_addr)
> >  {
> > @@ -495,19 +499,21 @@ static long madvise_cold(struct vm_area_struct *vma,
> >
> >         lru_add_drain();
> >         tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > -       madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
> > +       madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
> >         tlb_finish_mmu(&tlb, start_addr, end_addr);
> >
> >         return 0;
> >  }
> >
> 
> Is there any specific reason for adding the task in the middle of the
> list of arguments instead of just placing it at the end? It makes it a
> bit harder to review when arguments are added in the middle of the
> argument list, or at least that is my opinion.

Personally, I prefer top-down hierarchy list from bigger one to smaller.
I have thought it's popular way for code strucdturing(but I am only one
person to agree on it ;-)) for consistency and easy to think.
If we add back to the list for new arguments, it would hurt it, which
is my concern.

> 
> >  static void madvise_pageout_page_range(struct mmu_gather *tlb,
> > +                            struct task_struct *task,
> >                              struct vm_area_struct *vma,
> >                              unsigned long addr, unsigned long end)
> >  {
> >         struct madvise_walk_private walk_private = {
> >                 .pageout = true,
> >                 .tlb = tlb,
> > +               .task = task,
> >         };
> >
> >         tlb_start_vma(tlb, vma);
> > @@ -531,9 +537,9 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
> >                 inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
> >  }
> >
> > -static long madvise_pageout(struct vm_area_struct *vma,
> > -                       struct vm_area_struct **prev,
> > -                       unsigned long start_addr, unsigned long end_addr)
> > +static long madvise_pageout(struct task_struct *task,
> > +               struct vm_area_struct *vma, struct vm_area_struct **prev,
> > +               unsigned long start_addr, unsigned long end_addr)
> >  {
> >         struct mm_struct *mm = vma->vm_mm;
> >         struct mmu_gather tlb;
> > @@ -547,7 +553,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
> >
> >         lru_add_drain();
> >         tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > -       madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
> > +       madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
> >         tlb_finish_mmu(&tlb, start_addr, end_addr);
> >
> >         return 0;
> > @@ -751,7 +757,8 @@ static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
> >         return 0;
> >  }
> >
> 
> Okay so we were adding a task_struct pointer as an argument up to this
> point and now we are switching to adding an mm_struct pointer here. I
> would split these two off as separate patches and explain in the patch
> description why you are doing that instead of just passing the task
> here as well and then accessing task->mm.

Sure.

> 
> 
> > -static long madvise_dontneed_free(struct vm_area_struct *vma,
> > +static long madvise_dontneed_free(struct mm_struct *mm,
> > +                                 struct vm_area_struct *vma,
> >                                   struct vm_area_struct **prev,
> >                                   unsigned long start, unsigned long end,
> >                                   int behavior)
> > @@ -763,8 +770,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> >         if (!userfaultfd_remove(vma, start, end)) {
> >                 *prev = NULL; /* mmap_sem has been dropped, prev is stale */
> >
> > -               down_read(&current->mm->mmap_sem);
> > -               vma = find_vma(current->mm, start);
> > +               down_read(&mm->mmap_sem);
> > +               vma = find_vma(mm, start);
> >                 if (!vma)
> >                         return -ENOMEM;
> >                 if (start < vma->vm_start) {
> > @@ -811,7 +818,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> >   * Application wants to free up the pages and associated backing store.
> >   * This is effectively punching a hole into the middle of a file.
> >   */
> > -static long madvise_remove(struct vm_area_struct *vma,
> > +static long madvise_remove(struct mm_struct *mm,
> > +                               struct vm_area_struct *vma,
> >                                 struct vm_area_struct **prev,
> >                                 unsigned long start, unsigned long end)
> >  {
> > @@ -845,13 +853,13 @@ static long madvise_remove(struct vm_area_struct *vma,
> >         get_file(f);
> >         if (userfaultfd_remove(vma, start, end)) {
> >                 /* mmap_sem was not released by userfaultfd_remove() */
> > -               up_read(&current->mm->mmap_sem);
> > +               up_read(&mm->mmap_sem);
> >         }
> >         error = vfs_fallocate(f,
> >                                 FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> >                                 offset, end - start);
> >         fput(f);
> > -       down_read(&current->mm->mmap_sem);
> > +       down_read(&mm->mmap_sem);
> >         return error;
> >  }
> >
> > @@ -925,21 +933,23 @@ static int madvise_inject_error(int behavior,
> >  #endif
> >
> >  static long
> > -madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > +madvise_vma(struct task_struct *task, struct mm_struct *mm,
> > +               struct vm_area_struct *vma, struct vm_area_struct **prev,
> >                 unsigned long start, unsigned long end, int behavior)
> >  {
> >         switch (behavior) {
> >         case MADV_REMOVE:
> > -               return madvise_remove(vma, prev, start, end);
> > +               return madvise_remove(mm, vma, prev, start, end);
> >         case MADV_WILLNEED:
> >                 return madvise_willneed(vma, prev, start, end);
> >         case MADV_COLD:
> > -               return madvise_cold(vma, prev, start, end);
> > +               return madvise_cold(task, vma, prev, start, end);
> >         case MADV_PAGEOUT:
> > -               return madvise_pageout(vma, prev, start, end);
> > +               return madvise_pageout(task, vma, prev, start, end);
> >         case MADV_FREE:
> >         case MADV_DONTNEED:
> > -               return madvise_dontneed_free(vma, prev, start, end, behavior);
> > +               return madvise_dontneed_free(mm, vma, prev, start,
> > +                                               end, behavior);
> >         default:
> >                 return madvise_behavior(vma, prev, start, end, behavior);
> >         }
> 
> Here you added both task and mm. Is ti truly necessary to have both?
> Would it be possible to make use of either task->mm or mm->owner in
> order to make it so that you only had to carry one of these?

Good point. I guess task would be enough to pass mm_struct. I will look
into that.

> 
> > @@ -984,67 +994,19 @@ madvise_behavior_valid(int behavior)
> >  }
> >
> >  /*
> > - * The madvise(2) system call.
> > + * madvise_common - request behavior hint to address range of the target process
> >   *
> > - * Applications can use madvise() to advise the kernel how it should
> > - * handle paging I/O in this VM area.  The idea is to help the kernel
> > - * use appropriate read-ahead and caching techniques.  The information
> > - * provided is advisory only, and can be safely disregarded by the
> > - * kernel without affecting the correct operation of the application.
> > + * @task: task_struct got behavior hint, not giving the hint
> > + * @mm: mm_struct got behavior hint, not giving the hint
> > + * @start: base address of the hinted range
> > + * @len_in: length of the hinted range
> > + * @behavior: requested hint
> >   *
> > - * behavior values:
> > - *  MADV_NORMAL - the default behavior is to read clusters.  This
> > - *             results in some read-ahead and read-behind.
> > - *  MADV_RANDOM - the system should read the minimum amount of data
> > - *             on any access, since it is unlikely that the appli-
> > - *             cation will need more than what it asks for.
> > - *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> > - *             once, so they can be aggressively read ahead, and
> > - *             can be freed soon after they are accessed.
> > - *  MADV_WILLNEED - the application is notifying the system to read
> > - *             some pages ahead.
> > - *  MADV_DONTNEED - the application is finished with the given range,
> > - *             so the kernel can free resources associated with it.
> > - *  MADV_FREE - the application marks pages in the given range as lazy free,
> > - *             where actual purges are postponed until memory pressure happens.
> > - *  MADV_REMOVE - the application wants to free up the given range of
> > - *             pages and associated backing store.
> > - *  MADV_DONTFORK - omit this area from child's address space when forking:
> > - *             typically, to avoid COWing pages pinned by get_user_pages().
> > - *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> > - *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> > - *              range after a fork.
> > - *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> > - *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> > - *             were corrupted by unrecoverable hardware memory failure.
> > - *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> > - *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> > - *             this area with pages of identical content from other such areas.
> > - *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> > - *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> > - *             huge pages in the future. Existing pages might be coalesced and
> > - *             new pages might be allocated as THP.
> > - *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> > - *             transparent huge pages so the existing pages will not be
> > - *             coalesced into THP and new pages will not be allocated as THP.
> > - *  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.
> > - *
> > - * return values:
> > - *  zero    - success
> > - *  -EINVAL - start + len < 0, start is not page-aligned,
> > - *             "behavior" is not a valid value, or application
> > - *             is attempting to release locked or shared pages,
> > - *             or the specified address range includes file, Huge TLB,
> > - *             MAP_SHARED or VMPFNMAP range.
> > - *  -ENOMEM - addresses in the specified range are not currently
> > - *             mapped, or are outside the AS of the process.
> > - *  -EIO    - an I/O error occurred while paging in data.
> > - *  -EBADF  - map exists, but area maps something that isn't a file.
> > - *  -EAGAIN - a kernel resource was temporarily unavailable.
> > + * @task could be a zombie leader if it calls sys_exit so accessing mm_struct
> > + * via task->mm is prohibited. Please use @mm instead of task->mm.
> >   */
> > -SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > +static int madvise_common(struct task_struct *task, struct mm_struct *mm,
> > +                       unsigned long start, size_t len_in, int behavior)
> >  {
> >         unsigned long end, tmp;
> >         struct vm_area_struct *vma, *prev;
> 
> So I would save this piece and move it to a separate patch. All of the
> noise from the comment move would be greatly reduced and make it
> easier to review the rest of this.

Agree.

> 
> > @@ -1082,10 +1044,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >
> >         write = madvise_need_mmap_write(behavior);
> >         if (write) {
> > -               if (down_write_killable(&current->mm->mmap_sem))
> > +               if (down_write_killable(&mm->mmap_sem))
> >                         return -EINTR;
> >         } else {
> > -               down_read(&current->mm->mmap_sem);
> > +               down_read(&mm->mmap_sem);
> >         }
> >
> >         /*
> > @@ -1093,7 +1055,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >          * ranges, just ignore them, but return -ENOMEM at the end.
> >          * - different from the way of handling in mlock etc.
> >          */
> > -       vma = find_vma_prev(current->mm, start, &prev);
> > +       vma = find_vma_prev(mm, start, &prev);
> >         if (vma && start > vma->vm_start)
> >                 prev = vma;
> >
> > @@ -1118,7 +1080,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >                         tmp = end;
> >
> >                 /* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> > -               error = madvise_vma(vma, &prev, start, tmp, behavior);
> > +               error = madvise_vma(task, mm, vma, &prev, start, tmp, behavior);
> >                 if (error)
> >                         goto out;
> >                 start = tmp;
> > @@ -1130,14 +1092,80 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >                 if (prev)
> >                         vma = prev->vm_next;
> >                 else    /* madvise_remove dropped mmap_sem */
> > -                       vma = find_vma(current->mm, start);
> > +                       vma = find_vma(mm, start);
> >         }
> >  out:
> >         blk_finish_plug(&plug);
> >         if (write)
> > -               up_write(&current->mm->mmap_sem);
> > +               up_write(&mm->mmap_sem);
> >         else
> > -               up_read(&current->mm->mmap_sem);
> > +               up_read(&mm->mmap_sem);
> >
> >         return error;
> >  }
> 
> Okay so the rest of this is just more of the task/mm being added as
> separate arguments.
> 

Thanks for the review!

[PATCH v3 2/5] mm: introduce external memory hinting API

[Minchan Kim]

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.

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.

To solve the issue, this patch introduces a new syscall process_madvise(2).
It uses pidfd of an external process to give the hint.

 int process_madvise(int pidfd, void *addr, size_t length, int advise,
			unsigned long flag);

Since it could affect other process's address range, only privileged
process(CAP_SYS_PTRACE) 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 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.

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 that the SystemServer
is looping to monitor each process, it does tend to *know* which address
range of the application is not used / useful.

Besides, we can never rely on applications to clean things up themselves.
We've had the "hey app1, the system is low on memory, please trim your
memory usage down" notifications for a long time[1]. They rely on
applications honoring the broadcasts and very few do.

So, if we want to avoid the inevitable killing of the application and
restarting it, some way to be able to tell the OS about unimportant memory in
these applications will be useful.

- ssp

Q.2 - How to guarantee the race(i.e., object validation) between when giving a
hint from an external process and get the hint from the target process?

process_madvise operates on the target process's address space as it exists
at the instant that process_madvise is called. If the space target process
can run between the time the process_madvise process inspects the target
process address space and the time that process_madvise is actually called,
process_madvise may operate on memory regions that the calling process does
not expect. It's the responsibility of the process calling process_madvise
to close this race condition. For example, the calling process can suspend
the target process with ptrace, SIGSTOP, or the freezer cgroup so that it
doesn't have an opportunity to change its own address space before
process_madvise is called. Another option is to operate on memory regions
that the caller knows a priori will be unchanged in the target process.
Yet another option is to accept the race for certain process_madvise calls
after reasoning that mistargeting will do no harm. The suggested API itself
does not provide synchronization. It also apply other APIs like move_pages,
process_vm_write.

The race isn't really a problem though. Why is it so wrong to require
that callers do their own synchronization in some manner? Nobody objects
to write(2) merely because it's possible for two processes to open the same
file and clobber each other's writes --- instead, we tell people to use
flock or something. Think about mmap. It never guarantees newly allocated
address space is still valid when the user tries to access it because other
threads could unmap the memory right before. That's where we need
synchronization by using other API or design from userside. It shouldn't
be part of API itself. If someone needs more fine-grained synchronization
rather than process level, there were two ideas suggested - cookie[2] and
anon-fd[3]. Both are applicable via using last reserved argument of the API
but I don't think it's necessary right now since we have already ways to
prevent the race so don't want to add additional complexity with more
fine-grained optimization model.

To make the API extend, it reserved an unsigned long as last argument
so we could support it in future if someone really needs it.

Q.3 - Why doesn't ptrace work?

Injecting an madvise in the target process using ptrace would not work for us
because such injected madvise would have to be executed by the target process,
which means that process would have to be runnable and that creates the risk
of the abovementioned race and hinting a wrong VMA. Furthermore, we want to
act the hint in caller's context, not calle because calle is usually limited
in cpuset/cgroups or even freezed state so they can't act by themselves
quick enough, which causes more thrashing/kill. It doesn't work if the
target process are ptraced(e.g., strace, debugger, minidump) because  a
process can have at most one ptracer.

[1] https://developer.android.com/topic/performance/memory"
[2] process_getinfo for getting the cookie which is updated whenever
    vma of process address layout are changed - Daniel Colascione
- https://lore.kernel.org/lkml/20190520035254.57579-1-minchan@kernel.org/T/#m7694416fd179b2066a2c62b5b139b14e3894e224
[3] anonymous fd which is used for the object(i.e., address range)
    validation - Michal Hocko
- https://lore.kernel.org/lkml/20200120112722.GY18451@dhcp22.suse.cz/

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
 arch/arm/tools/syscall.tbl                  |  1 +
 arch/arm64/include/asm/unistd.h             |  2 +-
 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/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 +
 include/linux/syscalls.h                    |  2 +
 include/uapi/asm-generic/unistd.h           |  5 +-
 kernel/sys_ni.c                             |  1 +
 mm/madvise.c                                | 66 +++++++++++++++++++++
 21 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index e56950f23b49..776c61803315 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -477,3 +477,4 @@
 # 545 reserved for clone3
 546	common	watch_devices			sys_watch_devices
 547	common	openat2				sys_openat2
+548	common	process_madvise			sys_process_madvise
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 7fb2f4d59210..a43381542276 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -451,3 +451,4 @@
 435	common	clone3				sys_clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 8aa00ccb0b96..b722e47377a5 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
 #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
 #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
 
-#define __NR_compat_syscalls		438
+#define __NR_compat_syscalls		439
 #endif
 
 #define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index 31f0ce25719e..e3643d7fecc3 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
 __SYSCALL(__NR_watch_devices, sys_watch_devices)
 #define __NR_openat2 437
 __SYSCALL(__NR_openat2, sys_openat2)
+#define __NR_process_madvise 438
+__SYSCALL(__NR_process_madvise, process_madvise)
 
 /*
  * 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 b9aa59931905..c156abc9a298 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -358,3 +358,4 @@
 # 435 reserved for clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 868c1ef89d35..5b6034b6650f 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -437,3 +437,4 @@
 # 435 reserved for clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 544b4cef18b3..4bef584af09c 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -443,3 +443,4 @@
 435	common	clone3				sys_clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 05e8aee5dae7..7061b2103438 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -376,3 +376,4 @@
 435	n32	clone3				__sys_clone3
 436	n32	watch_devices			sys_watch_devices
 437	n32	openat2				sys_openat2
+438	n32	process_madivse			sys_process_madvise
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 24d6c01328fb..84042d57fbfb 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -352,3 +352,4 @@
 435	n64	clone3				__sys_clone3
 436	n64	watch_devices			sys_watch_devices
 437	n64	openat2				sys_openat2
+438	n64	process_madvise			sys_process_madvise
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 4b5f77a4e1a2..5bfd359c7e6f 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -435,3 +435,4 @@
 435	common	clone3				sys_clone3_wrapper
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 9716dc85a517..ffa0e679aca0 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -519,3 +519,4 @@
 435	nospu	clone3				ppc_clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index 7da330f8b03e..c301717216ca 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -440,3 +440,4 @@
 435  common	clone3			sys_clone3			sys_clone3
 436  common	watch_devices		sys_watch_devices		sys_watch_devices
 437  common	openat2			sys_openat2			sys_openat2
+438  common	process_madvise		sys_process_madvise		sys_process_madvise
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index bb7e68e25337..b8f15701f69f 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -440,3 +440,4 @@
 # 435 reserved for clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 646a1fad7218..7ea95f37b222 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -483,3 +483,4 @@
 # 435 reserved for clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2			sys_openat2
+438	common	process_madvise		sys_process_madvise
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 57c53acee290..76a2c266fe7e 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -442,3 +442,4 @@
 435	i386	clone3			sys_clone3			__ia32_sys_clone3
 436	i386	watch_devices		sys_watch_devices		__ia32_sys_watch_devices
 437	i386	openat2			sys_openat2			__ia32_sys_openat2
+438	i386	process_madvise		sys_process_madvise		__ia32_sys_process_madvise
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 1dd8d21f6500..b697cd8620cb 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -359,6 +359,7 @@
 435	common	clone3			__x64_sys_clone3/ptregs
 436	common	watch_devices		__x64_sys_watch_devices
 437	common	openat2			__x64_sys_openat2
+438	common	process_madvise		__x64_sys_process_madvise
 
 #
 # 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 0f48ab7bd75b..2e9813ecfd7d 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -408,3 +408,4 @@
 435	common	clone3				sys_clone3
 436	common	watch_devices			sys_watch_devices
 437	common	openat2				sys_openat2
+438	common	process_madvise			sys_process_madvise
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 433c8c85636e..1b58a11ff49f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -877,6 +877,8 @@ asmlinkage long sys_munlockall(void);
 asmlinkage long sys_mincore(unsigned long start, size_t len,
 				unsigned char __user * vec);
 asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
+asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
+			size_t len, int behavior, unsigned long flags);
 asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
 			unsigned long prot, unsigned long pgoff,
 			unsigned long flags);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 33f3856a9c3c..4a49fbaea013 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -856,8 +856,11 @@ __SYSCALL(__NR_watch_devices, sys_watch_devices)
 #define __NR_openat2 437
 __SYSCALL(__NR_openat2, sys_openat2)
 
+#define __NR_process_madvise 438
+__SYSCALL(__NR_process_madvise, sys_process_madvise)
+
 #undef __NR_syscalls
-#define __NR_syscalls 438
+#define __NR_syscalls 439
 
 /*
  * 32 bit systems traditionally used different
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 0e9b275260f8..10ce5eac8b4b 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -281,6 +281,7 @@ COND_SYSCALL(mlockall);
 COND_SYSCALL(munlockall);
 COND_SYSCALL(mincore);
 COND_SYSCALL(madvise);
+COND_SYSCALL(process_madvise);
 COND_SYSCALL(remap_file_pages);
 COND_SYSCALL(mbind);
 COND_SYSCALL_COMPAT(mbind);
diff --git a/mm/madvise.c b/mm/madvise.c
index 0c901de531e4..00ffa7e92f79 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>
@@ -315,6 +316,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
 
 	if (fatal_signal_pending(task))
 		return -EINTR;
+	else if (current != task && fatal_signal_pending(current))
+		return -EINTR;
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	if (pmd_trans_huge(*pmd)) {
@@ -993,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;
+	}
+}
+
 /*
  * madvise_common - request behavior hint to address range of the target process
  *
@@ -1151,6 +1166,11 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
  *  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 uses the memory less so the kernel can
+ *		deactivate the memory to evict them quickly when the memory
+ *		pressure happen.
+ *  MADV_PAGEOUT - the application uses the memroy very rarely so kernel can
+ *		page out the memory instantly.
  *
  * return values:
  *  zero    - success
@@ -1169,3 +1189,49 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 {
 	return madvise_common(current, current->mm, start, len_in, behavior);
 }
+
+SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
+		size_t, len_in, int, behavior, unsigned long, flags)
+{
+	int ret;
+	struct fd f;
+	struct pid *pid;
+	struct task_struct *task;
+	struct mm_struct *mm;
+
+	if (flags != 0)
+		return -EINVAL;
+
+	if (!process_madvise_behavior_valid(behavior))
+		return -EINVAL;
+
+	f = fdget(pidfd);
+	if (!f.file)
+		return -EBADF;
+
+	pid = pidfd_pid(f.file);
+	if (IS_ERR(pid)) {
+		ret = PTR_ERR(pid);
+		goto fdput;
+	}
+
+	task = get_pid_task(pid, PIDTYPE_PID);
+	if (!task) {
+		ret = -ESRCH;
+		goto fdput;
+	}
+
+	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;
+	}
+
+	ret = madvise_common(task, mm, start, len_in, behavior);
+	mmput(mm);
+release_task:
+	put_task_struct(task);
+fdput:
+	fdput(f);
+	return ret;
+}
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v3 2/5] mm: introduce external memory hinting API

[Suren Baghdasaryan]

On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> 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.
>
> 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.
>
> To solve the issue, this patch introduces a new syscall process_madvise(2).
> It uses pidfd of an external process to give the hint.
>
>  int process_madvise(int pidfd, void *addr, size_t length, int advise,
>                         unsigned long flag);
>
> Since it could affect other process's address range, only privileged
> process(CAP_SYS_PTRACE) 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 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.

I would definitely be interested in adding MADV_DONTNEED support for
process_madvise() to allow quick memory reclaim after a kill. The
scenario is that userspace daemon can kill a process and try to help
reclaim its memory. Having process_madvise(MADV_DONTNEED) support
helps in the following cases:
1. Process issuing process_madvise has a higher CPU bandwidth
allowance than the victim process, therefore can reclaim victim's
memory quicker.
2. In case the victim occupies large amounts of memory the process
issuing process_madvise can spawn multiple (possibly high priority)
threads each reclaiming portions of the victim's memory.
Such an extension will add a destructive kind of madvise into the set
supported by process_madvise and I want to make sure we can accomodate
for that in the future. Do you see any issues with supporting
MADV_DONTNEED in the future?

>
> 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 that the SystemServer
> is looping to monitor each process, it does tend to *know* which address
> range of the application is not used / useful.
>
> Besides, we can never rely on applications to clean things up themselves.
> We've had the "hey app1, the system is low on memory, please trim your
> memory usage down" notifications for a long time[1]. They rely on
> applications honoring the broadcasts and very few do.
>
> So, if we want to avoid the inevitable killing of the application and
> restarting it, some way to be able to tell the OS about unimportant memory in
> these applications will be useful.
>
> - ssp
>
> Q.2 - How to guarantee the race(i.e., object validation) between when giving a
> hint from an external process and get the hint from the target process?
>
> process_madvise operates on the target process's address space as it exists
> at the instant that process_madvise is called. If the space target process
> can run between the time the process_madvise process inspects the target
> process address space and the time that process_madvise is actually called,
> process_madvise may operate on memory regions that the calling process does
> not expect. It's the responsibility of the process calling process_madvise
> to close this race condition. For example, the calling process can suspend
> the target process with ptrace, SIGSTOP, or the freezer cgroup so that it
> doesn't have an opportunity to change its own address space before
> process_madvise is called. Another option is to operate on memory regions
> that the caller knows a priori will be unchanged in the target process.
> Yet another option is to accept the race for certain process_madvise calls
> after reasoning that mistargeting will do no harm. The suggested API itself
> does not provide synchronization. It also apply other APIs like move_pages,
> process_vm_write.
>
> The race isn't really a problem though. Why is it so wrong to require
> that callers do their own synchronization in some manner? Nobody objects
> to write(2) merely because it's possible for two processes to open the same
> file and clobber each other's writes --- instead, we tell people to use
> flock or something. Think about mmap. It never guarantees newly allocated
> address space is still valid when the user tries to access it because other
> threads could unmap the memory right before. That's where we need
> synchronization by using other API or design from userside. It shouldn't
> be part of API itself. If someone needs more fine-grained synchronization
> rather than process level, there were two ideas suggested - cookie[2] and
> anon-fd[3]. Both are applicable via using last reserved argument of the API
> but I don't think it's necessary right now since we have already ways to
> prevent the race so don't want to add additional complexity with more
> fine-grained optimization model.
>
> To make the API extend, it reserved an unsigned long as last argument
> so we could support it in future if someone really needs it.
>
> Q.3 - Why doesn't ptrace work?
>
> Injecting an madvise in the target process using ptrace would not work for us
> because such injected madvise would have to be executed by the target process,
> which means that process would have to be runnable and that creates the risk
> of the abovementioned race and hinting a wrong VMA. Furthermore, we want to
> act the hint in caller's context, not calle because calle is usually limited
> in cpuset/cgroups or even freezed state so they can't act by themselves
> quick enough, which causes more thrashing/kill. It doesn't work if the
> target process are ptraced(e.g., strace, debugger, minidump) because  a
> process can have at most one ptracer.
>
> [1] https://developer.android.com/topic/performance/memory"
> [2] process_getinfo for getting the cookie which is updated whenever
>     vma of process address layout are changed - Daniel Colascione
> - https://lore.kernel.org/lkml/20190520035254.57579-1-minchan@kernel.org/T/#m7694416fd179b2066a2c62b5b139b14e3894e224
> [3] anonymous fd which is used for the object(i.e., address range)
>     validation - Michal Hocko
> - https://lore.kernel.org/lkml/20200120112722.GY18451@dhcp22.suse.cz/
>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
>  arch/arm/tools/syscall.tbl                  |  1 +
>  arch/arm64/include/asm/unistd.h             |  2 +-
>  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/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 +
>  include/linux/syscalls.h                    |  2 +
>  include/uapi/asm-generic/unistd.h           |  5 +-
>  kernel/sys_ni.c                             |  1 +
>  mm/madvise.c                                | 66 +++++++++++++++++++++
>  21 files changed, 91 insertions(+), 2 deletions(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index e56950f23b49..776c61803315 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -477,3 +477,4 @@
>  # 545 reserved for clone3
>  546    common  watch_devices                   sys_watch_devices
>  547    common  openat2                         sys_openat2
> +548    common  process_madvise                 sys_process_madvise
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 7fb2f4d59210..a43381542276 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -451,3 +451,4 @@
>  435    common  clone3                          sys_clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> index 8aa00ccb0b96..b722e47377a5 100644
> --- a/arch/arm64/include/asm/unistd.h
> +++ b/arch/arm64/include/asm/unistd.h
> @@ -38,7 +38,7 @@
>  #define __ARM_NR_compat_set_tls                (__ARM_NR_COMPAT_BASE + 5)
>  #define __ARM_NR_COMPAT_END            (__ARM_NR_COMPAT_BASE + 0x800)
>
> -#define __NR_compat_syscalls           438
> +#define __NR_compat_syscalls           439
>  #endif
>
>  #define __ARCH_WANT_SYS_CLONE
> diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> index 31f0ce25719e..e3643d7fecc3 100644
> --- a/arch/arm64/include/asm/unistd32.h
> +++ b/arch/arm64/include/asm/unistd32.h
> @@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
>  __SYSCALL(__NR_watch_devices, sys_watch_devices)
>  #define __NR_openat2 437
>  __SYSCALL(__NR_openat2, sys_openat2)
> +#define __NR_process_madvise 438
> +__SYSCALL(__NR_process_madvise, process_madvise)
>
>  /*
>   * 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 b9aa59931905..c156abc9a298 100644
> --- a/arch/ia64/kernel/syscalls/syscall.tbl
> +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> @@ -358,3 +358,4 @@
>  # 435 reserved for clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index 868c1ef89d35..5b6034b6650f 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -437,3 +437,4 @@
>  # 435 reserved for clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 544b4cef18b3..4bef584af09c 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -443,3 +443,4 @@
>  435    common  clone3                          sys_clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index 05e8aee5dae7..7061b2103438 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -376,3 +376,4 @@
>  435    n32     clone3                          __sys_clone3
>  436    n32     watch_devices                   sys_watch_devices
>  437    n32     openat2                         sys_openat2
> +438    n32     process_madivse                 sys_process_madvise
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index 24d6c01328fb..84042d57fbfb 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -352,3 +352,4 @@
>  435    n64     clone3                          __sys_clone3
>  436    n64     watch_devices                   sys_watch_devices
>  437    n64     openat2                         sys_openat2
> +438    n64     process_madvise                 sys_process_madvise
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 4b5f77a4e1a2..5bfd359c7e6f 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -435,3 +435,4 @@
>  435    common  clone3                          sys_clone3_wrapper
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 9716dc85a517..ffa0e679aca0 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -519,3 +519,4 @@
>  435    nospu   clone3                          ppc_clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index 7da330f8b03e..c301717216ca 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -440,3 +440,4 @@
>  435  common    clone3                  sys_clone3                      sys_clone3
>  436  common    watch_devices           sys_watch_devices               sys_watch_devices
>  437  common    openat2                 sys_openat2                     sys_openat2
> +438  common    process_madvise         sys_process_madvise             sys_process_madvise
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index bb7e68e25337..b8f15701f69f 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -440,3 +440,4 @@
>  # 435 reserved for clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 646a1fad7218..7ea95f37b222 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -483,3 +483,4 @@
>  # 435 reserved for clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                 sys_openat2
> +438    common  process_madvise         sys_process_madvise
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 57c53acee290..76a2c266fe7e 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -442,3 +442,4 @@
>  435    i386    clone3                  sys_clone3                      __ia32_sys_clone3
>  436    i386    watch_devices           sys_watch_devices               __ia32_sys_watch_devices
>  437    i386    openat2                 sys_openat2                     __ia32_sys_openat2
> +438    i386    process_madvise         sys_process_madvise             __ia32_sys_process_madvise
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 1dd8d21f6500..b697cd8620cb 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -359,6 +359,7 @@
>  435    common  clone3                  __x64_sys_clone3/ptregs
>  436    common  watch_devices           __x64_sys_watch_devices
>  437    common  openat2                 __x64_sys_openat2
> +438    common  process_madvise         __x64_sys_process_madvise
>
>  #
>  # 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 0f48ab7bd75b..2e9813ecfd7d 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -408,3 +408,4 @@
>  435    common  clone3                          sys_clone3
>  436    common  watch_devices                   sys_watch_devices
>  437    common  openat2                         sys_openat2
> +438    common  process_madvise                 sys_process_madvise
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 433c8c85636e..1b58a11ff49f 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -877,6 +877,8 @@ asmlinkage long sys_munlockall(void);
>  asmlinkage long sys_mincore(unsigned long start, size_t len,
>                                 unsigned char __user * vec);
>  asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
> +asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
> +                       size_t len, int behavior, unsigned long flags);
>  asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
>                         unsigned long prot, unsigned long pgoff,
>                         unsigned long flags);
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 33f3856a9c3c..4a49fbaea013 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -856,8 +856,11 @@ __SYSCALL(__NR_watch_devices, sys_watch_devices)
>  #define __NR_openat2 437
>  __SYSCALL(__NR_openat2, sys_openat2)
>
> +#define __NR_process_madvise 438
> +__SYSCALL(__NR_process_madvise, sys_process_madvise)
> +
>  #undef __NR_syscalls
> -#define __NR_syscalls 438
> +#define __NR_syscalls 439
>
>  /*
>   * 32 bit systems traditionally used different
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index 0e9b275260f8..10ce5eac8b4b 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -281,6 +281,7 @@ COND_SYSCALL(mlockall);
>  COND_SYSCALL(munlockall);
>  COND_SYSCALL(mincore);
>  COND_SYSCALL(madvise);
> +COND_SYSCALL(process_madvise);
>  COND_SYSCALL(remap_file_pages);
>  COND_SYSCALL(mbind);
>  COND_SYSCALL_COMPAT(mbind);
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 0c901de531e4..00ffa7e92f79 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>
> @@ -315,6 +316,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>
>         if (fatal_signal_pending(task))
>                 return -EINTR;
> +       else if (current != task && fatal_signal_pending(current))
> +               return -EINTR;

I think this can be simplified as:

+       if (fatal_signal_pending(current))
+               return -EINTR;

current != task condition is not needed because if current == task
then you would return earlier after checking
fatal_signal_pending(task).


>
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>         if (pmd_trans_huge(*pmd)) {
> @@ -993,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;
> +       }
> +}
> +
>  /*
>   * madvise_common - request behavior hint to address range of the target process
>   *
> @@ -1151,6 +1166,11 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
>   *  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 uses the memory less so the kernel can
> + *             deactivate the memory to evict them quickly when the memory
> + *             pressure happen.
> + *  MADV_PAGEOUT - the application uses the memroy very rarely so kernel can
> + *             page out the memory instantly.
>   *
>   * return values:
>   *  zero    - success
> @@ -1169,3 +1189,49 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>  {
>         return madvise_common(current, current->mm, start, len_in, behavior);
>  }
> +
> +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> +               size_t, len_in, int, behavior, unsigned long, flags)
> +{
> +       int ret;
> +       struct fd f;
> +       struct pid *pid;
> +       struct task_struct *task;
> +       struct mm_struct *mm;
> +
> +       if (flags != 0)
> +               return -EINVAL;
> +
> +       if (!process_madvise_behavior_valid(behavior))
> +               return -EINVAL;
> +
> +       f = fdget(pidfd);
> +       if (!f.file)
> +               return -EBADF;
> +
> +       pid = pidfd_pid(f.file);
> +       if (IS_ERR(pid)) {
> +               ret = PTR_ERR(pid);
> +               goto fdput;
> +       }
> +
> +       task = get_pid_task(pid, PIDTYPE_PID);
> +       if (!task) {
> +               ret = -ESRCH;
> +               goto fdput;
> +       }
> +
> +       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;
> +       }
> +
> +       ret = madvise_common(task, mm, start, len_in, behavior);
> +       mmput(mm);
> +release_task:
> +       put_task_struct(task);
> +fdput:
> +       fdput(f);
> +       return ret;
> +}
> --
> 2.25.0.341.g760bfbb309-goog
>

Re: [PATCH v3 2/5] mm: introduce external memory hinting API

[Minchan Kim]

Hi Suren,

On Mon, Feb 10, 2020 at 09:50:20AM -0800, Suren Baghdasaryan wrote:
> On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> 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.
> >
> > 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.
> >
> > To solve the issue, this patch introduces a new syscall process_madvise(2).
> > It uses pidfd of an external process to give the hint.
> >
> >  int process_madvise(int pidfd, void *addr, size_t length, int advise,
> >                         unsigned long flag);
> >
> > Since it could affect other process's address range, only privileged
> > process(CAP_SYS_PTRACE) 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 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.
> 
> I would definitely be interested in adding MADV_DONTNEED support for
> process_madvise() to allow quick memory reclaim after a kill. The
> scenario is that userspace daemon can kill a process and try to help
> reclaim its memory. Having process_madvise(MADV_DONTNEED) support
> helps in the following cases:
> 1. Process issuing process_madvise has a higher CPU bandwidth
> allowance than the victim process, therefore can reclaim victim's
> memory quicker.
> 2. In case the victim occupies large amounts of memory the process
> issuing process_madvise can spawn multiple (possibly high priority)
> threads each reclaiming portions of the victim's memory.
> Such an extension will add a destructive kind of madvise into the set
> supported by process_madvise and I want to make sure we can accomodate
> for that in the future. Do you see any issues with supporting
> MADV_DONTNEED in the future?

Or kernel could do by themselves to spawn mulitple tasks if the system has
available badwidth and target process has a lot memory to be reclaimed

Anyway, it doesn't have any issue because we already have some synchrnoization
methods(e.g., signal or cgroup freezer) to freeze target processes before
giving a hint. It's not different with usual write syscall on shared file
among processes.

< snip >

> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 0c901de531e4..00ffa7e92f79 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>
> > @@ -315,6 +316,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >
> >         if (fatal_signal_pending(task))
> >                 return -EINTR;
> > +       else if (current != task && fatal_signal_pending(current))
> > +               return -EINTR;
> 
> I think this can be simplified as:
> 
> +       if (fatal_signal_pending(current))
> +               return -EINTR;
> 
> current != task condition is not needed because if current == task
> then you would return earlier after checking
> fatal_signal_pending(task).

True, I will remove it.

Thanks!

[PATCH v3 3/5] mm/madvise: employ mmget_still_valid for write lock

[Minchan Kim]

From: Oleksandr Natalenko <oleksandr@redhat.com>

Do the very same trick as we already do since 04f5866e41fb. KSM hints
will require locking mmap_sem for write since they modify vm_flags, so
for remote KSM hinting this additional check is needed.

Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/madvise.c b/mm/madvise.c
index 00ffa7e92f79..bc0a72dbf80e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1061,6 +1061,8 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
 	if (write) {
 		if (down_write_killable(&mm->mmap_sem))
 			return -EINTR;
+		if (current->mm != mm && !mmget_still_valid(mm))
+			goto skip_mm;
 	} else {
 		down_read(&mm->mmap_sem);
 	}
@@ -1111,6 +1113,7 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
 	}
 out:
 	blk_finish_plug(&plug);
+skip_mm:
 	if (write)
 		up_write(&mm->mmap_sem);
 	else
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v3 4/5] mm/madvise: allow KSM hints for remote API

[Minchan Kim]

From: Oleksandr Natalenko <oleksandr@redhat.com>

It all began with the fact that KSM works only on memory that is marked
by madvise(). And the only way to get around that is to either:

  * use LD_PRELOAD; or
  * patch the kernel with something like UKSM or PKSM.

(i skip ptrace can of worms here intentionally)

To overcome this restriction, lets employ a new remote madvise API. This
can be used by some small userspace helper daemon that will do auto-KSM
job for us.

I think of two major consumers of remote KSM hints:

  * hosts, that run containers, especially similar ones and especially in
    a trusted environment, sharing the same runtime like Node.js;

  * heavy applications, that can be run in multiple instances, not
    limited to opensource ones like Firefox, but also those that cannot be
    modified since they are binary-only and, maybe, statically linked.

Speaking of statistics, more numbers can be found in the very first
submission, that is related to this one [1]. For my current setup with
two Firefox instances I get 100 to 200 MiB saved for the second instance
depending on the amount of tabs.

1 FF instance with 15 tabs:

   $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
   410

2 FF instances, second one has 12 tabs (all the tabs are different):

   $ echo "$(cat /sys/kernel/mm/ksm/pages_sharing) * 4 / 1024" | bc
   592

At the very moment I do not have specific numbers for containerised
workload, but those should be comparable in case the containers share
similar/same runtime.

[1] https://lore.kernel.org/patchwork/patch/1012142/

Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/madvise.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/madvise.c b/mm/madvise.c
index bc0a72dbf80e..39c40cbb389e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1002,6 +1002,10 @@ process_madvise_behavior_valid(int behavior)
 	switch (behavior) {
 	case MADV_COLD:
 	case MADV_PAGEOUT:
+#ifdef CONFIG_KSM
+	case MADV_MERGEABLE:
+	case MADV_UNMERGEABLE:
+#endif
 		return true;
 	default:
 		return false;
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v3 5/5] mm: support both pid and pidfd for process_madvise

[Minchan Kim]

There is a demand[1] to support pid as well pidfd for process_madvise
to reduce unncessary syscall to get pidfd if the user has control of
the targer process(ie, they could gaurantee the process is not gone
or pid is not reused. Or, it might be okay to give a hint to wrong
process).

This patch aims for supporting both options like waitid(2). So, the
syscall is currently,

	int process_madvise(int which, pid_t pid, void *addr,
		size_t length, int advise, unsigned long flag);

@which is actually idtype_t for userspace libray and currently,
it supports P_PID and P_PIDFD.

[1]  https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/linux/pid.h      |  1 +
 include/linux/syscalls.h |  3 ++-
 kernel/exit.c            | 17 -----------------
 kernel/pid.c             | 17 +++++++++++++++++
 mm/madvise.c             | 34 ++++++++++++++++++++++------------
 5 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/include/linux/pid.h b/include/linux/pid.h
index 998ae7d24450..023d9c3a8edc 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
 struct file;
 
 extern struct pid *pidfd_pid(const struct file *file);
+extern struct pid *pidfd_get_pid(unsigned int fd);
 
 static inline struct pid *get_pid(struct pid *pid)
 {
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 1b58a11ff49f..27060e59db37 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -877,7 +877,8 @@ asmlinkage long sys_munlockall(void);
 asmlinkage long sys_mincore(unsigned long start, size_t len,
 				unsigned char __user * vec);
 asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
-asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
+
+asmlinkage long sys_process_madvise(int which, pid_t pid, unsigned long start,
 			size_t len, int behavior, unsigned long flags);
 asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
 			unsigned long prot, unsigned long pgoff,
diff --git a/kernel/exit.c b/kernel/exit.c
index bcbd59888e67..7698843b1411 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1466,23 +1466,6 @@ static long do_wait(struct wait_opts *wo)
 	return retval;
 }
 
-static struct pid *pidfd_get_pid(unsigned int fd)
-{
-	struct fd f;
-	struct pid *pid;
-
-	f = fdget(fd);
-	if (!f.file)
-		return ERR_PTR(-EBADF);
-
-	pid = pidfd_pid(f.file);
-	if (!IS_ERR(pid))
-		get_pid(pid);
-
-	fdput(f);
-	return pid;
-}
-
 static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
 			  int options, struct rusage *ru)
 {
diff --git a/kernel/pid.c b/kernel/pid.c
index 2278e249141d..a41a89d5dad2 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -496,6 +496,23 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
 	return idr_get_next(&ns->idr, &nr);
 }
 
+struct pid *pidfd_get_pid(unsigned int fd)
+{
+	struct fd f;
+	struct pid *pid;
+
+	f = fdget(fd);
+	if (!f.file)
+		return ERR_PTR(-EBADF);
+
+	pid = pidfd_pid(f.file);
+	if (!IS_ERR(pid))
+		get_pid(pid);
+
+	fdput(f);
+	return pid;
+}
+
 /**
  * pidfd_create() - Create a new pid file descriptor.
  *
diff --git a/mm/madvise.c b/mm/madvise.c
index 39c40cbb389e..ba3a9bd8ea27 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1197,11 +1197,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
 	return madvise_common(current, current->mm, start, len_in, behavior);
 }
 
-SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
+SYSCALL_DEFINE6(process_madvise, int, which, pid_t, upid, unsigned long, start,
 		size_t, len_in, int, behavior, unsigned long, flags)
 {
 	int ret;
-	struct fd f;
 	struct pid *pid;
 	struct task_struct *task;
 	struct mm_struct *mm;
@@ -1212,20 +1211,31 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
 	if (!process_madvise_behavior_valid(behavior))
 		return -EINVAL;
 
-	f = fdget(pidfd);
-	if (!f.file)
-		return -EBADF;
+	switch (which) {
+	case P_PID:
+		if (upid <= 0)
+			return -EINVAL;
+
+		pid = find_get_pid(upid);
+		if (!pid)
+			return -ESRCH;
+		break;
+	case P_PIDFD:
+		if (upid < 0)
+			return -EINVAL;
 
-	pid = pidfd_pid(f.file);
-	if (IS_ERR(pid)) {
-		ret = PTR_ERR(pid);
-		goto fdput;
+		pid = pidfd_get_pid(upid);
+		if (IS_ERR(pid))
+			return PTR_ERR(pid);
+		break;
+	default:
+		return -EINVAL;
 	}
 
 	task = get_pid_task(pid, PIDTYPE_PID);
 	if (!task) {
 		ret = -ESRCH;
-		goto fdput;
+		goto put_pid;
 	}
 
 	mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
@@ -1238,7 +1248,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
 	mmput(mm);
 release_task:
 	put_task_struct(task);
-fdput:
-	fdput(f);
+put_pid:
+	put_pid(pid);
 	return ret;
 }
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v3 5/5] mm: support both pid and pidfd for process_madvise

[Alexander Duyck]

On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> wrote:
>
> There is a demand[1] to support pid as well pidfd for process_madvise
> to reduce unncessary syscall to get pidfd if the user has control of
> the targer process(ie, they could gaurantee the process is not gone
> or pid is not reused. Or, it might be okay to give a hint to wrong
> process).

It looks like you misspelled several items in here including
"unnecessary", "target", and "guarantee".

> This patch aims for supporting both options like waitid(2). So, the
> syscall is currently,
>
>         int process_madvise(int which, pid_t pid, void *addr,
>                 size_t length, int advise, unsigned long flag);
>
> @which is actually idtype_t for userspace libray and currently,
> it supports P_PID and P_PIDFD.
>
> [1]  https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  include/linux/pid.h      |  1 +
>  include/linux/syscalls.h |  3 ++-
>  kernel/exit.c            | 17 -----------------
>  kernel/pid.c             | 17 +++++++++++++++++
>  mm/madvise.c             | 34 ++++++++++++++++++++++------------
>  5 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index 998ae7d24450..023d9c3a8edc 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
>  struct file;
>
>  extern struct pid *pidfd_pid(const struct file *file);
> +extern struct pid *pidfd_get_pid(unsigned int fd);
>
>  static inline struct pid *get_pid(struct pid *pid)
>  {

So really this is two patches interleaved. You have the moving of the
pidfd_get_pid function and the update of the syscall. Personally I
would make the function move a separate patch and place it before you
define the syscall and fold the syscall changes into your original
patch.

Doing that you wouldn't have to worry about the syscall changing in
behavior midway through a bisect. It would either be there or it
wouldn't.

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 1b58a11ff49f..27060e59db37 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -877,7 +877,8 @@ asmlinkage long sys_munlockall(void);
>  asmlinkage long sys_mincore(unsigned long start, size_t len,
>                                 unsigned char __user * vec);
>  asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
> -asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
> +
> +asmlinkage long sys_process_madvise(int which, pid_t pid, unsigned long start,
>                         size_t len, int behavior, unsigned long flags);
>  asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
>                         unsigned long prot, unsigned long pgoff,
> diff --git a/kernel/exit.c b/kernel/exit.c
> index bcbd59888e67..7698843b1411 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1466,23 +1466,6 @@ static long do_wait(struct wait_opts *wo)
>         return retval;
>  }
>
> -static struct pid *pidfd_get_pid(unsigned int fd)
> -{
> -       struct fd f;
> -       struct pid *pid;
> -
> -       f = fdget(fd);
> -       if (!f.file)
> -               return ERR_PTR(-EBADF);
> -
> -       pid = pidfd_pid(f.file);
> -       if (!IS_ERR(pid))
> -               get_pid(pid);
> -
> -       fdput(f);
> -       return pid;
> -}
> -
>  static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop,
>                           int options, struct rusage *ru)
>  {
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2278e249141d..a41a89d5dad2 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -496,6 +496,23 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
>         return idr_get_next(&ns->idr, &nr);
>  }
>
> +struct pid *pidfd_get_pid(unsigned int fd)
> +{
> +       struct fd f;
> +       struct pid *pid;
> +
> +       f = fdget(fd);
> +       if (!f.file)
> +               return ERR_PTR(-EBADF);
> +
> +       pid = pidfd_pid(f.file);
> +       if (!IS_ERR(pid))
> +               get_pid(pid);
> +
> +       fdput(f);
> +       return pid;
> +}
> +
>  /**
>   * pidfd_create() - Create a new pid file descriptor.
>   *
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 39c40cbb389e..ba3a9bd8ea27 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1197,11 +1197,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>         return madvise_common(current, current->mm, start, len_in, behavior);
>  }
>
> -SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> +SYSCALL_DEFINE6(process_madvise, int, which, pid_t, upid, unsigned long, start,
>                 size_t, len_in, int, behavior, unsigned long, flags)
>  {
>         int ret;
> -       struct fd f;
>         struct pid *pid;
>         struct task_struct *task;
>         struct mm_struct *mm;
> @@ -1212,20 +1211,31 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
>         if (!process_madvise_behavior_valid(behavior))
>                 return -EINVAL;
>
> -       f = fdget(pidfd);
> -       if (!f.file)
> -               return -EBADF;
> +       switch (which) {
> +       case P_PID:
> +               if (upid <= 0)
> +                       return -EINVAL;
> +
> +               pid = find_get_pid(upid);
> +               if (!pid)
> +                       return -ESRCH;
> +               break;
> +       case P_PIDFD:
> +               if (upid < 0)
> +                       return -EINVAL;
>
> -       pid = pidfd_pid(f.file);
> -       if (IS_ERR(pid)) {
> -               ret = PTR_ERR(pid);
> -               goto fdput;
> +               pid = pidfd_get_pid(upid);
> +               if (IS_ERR(pid))
> +                       return PTR_ERR(pid);
> +               break;
> +       default:
> +               return -EINVAL;
>         }
>
>         task = get_pid_task(pid, PIDTYPE_PID);
>         if (!task) {
>                 ret = -ESRCH;
> -               goto fdput;
> +               goto put_pid;
>         }
>
>         mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> @@ -1238,7 +1248,7 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
>         mmput(mm);
>  release_task:
>         put_task_struct(task);
> -fdput:
> -       fdput(f);
> +put_pid:
> +       put_pid(pid);
>         return ret;
>  }
> --
> 2.25.0.341.g760bfbb309-goog
>
>

Re: [PATCH v3 5/5] mm: support both pid and pidfd for process_madvise

[Minchan Kim]

On Mon, Feb 10, 2020 at 03:12:52PM -0800, Alexander Duyck wrote:
> On Mon, Jan 27, 2020 at 4:17 PM Minchan Kim <minchan@kernel.org> wrote:
> >
> > There is a demand[1] to support pid as well pidfd for process_madvise
> > to reduce unncessary syscall to get pidfd if the user has control of
> > the targer process(ie, they could gaurantee the process is not gone
> > or pid is not reused. Or, it might be okay to give a hint to wrong
> > process).
> 
> It looks like you misspelled several items in here including
> "unnecessary", "target", and "guarantee".

Thanks, will fix it.

> 
> > This patch aims for supporting both options like waitid(2). So, the
> > syscall is currently,
> >
> >         int process_madvise(int which, pid_t pid, void *addr,
> >                 size_t length, int advise, unsigned long flag);
> >
> > @which is actually idtype_t for userspace libray and currently,
> > it supports P_PID and P_PIDFD.
> >
> > [1]  https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  include/linux/pid.h      |  1 +
> >  include/linux/syscalls.h |  3 ++-
> >  kernel/exit.c            | 17 -----------------
> >  kernel/pid.c             | 17 +++++++++++++++++
> >  mm/madvise.c             | 34 ++++++++++++++++++++++------------
> >  5 files changed, 42 insertions(+), 30 deletions(-)
> >
> > diff --git a/include/linux/pid.h b/include/linux/pid.h
> > index 998ae7d24450..023d9c3a8edc 100644
> > --- a/include/linux/pid.h
> > +++ b/include/linux/pid.h
> > @@ -75,6 +75,7 @@ extern const struct file_operations pidfd_fops;
> >  struct file;
> >
> >  extern struct pid *pidfd_pid(const struct file *file);
> > +extern struct pid *pidfd_get_pid(unsigned int fd);
> >
> >  static inline struct pid *get_pid(struct pid *pid)
> >  {
> 
> So really this is two patches interleaved. You have the moving of the
> pidfd_get_pid function and the update of the syscall. Personally I
> would make the function move a separate patch and place it before you
> define the syscall and fold the syscall changes into your original
> patch.
> 
> Doing that you wouldn't have to worry about the syscall changing in
> behavior midway through a bisect. It would either be there or it
> wouldn't.

Will try it.
Thanks!

Re: [PATCH v3 0/5] introduce memory hinting API for external process

[Minchan Kim]

Hey Folks,


Can I get review further since I resolved most of concerns(especially,
description stuff) I got from previous iteration?

On Mon, Jan 27, 2020 at 04:16:36PM -0800, Minchan Kim wrote:
> Now, we have MADV_PAGEOUT and MADV_COLD as madvise hinting API. With that,
> application could give hints to kernel what memory range are preferred to be
> reclaimed. However, in some platform(e.g., Android), the information
> required to make the hinting decision is not known to the app.
> Instead, it is known to a centralized userspace daemon(e.g., ActivityManagerService),
> 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 - process_madvise(2).
> Bascially, it's same with madvise(2) syscall but it has some differences.
> 
> 1. It needs pidfd of target process to provide the hint
> 2. It supports only MADV_{COLD|PAGEOUT|MERGEABLE|UNMEREABLE} at this moment.
>    Other hints in madvise will be opened when there are explicit requests from
>    community to prevent unexpected bugs we couldn't support.
> 3. Only privileged processes can do something for other process's address
>    space.
> 
> For more detail of the new API, please see "mm: introduce external memory hinting API"
> description in this patchset.
> 
> Minchan Kim (3):
>   mm: factor out madvise's core functionality
>   mm: introduce external memory hinting API
>   mm: support both pid and pidfd for process_madvise
> 
> Oleksandr Natalenko (2):
>   mm/madvise: employ mmget_still_valid for write lock
>   mm/madvise: allow KSM hints for remote API
> 
> * from v2 - https://lore.kernel.org/linux-mm/20200116235953.163318-1-minchan@kernel.org/
>   * check signal callee and caller to bail out - Kirill Tkhai
>   * put more clarification for justification of new API
> 
> * from v1 - https://lore.kernel.org/linux-mm/20200110213433.94739-1-minchan@kernel.org/
>   * fix syscall number - SeongJae
>   * use get_pid_task - Kirill Tkhai
>   * extend API to support pid as well as pidfd - Kirill Tkhai
> 
>  arch/alpha/kernel/syscalls/syscall.tbl      |   1 +
>  arch/arm/tools/syscall.tbl                  |   1 +
>  arch/arm64/include/asm/unistd.h             |   2 +-
>  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/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 +
>  include/linux/pid.h                         |   1 +
>  include/linux/syscalls.h                    |   3 +
>  include/uapi/asm-generic/unistd.h           |   5 +-
>  kernel/exit.c                               |  17 --
>  kernel/pid.c                                |  17 ++
>  kernel/sys_ni.c                             |   1 +
>  mm/madvise.c                                | 277 ++++++++++++++------
>  24 files changed, 238 insertions(+), 102 deletions(-)
> 
> -- 
> 2.25.0.341.g760bfbb309-goog
>