Re: [PATCH] mm: make munlock fast when mlock is canceled by sigkill

[KAMEZAWA Hiroyuki]

On Mon, 24 Aug 2009 10:44:41 +0900
Minchan Kim <minchan.kim@xxxxxxxxx> wrote:

> On Sun, Aug 23, 2009 at 1:54 AM, Hiroaki
> Wakabayashi<primulaelatior@xxxxxxxxx> wrote:
> > From 27b2fde0222c59049026e7d0bdc4a2a68d0720f5 Mon Sep 17 00:00:00 2001
> > From: Hiroaki Wakabayashi <primulaelatior@xxxxxxxxx>
> > Date: Sat, 22 Aug 2009 19:14:53 +0900
> > Subject: [PATCH] mm: make munlock fast when mlock is canceled by sigkill
> >
> > This patch is for making commit 4779280d1e (mm: make get_user_pages()
> > interruptible) complete.
> >
> > At first, munlock() assumes that all pages in vma are pinned,
> >
> > Now, by the commit, mlock() can be interrupted by SIGKILL, etc ÂSo, part of
> > pages are not pinned.
> > If SIGKILL, In exit() path, munlock is called for unlocking pinned pages
> > in vma.
> >
> > But, there, get_user_pages(write) is used for munlock(). Then, pages are
> > allocated via page-fault for exsiting process !!! This is problem at canceling
> > big mlock.
> > This patch tries to avoid allocating new pages at munlock().
> >
> > Â mlock( big area )
> > Â Â Â Â<===== sig kill
> > Â do_exit()
> > Â Â->mmput()
> > Â Â Â -> do_munlock()
> > Â Â Â Â -> get_user_pages()
> > Â Â Â Â Â Â Â <allocate *never used* memory>
> > Â Â Â ->.....freeing allocated memory.
> >
> > * Test program
> > % cat run.sh
> > #!/bin/sh
> >
> > ./mlock_test 2000000000 &
> > sleep 2
> > kill -9 $!
> > wait
> >
> > % cat mlock_test.c
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <sys/mman.h>
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > #include <errno.h>
> > #include <time.h>
> > #include <unistd.h>
> > #include <sys/time.h>
> >
> > int main(int argc, char **argv)
> > {
> > Â Â Â Âsize_t length = 50 * 1024 * 1024;
> > Â Â Â Âvoid *addr;
> > Â Â Â Âtime_t timer;
> >
> > Â Â Â Âif (argc >= 2)
> > Â Â Â Â Â Â Â Âlength = strtoul(argv[1], NULL, 10);
> > Â Â Â Âprintf("PID = %d\n", getpid());
> > Â Â Â Âaddr = mmap(NULL, length, PROT_READ | PROT_WRITE,
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂMAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> > Â Â Â Âif (addr == MAP_FAILED) {
> > Â Â Â Â Â Â Â Âfprintf(stderr, "mmap failed: %s, length=%lu\n",
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstrerror(errno), length);
> > Â Â Â Â Â Â Â Âexit(EXIT_FAILURE);
> > Â Â Â Â}
> > Â Â Â Âprintf("try mlock length=%lu\n", length);
> > Â Â Â Âtimer = time(NULL);
> > Â Â Â Âif (mlock(addr, length) < 0) {
> > Â Â Â Â Â Â Â Âfprintf(stderr, "mlock failed: %s, time=%lu[sec]\n",
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstrerror(errno), time(NULL) - timer);
> > Â Â Â Â Â Â Â Âexit(EXIT_FAILURE);
> > Â Â Â Â}
> > Â Â Â Âprintf("mlock succeed, time=%lu[sec]\n\n", time(NULL) - timer);
> > Â Â Â Âprintf("try munlock length=%lu\n", length);
> > Â Â Â Âtimer = time(NULL);
> > Â Â Â Âif (munlock(addr, length) < 0) {
> > Â Â Â Â Â Â Â Âfprintf(stderr, "munlock failed: %s, time=%lu[sec]\n",
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstrerror(errno), time(NULL)-timer);
> > Â Â Â Â Â Â Â Âexit(EXIT_FAILURE);
> > Â Â Â Â}
> > Â Â Â Âprintf("munlock succeed, time=%lu[sec]\n\n", time(NULL) - timer);
> > Â Â Â Âif (munmap(addr, length) < 0) {
> > Â Â Â Â Â Â Â Âfprintf(stderr, "munmap failed: %s\n", strerror(errno));
> > Â Â Â Â Â Â Â Âexit(EXIT_FAILURE);
> > Â Â Â Â}
> > Â Â Â Âreturn 0;
> > }
> >
> > * Executed Result
> > -- Original executed result
> > % time ./run.sh
> >
> > PID = 2678
> > try mlock length=2000000000
> > ./run.sh: line 6: Â2678 Killed         Â./mlock_test 2000000000
> > ./run.sh Â0.00s user 2.59s system 13% cpu 18.781 total
> > %
> >
> > -- After applied this patch
> > % time ./run.sh
> >
> > PID = 2512
> > try mlock length=2000000000
> > ./run.sh: line 6: Â2512 Killed         Â./mlock_test 2000000000
> > ./run.sh Â0.00s user 1.15s system 45% cpu 2.507 total
> > %
> >
> > Signed-off-by: Hiroaki Wakabayashi <primulaelatior@xxxxxxxxx>
> > ---
> > Âmm/internal.h | Â Â1 +
> > Âmm/memory.c  |  Â9 +++++++--
> > Âmm/mlock.c  Â|  35 +++++++++++++++++++----------------
> > Â3 files changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index f290c4d..4ab5b24 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -254,6 +254,7 @@ static inline void
> > mminit_validate_memmodel_limits(unsigned long *start_pfn,
> > Â#define GUP_FLAGS_FORCE Â Â Â Â Â Â Â Â Â0x2
> > Â#define GUP_FLAGS_IGNORE_VMA_PERMISSIONS 0x4
> > Â#define GUP_FLAGS_IGNORE_SIGKILL Â Â Â Â 0x8
> > +#define GUP_FLAGS_ALLOW_NULL Â Â Â Â Â Â 0x10
> >
> 
> I am worried about adding new flag whenever we need it.
> But I think this case makes sense to me.
> In addition, I guess ZERO page can also use this flag.
> 
> Kame. What do you think about it?
> 
I do welcome this !
Then, I don't have to take care of mlock/munlock in ZERO_PAGE patch.
 
And without this patch, munlock() does copy-on-write just for unpinning memory.
So, this patch shows some right direction, I think.

One concern is flag name, ALLOW_NULL sounds not very good.

  GUP_FLAGS_NOFAULT ?

I wonder we can remove a hack of FOLL_ANON for core-dump by this flag, too.

Thanks,
-Kame


> 
> > Âint __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> > Â Â Â Â Â Â Â Â Â Â unsigned long start, int len, int flags,
> > diff --git a/mm/memory.c b/mm/memory.c
> > index aede2ce..b41fbf9 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1217,6 +1217,7 @@ int __get_user_pages(struct task_struct *tsk,
> > struct mm_struct *mm,
> > Â Â Â Âint force = !!(flags & GUP_FLAGS_FORCE);
> > Â Â Â Âint ignore = !!(flags & GUP_FLAGS_IGNORE_VMA_PERMISSIONS);
> > Â Â Â Âint ignore_sigkill = !!(flags & GUP_FLAGS_IGNORE_SIGKILL);
> > + Â Â Â int allow_null = !!(flags & GUP_FLAGS_ALLOW_NULL);
> >
> > Â Â Â Âif (nr_pages <= 0)
> > Â Â Â Â Â Â Â Âreturn 0;
> > @@ -1312,6 +1313,8 @@ int __get_user_pages(struct task_struct *tsk,
> > struct mm_struct *mm,
> > Â Â Â Â Â Â Â Â Â Â Â Âwhile (!(page = follow_page(vma, start, foll_flags))) {
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint ret;
> >
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (allow_null)
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âret = handle_mm_fault(mm, vma, start,
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â(foll_flags & FOLL_WRITE) ?
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂFAULT_FLAG_WRITE : 0);
> > @@ -1351,8 +1354,10 @@ int __get_user_pages(struct task_struct *tsk,
> > struct mm_struct *mm,
> > Â Â Â Â Â Â Â Â Â Â Â Âif (pages) {
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âpages[i] = page;
> >
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â flush_anon_page(vma, page, start);
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â flush_dcache_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (page) {
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â flush_anon_page(vma, page, start);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â flush_dcache_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
> > Â Â Â Â Â Â Â Â Â Â Â Â}
> > Â Â Â Â Â Â Â Â Â Â Â Âif (vmas)
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âvmas[i] = vma;
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index 45eb650..0f5827b 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -178,9 +178,10 @@ static long __mlock_vma_pages_range(struct
> > vm_area_struct *vma,
> > Â Â Â Â */
> > Â Â Â Âif (!mlock)
> > Â Â Â Â Â Â Â Âgup_flags |= GUP_FLAGS_IGNORE_VMA_PERMISSIONS |
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â ÂGUP_FLAGS_IGNORE_SIGKILL;
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂGUP_FLAGS_IGNORE_SIGKILL |
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂGUP_FLAGS_ALLOW_NULL;
> >
> > - Â Â Â if (vma->vm_flags & VM_WRITE)
> > + Â Â Â if (mlock && (vma->vm_flags & VM_WRITE))
> > Â Â Â Â Â Â Â Âgup_flags |= GUP_FLAGS_WRITE;
> >
> > Â Â Â Âwhile (nr_pages > 0) {
> > @@ -220,21 +221,23 @@ static long __mlock_vma_pages_range(struct
> > vm_area_struct *vma,
> > Â Â Â Â Â Â Â Âfor (i = 0; i < ret; i++) {
> > Â Â Â Â Â Â Â Â Â Â Â Âstruct page *page = pages[i];
> >
> > - Â Â Â Â Â Â Â Â Â Â Â lock_page(page);
> > - Â Â Â Â Â Â Â Â Â Â Â /*
> > - Â Â Â Â Â Â Â Â Â Â Â Â* Because we lock page here and migration is blocked
> > - Â Â Â Â Â Â Â Â Â Â Â Â* by the elevated reference, we need only check for
> > - Â Â Â Â Â Â Â Â Â Â Â Â* page truncation (file-cache only).
> > - Â Â Â Â Â Â Â Â Â Â Â Â*/
> > - Â Â Â Â Â Â Â Â Â Â Â if (page->mapping) {
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (mlock)
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â mlock_vma_page(page);
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â else
> > - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â munlock_vma_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â if (page) {
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â lock_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /*
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* Because we lock page here and migration is
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* blocked by the elevated reference, we need
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* only check for page truncation
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* (file-cache only).
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â*/
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (page->mapping) {
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (mlock)
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â mlock_vma_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â else
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â munlock_vma_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â unlock_page(page);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â put_page(page); /* ref from get_user_pages() */
> > Â Â Â Â Â Â Â Â Â Â Â Â}
> > - Â Â Â Â Â Â Â Â Â Â Â unlock_page(page);
> > - Â Â Â Â Â Â Â Â Â Â Â put_page(page); Â Â Â Â /* ref from get_user_pages() */
> > -
> > Â Â Â Â Â Â Â Â Â Â Â Â/*
> > Â Â Â Â Â Â Â Â Â Â Â Â * here we assume that get_user_pages() has given us
> > Â Â Â Â Â Â Â Â Â Â Â Â * a list of virtually contiguous pages.
> > --
> > 1.5.6.5
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at Âhttp://www.tux.org/lkml/
> >
> 
> 
> 
> -- 
> Kind regards,
> Minchan Kim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/