Cyrill,
Gimp is broken due to a kernel bug included in 3.12. It cannot open
large files without failing memory allocations due to exceeding
vm.max_map_count. The relevant bugzilla entries are
https://bugzilla.kernel.org/show_bug.cgi?id=67651
https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
They include details on how to reproduce the issue. In my case, a
failure shows messages like this
(gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
(file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
xinit: connection to X server lost
waiting for X server to shut down
/usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
/usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
/usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
X-related junk is there was because I was using a headless server and
xinit directly to launch gimp to reproduce the bug.
Automated bisection using mmtests (https://github.com/gormanm/mmtests)
and the configuration file configs/config-global-dhp__gimp-simple (needs
local web server with a copy of the image file) identified the following
commit. Test case was simple -- try and open the large file described in
the bug. I did not investigate the patch itself as I'm just reporting
the results of the bisection. If I had to guess, I'd say that VMA
merging has been affected.
d9104d1ca9662498339c0de975b4666c30485f4e is the first bad commit
commit d9104d1ca9662498339c0de975b4666c30485f4e
Author: Cyrill Gorcunov <[email protected]>
Date: Wed Sep 11 14:22:24 2013 -0700
mm: track vma changes with VM_SOFTDIRTY bit
Pavel reported that in case if vma area get unmapped and then mapped (or
expanded) in-place, the soft dirty tracker won't be able to recognize this
situation since it works on pte level and ptes are get zapped on unmap,
loosing soft dirty bit of course.
So to resolve this situation we need to track actions on vma level, there
VM_SOFTDIRTY flag comes in. When new vma area created (or old expanded)
we set this bit, and keep it here until application calls for clearing
soft dirty bit.
Thus when user space application track memory changes now it can detect if
vma area is renewed.
Reported-by: Pavel Emelyanov <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Matt Mackall <[email protected]>
Cc: Xiao Guangrong <[email protected]>
Cc: Marcelo Tosatti <[email protected]>
Cc: KOSAKI Motohiro <[email protected]>
Cc: Stephen Rothwell <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
Cc: Rob Landley <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
--
Mel Gorman
SUSE Labs
On Wed, Jan 22, 2014 at 07:08:16PM +0000, Mel Gorman wrote:
> Cyrill,
>
> Gimp is broken due to a kernel bug included in 3.12. It cannot open
> large files without failing memory allocations due to exceeding
> vm.max_map_count. The relevant bugzilla entries are
>
> https://bugzilla.kernel.org/show_bug.cgi?id=67651
> https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
>
> They include details on how to reproduce the issue. In my case, a
> failure shows messages like this
>
> (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
>
> (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
> xinit: connection to X server lost
>
> waiting for X server to shut down
> /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
> /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
> /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
>
> X-related junk is there was because I was using a headless server and
> xinit directly to launch gimp to reproduce the bug.
>
> Automated bisection using mmtests (https://github.com/gormanm/mmtests)
> and the configuration file configs/config-global-dhp__gimp-simple (needs
> local web server with a copy of the image file) identified the following
> commit. Test case was simple -- try and open the large file described in
> the bug. I did not investigate the patch itself as I'm just reporting
> the results of the bisection. If I had to guess, I'd say that VMA
> merging has been affected.
Thanks a lot for report, Mel! I'm investigating...
On Wed, 22 Jan 2014 19:08:16 +0000 Mel Gorman <[email protected]> wrote:
> X-related junk is there was because I was using a headless server and
> xinit directly to launch gimp to reproduce the bug.
I've never done this. Can you share the magic recipe for running an X
app in this way?
Thanks.
On Wed, Jan 22, 2014 at 11:19:28PM +0400, Cyrill Gorcunov wrote:
> > commit. Test case was simple -- try and open the large file described in
> > the bug. I did not investigate the patch itself as I'm just reporting
> > the results of the bisection. If I had to guess, I'd say that VMA
> > merging has been affected.
>
> Thanks a lot for report, Mel! I'm investigating...
Mel, here is a quick fix for bring merging back (just in case if you
have a minute to test it and confirm the merging were affected). It
seems I've lost setting up vma-softdirty bit somewhere and procedure
which tests vma flags mathcing fails, will continue investigating/testing
tomorrow.
---
mm/mmap.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Index: linux-2.6.git/mm/mmap.c
===================================================================
--- linux-2.6.git.orig/mm/mmap.c
+++ linux-2.6.git/mm/mmap.c
@@ -893,8 +893,18 @@ again: remove_next = 1 + (end > next->
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags)
{
+ /*
+ * VM_SOFTDIRTY should not prevent from VMA merging, if we
+ * match the flags but dirty bit -- just mark merged one as
+ * a dirty then.
+ */
+#ifdef CONFIG_MEM_SOFT_DIRTY
+ if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
+ return 0;
+#else
if (vma->vm_flags ^ vm_flags)
return 0;
+#endif
if (vma->vm_file != file)
return 0;
if (vma->vm_ops && vma->vm_ops->close)
@@ -1082,7 +1092,11 @@ static int anon_vma_compatible(struct vm
return a->vm_end == b->vm_start &&
mpol_equal(vma_policy(a), vma_policy(b)) &&
a->vm_file == b->vm_file &&
+#ifdef CONFIG_MEM_SOFT_DIRTY
+ !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
+#else
!((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
+#endif
b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
}
On 01/22/2014 11:08 AM, Mel Gorman wrote:
> Cyrill,
>
> Gimp is broken due to a kernel bug included in 3.12. It cannot open
> large files without failing memory allocations due to exceeding
> vm.max_map_count. The relevant bugzilla entries are
>
> https://bugzilla.kernel.org/show_bug.cgi?id=67651
> https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
>
> They include details on how to reproduce the issue. In my case, a
> failure shows messages like this
>
> (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
>
> (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
> xinit: connection to X server lost
>
> waiting for X server to shut down
> /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
> /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
> /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
>
> X-related junk is there was because I was using a headless server and
> xinit directly to launch gimp to reproduce the bug.
>
> Automated bisection using mmtests (https://github.com/gormanm/mmtests)
> and the configuration file configs/config-global-dhp__gimp-simple (needs
> local web server with a copy of the image file) identified the following
> commit. Test case was simple -- try and open the large file described in
> the bug. I did not investigate the patch itself as I'm just reporting
> the results of the bisection. If I had to guess, I'd say that VMA
> merging has been affected.
>
> d9104d1ca9662498339c0de975b4666c30485f4e is the first bad commit
> commit d9104d1ca9662498339c0de975b4666c30485f4e
> Author: Cyrill Gorcunov <[email protected]>
> Date: Wed Sep 11 14:22:24 2013 -0700
>
> mm: track vma changes with VM_SOFTDIRTY bit
>
> Pavel reported that in case if vma area get unmapped and then mapped (or
> expanded) in-place, the soft dirty tracker won't be able to recognize this
> situation since it works on pte level and ptes are get zapped on unmap,
> loosing soft dirty bit of course.
>
> So to resolve this situation we need to track actions on vma level, there
> VM_SOFTDIRTY flag comes in. When new vma area created (or old expanded)
> we set this bit, and keep it here until application calls for clearing
> soft dirty bit.
>
> Thus when user space application track memory changes now it can detect if
> vma area is renewed.
Presumably some path is failing to set VM_SOFTDIRTY, thus preventing mms
from being merged.
That being said, this could cause vma blowups for programs that are
actually using this thing.
--Andy
On Wed, Jan 22, 2014 at 02:45:53PM -0800, Andy Lutomirski wrote:
> >
> > Thus when user space application track memory changes now it can detect if
> > vma area is renewed.
>
> Presumably some path is failing to set VM_SOFTDIRTY, thus preventing mms
> from being merged.
>
> That being said, this could cause vma blowups for programs that are
> actually using this thing.
Hi Andy, indeed, this could happen. The easiest way is to ignore softdirty bit
when we're trying to merge vmas and set it one new merged. I think this should
be correct. Once I finish I'll send the patch.
Cyrill
On Thu, 23 Jan 2014 09:59:06 +0400 Cyrill Gorcunov <[email protected]> wrote:
> On Wed, Jan 22, 2014 at 02:45:53PM -0800, Andy Lutomirski wrote:
> > >
> > > Thus when user space application track memory changes now it can detect if
> > > vma area is renewed.
> >
> > Presumably some path is failing to set VM_SOFTDIRTY, thus preventing mms
> > from being merged.
> >
> > That being said, this could cause vma blowups for programs that are
> > actually using this thing.
>
> Hi Andy, indeed, this could happen. The easiest way is to ignore softdirty bit
> when we're trying to merge vmas and set it one new merged. I think this should
> be correct. Once I finish I'll send the patch.
Hang on. We think the problem is that gimp is generating vmas which
*should* be merged, but for unknown reasons they differ in
VM_SOFTDIRTY, yes?
Shouldn't we work out where we're forgetting to set VM_SOFTDIRTY?
Putting bandaids over this error when we come to trying to merge the
vmas sounds very wrong?
On Wed, Jan 22, 2014 at 10:09:10PM -0800, Andrew Morton wrote:
> > >
> > > That being said, this could cause vma blowups for programs that are
> > > actually using this thing.
> >
> > Hi Andy, indeed, this could happen. The easiest way is to ignore softdirty bit
> > when we're trying to merge vmas and set it one new merged. I think this should
> > be correct. Once I finish I'll send the patch.
>
> Hang on. We think the problem is that gimp is generating vmas which
> *should* be merged, but for unknown reasons they differ in
> VM_SOFTDIRTY, yes?
Yes. One place where I forgot to set softdirty bit is setup_arg_pages. But
it called once on elf load, so it can't cause such effect (but should be
fixed too). Also there is do_brk where vmasoftdirty is missed too :/
Another problem is the potential scenario when we have a bunch of vmas
and clear vma-softdirty bit on them, then we try to map new one, flags
won't match and instead of extending old vma the new one will be created.
I think (if only I'm not missing something) that vma-softdirty should
be ignored in such case (ie inside is_mergeable_vma) and once vma extended
it should be marked as dirty one. Again, I need to think and test more.
> Shouldn't we work out where we're forgetting to set VM_SOFTDIRTY?
> Putting bandaids over this error when we come to trying to merge the
> vmas sounds very wrong?
I'm looking into this as well.
Cyrill
On Wed, Jan 22, 2014 at 11:52:15AM -0800, Andrew Morton wrote:
> On Wed, 22 Jan 2014 19:08:16 +0000 Mel Gorman <[email protected]> wrote:
>
> > X-related junk is there was because I was using a headless server and
> > xinit directly to launch gimp to reproduce the bug.
>
> I've never done this. Can you share the magic recipe for running an X
> app in this way?
>
The relevant part of the test script is
# Build a wrapper script to launch gimp
cat > gimp-launch.sh << EOF
/usr/bin/gimp -i -b "(mmtests-open-image \"$FILENAME\")" -b "(gimp-quit 0)" > $LOGDIR_RESULTS/gimp-out.1 2>&1
echo \$? > gimp-exit-code
EOF
chmod u+x gimp-launch.sh
$TIME_CMD xinit ./gimp-launch.sh 2> $LOGDIR_RESULTS/time.1
RETVAL=`cat gimp-exit-code`
It's clumsy because the application would start with no window manager
and looking at it again, it probably was not even necessary because of
the -i switch in gimp.
Previously when I needed to automate an X app I configured the machine to
login automatically, exported the DISPLAY variable in the test script and
used wmctrl to detect if an application had a window displayed yet.
--
Mel Gorman
SUSE Labs
On Thu, Jan 23, 2014 at 02:33:25AM +0400, Cyrill Gorcunov wrote:
> On Wed, Jan 22, 2014 at 11:19:28PM +0400, Cyrill Gorcunov wrote:
> > > commit. Test case was simple -- try and open the large file described in
> > > the bug. I did not investigate the patch itself as I'm just reporting
> > > the results of the bisection. If I had to guess, I'd say that VMA
> > > merging has been affected.
> >
> > Thanks a lot for report, Mel! I'm investigating...
>
> Mel, here is a quick fix for bring merging back (just in case if you
> have a minute to test it and confirm the merging were affected). It
> seems I've lost setting up vma-softdirty bit somewhere and procedure
> which tests vma flags mathcing fails, will continue investigating/testing
> tomorrow.
The test case passes with this patch applied to 3.13 so that appears
to confirm that this is related to VM_SOFTDIRTY preventing merges.
Unfortunately I did not have slabinfo tracking enabled to double check
the number of vm_area_structs in teh system.
--
Mel Gorman
SUSE Labs
On Thu, Jan 23, 2014 at 02:33:25AM +0400, Cyrill Gorcunov wrote:
> On Wed, Jan 22, 2014 at 11:19:28PM +0400, Cyrill Gorcunov wrote:
> > > commit. Test case was simple -- try and open the large file described in
> > > the bug. I did not investigate the patch itself as I'm just reporting
> > > the results of the bisection. If I had to guess, I'd say that VMA
> > > merging has been affected.
> >
> > Thanks a lot for report, Mel! I'm investigating...
>
> Mel, here is a quick fix for bring merging back (just in case if you
> have a minute to test it and confirm the merging were affected). It
> seems I've lost setting up vma-softdirty bit somewhere and procedure
> which tests vma flags mathcing fails, will continue investigating/testing
> tomorrow.
The test case passes with this patch applied to 3.13 so that appears
to confirm that this is related to VM_SOFTDIRTY preventing merges.
Unfortunately I did not have slabinfo tracking enabled to double check
the number of vm_area_structs in teh system.
--
Mel Gorman
SUSE Labs
On Thu, Jan 23, 2014 at 09:55:41AM +0000, Mel Gorman wrote:
> On Thu, Jan 23, 2014 at 02:33:25AM +0400, Cyrill Gorcunov wrote:
> > On Wed, Jan 22, 2014 at 11:19:28PM +0400, Cyrill Gorcunov wrote:
> > > > commit. Test case was simple -- try and open the large file described in
> > > > the bug. I did not investigate the patch itself as I'm just reporting
> > > > the results of the bisection. If I had to guess, I'd say that VMA
> > > > merging has been affected.
> > >
> > > Thanks a lot for report, Mel! I'm investigating...
> >
> > Mel, here is a quick fix for bring merging back (just in case if you
> > have a minute to test it and confirm the merging were affected). It
> > seems I've lost setting up vma-softdirty bit somewhere and procedure
> > which tests vma flags mathcing fails, will continue investigating/testing
> > tomorrow.
>
> The test case passes with this patch applied to 3.13 so that appears
> to confirm that this is related to VM_SOFTDIRTY preventing merges.
> Unfortunately I did not have slabinfo tracking enabled to double check
> the number of vm_area_structs in teh system.
Thanks a lot, Mel! I'm testing the patch as well (manually though :).
I'll send the final fix today.
On Thu, Jan 23, 2014 at 02:36:06PM +0400, Cyrill Gorcunov wrote:
> > The test case passes with this patch applied to 3.13 so that appears
> > to confirm that this is related to VM_SOFTDIRTY preventing merges.
> > Unfortunately I did not have slabinfo tracking enabled to double check
> > the number of vm_area_structs in teh system.
>
> Thanks a lot, Mel! I'm testing the patch as well (manually though :).
> I'll send the final fix today.
The patch below should fix the problem. I would really appreaciate
some additional testing.
---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging
VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
bits in vm_flags matched except dirty bit the kernel can't longer
merge them and this forces the kernel to generate new VMAs instead.
It finally may lead to the situation when userspace application
reaches vm.max_map_count limit and get crashed in worse case
| (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
|
| (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
| xinit: connection to X server lost
|
| waiting for X server to shut down
| /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
https://bugzilla.kernel.org/show_bug.cgi?id=67651
https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
Initial problem came from missed VM_SOFTDIRTY in do_brk() routine
but even if we would set up VM_SOFTDIRTY here, there is still a way to
prevent VMAs from merging: one can call
| echo 4 > /proc/$PID/clear_refs
and clear all VM_SOFTDIRTY over all VMAs presented in memory map,
then new do_brk() will try to extend old VMA and finds that dirty
bit doesn't match thus new VMA will be generated.
As discussed to Pavel, the right approach should be to ignore
VM_SOFTDIRTY bit when we're trying to merge VMAs and if merged
successed we mark extended VMA with dirty bit.
Reported-by: Mel Gorman <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Andrew Morton <[email protected]>
---
mm/mmap.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
Index: linux-2.6.git/mm/mmap.c
===================================================================
--- linux-2.6.git.orig/mm/mmap.c
+++ linux-2.6.git/mm/mmap.c
@@ -893,7 +893,15 @@ again: remove_next = 1 + (end > next->
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags)
{
- if (vma->vm_flags ^ vm_flags)
+ /*
+ * VM_SOFTDIRTY should not prevent from VMA merging, if we
+ * match the flags but dirty bit -- the caller should mark
+ * merged VMA as dirty. If dirty bit won't be excluded from
+ * comparison, we increase pressue on the memory system forcing
+ * the kernel to generate new VMAs when old one could be extended
+ * instead.
+ */
+ if ((vma->vm_flags ^ vm_flags) & VM_SOFTDIRTY)
return 0;
if (vma->vm_file != file)
return 0;
@@ -1038,6 +1046,8 @@ struct vm_area_struct *vma_merge(struct
end, prev->vm_pgoff, NULL);
if (err)
return NULL;
+ else
+ prev->vm_flags |= VM_SOFTDIRTY;
khugepaged_enter_vma_merge(prev);
return prev;
}
@@ -1057,6 +1067,8 @@ struct vm_area_struct *vma_merge(struct
next->vm_pgoff - pglen, NULL);
if (err)
return NULL;
+ else
+ area->vm_flags |= VM_SOFTDIRTY;
khugepaged_enter_vma_merge(area);
return area;
}
@@ -1082,7 +1094,7 @@ static int anon_vma_compatible(struct vm
return a->vm_end == b->vm_start &&
mpol_equal(vma_policy(a), vma_policy(b)) &&
a->vm_file == b->vm_file &&
- !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
+ !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
}
On Thu, Jan 23, 2014 at 04:15:55PM +0400, Cyrill Gorcunov wrote:
> On Thu, Jan 23, 2014 at 02:36:06PM +0400, Cyrill Gorcunov wrote:
> > > The test case passes with this patch applied to 3.13 so that appears
> > > to confirm that this is related to VM_SOFTDIRTY preventing merges.
> > > Unfortunately I did not have slabinfo tracking enabled to double check
> > > the number of vm_area_structs in teh system.
> >
> > Thanks a lot, Mel! I'm testing the patch as well (manually though :).
> > I'll send the final fix today.
>
> The patch below should fix the problem. I would really appreaciate
> some additional testing.
Forgot to refresh the patch, sorry.
---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging
VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
bits in vm_flags matched except dirty bit the kernel can't longer
merge them and this forces the kernel to generate new VMAs instead.
It finally may lead to the situation when userspace application
reaches vm.max_map_count limit and get crashed in worse case
| (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
|
| (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
| xinit: connection to X server lost
|
| waiting for X server to shut down
| /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
https://bugzilla.kernel.org/show_bug.cgi?id=67651
https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
Initial problem came from missed VM_SOFTDIRTY in do_brk() routine
but even if we would set up VM_SOFTDIRTY here, there is still a way to
prevent VMAs from merging: one can call
| echo 4 > /proc/$PID/clear_refs
and clear all VM_SOFTDIRTY over all VMAs presented in memory map,
then new do_brk() will try to extend old VMA and finds that dirty
bit doesn't match thus new VMA will be generated.
As discussed to Pavel, the right approach should be to ignore
VM_SOFTDIRTY bit when we're trying to merge VMAs and if merged
successed we mark extended VMA with dirty bit.
Reported-by: Mel Gorman <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Andrew Morton <[email protected]>
---
mm/mmap.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
Index: linux-2.6.git/mm/mmap.c
===================================================================
--- linux-2.6.git.orig/mm/mmap.c
+++ linux-2.6.git/mm/mmap.c
@@ -893,7 +893,15 @@ again: remove_next = 1 + (end > next->
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags)
{
- if (vma->vm_flags ^ vm_flags)
+ /*
+ * VM_SOFTDIRTY should not prevent from VMA merging, if we
+ * match the flags but dirty bit -- the caller should mark
+ * merged VMA as dirty. If dirty bit won't be excluded from
+ * comparison, we increase pressue on the memory system forcing
+ * the kernel to generate new VMAs when old one could be extended
+ * instead.
+ */
+ if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
return 0;
if (vma->vm_file != file)
return 0;
@@ -1038,6 +1046,8 @@ struct vm_area_struct *vma_merge(struct
end, prev->vm_pgoff, NULL);
if (err)
return NULL;
+ else
+ prev->vm_flags |= VM_SOFTDIRTY;
khugepaged_enter_vma_merge(prev);
return prev;
}
@@ -1057,6 +1067,8 @@ struct vm_area_struct *vma_merge(struct
next->vm_pgoff - pglen, NULL);
if (err)
return NULL;
+ else
+ area->vm_flags |= VM_SOFTDIRTY;
khugepaged_enter_vma_merge(area);
return area;
}
@@ -1082,7 +1094,7 @@ static int anon_vma_compatible(struct vm
return a->vm_end == b->vm_start &&
mpol_equal(vma_policy(a), vma_policy(b)) &&
a->vm_file == b->vm_file &&
- !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
+ !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
}
On Thu, Jan 23, 2014 at 04:55:43PM +0400, Cyrill Gorcunov wrote:
> On Thu, Jan 23, 2014 at 04:15:55PM +0400, Cyrill Gorcunov wrote:
> > >
> > > Thanks a lot, Mel! I'm testing the patch as well (manually though :).
> > > I'll send the final fix today.
> >
> > The patch below should fix the problem. I would really appreaciate
> > some additional testing.
>
> Forgot to refresh the patch, sorry.
> ---
I think setting up dirty bit inside vma_merge() body is a big hammer
which should not be used, but it's up to caller of vma_merge() to figure
out if dirty bit should be set or not if merge successed. Thus softdirty
vma bit should be (and it already is) set at the end of mmap_region and do_brk
routines. So patch could be simplified (below). Pavel, what do you think?
---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
bits in vm_flags matched except dirty bit the kernel can't longer
merge them and this forces the kernel to generate new VMAs instead.
It finally may lead to the situation when userspace application
reaches vm.max_map_count limit and get crashed in worse case
| (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
|
| (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
| xinit: connection to X server lost
|
| waiting for X server to shut down
| /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
| /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
https://bugzilla.kernel.org/show_bug.cgi?id=67651
https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
Initial problem came from missed VM_SOFTDIRTY in do_brk() routine
but even if we would set up VM_SOFTDIRTY here, there is still a way to
prevent VMAs from merging: one can call
| echo 4 > /proc/$PID/clear_refs
and clear all VM_SOFTDIRTY over all VMAs presented in memory map,
then new do_brk() will try to extend old VMA and finds that dirty
bit doesn't match thus new VMA will be generated.
As discussed to Pavel, the right approach should be to ignore
VM_SOFTDIRTY bit when we're trying to merge VMAs and if merge
successed we mark extended VMA with dirty bit where needed.
v2: Don't mark VMA as dirty inside vma_merge() body, it's up
to calling code to set up dirty bit where needed.
Reported-by: Mel Gorman <[email protected]>
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Andrew Morton <[email protected]>
---
mm/mmap.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
Index: linux-2.6.git/mm/mmap.c
===================================================================
--- linux-2.6.git.orig/mm/mmap.c
+++ linux-2.6.git/mm/mmap.c
@@ -893,7 +893,15 @@ again: remove_next = 1 + (end > next->
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags)
{
- if (vma->vm_flags ^ vm_flags)
+ /*
+ * VM_SOFTDIRTY should not prevent from VMA merging, if we
+ * match the flags but dirty bit -- the caller should mark
+ * merged VMA as dirty. If dirty bit won't be excluded from
+ * comparison, we increase pressue on the memory system forcing
+ * the kernel to generate new VMAs when old one could be
+ * extended instead.
+ */
+ if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
return 0;
if (vma->vm_file != file)
return 0;
@@ -1082,7 +1090,7 @@ static int anon_vma_compatible(struct vm
return a->vm_end == b->vm_start &&
mpol_equal(vma_policy(a), vma_policy(b)) &&
a->vm_file == b->vm_file &&
- !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
+ !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
}
On 01/23/2014 07:14 PM, Cyrill Gorcunov wrote:
> I think setting up dirty bit inside vma_merge() body is a big hammer
> which should not be used, but it's up to caller of vma_merge() to figure
> out if dirty bit should be set or not if merge successed. Thus softdirty
> vma bit should be (and it already is) set at the end of mmap_region and do_brk
> routines. So patch could be simplified (below). Pavel, what do you think?
Looks correct, thank you!
Acked-by: Pavel Emelyanov <xemul@parallels,com>
> ---
> From: Cyrill Gorcunov <[email protected]>
> Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
>
> VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
> bits in vm_flags matched except dirty bit the kernel can't longer
> merge them and this forces the kernel to generate new VMAs instead.
>
> It finally may lead to the situation when userspace application
> reaches vm.max_map_count limit and get crashed in worse case
>
> | (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
> |
> | (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
> | xinit: connection to X server lost
> |
> | waiting for X server to shut down
> | /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
> | /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
> | /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
>
> https://bugzilla.kernel.org/show_bug.cgi?id=67651
> https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
>
> Initial problem came from missed VM_SOFTDIRTY in do_brk() routine
> but even if we would set up VM_SOFTDIRTY here, there is still a way to
> prevent VMAs from merging: one can call
>
> | echo 4 > /proc/$PID/clear_refs
>
> and clear all VM_SOFTDIRTY over all VMAs presented in memory map,
> then new do_brk() will try to extend old VMA and finds that dirty
> bit doesn't match thus new VMA will be generated.
>
> As discussed to Pavel, the right approach should be to ignore
> VM_SOFTDIRTY bit when we're trying to merge VMAs and if merge
> successed we mark extended VMA with dirty bit where needed.
>
> v2: Don't mark VMA as dirty inside vma_merge() body, it's up
> to calling code to set up dirty bit where needed.
>
> Reported-by: Mel Gorman <[email protected]>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
> CC: Pavel Emelyanov <[email protected]>
> CC: Andrew Morton <[email protected]>
> ---
> mm/mmap.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> Index: linux-2.6.git/mm/mmap.c
> ===================================================================
> --- linux-2.6.git.orig/mm/mmap.c
> +++ linux-2.6.git/mm/mmap.c
> @@ -893,7 +893,15 @@ again: remove_next = 1 + (end > next->
> static inline int is_mergeable_vma(struct vm_area_struct *vma,
> struct file *file, unsigned long vm_flags)
> {
> - if (vma->vm_flags ^ vm_flags)
> + /*
> + * VM_SOFTDIRTY should not prevent from VMA merging, if we
> + * match the flags but dirty bit -- the caller should mark
> + * merged VMA as dirty. If dirty bit won't be excluded from
> + * comparison, we increase pressue on the memory system forcing
> + * the kernel to generate new VMAs when old one could be
> + * extended instead.
> + */
> + if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
> return 0;
> if (vma->vm_file != file)
> return 0;
> @@ -1082,7 +1090,7 @@ static int anon_vma_compatible(struct vm
> return a->vm_end == b->vm_start &&
> mpol_equal(vma_policy(a), vma_policy(b)) &&
> a->vm_file == b->vm_file &&
> - !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
> + !((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
> b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
> }
>
> .
>
On Thu, 23 Jan 2014 19:14:45 +0400 Cyrill Gorcunov <[email protected]> wrote:
> VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
> bits in vm_flags matched except dirty bit the kernel can't longer
> merge them and this forces the kernel to generate new VMAs instead.
Do you intend to alter the brk() and binprm code to set VM_SOFTDIRTY?
On Thu, Jan 23, 2014 at 01:02:35PM -0800, Andrew Morton wrote:
> On Thu, 23 Jan 2014 19:14:45 +0400 Cyrill Gorcunov <[email protected]> wrote:
>
> > VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
> > bits in vm_flags matched except dirty bit the kernel can't longer
> > merge them and this forces the kernel to generate new VMAs instead.
>
> Do you intend to alter the brk() and binprm code to set VM_SOFTDIRTY?
brk() will be "dirtified" now with this merge fix.
brk
do_brk
out:
...
vma->vm_flags |= VM_SOFTDIRTY;
this will work even if vma get merged, the problem was that earlier
we tried to merge without VM_SOFTDIRTY flag. And matcher failed.
do_brk
flags = VM_DATA_DEFAULT_FLAGS | VM_ACCOUNT | mm->def_flags;
vma = vma_merge(mm, prev, addr, addr + len, flags,
NULL, NULL, pgoff, NULL);
if (vma)
goto out;
...
out:
...
vma->vm_flags |= VM_SOFTDIRTY;
That said I'm not really sure now if I should alert @flags in code above.
Should I add VM_SOFTDIRTY into @flags for clarity?
Same for binprm -- the vma allocated for bprm->vma is dirtified
__bprm_mm_init
vma->vm_flags = VM_SOFTDIRTY | VM_STACK_FLAGS | VM_STACK_INCOMPLETE_SETUP;
then setup_arg_pages calls mprotect_fixup with @vm_flags having dirty bit
set thus it'll be propagated to vma
mprotect_fixup
...
vma->vm_flags = newflags;
the @newflags will have dirty bit set from caller code.
Or you mean something else which I'm missing?
On Thu, Jan 23, 2014 at 07:14:45PM +0400, Cyrill Gorcunov wrote:
> On Thu, Jan 23, 2014 at 04:55:43PM +0400, Cyrill Gorcunov wrote:
> > On Thu, Jan 23, 2014 at 04:15:55PM +0400, Cyrill Gorcunov wrote:
> > > >
> > > > Thanks a lot, Mel! I'm testing the patch as well (manually though :).
> > > > I'll send the final fix today.
> > >
> > > The patch below should fix the problem. I would really appreaciate
> > > some additional testing.
> >
> > Forgot to refresh the patch, sorry.
> > ---
>
> I think setting up dirty bit inside vma_merge() body is a big hammer
> which should not be used, but it's up to caller of vma_merge() to figure
> out if dirty bit should be set or not if merge successed. Thus softdirty
> vma bit should be (and it already is) set at the end of mmap_region and do_brk
> routines. So patch could be simplified (below). Pavel, what do you think?
> ---
> From: Cyrill Gorcunov <[email protected]>
> Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
>
It passed the gimp launching test. Patch looks sane but I confess I did
not put a whole lot of thought into it because I see that Andrew is
already reviewing it so
Tested-by: Mel Gorman <[email protected]>
If this is merged then remember that it should be tagged for 3.12-stable
as 3.12.7 and 3.12.8 are affected by this bug.
--
Mel Gorman
SUSE Labs
On Fri, Jan 24, 2014 at 10:14:16AM +0000, Mel Gorman wrote:
> > From: Cyrill Gorcunov <[email protected]>
> > Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
> >
>
> It passed the gimp launching test. Patch looks sane but I confess I did
> not put a whole lot of thought into it because I see that Andrew is
> already reviewing it so
>
> Tested-by: Mel Gorman <[email protected]>
>
> If this is merged then remember that it should be tagged for 3.12-stable
> as 3.12.7 and 3.12.8 are affected by this bug.
Thanks a huge, Mel! Andrew has picked it up and CC'ed stable@ team.
On Fri, Jan 24, 2014 at 03:56:29PM +0400, Cyrill Gorcunov wrote:
> On Fri, Jan 24, 2014 at 10:14:16AM +0000, Mel Gorman wrote:
> > > From: Cyrill Gorcunov <[email protected]>
> > > Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
> > >
> >
> > It passed the gimp launching test. Patch looks sane but I confess I did
> > not put a whole lot of thought into it because I see that Andrew is
> > already reviewing it so
> >
> > Tested-by: Mel Gorman <[email protected]>
> >
> > If this is merged then remember that it should be tagged for 3.12-stable
> > as 3.12.7 and 3.12.8 are affected by this bug.
>
> Thanks a huge, Mel! Andrew has picked it up and CC'ed stable@ team.
Big thanks to the gimp developers that actually pinned this down as a
kernel bug and the people who shoved it through the kernel bugzilla. I
just did a light bit of legwork shuffling the paperwork around :P
--
Mel Gorman
SUSE Labs
On Fri, Jan 24, 2014 at 01:41:35PM +0000, Mel Gorman wrote:
> >
> > Thanks a huge, Mel! Andrew has picked it up and CC'ed stable@ team.
>
> Big thanks to the gimp developers that actually pinned this down as a
> kernel bug and the people who shoved it through the kernel bugzilla. I
> just did a light bit of legwork shuffling the paperwork around :P
Sure! The help in testing and finding kernel problem is always invaluable!
Sorry for replying to so much older thread. The original thread:
https://lore.kernel.org/all/20140123151445.GX1574@moon/
On 1/23/14 11:27 AM, Cyrill Gorcunov wrote:
> On Wed, Jan 22, 2014 at 10:09:10PM -0800, Andrew Morton wrote:
>>>>
>>>> That being said, this could cause vma blowups for programs that are
>>>> actually using this thing.
>>>
>>> Hi Andy, indeed, this could happen. The easiest way is to ignore softdirty bit
>>> when we're trying to merge vmas and set it one new merged. I think this should
>>> be correct. Once I finish I'll send the patch.
>>
>> Hang on. We think the problem is that gimp is generating vmas which
>> *should* be merged, but for unknown reasons they differ in
>> VM_SOFTDIRTY, yes?
>
> Yes. One place where I forgot to set softdirty bit is setup_arg_pages. But
> it called once on elf load, so it can't cause such effect (but should be
> fixed too). Also there is do_brk where vmasoftdirty is missed too :/
>
> Another problem is the potential scenario when we have a bunch of vmas
> and clear vma-softdirty bit on them, then we try to map new one, flags
> won't match and instead of extending old vma the new one will be created.
> I think (if only I'm not missing something) that vma-softdirty should
> be ignored in such case (ie inside is_mergeable_vma) and once vma extended
> it should be marked as dirty one. Again, I need to think and test more.
>
>> Shouldn't we work out where we're forgetting to set VM_SOFTDIRTY?
>> Putting bandaids over this error when we come to trying to merge the
>> vmas sounds very wrong?
>
> I'm looking into this as well.
I've looked at it while working on adding an IOCTL to get and/or clear the
soft dirty bit for a particular region only [1]. The VM_SOFTDIRTY should be
set in the vm_flags to be tested if new allocation should be merged in
previous vma or not. With the following patch, the new allocations are
merged in the previous VMAs.
I've tested it by reverting the 34228d473efe ("mm: ignore VM_SOFTDIRTY on
VMA merging") commit and after adding the following patch, I'm seeing that
all the new allocations done through mmap() (in my testing) are merged in
the previous VMAs. The number of VMAs doesn't increase drastically which
had contributed to the crash of gimp. If I run the same test after
reverting and not including the following, the number of VMAs keep on
increasing with every mmap() syscall which proves this patch.
diff --git a/mm/mmap.c b/mm/mmap.c
index f9b96b387a6f..b132d52f6fe1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1708,6 +1708,15 @@ unsigned long mmap_region(struct file *file,
unsigned long addr,
vm_flags |= VM_ACCOUNT;
}
+ /*
+ * New (or expanded) vma always get soft dirty status.
+ * Otherwise user-space soft-dirty page tracker won't
+ * be able to distinguish situation when vma area unmapped,
+ * then new mapped in-place (which must be aimed as
+ * a completely new data area).
+ */
+ vm_flags |= VM_SOFTDIRTY;
+
/*
* Can we just expand an old mapping?
*/
@@ -1823,15 +1832,6 @@ unsigned long mmap_region(struct file *file,
unsigned long addr,
if (file)
uprobe_mmap(vma);
- /*
- * New (or expanded) vma always get soft dirty status.
- * Otherwise user-space soft-dirty page tracker won't
- * be able to distinguish situation when vma area unmapped,
- * then new mapped in-place (which must be aimed as
- * a completely new data area).
- */
- vma->vm_flags |= VM_SOFTDIRTY;
-
vma_set_page_prot(vma);
return addr;
@@ -2998,6 +2998,8 @@ static int do_brk_flags(unsigned long addr, unsigned
long len, unsigned long fla
if (security_vm_enough_memory_mm(mm, len >> PAGE_SHIFT))
return -ENOMEM;
+ flags |= VM_SOFTDIRTY;
+
/* Can we just expand an old private anonymous mapping? */
vma = vma_merge(mm, prev, addr, addr + len, flags,
NULL, NULL, pgoff, NULL, NULL_VM_UFFD_CTX, NULL);
@@ -3026,7 +3028,7 @@ static int do_brk_flags(unsigned long addr, unsigned
long len, unsigned long fla
mm->data_vm += len >> PAGE_SHIFT;
if (flags & VM_LOCKED)
mm->locked_vm += (len >> PAGE_SHIFT);
- vma->vm_flags |= VM_SOFTDIRTY;
+
return 0;
}
This patch should be included in the kernel regardless if we revert
34228d473efe or not. Thoughts?
Cyrill is correct about the following:
> Another problem is the potential scenario when we have a bunch of vmas
> and clear vma-softdirty bit on them, then we try to map new one, flags
> won't match and instead of extending old vma the new one will be created.
> I think (if only I'm not missing something) that vma-softdirty should
> be ignored in such case (ie inside is_mergeable_vma) and once vma
> extended
> it should be marked as dirty one. Again, I need to think and test more.
While implementing clear soft-dirty bit for a range of address space, I'm
facing an issue. The non-soft dirty VMA gets merged sometimes with the soft
dirty VMA. Thus the non-soft dirty VMA become dirty which is undesirable.
When discussed with the some other developers they consider it the
regression. Why the non-soft dirty page should appear as soft dirty when it
isn't soft dirty in reality? I agree with them. Should we revert
34228d473efe or find a workaround in the IOCTL?
* Revert may cause the VMAs to expand in uncontrollable situation where the
soft dirty bit of a lot of memory regions or the whole address space is
being cleared again and again. AFAIK normal process must either be only
clearing a few memory regions. So the applications should be okay. There is
still chance of regressions if some applications are already using the
soft-dirty bit. I'm not sure how to test it.
* Add a flag in the IOCTL to ignore the dirtiness of VMA. The user will
surely lose the functionality to detect reused memory regions. But the
extraneous soft-dirty pages would not appear. I'm trying to do this in the
patch series [1]. Some discussion is going on that this fails with some
mprotect use case [2]. I still need to have a look at the mprotect selftest
to see how and why this fails. I think this can be implemented after some
more work.
What are your thoughts?
>
> Cyrill
>
[1]
https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/all/[email protected]/
--
BR,
Muhammad Usama Anjum
On Thu, Jan 23, 2014 at 10:30:44AM +0000, Mel Gorman wrote:
>
> The test case passes with this patch applied to 3.13 so that appears
> to confirm that this is related to VM_SOFTDIRTY preventing merges.
> Unfortunately I did not have slabinfo tracking enabled to double check
> the number of vm_area_structs in teh system.
Hi Mel! I'm really really sorry for replying that late, somehow missed the former
bug report (note the bugzilla date message as Jan 23, 2014) so no wonder. Actually
I don't understand yet how SOFTDIRTY can prevent merging. When VMAs are to merge
we explicitly ignore softdirty flag
/*
* If the vma has a ->close operation then the driver probably needs to release
* per-vma resources, so we don't attempt to merge those.
*/
static inline int is_mergeable_vma(struct vm_area_struct *vma,
struct file *file, unsigned long vm_flags,
struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
struct anon_vma_name *anon_name)
{
/*
* VM_SOFTDIRTY should not prevent from VMA merging, if we
* match the flags but dirty bit -- the caller should mark
* merged VMA as dirty. If dirty bit won't be excluded from
* comparison, we increase pressure on the memory system forcing
* the kernel to generate new VMAs when old one could be
* extended instead.
*/
if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
return 0;
...
}
so the softdirty flag is not preventing VMAs from being merged.
Cyrill