Re: [PATCH]mmap: avoid unnecessary anon_vma lock

[Andrew Morton]

On Mon, 28 Mar 2011 09:57:39 -0700
Andi Kleen <andi@xxxxxxxxxxxxxx> wrote:

> Shaohua Li <shaohua.li@xxxxxxxxx> 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().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/