Date: Fri, 13 Oct 2006 18:03:08 +0100 (BST)
From: Hugh Dickins <hugh@veritas.com>
To: "Chen, Kenneth W" <kenneth.w.chen@intel.com>
cc: "'David Gibson'" <david@gibson.dropbear.id.au>,
       Andrew Morton <akpm@osdl.org>, linux-kernel@vger.kernel.org
Subject: RE: Hugepage regression
In-Reply-To: <000201c6ecc0$565cdc00$cb34030a@amr.corp.intel.com>
Message-ID: <Pine.LNX.4.64.0610131744580.14935@blonde.wat.veritas.com>
References: <000201c6ecc0$565cdc00$cb34030a@amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2026
Lines: 56

On Tue, 10 Oct 2006, Chen, Kenneth W wrote:
> Hugh Dickins wrote on Tuesday, October 10, 2006 1:10 PM
> > > can we reverse that order (call unmap_region
> > > and then nulls out vma->vmfile and fput)?
> > 
> > I'm pretty sure we cannot: the ordering is quite intentional, that if
> > a driver ->mmap failed, then it'd be wrong to call down to driver in
> > the unmap_region (if a driver is nicely behaved, that unmap_region
> > shouldn't be unnecessary; but some do rely on us clearing ptes there).

Looking at it again, my explanation seems wrong: I can't see any danger
of calling down to the _driver_ there (there's no remove_vma): rather,
it's __remove_shared_vm_struct we're avoiding by setting vm_file NULL.

> 
> Even not something like the following?  I believe you that nullifying
> vma->vm_file can not be done after unmap_region(),

Yet in your patch below, you do nullify vm_file _after_ unmap_region.

> I just want to make sure we are talking the same thing.

So I'm not sure if we are!

> It looks OK to me to defer the fput in the do_mmap_pgoff clean up path.

Yes, it would be quite okay to defer the fput until after the
unmap_region; but there's no point in making that change, is there?

Hugh (sorry, I was off sick for a couple of days)

> 
> 
> --- ./mm/mmap.c.orig	2006-10-10 15:58:17.000000000 -0700
> +++ ./mm/mmap.c	2006-10-10 15:59:02.000000000 -0700
> @@ -1159,11 +1159,12 @@ out:	
>  unmap_and_free_vma:
>  	if (correct_wcount)
>  		atomic_inc(&inode->i_writecount);
> -	vma->vm_file = NULL;
> -	fput(file);
>  
>  	/* Undo any partial mapping done by a device driver. */
>  	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> +
> +	vma->vm_file = NULL;
> +	fput(file);
>  	charged = 0;
>  free_vma:
>  	kmem_cache_free(vm_area_cachep, vma);
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/