Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Linus Torvalds
On Fri, Nov 1, 2013 at 7:09 AM, Yuanhan Liu wrote: > > Sorry, I may be wrong again this time. But, isn't vma->anon_vma_chain > list being protect by mmap_sem & page_table_lock? No. The mmap_sem (and the page_table_lock) only protects a single VM instance. The anon_vma chains, in contrast, can

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 11:22:24AM +0100, Peter Zijlstra wrote: > On Fri, Nov 01, 2013 at 05:38:44PM +0800, Yuanhan Liu wrote: > > On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: > > > On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: > > > > @@ -497,15 +495,20 @@ static

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 01:07:45PM +0100, Peter Zijlstra wrote: > On Fri, Nov 01, 2013 at 07:44:29PM +0800, Yuanhan Liu wrote: > > commit 012f18004da33ba672e3c60838cc4898126174d3 > > Author: Rik van Riel > > Date: Mon Aug 9 17:18:40 2010 -0700 > > > > mm: always lock the root (oldest)

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 07:44:29PM +0800, Yuanhan Liu wrote: > commit 012f18004da33ba672e3c60838cc4898126174d3 > Author: Rik van Riel > Date: Mon Aug 9 17:18:40 2010 -0700 > > mm: always lock the root (oldest) anon_vma > > Always (and only) lock the root (oldest) anon_vma whenever we

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 11:15:14AM +0100, Peter Zijlstra wrote: > On Fri, Nov 01, 2013 at 06:07:07PM +0800, Yuanhan Liu wrote: > > > I also want to point out that lately we've seen several changes sent > > > out that relax locking with no accompanying explanation of why the > > > relaxed locking

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 05:38:44PM +0800, Yuanhan Liu wrote: > On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: > > On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: > > > @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct > > > *vma, struct rb_root

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 06:07:07PM +0800, Yuanhan Liu wrote: > > I also want to point out that lately we've seen several changes sent > > out that relax locking with no accompanying explanation of why the > > relaxed locking would be safe. Please don't do that - having a lot of > > performance

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 02:22:25AM -0700, Michel Lespinasse wrote: > On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra wrote: > > AFAICT this isn't correct at all. We used to protect the vma interval > > tree with the root lock, now we don't. All we've got left is the > > mmap_sem, but anon_vma

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: > On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: > > @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct *vma, > > struct rb_root *root) > > * anon_vma_interval_tree_post_update_vma(). > > * > > *

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Ingo Molnar
* Michel Lespinasse wrote: > On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra wrote: > > > AFAICT this isn't correct at all. We used to protect the vma > > interval tree with the root lock, now we don't. All we've got > > left is the mmap_sem, but anon_vma chains can cross > > address-spaces

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Michel Lespinasse
On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra wrote: > AFAICT this isn't correct at all. We used to protect the vma interval > tree with the root lock, now we don't. All we've got left is the > mmap_sem, but anon_vma chains can cross address-spaces and thus we're up > some creek without no

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: > @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct *vma, > struct rb_root *root) > * anon_vma_interval_tree_post_update_vma(). > * > * The entire update must be protected by exclusive mmap_sem and by > - * the

[PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
It used to be per anon_vma lock. At then, it was a spinlock. It was changed to mutex and an regression was reported. Commit bb4aa396("mm: avoid repeated anon_vma lock/unlock sequences in anon_vma_clone") turned locking a anon_vma to locking its root. Change it back to per anon_vma lock for: -

[PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
It used to be per anon_vma lock. At then, it was a spinlock. It was changed to mutex and an regression was reported. Commit bb4aa396(mm: avoid repeated anon_vma lock/unlock sequences in anon_vma_clone) turned locking a anon_vma to locking its root. Change it back to per anon_vma lock for: - next

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root) * anon_vma_interval_tree_post_update_vma(). * * The entire update must be protected by exclusive mmap_sem and by - * the root

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Michel Lespinasse
On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra pet...@infradead.org wrote: AFAICT this isn't correct at all. We used to protect the vma interval tree with the root lock, now we don't. All we've got left is the mmap_sem, but anon_vma chains can cross address-spaces and thus we're up some creek

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Ingo Molnar
* Michel Lespinasse wal...@google.com wrote: On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra pet...@infradead.org wrote: AFAICT this isn't correct at all. We used to protect the vma interval tree with the root lock, now we don't. All we've got left is the mmap_sem, but anon_vma chains

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root) * anon_vma_interval_tree_post_update_vma(). * * The entire

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 02:22:25AM -0700, Michel Lespinasse wrote: On Fri, Nov 1, 2013 at 1:43 AM, Peter Zijlstra pet...@infradead.org wrote: AFAICT this isn't correct at all. We used to protect the vma interval tree with the root lock, now we don't. All we've got left is the mmap_sem, but

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 06:07:07PM +0800, Yuanhan Liu wrote: I also want to point out that lately we've seen several changes sent out that relax locking with no accompanying explanation of why the relaxed locking would be safe. Please don't do that - having a lot of performance data is

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 05:38:44PM +0800, Yuanhan Liu wrote: On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: @@ -497,15 +495,20 @@ static void vma_rb_erase(struct vm_area_struct *vma, struct rb_root *root)

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 11:15:14AM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 06:07:07PM +0800, Yuanhan Liu wrote: I also want to point out that lately we've seen several changes sent out that relax locking with no accompanying explanation of why the relaxed locking would be

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Peter Zijlstra
On Fri, Nov 01, 2013 at 07:44:29PM +0800, Yuanhan Liu wrote: commit 012f18004da33ba672e3c60838cc4898126174d3 Author: Rik van Riel r...@redhat.com Date: Mon Aug 9 17:18:40 2010 -0700 mm: always lock the root (oldest) anon_vma Always (and only) lock the root (oldest) anon_vma

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 01:07:45PM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 07:44:29PM +0800, Yuanhan Liu wrote: commit 012f18004da33ba672e3c60838cc4898126174d3 Author: Rik van Riel r...@redhat.com Date: Mon Aug 9 17:18:40 2010 -0700 mm: always lock the root (oldest)

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Yuanhan Liu
On Fri, Nov 01, 2013 at 11:22:24AM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 05:38:44PM +0800, Yuanhan Liu wrote: On Fri, Nov 01, 2013 at 09:43:29AM +0100, Peter Zijlstra wrote: On Fri, Nov 01, 2013 at 03:54:24PM +0800, Yuanhan Liu wrote: @@ -497,15 +495,20 @@ static void

Re: [PATCH 1/4] mm/rmap: per anon_vma lock

2013-11-01 Thread Linus Torvalds
On Fri, Nov 1, 2013 at 7:09 AM, Yuanhan Liu yuanhan@linux.intel.com wrote: Sorry, I may be wrong again this time. But, isn't vma-anon_vma_chain list being protect by mmap_sem page_table_lock? No. The mmap_sem (and the page_table_lock) only protects a single VM instance. The anon_vma