If we only change vma->vm_end, we can avoid taking anon_vma lock even 'insert'
isn't NULL, which is the case of split_vma.
>From my understanding, we need the lock before because rmap must get the
'insert' VMA when we adjust old VMA's vm_end (the 'insert' VMA is linked to
anon_vma list in __insert_vm_struct before).
But now this isn't true any more. The 'insert' VMA is already linked to
anon_vma list in __split_vma(with anon_vma_clone()) instead of
__insert_vm_struct. There is no race rmap can't get required VMAs.
So the anon_vma lock is unnecessary, and this can reduce one locking in brk
case and improve scalability.
Signed-off-by: Shaohua Li<[email protected]>
---
mm/mmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/mm/mmap.c
===================================================================
--- linux.orig/mm/mmap.c 2011-03-24 09:08:27.000000000 +0800
+++ linux/mm/mmap.c 2011-03-24 09:14:03.000000000 +0800
@@ -605,7 +605,7 @@ again: remove_next = 1 + (end > next->
* lock may be shared between many sibling processes. Skipping
* the lock for brk adjustments makes a difference sometimes.
*/
- if (vma->anon_vma && (insert || importer || start != vma->vm_start)) {
+ if (vma->anon_vma && (importer || start != vma->vm_start)) {
anon_vma = vma->anon_vma;
anon_vma_lock(anon_vma);
}
Shaohua Li <[email protected]> writes:
> If we only change vma->vm_end, we can avoid taking anon_vma lock even 'insert'
> isn't NULL, which is the case of split_vma.
> From my understanding, we need the lock before because rmap must get the
> 'insert' VMA when we adjust old VMA's vm_end (the 'insert' VMA is linked to
> anon_vma list in __insert_vm_struct before).
> But now this isn't true any more. The 'insert' VMA is already linked to
> anon_vma list in __split_vma(with anon_vma_clone()) instead of
> __insert_vm_struct. There is no race rmap can't get required VMAs.
> So the anon_vma lock is unnecessary, and this can reduce one locking in brk
> case and improve scalability.
Looks good to me.
-Andi
--
[email protected] -- Speaking for myself only
On Mon, 28 Mar 2011 09:57:39 -0700
Andi Kleen <[email protected]> wrote:
> Shaohua Li <[email protected]> writes:
>
> > If we only change vma->vm_end, we can avoid taking anon_vma lock even 'insert'
> > isn't NULL, which is the case of split_vma.
> > From my understanding, we need the lock before because rmap must get the
> > 'insert' VMA when we adjust old VMA's vm_end (the 'insert' VMA is linked to
> > anon_vma list in __insert_vm_struct before).
> > But now this isn't true any more. The 'insert' VMA is already linked to
> > anon_vma list in __split_vma(with anon_vma_clone()) instead of
> > __insert_vm_struct. There is no race rmap can't get required VMAs.
> > So the anon_vma lock is unnecessary, and this can reduce one locking in brk
> > case and improve scalability.
>
> Looks good to me.
Looks way too tricky to me.
Please review this code for maintainability. Have we documented what
we're doing as completely and as clearly as we are able?
This comment:
/*
* split_vma has split insert from vma, and needs
* us to insert it before dropping the locks
* (it may either follow vma or precede it).
*/
is now at least misleading. It doesn't explain which "locks" it means,
and with this patch we only drop a single lock.
And this comment:
/*
* When changing only vma->vm_end, we don't really need anon_vma
* lock. This is a fairly rare case by itself, but the anon_vma
* lock may be shared between many sibling processes. Skipping
* the lock for brk adjustments makes a difference sometimes.
*/
fails to explain _why_ the anon_vma lock isn't needed in this case, and
didn't tell readers why it is safe to alter vma->vm_pgoff without
anon_vma_lock().
On Tue, Mar 29, 2011 at 03:35:17PM -0700, Andrew Morton wrote:
> On Mon, 28 Mar 2011 09:57:39 -0700
> Andi Kleen <[email protected]> wrote:
>
> > Shaohua Li <[email protected]> writes:
> >
> > > If we only change vma->vm_end, we can avoid taking anon_vma lock even 'insert'
> > > isn't NULL, which is the case of split_vma.
> > > From my understanding, we need the lock before because rmap must get the
> > > 'insert' VMA when we adjust old VMA's vm_end (the 'insert' VMA is linked to
> > > anon_vma list in __insert_vm_struct before).
> > > But now this isn't true any more. The 'insert' VMA is already linked to
> > > anon_vma list in __split_vma(with anon_vma_clone()) instead of
> > > __insert_vm_struct. There is no race rmap can't get required VMAs.
> > > So the anon_vma lock is unnecessary, and this can reduce one locking in brk
> > > case and improve scalability.
> >
> > Looks good to me.
>
> Looks way too tricky to me.
>
> Please review this code for maintainability. Have we documented what
> we're doing as completely and as clearly as we are able?
I agree the comments could be improved, but the code change looked good
to me. I don't think it impacts maintainability by itself because
we already do similar magic.
-Andi
On Mon, 28 Mar 2011, Shaohua Li wrote:
> If we only change vma->vm_end, we can avoid taking anon_vma lock even 'insert'
> isn't NULL, which is the case of split_vma.
> From my understanding, we need the lock before because rmap must get the
> 'insert' VMA when we adjust old VMA's vm_end (the 'insert' VMA is linked to
> anon_vma list in __insert_vm_struct before).
> But now this isn't true any more. The 'insert' VMA is already linked to
> anon_vma list in __split_vma(with anon_vma_clone()) instead of
> __insert_vm_struct. There is no race rmap can't get required VMAs.
> So the anon_vma lock is unnecessary, and this can reduce one locking in brk
> case and improve scalability.
>
> Signed-off-by: Shaohua Li<[email protected]>
I was sceptical at first, but yes, you're right: thanks.
Acked-by: Hugh Dickins <[email protected]>
>
> ---
> mm/mmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: linux/mm/mmap.c
> ===================================================================
> --- linux.orig/mm/mmap.c 2011-03-24 09:08:27.000000000 +0800
> +++ linux/mm/mmap.c 2011-03-24 09:14:03.000000000 +0800
> @@ -605,7 +605,7 @@ again: remove_next = 1 + (end > next->
> * lock may be shared between many sibling processes. Skipping
> * the lock for brk adjustments makes a difference sometimes.
> */
> - if (vma->anon_vma && (insert || importer || start != vma->vm_start)) {
> + if (vma->anon_vma && (importer || start != vma->vm_start)) {
> anon_vma = vma->anon_vma;
> anon_vma_lock(anon_vma);
> }