Message ID | 20150220132747.GV3087@suse.de |
---|---|
State | New |
Headers | show |
On Fri, Feb 20, 2015 at 01:27:47PM +0000, Mel Gorman wrote: > 2015-02-10 Mel Gorman <mgorman@suse.de> > > [BZ #17195] > * malloc/arena.c (free): Apply trim threshold to per-thread heaps > as well as the main arena. > --- > malloc/arena.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/malloc/arena.c b/malloc/arena.c > index 886defb074a2..fec339819309 100644 > --- a/malloc/arena.c > +++ b/malloc/arena.c > @@ -696,7 +696,7 @@ heap_trim (heap_info *heap, size_t pad) > } > top_size = chunksize (top_chunk); > extra = (top_size - pad - MINSIZE - 1) & ~(pagesz - 1); > - if (extra < (long) pagesz) > + if ((unsigned long) extra < mp_.trim_threshold) > return 0; > > /* Try to shrink. */ Looks like this exposes some kind of a race. I ran the testsuite with this patch and intl/tst-gettext4 and intl/tst-gettext5 fail with a segfault. From a quick peek it looks like a malloc trying to tinker with the part of an arena that has not yet been allocated, i.e. does not have any permissions. Can you please look into it? I can't push this till that problem is solved. Thanks, Siddhesh
On Tue, Feb 24, 2015 at 12:31:18PM +0530, Siddhesh Poyarekar wrote: > On Fri, Feb 20, 2015 at 01:27:47PM +0000, Mel Gorman wrote: > > 2015-02-10 Mel Gorman <mgorman@suse.de> > > > > [BZ #17195] > > * malloc/arena.c (free): Apply trim threshold to per-thread heaps > > as well as the main arena. > > --- > > malloc/arena.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/malloc/arena.c b/malloc/arena.c > > index 886defb074a2..fec339819309 100644 > > --- a/malloc/arena.c > > +++ b/malloc/arena.c > > @@ -696,7 +696,7 @@ heap_trim (heap_info *heap, size_t pad) > > } > > top_size = chunksize (top_chunk); > > extra = (top_size - pad - MINSIZE - 1) & ~(pagesz - 1); > > - if (extra < (long) pagesz) > > + if ((unsigned long) extra < mp_.trim_threshold) > > return 0; > > > > /* Try to shrink. */ > > Looks like this exposes some kind of a race. I ran the testsuite with > this patch and intl/tst-gettext4 and intl/tst-gettext5 fail with a > segfault. From a quick peek it looks like a malloc trying to tinker > with the part of an arena that has not yet been allocated, i.e. does > not have any permissions. Can you please look into it? I can't push > this till that problem is solved. > Ok, I'll start taking a look. I have yet to properly investigate but I do think this is an existing problem that the patch makes worse because I find this # This is a test run using the glibc installed on the system mel@stampy:~/git-public/glibc/obj-baseline > ./intl/tst-gettext4 beauty thread 1 call 1 returned: beauty beauty thread 2 call 1 returned: beauty beauty thread 1 call 2 returned: beauty beauty thread 2 call 2 returned: beauty # This is a test run using glibc 2.21 mel@stampy:~/git-public/glibc/obj-baseline > ./testrun.sh ./intl/tst-gettext4 Schᅵnheit beautᅵ Schᅵnheit beautᅵ # This is a test run using glibc 2.21 + the patch mel@stampy:~/git-public/glibc/obj-madvise-reduce-v6r1 > ./testrun.sh ./intl/tst-gettext4 Segmentation fault (core dumped) glibc 2.21 does not segfault but the output from glibc 2.21 looks pretty screwed up relative to glibc 2.19 that is installed on the system.
Joseph, there is a problem with one of your commits since glibc 2.20. On Tue, Feb 24, 2015 at 10:47:12AM +0000, Mel Gorman wrote: > On Tue, Feb 24, 2015 at 12:31:18PM +0530, Siddhesh Poyarekar wrote: > > On Fri, Feb 20, 2015 at 01:27:47PM +0000, Mel Gorman wrote: > > > 2015-02-10 Mel Gorman <mgorman@suse.de> > > > > > > [BZ #17195] > > > * malloc/arena.c (free): Apply trim threshold to per-thread heaps > > > as well as the main arena. > > > --- > > > malloc/arena.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/malloc/arena.c b/malloc/arena.c > > > index 886defb074a2..fec339819309 100644 > > > --- a/malloc/arena.c > > > +++ b/malloc/arena.c > > > @@ -696,7 +696,7 @@ heap_trim (heap_info *heap, size_t pad) > > > } > > > top_size = chunksize (top_chunk); > > > extra = (top_size - pad - MINSIZE - 1) & ~(pagesz - 1); > > > - if (extra < (long) pagesz) > > > + if ((unsigned long) extra < mp_.trim_threshold) > > > return 0; > > > > > > /* Try to shrink. */ > > > > Looks like this exposes some kind of a race. I ran the testsuite with > > this patch and intl/tst-gettext4 and intl/tst-gettext5 fail with a > > segfault. From a quick peek it looks like a malloc trying to tinker > > with the part of an arena that has not yet been allocated, i.e. does > > not have any permissions. Can you please look into it? I can't push > > this till that problem is solved. > > > > Ok, I'll start taking a look. I have yet to properly investigate but I > do think this is an existing problem that the patch makes worse because > I find this > > # This is a test run using the glibc installed on the system > mel@stampy:~/git-public/glibc/obj-baseline > ./intl/tst-gettext4 > beauty > thread 1 call 1 returned: beauty > beauty > thread 2 call 1 returned: beauty > beauty > thread 1 call 2 returned: beauty > beauty > thread 2 call 2 returned: beauty > > # This is a test run using glibc 2.21 > mel@stampy:~/git-public/glibc/obj-baseline > ./testrun.sh ./intl/tst-gettext4 > Schᅵnheit > beautᅵ > Schᅵnheit > beautᅵ > I bisected this down to commit 8540f6d2a74 ("Don't require test wrappers to preserve environment variables, use more consistent environment."). The test case does not crash but it's clearly corrupted. Before that commit we have mel@stampy:~/git-public/glibc > rm -rf obj-verify; git checkout 8540f6d2a74fe9d67440535ebbcfa252180a3172^ | head; mkdir obj-verify; cd obj-verify; ../configure --prefix=/tmp/blah > /dev/null 2>&1 && make -j8 > /dev/null 2>&1 && make -j8 tests > /dev/null 2>&1 ; ./testrun.sh > ./intl/tst-gettext4; cd .. HEAD is now at ed36bfa18faf... PowerPC: Fix optimized strncat strlen call beauty thread 1 call 1 returned: beauty beauty thread 2 call 1 returned: beauty beauty thread 1 call 2 returned: beauty beauty thread 2 call 2 returned: beauty And on the commit we get mel@stampy:~/git-public/glibc > rm -rf obj-verify; git checkout 8540f6d2a74fe9d67440535ebbcfa252180a3172 | head; mkdir obj-verify; cd obj-verify; ../configure --prefix=/tmp/blah > /dev/null 2>&1 && make -j8 > /dev/null 2>&1 && make -j8 tests > /dev/null 2>&1 ; ./testrun.sh > ./intl/tst-gettext4; cd .. Previous HEAD position was ed36bfa18faf... PowerPC: Fix optimized strncat strlen call HEAD is now at 8540f6d2a74f... Don't require test wrappers to preserve environment variables, use more consistent environment. Schᅵnheit beautᅵ Schᅵnheit beautᅵ
On Tue, 24 Feb 2015, Mel Gorman wrote: > The test case does not crash but it's clearly corrupted. > > Before that commit we have > > mel@stampy:~/git-public/glibc > rm -rf obj-verify; git checkout 8540f6d2a74fe9d67440535ebbcfa252180a3172^ | head; mkdir obj-verify; cd obj-verify; ../configure --prefix=/tmp/blah > /dev/null 2>&1 && make -j8 > /dev/null 2>&1 && make -j8 tests > /dev/null 2>&1 ; ./testrun.sh > ./intl/tst-gettext4; cd .. > HEAD is now at ed36bfa18faf... PowerPC: Fix optimized strncat strlen call > beauty > thread 1 call 1 returned: beauty > beauty > thread 2 call 1 returned: beauty > beauty > thread 1 call 2 returned: beauty > beauty > thread 2 call 2 returned: beauty This is the output when the test fails; "thread 1 call 1 returned" etc. are failure messages. Before my commit, testrun.sh was not by itself a correct way of running this test, because it lacked the environment setup required by this test; that environment setup was local to the tst-gettext4.sh script. > And on the commit we get > mel@stampy:~/git-public/glibc > rm -rf obj-verify; git checkout 8540f6d2a74fe9d67440535ebbcfa252180a3172 | head; mkdir obj-verify; cd obj-verify; ../configure --prefix=/tmp/blah > /dev/null 2>&1 && make -j8 > /dev/null 2>&1 && make -j8 tests > /dev/null 2>&1 ; ./testrun.sh > ./intl/tst-gettext4; cd .. > Previous HEAD position was ed36bfa18faf... PowerPC: Fix optimized strncat strlen call > HEAD is now at 8540f6d2a74f... Don't require test wrappers to preserve environment variables, use more consistent environment. > Schᅵnheit > beautᅵ > Schᅵnheit > beautᅵ This is passing output (modulo a few character set conversions after the test produced its output in ISO-8859-1, but the test clearly worked because it didn't produce any of the "returned" messages from failed comparisons). Because the environment setup is now shared for all tests in the makefiles, after my commit testrun.sh is suitable to use to run this test, and so it runs with the correct passing output.
On Tue, Feb 24, 2015 at 02:35:58PM +0000, Joseph Myers wrote: > On Tue, 24 Feb 2015, Mel Gorman wrote: > > > The test case does not crash but it's clearly corrupted. > > > > Before that commit we have > > > > mel@stampy:~/git-public/glibc > rm -rf obj-verify; git checkout 8540f6d2a74fe9d67440535ebbcfa252180a3172^ | head; mkdir obj-verify; cd obj-verify; ../configure --prefix=/tmp/blah > /dev/null 2>&1 && make -j8 > /dev/null 2>&1 && make -j8 tests > /dev/null 2>&1 ; ./testrun.sh > ./intl/tst-gettext4; cd .. > > HEAD is now at ed36bfa18faf... PowerPC: Fix optimized strncat strlen call > > beauty > > thread 1 call 1 returned: beauty > > beauty > > thread 2 call 1 returned: beauty > > beauty > > thread 1 call 2 returned: beauty > > beauty > > thread 2 call 2 returned: beauty > > This is the output when the test fails; "thread 1 call 1 returned" etc. > are failure messages. Before my commit, testrun.sh was not by itself a > correct way of running this test, because it lacked the environment setup > required by this test; that environment setup was local to the > tst-gettext4.sh script. > Sorry for the noise. The actual problem is my patch due to the fact that extra can go negative.
diff --git a/malloc/arena.c b/malloc/arena.c index 886defb074a2..fec339819309 100644 --- a/malloc/arena.c +++ b/malloc/arena.c @@ -696,7 +696,7 @@ heap_trim (heap_info *heap, size_t pad) } top_size = chunksize (top_chunk); extra = (top_size - pad - MINSIZE - 1) & ~(pagesz - 1); - if (extra < (long) pagesz) + if ((unsigned long) extra < mp_.trim_threshold) return 0; /* Try to shrink. */