Date:	Fri, 30 Jan 2009 21:12:44 +0000 (GMT)
From:	Hugh Dickins <hugh@...itas.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
cc:	Lee Schermerhorn <Lee.Schermerhorn@...com>,
	Greg KH <gregkh@...e.de>,
	Maksim Yevmenkin <maksim.yevmenkin@...il.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Nick Piggin <npiggin@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	will@...wder-design.com, Rik van Riel <riel@...hat.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Mikos Szeredi <miklos@...redi.hu>
Subject: Re: [PATCH] Fix OOPS in mmap_region() when merging adjacent VM_LOCKED
 file segments

On Fri, 30 Jan 2009, Linus Torvalds wrote:
> On Fri, 30 Jan 2009, Lee Schermerhorn wrote:
> > 
> > So happens, I'm mapping with MAP_SHARED, so the VM_ACCOUNT flag gets
> > cleared later in mmap_region().  Comments say that this is for checking
> > memory availability during shmem_file_setup().  Maybe we can move the
> > temporary setting of VM_ACCOUNT until just before the call to
> > shmem_zero_setup()?
> 
> Yeah, that would probably fix it, and looks like the right thing to do. 

I do need to refresh my memory on that in a moment...

> 
> It all looks pretty confused wrong to set the whole VM_ACCOUNT flag for a 
> file-backed file AT ALL in the first place, but the code knows that it 
> won't matter for a shared file, and will be cleared again later.
> 
> So it plays these temporary games with vm_flags, and it didn't matter 
> because of how we used to call "vma_merge()" either early only for the 
> anonymous memory case (that had VM_ACCOUNT stable and didn't have that 
> temporary case at all) or much later (after having undone the temporary 
> flag setting) for files.

I'm to blame for those games, and now they've given trouble,
the right thing may be to put an end to them.

> 
> Why do we pass in that "accountable" flag, btw? It's only ever set to 0 by 
> a MAP_PRIVATE mapping that hits is_file_hugepages() (see do_mmap_pgoff), 
> and we could just do that decision all inside mmap_region(). So the flag 
> doesn't really seem to have any real meaning, and is just passed around 
> for some odd historical reason?

It looks like the "accountable" flag dates from before Miklos separated
mmap_region() out from do_mmap_pgoff(): so he just passed it on down to
mmap_region() as an additional argument, preferring to leave the more
complex MAP_PRIVATE/is_file_hugepages test behind in do_mmap_pgoff().

It seemed rather a random refactoring to me.  Looking at it again,
I wonder if we should be getting do_brk() to use mmap_region() too;
but my appetite for cleanups is low at present, bugs we have enough.

By the way, there's an argument to say that you should add
VM_MIXEDMAP to VM_CAN_NONLINEAR in VM_MERGEABLE_FLAGS: I don't
really care whether we merge the odd filemap_xip vma or not,
but it used to do so and now won't.

By the same (used to merge, now won't) argument, one could say
VM_INSERTPAGE should be there too; but whereas VM_MIXEDMAP is used
in one place only, quite a lot of drivers use vm_insert_page(), so
I feel more comfortable with the idea that it's stopping merges -
though in that case, shouldn't we add it to VM_SPECIAL?

But I'm caring more about that VM_ACCOUNT...

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

Powered by blists - more mailing lists