Messages in this thread | | | Date | Fri, 13 Oct 2006 18:03:08 +0100 (BST) | From | Hugh Dickins <> | Subject | RE: Hugepage regression |
| |
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/
| |