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

[Minchan Kim]

On Mon, Aug 24, 2009 at 10:51 AM, KAMEZAWA
Hiroyuki<kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> 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.

That's a good point.
It can remove little cache footprint and
unnecessary calls[flush_xxx_page in GUP].

-- 
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/