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. * from v6 - https://lore.kernel.org/linux-api/20200219014433.88424-1-minchan@kernel.org/ * fix comments and descriptions - Suren * Add Reviewed-by - Suren * fix build break reported by 0-day * from v5 - https://lore.kernel.org/linux-mm/20200214170520.160271-1-minchan@kernel.org/ * use null task and requestor's mm for io_madvise - Jann and Jens * use right commit description for moving pidfd_get_pid - Christoph * from v4 - https://lore.kernel.org/linux-mm/20200212233946.246210-1-minchan@kernel.org/ * pass mm down to functions, not accessing task->mm - Jann * clean up - Alexander * add Reviewed-by - Alexander, SeongJae * patch reordering * from v3 - https://lore.kernel.org/linux-mm/20200128001641.5086-1-minchan@kernel.org/ * verify task->mm aftere access_mm - Oleg * split some patches for easy review - Alexander * clean up fatal signal checking - Suren * 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 Minchan Kim (5): mm: pass task and mm to do_madvise mm: introduce external memory hinting API mm: check fatal signal pending of target process pid: move pidfd_get_pid function to pid.c 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 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 + fs/io_uring.c | 2 +- include/linux/mm.h | 3 +- include/linux/pid.h | 1 + include/linux/syscalls.h | 3 + include/uapi/asm-generic/unistd.h | 4 +- kernel/exit.c | 17 --- kernel/pid.c | 17 +++ kernel/sys_ni.c | 1 + mm/madvise.c | 144 ++++++++++++++++---- 26 files changed, 167 insertions(+), 44 deletions(-) -- 2.25.0.265.gbab2e86ba0-goog
In upcoming patches, do_madvise will be called from external process context so we shouldn't asssume "current" is always hinted process's task_struct. Furthermore, we couldn't access mm_struct via task->mm once it's verified by access_mm which will be introduced in next patch[1]. And let's pass *current* and current->mm as arguments of do_madvise so it shouldn't change existing behavior but prepare next patch to make review easy. Note: io_madvise pass NULL as target_task argument of do_madvise because it couldn't know who is target. [1] http://lore.kernel.org/r/CAG48ez27=pwm5m_N_988xT1huO7g7h6arTQL44zev6TD-h-7Tg@mail.gmail.com Cc: Jens Axboe <axboe@kernel.dk> Cc: Jann Horn <jannh@google.com> Reviewed-by: Suren Baghdasaryan <surenb@google.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- fs/io_uring.c | 2 +- include/linux/mm.h | 3 ++- mm/madvise.c | 34 +++++++++++++++++++--------------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 8866bd60783f..8b53f150560e 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2736,7 +2736,7 @@ static int io_madvise(struct io_kiocb *req, struct io_kiocb **nxt, if (force_nonblock) return -EAGAIN; - ret = do_madvise(ma->addr, ma->len, ma->advice); + ret = do_madvise(NULL, req->work.mm, ma->addr, ma->len, ma->advice); if (ret < 0) req_set_fail_links(req); io_cqring_add_event(req, ret); diff --git a/include/linux/mm.h b/include/linux/mm.h index c1180112102d..378de56d8c5b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2407,7 +2407,8 @@ extern int __do_munmap(struct mm_struct *, unsigned long, size_t, struct list_head *uf, bool downgrade); extern int do_munmap(struct mm_struct *, unsigned long, size_t, struct list_head *uf); -extern int do_madvise(unsigned long start, size_t len_in, int behavior); +extern int do_madvise(struct task_struct *target_task, struct mm_struct *mm, + unsigned long start, size_t len_in, int behavior); static inline unsigned long do_mmap_pgoff(struct file *file, unsigned long addr, diff --git a/mm/madvise.c b/mm/madvise.c index 43b47d3fae02..f75c86b6c463 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -254,6 +254,7 @@ static long madvise_willneed(struct vm_area_struct *vma, struct vm_area_struct **prev, unsigned long start, unsigned long end) { + struct mm_struct *mm = vma->vm_mm; struct file *file = vma->vm_file; loff_t offset; @@ -288,12 +289,12 @@ static long madvise_willneed(struct vm_area_struct *vma, */ *prev = NULL; /* tell sys_madvise we drop mmap_sem */ get_file(file); - up_read(¤t->mm->mmap_sem); + up_read(&mm->mmap_sem); offset = (loff_t)(start - vma->vm_start) + ((loff_t)vma->vm_pgoff << PAGE_SHIFT); vfs_fadvise(file, offset, end - start, POSIX_FADV_WILLNEED); fput(file); - down_read(¤t->mm->mmap_sem); + down_read(&mm->mmap_sem); return 0; } @@ -676,7 +677,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, if (nr_swap) { if (current->mm == mm) sync_mm_rss(mm); - add_mm_counter(mm, MM_SWAPENTS, nr_swap); } arch_leave_lazy_mmu_mode(); @@ -756,6 +756,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, unsigned long start, unsigned long end, int behavior) { + struct mm_struct *mm = vma->vm_mm; + *prev = vma; if (!can_madv_lru_vma(vma)) return -EINVAL; @@ -763,8 +765,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(¤t->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) { @@ -818,6 +820,7 @@ static long madvise_remove(struct vm_area_struct *vma, loff_t offset; int error; struct file *f; + struct mm_struct *mm = vma->vm_mm; *prev = NULL; /* tell sys_madvise we drop mmap_sem */ @@ -845,13 +848,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(¤t->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(¤t->mm->mmap_sem); + down_read(&mm->mmap_sem); return error; } @@ -1044,7 +1047,8 @@ madvise_behavior_valid(int behavior) * -EBADF - map exists, but area maps something that isn't a file. * -EAGAIN - a kernel resource was temporarily unavailable. */ -int do_madvise(unsigned long start, size_t len_in, int behavior) +int do_madvise(struct task_struct *target_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 +1086,10 @@ int do_madvise(unsigned long start, size_t len_in, int behavior) write = madvise_need_mmap_write(behavior); if (write) { - if (down_write_killable(¤t->mm->mmap_sem)) + if (down_write_killable(&mm->mmap_sem)) return -EINTR; } else { - down_read(¤t->mm->mmap_sem); + down_read(&mm->mmap_sem); } /* @@ -1093,7 +1097,7 @@ int do_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; @@ -1130,19 +1134,19 @@ int do_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(¤t->mm->mmap_sem); + up_write(&mm->mmap_sem); else - up_read(¤t->mm->mmap_sem); + up_read(&mm->mmap_sem); return error; } SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior) { - return do_madvise(start, len_in, behavior); + return do_madvise(current, current->mm, start, len_in, behavior); } -- 2.25.0.265.gbab2e86ba0-goog
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/ Reviewed-by: Suren Baghdasaryan <surenb@google.com> 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 | 4 +- kernel/sys_ni.c | 1 + mm/madvise.c | 64 +++++++++++++++++++++ 21 files changed, 88 insertions(+), 2 deletions(-) diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl index 36d42da7466a..c82952e6fb80 100644 --- a/arch/alpha/kernel/syscalls/syscall.tbl +++ b/arch/alpha/kernel/syscalls/syscall.tbl @@ -477,3 +477,4 @@ # 545 reserved for clone3 547 common openat2 sys_openat2 548 common pidfd_getfd sys_pidfd_getfd +549 common process_madvise sys_process_madvise diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl index 4d1cf74a2caa..54c2719fec46 100644 --- a/arch/arm/tools/syscall.tbl +++ b/arch/arm/tools/syscall.tbl @@ -451,3 +451,4 @@ 435 common clone3 sys_clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h index 1dd22da1c3a9..75f04a1023be 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 439 +#define __NR_compat_syscalls 440 #endif #define __ARCH_WANT_SYS_CLONE diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index c1c61635f89c..2a27be7a1f91 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_openat2, sys_openat2) #define __NR_pidfd_getfd 438 __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd) +#define __NR_process_madvise 439 +__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 042911e670b8..9524af1c318c 100644 --- a/arch/ia64/kernel/syscalls/syscall.tbl +++ b/arch/ia64/kernel/syscalls/syscall.tbl @@ -358,3 +358,4 @@ # 435 reserved for clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl index f4f49fcb76d0..8197050c097c 100644 --- a/arch/m68k/kernel/syscalls/syscall.tbl +++ b/arch/m68k/kernel/syscalls/syscall.tbl @@ -437,3 +437,4 @@ 435 common clone3 __sys_clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl index 4c67b11f9c9e..c5b6c8afe445 100644 --- a/arch/microblaze/kernel/syscalls/syscall.tbl +++ b/arch/microblaze/kernel/syscalls/syscall.tbl @@ -443,3 +443,4 @@ 435 common clone3 sys_clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 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 1f9e8ad636cc..8ec8c558aa9c 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 437 n32 openat2 sys_openat2 438 n32 pidfd_getfd sys_pidfd_getfd +439 n32 process_madvise sys_process_madvise diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl index c0b9d802dbf6..0078f891bb92 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 437 n64 openat2 sys_openat2 438 n64 pidfd_getfd sys_pidfd_getfd +439 n64 process_madvise sys_process_madvise diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl index 52a15f5cd130..09c3b5dc6855 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 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index 35b61bfc1b1a..97eac48c2937 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -519,3 +519,4 @@ 435 nospu clone3 ppc_clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl index bd7bd3581a0f..8dc8bfd958ea 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 437 common openat2 sys_openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd +439 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 c7a30fcd135f..e69d98040777 100644 --- a/arch/sh/kernel/syscalls/syscall.tbl +++ b/arch/sh/kernel/syscalls/syscall.tbl @@ -440,3 +440,4 @@ # 435 reserved for clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl index f13615ecdecc..6f6e66dd51f9 100644 --- a/arch/sparc/kernel/syscalls/syscall.tbl +++ b/arch/sparc/kernel/syscalls/syscall.tbl @@ -483,3 +483,4 @@ # 435 reserved for clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 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 c17cb77eb150..1b2184549e27 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 437 i386 openat2 sys_openat2 __ia32_sys_openat2 438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd +439 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 44d510bc9b78..82d60eb1e00d 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 437 common openat2 __x64_sys_openat2 438 common pidfd_getfd __x64_sys_pidfd_getfd +439 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 85a9ab1bc04d..165cae047770 100644 --- a/arch/xtensa/kernel/syscalls/syscall.tbl +++ b/arch/xtensa/kernel/syscalls/syscall.tbl @@ -408,3 +408,4 @@ 435 common clone3 sys_clone3 437 common openat2 sys_openat2 438 common pidfd_getfd sys_pidfd_getfd +439 common process_madvise sys_process_madvise diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index 1815065d52f3..e4cd2c2f8bb4 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -876,6 +876,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 3a3201e4618e..fa289b91410e 100644 --- a/include/uapi/asm-generic/unistd.h +++ b/include/uapi/asm-generic/unistd.h @@ -855,9 +855,11 @@ __SYSCALL(__NR_clone3, sys_clone3) __SYSCALL(__NR_openat2, sys_openat2) #define __NR_pidfd_getfd 438 __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd) +#define __NR_process_madvise 439 +__SYSCALL(__NR_process_madvise, sys_process_madvise) #undef __NR_syscalls -#define __NR_syscalls 439 +#define __NR_syscalls 440 /* * 32 bit systems traditionally used different diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c index 3b69a560a7ac..6c7332776e8e 100644 --- a/kernel/sys_ni.c +++ b/kernel/sys_ni.c @@ -280,6 +280,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 f75c86b6c463..349473fc6683 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> @@ -986,6 +987,18 @@ madvise_behavior_valid(int behavior) } } +static bool +process_madvise_behavior_valid(int behavior) +{ + switch (behavior) { + case MADV_COLD: + case MADV_PAGEOUT: + return true; + default: + return false; + } +} + /* * The madvise(2) system call. * @@ -1033,6 +1046,11 @@ madvise_behavior_valid(int behavior) * MADV_DONTDUMP - the application wants to prevent pages in the given range * from being included in its core dump. * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump. + * MADV_COLD - the application is not expected to use this memory soon, + * deactivate pages in this range so that they can be reclaimed + * easily if memory pressure hanppens. + * MADV_PAGEOUT - the application is not expected to use this memory soon, + * page out the pages in this range immediately. * * return values: * zero - success @@ -1150,3 +1168,49 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior) { return do_madvise(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 = do_madvise(task, mm, start, len_in, behavior); + mmput(mm); +release_task: + put_task_struct(task); +fdput: + fdput(f); + return ret; +} -- 2.25.0.265.gbab2e86ba0-goog
Bail out to prevent unnecessary CPU overhead if target process has pending fatal signal during (MADV_COLD|MADV_PAGEOUT) operation. Reviewed-by: Suren Baghdasaryan <surenb@google.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- mm/madvise.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/mm/madvise.c b/mm/madvise.c index 349473fc6683..6543f2bfc3d8 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -36,6 +36,7 @@ struct madvise_walk_private { struct mmu_gather *tlb; bool pageout; + struct task_struct *target_task; }; /* @@ -316,6 +317,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, if (fatal_signal_pending(current)) return -EINTR; + if (private->target_task && + fatal_signal_pending(private->target_task)) + return -EINTR; + #ifdef CONFIG_TRANSPARENT_HUGEPAGE if (pmd_trans_huge(*pmd)) { pmd_t orig_pmd; @@ -471,12 +476,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, + .target_task = task, }; tlb_start_vma(tlb, vma); @@ -484,7 +491,8 @@ 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) { @@ -497,19 +505,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, + .target_task = task, }; tlb_start_vma(tlb, vma); @@ -533,7 +543,8 @@ 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, +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) { @@ -549,7 +560,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; @@ -929,7 +940,8 @@ 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 vm_area_struct *vma, + struct vm_area_struct **prev, unsigned long start, unsigned long end, int behavior) { switch (behavior) { @@ -938,9 +950,9 @@ madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev, 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); @@ -1140,7 +1152,8 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, tmp = end; /* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */ - error = madvise_vma(vma, &prev, start, tmp, behavior); + error = madvise_vma(target_task, vma, &prev, + start, tmp, behavior); if (error) goto out; start = tmp; -- 2.25.0.265.gbab2e86ba0-goog
process_madvise syscall needs pidfd_get_pid function to translate pidfd to pid so this patch move the function to kernel/pid.c. Cc: Christoph Hellwig <hch@infradead.org> Cc: Christian Brauner <christian@brauner.io> Reviewed-by: Suren Baghdasaryan <surenb@google.com> Suggested-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- include/linux/pid.h | 1 + kernel/exit.c | 17 ----------------- kernel/pid.c | 17 +++++++++++++++++ 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/include/linux/pid.h b/include/linux/pid.h index 998ae7d24450..993f68cb45c2 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); +struct pid *pidfd_get_pid(unsigned int fd); static inline struct pid *get_pid(struct pid *pid) { diff --git a/kernel/exit.c b/kernel/exit.c index 764960fabfa1..8cad838c26d6 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1471,23 +1471,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 0f4ecb57214c..360ba480a2a9 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. * -- 2.25.0.265.gbab2e86ba0-goog
There is a demand[1] to support pid as well pidfd for process_madvise to reduce unnecessary syscall to get pidfd if the user has control of the target process(ie, they could guarantee the process is not gone or pid is not reused). 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/ Cc: Christian Brauner <christian@brauner.io> Reviewed-by: Suren Baghdasaryan <surenb@google.com> Suggested-by: Kirill Tkhai <ktkhai@virtuozzo.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- include/linux/syscalls.h | 3 ++- mm/madvise.c | 34 ++++++++++++++++++++++------------ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index e4cd2c2f8bb4..f5ada20e2943 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -876,7 +876,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/mm/madvise.c b/mm/madvise.c index 6543f2bfc3d8..e794367f681e 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1182,11 +1182,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior) return do_madvise(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; @@ -1197,20 +1196,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); @@ -1223,7 +1233,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.265.gbab2e86ba0-goog
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. Reviewed-by: Suren Baghdasaryan <surenb@google.com> 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 e794367f681e..e77c6c1fad34 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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); } @@ -1169,6 +1171,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, } out: blk_finish_plug(&plug); +skip_mm: if (write) up_write(&mm->mmap_sem); else -- 2.25.0.265.gbab2e86ba0-goog
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/ Reviewed-by: SeongJae Park <sjpark@amazon.de> 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 e77c6c1fad34..f4fa962ee74d 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1005,6 +1005,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.265.gbab2e86ba0-goog
On Mon, 2 Mar 2020 11:36:23 -0800 Minchan Kim <minchan@kernel.org> 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.
Thanks, I grabbed these.
I massaged the patch titles significantly - mainly to alert readers to
the fact that we're proposing a new syscall.
Is a manpage for process_madvise(2) being prepared?
On Mon, Mar 02, 2020 at 01:16:18PM -0800, Andrew Morton wrote:
> On Mon, 2 Mar 2020 11:36:23 -0800 Minchan Kim <minchan@kernel.org> 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.
>
> Thanks, I grabbed these.
>
> I massaged the patch titles significantly - mainly to alert readers to
> the fact that we're proposing a new syscall.
>
> Is a manpage for process_madvise(2) being prepared?
I will prepare it, Thanks!
[-- Attachment #1: Type: text/plain, Size: 2045 bytes --] Hi Minchan, I love your patch! Yet something to improve: [auto build test ERROR on arm64/for-next/core] [also build test ERROR on arm/for-next m68k/for-next powerpc/next s390/features linus/master v5.6-rc4 next-20200302] [cannot apply to tip/x86/asm hp-parisc/for-next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Minchan-Kim/introduce-memory-hinting-API-for-external-process/20200303-044625 base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core config: arm64-defconfig (attached as .config) compiler: aarch64-linux-gcc (GCC) 7.5.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.5.0 make.cross ARCH=arm64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All errors (new ones prefixed by >>): aarch64-linux-ld: arch/arm64/kernel/head.o: relocation R_AARCH64_ABS32 against `_kernel_offset_le_lo32' can not be used when making a shared object aarch64-linux-ld: arch/arm64/kernel/efi-entry.stub.o: relocation R_AARCH64_ABS32 against `__efistub_stext_offset' can not be used when making a shared object arch/arm64/kernel/head.o: In function `kimage_vaddr': (.idmap.text+0x0): dangerous relocation: unsupported relocation arch/arm64/kernel/head.o: In function `__primary_switch': (.idmap.text+0x378): dangerous relocation: unsupported relocation (.idmap.text+0x380): dangerous relocation: unsupported relocation >> arch/arm64/kernel/sys32.o:(.rodata+0xdb8): undefined reference to `__arm64_process_madvise' --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 47445 bytes --]
On Tue, Mar 03, 2020 at 06:33:03PM +0800, kbuild test robot wrote: > Hi Minchan, > > I love your patch! Yet something to improve: > > [auto build test ERROR on arm64/for-next/core] > [also build test ERROR on arm/for-next m68k/for-next powerpc/next s390/features linus/master v5.6-rc4 next-20200302] > [cannot apply to tip/x86/asm hp-parisc/for-next] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/Minchan-Kim/introduce-memory-hinting-API-for-external-process/20200303-044625 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core > config: arm64-defconfig (attached as .config) > compiler: aarch64-linux-gcc (GCC) 7.5.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.5.0 make.cross ARCH=arm64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > aarch64-linux-ld: arch/arm64/kernel/head.o: relocation R_AARCH64_ABS32 against `_kernel_offset_le_lo32' can not be used when making a shared object > aarch64-linux-ld: arch/arm64/kernel/efi-entry.stub.o: relocation R_AARCH64_ABS32 against `__efistub_stext_offset' can not be used when making a shared object > arch/arm64/kernel/head.o: In function `kimage_vaddr': > (.idmap.text+0x0): dangerous relocation: unsupported relocation > arch/arm64/kernel/head.o: In function `__primary_switch': > (.idmap.text+0x378): dangerous relocation: unsupported relocation > (.idmap.text+0x380): dangerous relocation: unsupported relocation > >> arch/arm64/kernel/sys32.o:(.rodata+0xdb8): undefined reference to `__arm64_process_madvise' > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Thanks. I guess I had a mistake so I hope this patch should fix the issue. From 3cd8bc4dda41d13bcce6fcf6d9252ff684a1929d Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@kernel.org> Date: Tue, 3 Mar 2020 06:53:13 -0800 Subject: [PATCH] mm: fix process_madvise build break for arm64 0-day reported build break from process_madvise on ARM64. aarch64-linux-ld: arch/arm64/kernel/head.o: relocation R_AARCH64_ABS32 against `_kernel_offset_le_lo32' can not be used when making a shared object aarch64-linux-ld: arch/arm64/kernel/efi-entry.stub.o: relocation R_AARCH64_ABS32 against `__efistub_stext_offset' can not be used when making a shared object arch/arm64/kernel/head.o: In function `kimage_vaddr': (.idmap.text+0x0): dangerous relocation: unsupported relocation arch/arm64/kernel/head.o: In function `__primary_switch': (.idmap.text+0x378): dangerous relocation: unsupported relocation (.idmap.text+0x380): dangerous relocation: unsupported relocation >> arch/arm64/kernel/sys32.o:(.rodata+0xdb8): undefined reference to `__arm64_process_madvise' This patch should fix it. Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- arch/arm64/include/asm/unistd32.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h index 2a27be7a1f91..a1eec8d879d4 100644 --- a/arch/arm64/include/asm/unistd32.h +++ b/arch/arm64/include/asm/unistd32.h @@ -884,7 +884,7 @@ __SYSCALL(__NR_openat2, sys_openat2) #define __NR_pidfd_getfd 438 __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd) #define __NR_process_madvise 439 -__SYSCALL(__NR_process_madvise, process_madvise) +__SYSCALL(__NR_process_madvise, sys_process_madvise) /* * Please add new compat syscalls above this comment and update -- 2.25.1.481.gfbce0eb801-goog
On 3/2/20 8:36 PM, Minchan Kim wrote: > In upcoming patches, do_madvise will be called from external process > context so we shouldn't asssume "current" is always hinted process's > task_struct. > Furthermore, we couldn't access mm_struct via task->mm > once it's verified by access_mm which will be introduced in next > patch[1]. I would suggest to replace with: Furthermore, we must not access mm_struct via task->mm, but obtain it via access_mm() once (in the following patch) and only use that pointer [1], so pass it to do_madvise() as well. Note the vma->vm_mm pointers are safe, so we can use them further down the call stack. > And let's pass *current* and current->mm as arguments of > do_madvise so it shouldn't change existing behavior but prepare > next patch to make review easy. > > Note: io_madvise pass NULL as target_task argument of do_madvise > because it couldn't know who is target. can't > [1] http://lore.kernel.org/r/CAG48ez27=pwm5m_N_988xT1huO7g7h6arTQL44zev6TD-h-7Tg@mail.gmail.com > > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Jann Horn <jannh@google.com> > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Nit: > @@ -676,7 +677,6 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, > if (nr_swap) { > if (current->mm == mm) > sync_mm_rss(mm); > - > add_mm_counter(mm, MM_SWAPENTS, nr_swap); > } > arch_leave_lazy_mmu_mode(); This looks stray and unrelated.
On 3/2/20 8:36 PM, Minchan Kim wrote: > There is usecase that System Management Software(SMS) want to give > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and > in the case of Android, it is the ActivityManagerService. > > It's similar in spirit to madvise(MADV_WONTNEED), but the information You mean MADV_DONTNEED? > required to make the reclaim decision is not known to the app. This seems to be mixing up the differences between MADV_DONTNEED and COLD/PAGEOUT and self-imposed vs external memory hints? > 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); It's more common to call the argument 'flags' not 'flag'? The code seems to call it flags. Also in line with madvise(2), the 'advise' argument should rather be 'advice'. This is more important for the manpage, but let's be consistent. ... > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> For the core parts, Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
On 3/2/20 8:36 PM, Minchan Kim wrote: > Bail out to prevent unnecessary CPU overhead if target process has > pending fatal signal during (MADV_COLD|MADV_PAGEOUT) operation. > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Nit below: > --- > mm/madvise.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 349473fc6683..6543f2bfc3d8 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -36,6 +36,7 @@ > struct madvise_walk_private { > struct mmu_gather *tlb; > bool pageout; > + struct task_struct *target_task; > }; > > /* > @@ -316,6 +317,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd, > if (fatal_signal_pending(current)) > return -EINTR; > > + if (private->target_task && > + fatal_signal_pending(private->target_task)) > + return -EINTR; With madvise(2) private->target_task will be current, thus current will be tested twice. Not wrong, but maybe add a "private->target_task != current" condition?
On 3/2/20 8:36 PM, Minchan Kim wrote:
> process_madvise syscall needs pidfd_get_pid function to translate
> pidfd to pid so this patch move the function to kernel/pid.c.
>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Christian Brauner <christian@brauner.io>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> Suggested-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
On Mon, Mar 02, 2020 at 11:36:27AM -0800, Minchan Kim wrote:
> process_madvise syscall needs pidfd_get_pid function to translate
> pidfd to pid so this patch move the function to kernel/pid.c.
>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Christian Brauner <christian@brauner.io>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> Suggested-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
On 3/2/20 8:36 PM, Minchan Kim wrote: > There is a demand[1] to support pid as well pidfd for process_madvise > to reduce unnecessary syscall to get pidfd if the user has control of > the target process(ie, they could guarantee the process is not gone > or pid is not reused). > > 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); This is again halfway between kernel and userspace description, so if we stick to userspace then it's: int process_madvise(idtype_t idtype, id_t id, void *addr, size_t length, int advice, unsigned long flags); > @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/ > > Cc: Christian Brauner <christian@brauner.io> > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > Suggested-by: Kirill Tkhai <ktkhai@virtuozzo.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
On 3/2/20 8:36 PM, Minchan Kim wrote: > 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. > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > 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 e794367f681e..e77c6c1fad34 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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; This will return 0, is that correct? Shoudln't there be a similar error e.g. as when finding the task by pid fails (-ESRCH ?), because IIUC the task here is going away and dumping the core? > } else { > down_read(&mm->mmap_sem); > } > @@ -1169,6 +1171,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, > } > out: > blk_finish_plug(&plug); > +skip_mm: > if (write) > up_write(&mm->mmap_sem); > else >
Hello. On Fri, Mar 06, 2020 at 01:52:07PM +0100, Vlastimil Babka wrote: > > diff --git a/mm/madvise.c b/mm/madvise.c > > index e794367f681e..e77c6c1fad34 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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; > > This will return 0, is that correct? Shoudln't there be a similar error e.g. as > when finding the task by pid fails (-ESRCH ?), because IIUC the task here is > going away and dumping the core? Yeah. Something like this then: === diff --git a/mm/madvise.c b/mm/madvise.c index 48d1da08c160..7ed2f4d13924 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -1122,6 +1122,10 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, if (write) { if (down_write_killable(&mm->mmap_sem)) return -EINTR; + if (current->mm != mm && !mmget_still_valid(mm)) { + error = -ESRCH; + goto skip_mm; + } } else { down_read(&mm->mmap_sem); } @@ -1173,6 +1177,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, } out: blk_finish_plug(&plug); +skip_mm: if (write) up_write(&mm->mmap_sem); else === ? > > > } else { > > down_read(&mm->mmap_sem); > > } > > @@ -1169,6 +1171,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm, > > } > > out: > > blk_finish_plug(&plug); > > +skip_mm: > > if (write) > > up_write(&mm->mmap_sem); > > else > > > -- Best regards, Oleksandr Natalenko (post-factum) Principal Software Maintenance Engineer
On 3/2/20 8:36 PM, Minchan Kim wrote: > 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/ > > Reviewed-by: SeongJae Park <sjpark@amazon.de> > Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> This will lead to one process calling unmerge_ksm_pages() of another. There's a (signal_pending(current)) test there, should it check also the other task, analogically to task 3? Then break_ksm() is fine as it is, as ksmd also calls it, right? > --- > mm/madvise.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/mm/madvise.c b/mm/madvise.c > index e77c6c1fad34..f4fa962ee74d 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1005,6 +1005,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; >
On Fri, Mar 06, 2020 at 02:13:49PM +0100, Vlastimil Babka wrote: > On 3/2/20 8:36 PM, Minchan Kim wrote: > > 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/ > > > > Reviewed-by: SeongJae Park <sjpark@amazon.de> > > Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com> > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > This will lead to one process calling unmerge_ksm_pages() of another. There's a > (signal_pending(current)) test there, should it check also the other task, > analogically to task 3? Do we care about current there then? Shall we just pass mm into unmerge_ksm_pages and check the signals of the target task only, be it current or something else? > Then break_ksm() is fine as it is, as ksmd also calls it, right? I think break_ksm() cares only about mmap_sem protection, so we should be fine here. > > > --- > > mm/madvise.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > index e77c6c1fad34..f4fa962ee74d 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -1005,6 +1005,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; > > > -- Best regards, Oleksandr Natalenko (post-factum) Principal Software Maintenance Engineer
On 3/6/20 2:03 PM, Oleksandr Natalenko wrote:
> Hello.
>
> On Fri, Mar 06, 2020 at 01:52:07PM +0100, Vlastimil Babka wrote:
>> > diff --git a/mm/madvise.c b/mm/madvise.c
>> > index e794367f681e..e77c6c1fad34 100644
>> > --- a/mm/madvise.c
>> > +++ b/mm/madvise.c
>> > @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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;
>>
>> This will return 0, is that correct? Shoudln't there be a similar error e.g. as
>> when finding the task by pid fails (-ESRCH ?), because IIUC the task here is
>> going away and dumping the core?
>
> Yeah.
>
> Something like this then:
>
> ===
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 48d1da08c160..7ed2f4d13924 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1122,6 +1122,10 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> if (write) {
> if (down_write_killable(&mm->mmap_sem))
> return -EINTR;
> + if (current->mm != mm && !mmget_still_valid(mm)) {
> + error = -ESRCH;
> + goto skip_mm;
> + }
> } else {
> down_read(&mm->mmap_sem);
> }
> @@ -1173,6 +1177,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> }
> out:
> blk_finish_plug(&plug);
> +skip_mm:
> if (write)
> up_write(&mm->mmap_sem);
> else
>
> ===
>
> ?
Yep, thanks.
On 3/6/20 2:41 PM, Oleksandr Natalenko wrote: > On Fri, Mar 06, 2020 at 02:13:49PM +0100, Vlastimil Babka wrote: >> On 3/2/20 8:36 PM, Minchan Kim wrote: >> > 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; Ah, I forgot to ask, given the discussion of races in patch 2 (Question 2), where android can stop the tasks to apply the madvise hints in a race-free manner, how does that work for remote KSM hints in your scenarios, especially the one above? >> > >> > * 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/ >> > >> > Reviewed-by: SeongJae Park <sjpark@amazon.de> >> > Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com> >> > Signed-off-by: Minchan Kim <minchan@kernel.org> >> >> This will lead to one process calling unmerge_ksm_pages() of another. There's a >> (signal_pending(current)) test there, should it check also the other task, >> analogically to task 3? > > Do we care about current there then? Shall we just pass mm into unmerge_ksm_pages and check the signals of the target task only, be it current or something else? Dunno, it's nice to react to signals quickly, for any proces that gets them, no? >> Then break_ksm() is fine as it is, as ksmd also calls it, right? > > I think break_ksm() cares only about mmap_sem protection, so we should > be fine here. > >> >> > --- >> > mm/madvise.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/mm/madvise.c b/mm/madvise.c >> > index e77c6c1fad34..f4fa962ee74d 100644 >> > --- a/mm/madvise.c >> > +++ b/mm/madvise.c >> > @@ -1005,6 +1005,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; >> > >> >
On Fri, Mar 06, 2020 at 05:03:50PM +0100, Vlastimil Babka wrote:
> On 3/6/20 2:03 PM, Oleksandr Natalenko wrote:
> > Hello.
> >
> > On Fri, Mar 06, 2020 at 01:52:07PM +0100, Vlastimil Babka wrote:
> >> > diff --git a/mm/madvise.c b/mm/madvise.c
> >> > index e794367f681e..e77c6c1fad34 100644
> >> > --- a/mm/madvise.c
> >> > +++ b/mm/madvise.c
> >> > @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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;
> >>
> >> This will return 0, is that correct? Shoudln't there be a similar error e.g. as
> >> when finding the task by pid fails (-ESRCH ?), because IIUC the task here is
> >> going away and dumping the core?
> >
> > Yeah.
> >
> > Something like this then:
> >
> > ===
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 48d1da08c160..7ed2f4d13924 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1122,6 +1122,10 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> > if (write) {
> > if (down_write_killable(&mm->mmap_sem))
> > return -EINTR;
> > + if (current->mm != mm && !mmget_still_valid(mm)) {
> > + error = -ESRCH;
> > + goto skip_mm;
> > + }
> > } else {
> > down_read(&mm->mmap_sem);
> > }
> > @@ -1173,6 +1177,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> > }
> > out:
> > blk_finish_plug(&plug);
> > +skip_mm:
> > if (write)
> > up_write(&mm->mmap_sem);
> > else
> >
> > ===
> >
> > ?
>
> Yep, thanks.
>
Minchan, shall you take this change into the next submission, or you'd
prefer me sending it to you as a new patch?
--
Best regards,
Oleksandr Natalenko (post-factum)
Principal Software Maintenance Engineer
On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote: > On 3/6/20 2:41 PM, Oleksandr Natalenko wrote: > > On Fri, Mar 06, 2020 at 02:13:49PM +0100, Vlastimil Babka wrote: > >> On 3/2/20 8:36 PM, Minchan Kim wrote: > >> > 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; > > Ah, I forgot to ask, given the discussion of races in patch 2 (Question 2), > where android can stop the tasks to apply the madvise hints in a race-free > manner, how does that work for remote KSM hints in your scenarios, especially > the one above? We have cgroup.freeze for that. > > >> > > >> > * 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/ > >> > > >> > Reviewed-by: SeongJae Park <sjpark@amazon.de> > >> > Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com> > >> > Signed-off-by: Minchan Kim <minchan@kernel.org> > >> > >> This will lead to one process calling unmerge_ksm_pages() of another. There's a > >> (signal_pending(current)) test there, should it check also the other task, > >> analogically to task 3? > > > > Do we care about current there then? Shall we just pass mm into unmerge_ksm_pages and check the signals of the target task only, be it current or something else? > > Dunno, it's nice to react to signals quickly, for any proces that gets them, no? So, do you mean something like this? === diff --git a/mm/ksm.c b/mm/ksm.c index 363ec8189561..b39c237cfcf4 100644 --- a/mm/ksm.c +++ b/mm/ksm.c @@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma, for (addr = start; addr < end && !err; addr += PAGE_SIZE) { if (ksm_test_exit(vma->vm_mm)) break; - if (signal_pending(current)) + if (signal_pending(current) || + signal_pending(rcu_dereference(vma->vm_mm->owner))) err = -ERESTARTSYS; else err = break_ksm(vma, addr); === BTW, this won't work with !CONFIG_MEMCG, so probably task_struct should be passed through instead. IIUC, this would also require amending struct mm_slot in order to share the same code path with ksmd. I'm not sure I've seen such a culprit anywhere else, so I'm in doubt this would be a correct thing to do. Ideas? > > >> Then break_ksm() is fine as it is, as ksmd also calls it, right? > > > > I think break_ksm() cares only about mmap_sem protection, so we should > > be fine here. > > > >> > >> > --- > >> > mm/madvise.c | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > > >> > diff --git a/mm/madvise.c b/mm/madvise.c > >> > index e77c6c1fad34..f4fa962ee74d 100644 > >> > --- a/mm/madvise.c > >> > +++ b/mm/madvise.c > >> > @@ -1005,6 +1005,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; > >> > > >> > > > -- Best regards, Oleksandr Natalenko (post-factum) Principal Software Maintenance Engineer
On Mon 09-03-20 14:11:17, Oleksandr Natalenko wrote: > On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote: [...] > > Dunno, it's nice to react to signals quickly, for any proces that gets them, no? > > So, do you mean something like this? > > === > diff --git a/mm/ksm.c b/mm/ksm.c > index 363ec8189561..b39c237cfcf4 100644 > --- a/mm/ksm.c > +++ b/mm/ksm.c > @@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma, > for (addr = start; addr < end && !err; addr += PAGE_SIZE) { > if (ksm_test_exit(vma->vm_mm)) > break; > - if (signal_pending(current)) > + if (signal_pending(current) || > + signal_pending(rcu_dereference(vma->vm_mm->owner))) > err = -ERESTARTSYS; > else > err = break_ksm(vma, addr); > === This is broken because mm might be attached to different tasks. AFAIU this check is meant to allow quick backoff of the _calling_ process so that it doesn't waste time when the context is killed already. I do not understand why should we care about any other context here? What is the actual problem this would solve? -- Michal Hocko SUSE Labs
On Mon, Mar 09, 2020 at 04:08:15PM +0100, Michal Hocko wrote: > On Mon 09-03-20 14:11:17, Oleksandr Natalenko wrote: > > On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote: > [...] > > > Dunno, it's nice to react to signals quickly, for any proces that gets them, no? > > > > So, do you mean something like this? > > > > === > > diff --git a/mm/ksm.c b/mm/ksm.c > > index 363ec8189561..b39c237cfcf4 100644 > > --- a/mm/ksm.c > > +++ b/mm/ksm.c > > @@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma, > > for (addr = start; addr < end && !err; addr += PAGE_SIZE) { > > if (ksm_test_exit(vma->vm_mm)) > > break; > > - if (signal_pending(current)) > > + if (signal_pending(current) || > > + signal_pending(rcu_dereference(vma->vm_mm->owner))) > > err = -ERESTARTSYS; > > else > > err = break_ksm(vma, addr); > > === > > This is broken because mm might be attached to different tasks. > AFAIU this check is meant to allow quick backoff of the _calling_ > process so that it doesn't waste time when the context is killed > already. I do not understand why should we care about any other context > here? What is the actual problem this would solve? I agree with you, but still trying to understand what does Vlastimil mean :). > > -- > Michal Hocko > SUSE Labs > -- Best regards, Oleksandr Natalenko (post-factum) Principal Software Maintenance Engineer
On 3/9/20 4:19 PM, Oleksandr Natalenko wrote: > On Mon, Mar 09, 2020 at 04:08:15PM +0100, Michal Hocko wrote: >> On Mon 09-03-20 14:11:17, Oleksandr Natalenko wrote: >> > On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote: >> [...] >> > > Dunno, it's nice to react to signals quickly, for any proces that gets them, no? >> > >> > So, do you mean something like this? >> > >> > === >> > diff --git a/mm/ksm.c b/mm/ksm.c >> > index 363ec8189561..b39c237cfcf4 100644 >> > --- a/mm/ksm.c >> > +++ b/mm/ksm.c >> > @@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma, >> > for (addr = start; addr < end && !err; addr += PAGE_SIZE) { >> > if (ksm_test_exit(vma->vm_mm)) >> > break; >> > - if (signal_pending(current)) >> > + if (signal_pending(current) || >> > + signal_pending(rcu_dereference(vma->vm_mm->owner))) >> > err = -ERESTARTSYS; >> > else >> > err = break_ksm(vma, addr); >> > === >> >> This is broken because mm might be attached to different tasks. >> AFAIU this check is meant to allow quick backoff of the _calling_ >> process so that it doesn't waste time when the context is killed >> already. I do not understand why should we care about any other context >> here? What is the actual problem this would solve? > > I agree with you, but still trying to understand what does Vlastimil mean > :). Well you wondered if we should stop caring about current, and I said that probably wouldn't be nice. As for caring about the other task, patch 3/7 does that for (MADV_COLD|MADV_PAGEOUT) so I just pointed out that the KSM case doesn't. AFAIU if we don't check the signals, we might be blocking the killed task from exiting? >> >> -- >> Michal Hocko >> SUSE Labs >> >
On Mon 09-03-20 16:42:43, Vlastimil Babka wrote:
> On 3/9/20 4:19 PM, Oleksandr Natalenko wrote:
> > On Mon, Mar 09, 2020 at 04:08:15PM +0100, Michal Hocko wrote:
> >> On Mon 09-03-20 14:11:17, Oleksandr Natalenko wrote:
> >> > On Fri, Mar 06, 2020 at 05:08:18PM +0100, Vlastimil Babka wrote:
> >> [...]
> >> > > Dunno, it's nice to react to signals quickly, for any proces that gets them, no?
> >> >
> >> > So, do you mean something like this?
> >> >
> >> > ===
> >> > diff --git a/mm/ksm.c b/mm/ksm.c
> >> > index 363ec8189561..b39c237cfcf4 100644
> >> > --- a/mm/ksm.c
> >> > +++ b/mm/ksm.c
> >> > @@ -849,7 +849,8 @@ static int unmerge_ksm_pages(struct vm_area_struct *vma,
> >> > for (addr = start; addr < end && !err; addr += PAGE_SIZE) {
> >> > if (ksm_test_exit(vma->vm_mm))
> >> > break;
> >> > - if (signal_pending(current))
> >> > + if (signal_pending(current) ||
> >> > + signal_pending(rcu_dereference(vma->vm_mm->owner)))
> >> > err = -ERESTARTSYS;
> >> > else
> >> > err = break_ksm(vma, addr);
> >> > ===
> >>
> >> This is broken because mm might be attached to different tasks.
> >> AFAIU this check is meant to allow quick backoff of the _calling_
> >> process so that it doesn't waste time when the context is killed
> >> already. I do not understand why should we care about any other context
> >> here? What is the actual problem this would solve?
> >
> > I agree with you, but still trying to understand what does Vlastimil mean
> > :).
>
> Well you wondered if we should stop caring about current, and I said that
> probably wouldn't be nice.
> As for caring about the other task, patch 3/7 does that for
> (MADV_COLD|MADV_PAGEOUT) so I just pointed out that the KSM case doesn't. AFAIU
> if we don't check the signals, we might be blocking the killed task from exiting?
I would have to double check but I do not think this would be a problem
because the remote task should take mmget to prevent address space to
vanish under its feet. That should also rule out the exclusive mmap_sem
usage from the exit path.
--
Michal Hocko
SUSE Labs
On Thu, Mar 05, 2020 at 07:15:10PM +0100, Vlastimil Babka wrote: > On 3/2/20 8:36 PM, Minchan Kim wrote: > > There is usecase that System Management Software(SMS) want to give > > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and > > in the case of Android, it is the ActivityManagerService. > > > > It's similar in spirit to madvise(MADV_WONTNEED), but the information > > You mean MADV_DONTNEED? Mean to DONT_NEED's future version. > > > required to make the reclaim decision is not known to the app. > > This seems to be mixing up the differences between MADV_DONTNEED and > COLD/PAGEOUT and self-imposed vs external memory hints? Sorry, I don't understand what you want here. > > > 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); > > It's more common to call the argument 'flags' not 'flag'? The code seems to call > it flags. Also in line with madvise(2), the 'advise' argument should rather be > 'advice'. This is more important for the manpage, but let's be consistent. Yub, I will send the patch along with your comment in previous patch. > > ... > > > > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > For the core parts, > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks for the review!
On Fri, Mar 06, 2020 at 11:22:07AM +0100, Vlastimil Babka wrote:
> On 3/2/20 8:36 PM, Minchan Kim wrote:
> > Bail out to prevent unnecessary CPU overhead if target process has
> > pending fatal signal during (MADV_COLD|MADV_PAGEOUT) operation.
> >
> > Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
> Nit below:
>
> > ---
> > mm/madvise.c | 29 +++++++++++++++++++++--------
> > 1 file changed, 21 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 349473fc6683..6543f2bfc3d8 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -36,6 +36,7 @@
> > struct madvise_walk_private {
> > struct mmu_gather *tlb;
> > bool pageout;
> > + struct task_struct *target_task;
> > };
> >
> > /*
> > @@ -316,6 +317,10 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> > if (fatal_signal_pending(current))
> > return -EINTR;
> >
> > + if (private->target_task &&
> > + fatal_signal_pending(private->target_task))
> > + return -EINTR;
>
> With madvise(2) private->target_task will be current, thus current will be
> tested twice. Not wrong, but maybe add a "private->target_task != current"
> condition?
It was in old series but removed because reviewer(IIRC, suren) wanted it.
I am not strong preference either way. Since you said it's nit and
considering other reviewer wanted to remove it, I will not change
further.
Thanks!
On Mon, Mar 09, 2020 at 01:30:45PM +0100, Oleksandr Natalenko wrote:
> On Fri, Mar 06, 2020 at 05:03:50PM +0100, Vlastimil Babka wrote:
> > On 3/6/20 2:03 PM, Oleksandr Natalenko wrote:
> > > Hello.
> > >
> > > On Fri, Mar 06, 2020 at 01:52:07PM +0100, Vlastimil Babka wrote:
> > >> > diff --git a/mm/madvise.c b/mm/madvise.c
> > >> > index e794367f681e..e77c6c1fad34 100644
> > >> > --- a/mm/madvise.c
> > >> > +++ b/mm/madvise.c
> > >> > @@ -1118,6 +1118,8 @@ int do_madvise(struct task_struct *target_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;
> > >>
> > >> This will return 0, is that correct? Shoudln't there be a similar error e.g. as
> > >> when finding the task by pid fails (-ESRCH ?), because IIUC the task here is
> > >> going away and dumping the core?
> > >
> > > Yeah.
> > >
> > > Something like this then:
> > >
> > > ===
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 48d1da08c160..7ed2f4d13924 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1122,6 +1122,10 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> > > if (write) {
> > > if (down_write_killable(&mm->mmap_sem))
> > > return -EINTR;
> > > + if (current->mm != mm && !mmget_still_valid(mm)) {
> > > + error = -ESRCH;
> > > + goto skip_mm;
> > > + }
> > > } else {
> > > down_read(&mm->mmap_sem);
> > > }
> > > @@ -1173,6 +1177,7 @@ int do_madvise(struct task_struct *target_task, struct mm_struct *mm,
> > > }
> > > out:
> > > blk_finish_plug(&plug);
> > > +skip_mm:
> > > if (write)
> > > up_write(&mm->mmap_sem);
> > > else
> > >
> > > ===
> > >
> > > ?
> >
> > Yep, thanks.
> >
>
> Minchan, shall you take this change into the next submission, or you'd
> prefer me sending it to you as a new patch?
I should send patchset again so I will take it.
Thanks!
On Tue, Mar 10, 2020 at 03:20:08PM -0700, Minchan Kim wrote:
> On Thu, Mar 05, 2020 at 07:15:10PM +0100, Vlastimil Babka wrote:
> > On 3/2/20 8:36 PM, Minchan Kim wrote:
> > > There is usecase that System Management Software(SMS) want to give
> > > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and
> > > in the case of Android, it is the ActivityManagerService.
> > >
> > > It's similar in spirit to madvise(MADV_WONTNEED), but the information
> >
> > You mean MADV_DONTNEED?
>
> Mean to DONT_NEED's future version.
>
> >
> > > required to make the reclaim decision is not known to the app.
> >
> > This seems to be mixing up the differences between MADV_DONTNEED and
> > COLD/PAGEOUT and self-imposed vs external memory hints?
>
> Sorry, I don't understand what you want here.
>
> >
> > > 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);
> >
> > It's more common to call the argument 'flags' not 'flag'? The code seems to call
> > it flags. Also in line with madvise(2), the 'advise' argument should rather be
> > 'advice'. This is more important for the manpage, but let's be consistent.
>
> Yub, I will send the patch along with your comment in previous patch.
Only place to use *advice* in kernel is comment in madvise_willneed.
Al other places use advise in kernel so I wanted to be more consistent
with other kernel sources. For man page, we could use the term "advice",
no problem.
On Fri, Mar 06, 2020 at 12:14:19PM +0100, Vlastimil Babka wrote: > On 3/2/20 8:36 PM, Minchan Kim wrote: > > There is a demand[1] to support pid as well pidfd for process_madvise > > to reduce unnecessary syscall to get pidfd if the user has control of > > the target process(ie, they could guarantee the process is not gone > > or pid is not reused). > > > > 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); > > This is again halfway between kernel and userspace description, so if we stick > to userspace then it's: > > int process_madvise(idtype_t idtype, id_t id, void *addr, > size_t length, int advice, unsigned long flags); Yub. > > > > @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/ > > > > Cc: Christian Brauner <christian@brauner.io> > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > > Suggested-by: Kirill Tkhai <ktkhai@virtuozzo.com> > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks!
On 3/10/20 11:20 PM, Minchan Kim wrote: > On Thu, Mar 05, 2020 at 07:15:10PM +0100, Vlastimil Babka wrote: >> On 3/2/20 8:36 PM, Minchan Kim wrote: >> > There is usecase that System Management Software(SMS) want to give >> > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and >> > in the case of Android, it is the ActivityManagerService. >> > >> > It's similar in spirit to madvise(MADV_WONTNEED), but the information >> >> You mean MADV_DONTNEED? > > Mean to DONT_NEED's future version. What's that exactly? >> >> > required to make the reclaim decision is not known to the app. >> >> This seems to be mixing up the differences between MADV_DONTNEED and >> COLD/PAGEOUT and self-imposed vs external memory hints? > > Sorry, I don't understand what you want here. You say that process_madvise(MADV_[COLD|PAGEEOUT]) is similar to madvise(MADV_WONTNEED) but the difference is that the information required to make the reclaim decision is not known to the app. I see two differences. One is madvise vs process_madvise, which is explained by "reclaim decision is not known to the app." The other is MADV_WONTNEED vs MADV_[COLD|PAGEEOUT], which is... I'm not sure until you say what's "DONT_NEED's future version" :D Anyway I assume this part is from the versions where the new COLD and PAGEOUT flags were introduced together with external memory hinting API?
On Thu, Mar 12, 2020 at 01:40:26PM +0100, Vlastimil Babka wrote: > On 3/10/20 11:20 PM, Minchan Kim wrote: > > On Thu, Mar 05, 2020 at 07:15:10PM +0100, Vlastimil Babka wrote: > >> On 3/2/20 8:36 PM, Minchan Kim wrote: > >> > There is usecase that System Management Software(SMS) want to give > >> > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and > >> > in the case of Android, it is the ActivityManagerService. > >> > > >> > It's similar in spirit to madvise(MADV_WONTNEED), but the information > >> > >> You mean MADV_DONTNEED? > > > > Mean to DONT_NEED's future version. > > What's that exactly? For zapping timing point of view, dontneed acts immediately so it's very strong hint. However, MADV_COLD and MADV_PAGEOUT does lazily depending on the future. For example, the page never discarded if it's touched before the tail of LRU. If other process which shared the page has touched the page, never paging out. > > >> > >> > required to make the reclaim decision is not known to the app. > >> > >> This seems to be mixing up the differences between MADV_DONTNEED and > >> COLD/PAGEOUT and self-imposed vs external memory hints? > > > > Sorry, I don't understand what you want here. > > You say that process_madvise(MADV_[COLD|PAGEEOUT]) is similar to > madvise(MADV_WONTNEED) but the difference is that the information > required to make the reclaim decision is not known to the app. > > I see two differences. One is madvise vs process_madvise, which is explained by > "reclaim decision is not known to the app." > The other is MADV_WONTNEED vs MADV_[COLD|PAGEEOUT], which is... I'm not sure > until you say what's "DONT_NEED's future version" :D > > Anyway I assume this part is from the versions where the new COLD and PAGEOUT > flags were introduced together with external memory hinting API? Exactly. Maybe it would be better to remove the part once we merged the COLD and PAGEOUT now. Thanks for the review, Vlastimil!
On Thu, Mar 05, 2020 at 04:48:12PM +0100, Vlastimil Babka wrote:
> On 3/2/20 8:36 PM, Minchan Kim wrote:
> > In upcoming patches, do_madvise will be called from external process
> > context so we shouldn't asssume "current" is always hinted process's
> > task_struct.
>
>
> > Furthermore, we couldn't access mm_struct via task->mm
> > once it's verified by access_mm which will be introduced in next
> > patch[1].
>
> I would suggest to replace with:
>
> Furthermore, we must not access mm_struct via task->mm, but obtain it via
> access_mm() once (in the following patch) and only use that pointer [1], so pass
> it to do_madvise() as well. Note the vma->vm_mm pointers are safe, so we can use
> them further down the call stack.
>
> > And let's pass *current* and current->mm as arguments of
> > do_madvise so it shouldn't change existing behavior but prepare
> > next patch to make review easy.
> >
> > Note: io_madvise pass NULL as target_task argument of do_madvise
> > because it couldn't know who is target.
>
> can't
Andrew already picked up your suggestion except "can't" part. I don't
think it's enough to resend fix it so I will leave it as-is.
On Thu, Mar 12, 2020 at 01:23:39PM -0700, Minchan Kim wrote: > On Thu, Mar 12, 2020 at 01:40:26PM +0100, Vlastimil Babka wrote: > > On 3/10/20 11:20 PM, Minchan Kim wrote: > > > On Thu, Mar 05, 2020 at 07:15:10PM +0100, Vlastimil Babka wrote: > > >> On 3/2/20 8:36 PM, Minchan Kim wrote: > > >> > There is usecase that System Management Software(SMS) want to give > > >> > a memory hint like MADV_[COLD|PAGEEOUT] to other processes and > > >> > in the case of Android, it is the ActivityManagerService. > > >> > > > >> > It's similar in spirit to madvise(MADV_WONTNEED), but the information > > >> > > >> You mean MADV_DONTNEED? > > > > > > Mean to DONT_NEED's future version. > > > > What's that exactly? > > For zapping timing point of view, dontneed acts immediately so it's very > strong hint. However, MADV_COLD and MADV_PAGEOUT does lazily depending > on the future. For example, the page never discarded if it's touched > before the tail of LRU. If other process which shared the page has > touched the page, never paging out. > > > > > >> > > >> > required to make the reclaim decision is not known to the app. > > >> > > >> This seems to be mixing up the differences between MADV_DONTNEED and > > >> COLD/PAGEOUT and self-imposed vs external memory hints? > > > > > > Sorry, I don't understand what you want here. > > > > You say that process_madvise(MADV_[COLD|PAGEEOUT]) is similar to > > madvise(MADV_WONTNEED) but the difference is that the information > > required to make the reclaim decision is not known to the app. > > > > I see two differences. One is madvise vs process_madvise, which is explained by > > "reclaim decision is not known to the app." > > The other is MADV_WONTNEED vs MADV_[COLD|PAGEEOUT], which is... I'm not sure > > until you say what's "DONT_NEED's future version" :D > > > > Anyway I assume this part is from the versions where the new COLD and PAGEOUT > > flags were introduced together with external memory hinting API? > > Exactly. Maybe it would be better to remove the part once we merged the > COLD and PAGEOUT now. > > Thanks for the review, Vlastimil! Hi Andrew, Per Vlastimil's review, I removed unnecessary part and changed syscall argument name "advise and flag" to "advice and flags" in description. Could you replace the description with this one? Code part is same so no need to be changed. Thanks. From fdb29014c84aebcca4737de735993e87d43ebbbf Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@kernel.org> Date: Wed, 6 May 2020 13:54:39 +0000 Subject: [PATCH] mm/madvise: introduce process_madvise() syscall: an external memory hinting API There is usecase that System Management Software(SMS) want to give a memory hint like MADV_[COLD|PAGEEOUT] to other processes and in the case of Android, it is the ActivityManagerService. The information required to make the reclaim decision is not known to the app. Instead, it is known to the centralized userspace daemon(ActivityManagerService), and that daemon must be able to initiate reclaim on its own without any app involvement. To solve the issue, this patch introduces a new syscall process_madvise(2). It uses pidfd of an external process to give the hint. int process_madvise(int pidfd, void *addr, size_t length, int advice, unsigned long flags); 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 the callee's, because the callee 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/ Link: http://lkml.kernel.org/r/20200302193630.68771-3-minchan@kernel.org Signed-off-by: Minchan Kim <minchan@kernel.org> Reviewed-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> Cc: Brian Geffon <bgeffon@google.com> Cc: Christian Brauner <christian@brauner.io> Cc: Daniel Colascione <dancol@google.com> Cc: Jann Horn <jannh@google.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: John Dias <joaodias@google.com> Cc: Kirill Tkhai <ktkhai@virtuozzo.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Oleksandr Natalenko <oleksandr@redhat.com> Cc: Sandeep Patil <sspatil@google.com> Cc: SeongJae Park <sj38.park@gmail.com> Cc: SeongJae Park <sjpark@amazon.de> Cc: Shakeel Butt <shakeelb@google.com> Cc: Sonny Rao <sonnyrao@google.com> Cc: Tim Murray <timmurray@google.com> Cc: <linux-man@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
On Tue, Mar 10, 2020 at 05:42:51PM -0700, Minchan Kim wrote: > On Fri, Mar 06, 2020 at 12:14:19PM +0100, Vlastimil Babka wrote: > > On 3/2/20 8:36 PM, Minchan Kim wrote: > > > There is a demand[1] to support pid as well pidfd for process_madvise > > > to reduce unnecessary syscall to get pidfd if the user has control of > > > the target process(ie, they could guarantee the process is not gone > > > or pid is not reused). > > > > > > 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); > > > > This is again halfway between kernel and userspace description, so if we stick > > to userspace then it's: > > > > int process_madvise(idtype_t idtype, id_t id, void *addr, > > size_t length, int advice, unsigned long flags); > > Yub. > Hi Andrew, Per Vlastimil's request, I changed "which and advise" with "idtype and advice" in function prototype of description. Could you replace the part in the description? Code is never changed. Thanks. From f11cfd023746ae67b89f2d84d960706ba6c5c911 Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@kernel.org> Date: Wed, 6 May 2020 13:54:40 +0000 Subject: [PATCH] mm/madvise: support both pid and pidfd for process_madvise There is a demand[1] to support pid as well pidfd for process_madvise to reduce unnecessary syscall to get pidfd if the user has control of the target process(ie, they could guarantee the process is not gone or pid is not reused). This patch aims for supporting both options like waitid(2). So, the syscall is currently, int process_madvise(idtype_t idtype, id_t id, void *addr, size_t length, int advice, unsigned long flags); @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/ Link: http://lkml.kernel.org/r/20200302193630.68771-6-minchan@kernel.org Signed-off-by: Minchan Kim <minchan@kernel.org> Suggested-by: Kirill Tkhai <ktkhai@virtuozzo.com> Reviewed-by: Suren Baghdasaryan <surenb@google.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Cc: Christian Brauner <christian@brauner.io> Cc: Alexander Duyck <alexander.h.duyck@linux.intel.com> Cc: Brian Geffon <bgeffon@google.com> Cc: Daniel Colascione <dancol@google.com> Cc: Jann Horn <jannh@google.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Joel Fernandes <joel@joelfernandes.org> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: John Dias <joaodias@google.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Oleksandr Natalenko <oleksandr@redhat.com> Cc: Sandeep Patil <sspatil@google.com> Cc: SeongJae Park <sj38.park@gmail.com> Cc: SeongJae Park <sjpark@amazon.de> Cc: Shakeel Butt <shakeelb@google.com> Cc: Sonny Rao <sonnyrao@google.com> Cc: Tim Murray <timmurray@google.com> Cc: <linux-man@vger.kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <minchan@kernel.org> wrote: > > ... > > Per Vlastimil's request, I changed "which and advise" with "idtype and > advice" in function prototype of description. > Could you replace the part in the description? Code is never changed. > Done, but... > > ... > > There is a demand[1] to support pid as well pidfd for process_madvise to > reduce unnecessary syscall to get pidfd if the user has control of the > target process(ie, they could guarantee the process is not gone or pid is > not reused). > > This patch aims for supporting both options like waitid(2). So, the > syscall is currently, > > int process_madvise(idtype_t idtype, id_t id, void *addr, > size_t length, int advice, unsigned long flags); > > @which is actually idtype_t for userspace libray and currently, it > supports P_PID and P_PIDFD. What does "@which is actually idtype_t for userspace libray" mean? Can you clarify and expand? Also, does this userspace library exist? If so, where is it? > > ... >
On Fri, May 08, 2020 at 04:04:15PM -0700, Andrew Morton wrote: > On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <minchan@kernel.org> wrote: > > > > > ... > > > > Per Vlastimil's request, I changed "which and advise" with "idtype and > > advice" in function prototype of description. > > Could you replace the part in the description? Code is never changed. > > > > Done, but... > > > > > ... > > > > There is a demand[1] to support pid as well pidfd for process_madvise to > > reduce unnecessary syscall to get pidfd if the user has control of the > > target process(ie, they could guarantee the process is not gone or pid is > > not reused). > > > > This patch aims for supporting both options like waitid(2). So, the > > syscall is currently, > > > > int process_madvise(idtype_t idtype, id_t id, void *addr, > > size_t length, int advice, unsigned long flags); > > > > @which is actually idtype_t for userspace libray and currently, it > > supports P_PID and P_PIDFD. > > What does "@which is actually idtype_t for userspace libray" mean? Can > you clarify and expand? If I may clarify, the only case where we've supported both pidfd and pid in the same system call is waitid() to avoid adding a dedicated system call for waiting and because waitid() already had this (imho insane) argument type switching. The idtype_t thing comes from waitid() and is located int sys/wait.h and is defined as "The type idtype_t is defined as an enumeration type whose possible values include at least the following: P_ALL P_PID P_PGID " int waitid(idtype_t idtype, id_t id, siginfo_t *infop, int options); If idtype is P_PID, waitid() shall wait for the child with a process ID equal to (pid_t)id. If idtype is P_PGID, waitid() shall wait for any child with a process group ID equal to (pid_t)id. If idtype is P_ALL, waitid() shall wait for any children and id is ignored. I'm personally not a fan of this idtype_t thing and think this should just have been > > int pidfd_madvise(int pidfd, void *addr, > > size_t length, int advice, unsigned long flags); and call it a day. Also, if I may ask, why is the flag argument "unsigned long"? That's pretty unorthodox. The expectation is that flag arguments are not word-size dependent and should usually use "unsigned int". All new system calls follow this pattern too. The current syscall layout will mean that on 64 bit systems you have 64 flag bits and on 32 bit you have 32 flag bits, I think. That has just recently led to some problems with the clone() syscall (fixed in [1] which I'm sending Monday) which has the same weird word-size-dependent flag argument layout. If a system does sign-extension and a userspace api or glibc uses e.g. an int for the flag argument in the system call wrapper - which is fairly common - you can get sign extended and then you end up with garbage in the upper 32 bits of your system call. > > Also, does this userspace library exist? If so, where is it? [1]: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/commit/?h=fixes&id=3f2c788a13143620c5471ac96ac4f033fc9ac3f3 Christian
Hi Christian, On Sat, May 09, 2020 at 02:48:17PM +0200, Christian Brauner wrote: > On Fri, May 08, 2020 at 04:04:15PM -0700, Andrew Morton wrote: > > On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > ... > > > > > > Per Vlastimil's request, I changed "which and advise" with "idtype and > > > advice" in function prototype of description. > > > Could you replace the part in the description? Code is never changed. > > > > > > > Done, but... > > > > > > > > ... > > > > > > There is a demand[1] to support pid as well pidfd for process_madvise to > > > reduce unnecessary syscall to get pidfd if the user has control of the > > > target process(ie, they could guarantee the process is not gone or pid is > > > not reused). > > > > > > This patch aims for supporting both options like waitid(2). So, the > > > syscall is currently, > > > > > > int process_madvise(idtype_t idtype, id_t id, void *addr, > > > size_t length, int advice, unsigned long flags); > > > > > > @which is actually idtype_t for userspace libray and currently, it > > > supports P_PID and P_PIDFD. > > > > What does "@which is actually idtype_t for userspace libray" mean? Can > > you clarify and expand? > > If I may clarify, the only case where we've supported both pidfd and pid > in the same system call is waitid() to avoid adding a dedicated system > call for waiting and because waitid() already had this (imho insane) > argument type switching. The idtype_t thing comes from waitid() and is > located int sys/wait.h and is defined as > > "The type idtype_t is defined as an enumeration type whose possible > values include at least the following: > > P_ALL > P_PID > P_PGID > " > > int waitid(idtype_t idtype, id_t id, siginfo_t *infop, int options); > If idtype is P_PID, waitid() shall wait for the child with a process ID equal to (pid_t)id. > If idtype is P_PGID, waitid() shall wait for any child with a process group ID equal to (pid_t)id. > If idtype is P_ALL, waitid() shall wait for any children and id is ignored. > > I'm personally not a fan of this idtype_t thing and think this should > just have been > > > int pidfd_madvise(int pidfd, void *addr, > > > size_t length, int advice, unsigned long flags); > and call it a day. That was the argument at that time, Daniel and I didn't want to have pid along with pidfd even though Kirill strongly wanted to have it. However you said " Overall, I don't particularly care how or if you integrate pidfd here." at that time. https://lore.kernel.org/linux-mm/20200113104256.5ujbplyec2sk4onn@wittgenstein/ I asked a question to Kirll at that time. " > Sounds like that you want to support both options for every upcoming API > which deals with pid. I'm not sure how it's critical for process_madvise > API this case. In general, we sacrifice some performance for the nicer one > and later, once it's reported as hurdle for some workload, we could fix it > via introducing new flag. What I don't like at this moment is to make > syscall complicated with potential scenarios without real workload. Yes, I suggest allowing both options for every new process api " https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/ You didn't give the opinion at that time, either(I expected you will make some voice then). What I could do to proceed work was separate it as different patch like this one to get more attention in future. And now it works. Let me clarify my side: I still don't like to introduce pid for new API since we have pidfd. Since you just brought this issue again, I want to hear *opinions* from others, again. > > Also, if I may ask, why is the flag argument "unsigned long"? > That's pretty unorthodox. The expectation is that flag arguments are > not word-size dependent and should usually use "unsigned int". All new > system calls follow this pattern too. Nothing special in this flag: Let me change it as "unsigned int". I will send the change once we have an agreement on "pidfd" argument. Thanks for the review, Christian!
On Sat, May 9, 2020 at 4:14 PM Minchan Kim <minchan@kernel.org> wrote: > > Hi Christian, > > On Sat, May 09, 2020 at 02:48:17PM +0200, Christian Brauner wrote: > > On Fri, May 08, 2020 at 04:04:15PM -0700, Andrew Morton wrote: > > > On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <minchan@kernel.org> wrote: > > > > > > > > > > > ... > > > > > > > > Per Vlastimil's request, I changed "which and advise" with "idtype and > > > > advice" in function prototype of description. > > > > Could you replace the part in the description? Code is never changed. > > > > > > > > > > Done, but... > > > > > > > > > > > ... > > > > > > > > There is a demand[1] to support pid as well pidfd for process_madvise to > > > > reduce unnecessary syscall to get pidfd if the user has control of the > > > > target process(ie, they could guarantee the process is not gone or pid is > > > > not reused). > > > > > > > > This patch aims for supporting both options like waitid(2). So, the > > > > syscall is currently, > > > > > > > > int process_madvise(idtype_t idtype, id_t id, void *addr, > > > > size_t length, int advice, unsigned long flags); > > > > > > > > @which is actually idtype_t for userspace libray and currently, it > > > > supports P_PID and P_PIDFD. > > > > > > What does "@which is actually idtype_t for userspace libray" mean? Can > > > you clarify and expand? > > > > If I may clarify, the only case where we've supported both pidfd and pid > > in the same system call is waitid() to avoid adding a dedicated system > > call for waiting and because waitid() already had this (imho insane) > > argument type switching. The idtype_t thing comes from waitid() and is > > located int sys/wait.h and is defined as > > > > "The type idtype_t is defined as an enumeration type whose possible > > values include at least the following: > > > > P_ALL > > P_PID > > P_PGID > > " > > > > int waitid(idtype_t idtype, id_t id, siginfo_t *infop, int options); > > If idtype is P_PID, waitid() shall wait for the child with a process ID equal to (pid_t)id. > > If idtype is P_PGID, waitid() shall wait for any child with a process group ID equal to (pid_t)id. > > If idtype is P_ALL, waitid() shall wait for any children and id is ignored. > > > > I'm personally not a fan of this idtype_t thing and think this should > > just have been > > > > int pidfd_madvise(int pidfd, void *addr, > > > > size_t length, int advice, unsigned long flags); > > and call it a day. > > That was the argument at that time, Daniel and I didn't want to have > pid along with pidfd even though Kirill strongly wanted to have it. > However you said " Overall, I don't particularly care how or if you > integrate pidfd here." at that time. > > https://lore.kernel.org/linux-mm/20200113104256.5ujbplyec2sk4onn@wittgenstein/ > > I asked a question to Kirll at that time. > > " > > Sounds like that you want to support both options for every upcoming API > > which deals with pid. I'm not sure how it's critical for process_madvise > > API this case. In general, we sacrifice some performance for the nicer one > > and later, once it's reported as hurdle for some workload, we could fix it > > via introducing new flag. What I don't like at this moment is to make > > syscall complicated with potential scenarios without real workload. > > Yes, I suggest allowing both options for every new process api > " > https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/ > > You didn't give the opinion at that time, either(I expected you will > make some voice then). What I could do to proceed work was separate it > as different patch like this one to get more attention in future. > And now it works. > > Let me clarify my side: I still don't like to introduce pid for new API > since we have pidfd. Since you just brought this issue again, I want to > hear *opinions* from others, again. IIRC Kirill's main complaint was that if we support only pidfds and userspace has a pid of the process then it would have to convert that pid into pidfd before calling process_madvise, which involves additional syscall(s). The overhead would be more tangible if there are multiple processes needing to be madvised. I'm not sure how often such a need arises to madvise multiple processes in a bulk like that and how critical is the overhead of obtaining pidfd. With pid reuse possibility pid-based API will still have the issue of possibly sending the request to a wrong process, so this pidfd obtaining overhead arguably makes the usage more robust and therefore is not a pure loss. I don't have a real strong opinion against supporting pid in this syscall but I think API maintainers should decide going forward whether new APIs should support pid along with pidfd or switch to pidfd only. Thanks! > > > > > Also, if I may ask, why is the flag argument "unsigned long"? > > That's pretty unorthodox. The expectation is that flag arguments are > > not word-size dependent and should usually use "unsigned int". All new > > system calls follow this pattern too. > > Nothing special in this flag: Let me change it as "unsigned int". > I will send the change once we have an agreement on "pidfd" argument. > > Thanks for the review, Christian!
On Mon, Mar 2, 2020 at 8:36 PM Minchan Kim <minchan@kernel.org> wrote: > 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: [...] > 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: [...] > * 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. Just as a note, since you're mentioning Firefox as a usecase: Memory deduplication between browser renderers creates new side channels and is a questionable idea from a security standpoint. Memory deduplication is (mostly) fine if either all involved processes are trusted or no involved processes contain secrets, but browsers usually run tons of untrusted code while at the same time containing lots of valuable secrets.