linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Mel Gorman <mel@csn.ul.ie>
To: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Minchan Kim <minchan.kim@gmail.com>,
	Linux-MM <linux-mm@kvack.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Lee Schermerhorn <Lee.Schermerhorn@hp.com>
Subject: Re: [PATCH 4/5] always lock the root (oldest) anon_vma
Date: Thu, 13 May 2010 10:54:39 +0100	[thread overview]
Message-ID: <20100513095439.GA27949@csn.ul.ie> (raw)
In-Reply-To: <4BEB18BB.5010803@redhat.com>

On Wed, May 12, 2010 at 05:08:11PM -0400, Rik van Riel wrote:
> On 05/12/2010 05:02 PM, Mel Gorman wrote:
>
>> This last comment is a bit light. It's actually restoring the lock that
>> was taken in 2.6.33 to some extent except we are always taking it now.
>> In 2.6.33, it was resricted to
>>
>>         if (vma->anon_vma&&  (insert || importer || start != vma->vm_start))
>>                  anon_vma = vma->anon_vma;
>>
>> but now it's always. Has it been determined that the locking in 2.6.33
>> was insufficient or are we playing it safe now?
>
> Playing it safe, mostly.
>

Sure. I did the same, got the same question from Andrea and more or less
gave the same answer :) . I asked again in case you spotted something I
didn't.

> Another aspect is that, if you look at the if condition above,
> the number of cases where we have an anon_vma and do not take
> the lock is pretty small.
>
> Basically only the case where we expand a VMA upward or merge
> VMAs in an mprotect.  I believe in pretty much all other cases
> we end up needing to take the lock.
>

Looking at the if condition, brk() would appear to be the most important
case, right? This would appear to correlate with the reasoning behind
that condition in the first place in commit
252c5f94d944487e9f50ece7942b0fbf659c5c31 where sbrk contended on the
lock heavily.

I can't convince myself 100% but it is possible we will regress on that
test case again if the same logic is not applied to the locking. I ran a
brk() microbenchmark from aim9 and the results were really bad - 48%
regression. I didn't rerun with the old logic to see the results
unfortunately and right now I'm on the road. It'll be tomorrow morning
before I get the chance.

> I am not entirely convinced the old code took the lock in all
> of the required cases.
>

I have vague worries about expand_upwards but otherwise the reasoning
seemed solid and even with the new anon_vma code, we are not doing
anything fundamentally different in this area. Maybe it's best to play
it safe now and always take the lock, but it's worth reconsidering later
particularly if this patch gets fingered in some performance-related
bisection later.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2010-05-13  9:55 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-12 17:38 [PATCH 0/5] always lock the root anon_vma Rik van Riel
2010-05-12 17:39 ` [PATCH 1/5] rename anon_vma_lock to vma_lock_anon_vma Rik van Riel
2010-05-12 20:57   ` Mel Gorman
2010-05-13  0:30   ` KAMEZAWA Hiroyuki
2010-05-12 17:39 ` [PATCH 3/5] track the root (oldest) anon_vma Rik van Riel
2010-05-12 20:59   ` Mel Gorman
2010-05-12 21:01     ` Rik van Riel
2010-05-13  0:38   ` KAMEZAWA Hiroyuki
2010-05-13  2:25     ` Rik van Riel
2010-05-14  0:04       ` KAMEZAWA Hiroyuki
2010-05-12 17:40 ` [PATCH 4/5] always lock " Rik van Riel
2010-05-12 21:02   ` Mel Gorman
2010-05-12 21:08     ` Rik van Riel
2010-05-13  9:54       ` Mel Gorman [this message]
2010-05-13 14:33         ` [PATCH -v2 " Rik van Riel
2010-05-13 21:09           ` Andrew Morton
2010-05-13 22:50             ` Rik van Riel
2010-05-14  9:33               ` Mel Gorman
2010-05-26  4:00             ` Rik van Riel
2010-05-26  4:15               ` Andrew Morton
2010-05-26  5:46                 ` james toy
2010-06-01  0:57                   ` james toy
2010-05-26 15:24               ` [PATCH -v2 0/5] always lock the root anon_vma Rik van Riel
2010-05-26 15:25                 ` [PATCH 1/5] rename anon_vma_lock to vma_lock_anon_vma Rik van Riel
2010-05-26 17:25                   ` Linus Torvalds
2010-05-26 19:01                     ` Rik van Riel
2010-05-26 19:25                       ` Linus Torvalds
2010-05-26 19:35                         ` Rik van Riel
2010-05-26 15:25                 ` [PATCH 2/5] change direct call of spin_lock(anon_vma->lock) to inline function Rik van Riel
2010-05-26 15:26                 ` [PATCH 3/5] track the root (oldest) anon_vma Rik van Riel
2010-05-26 15:27                 ` [PATCH 4/5] always lock " Rik van Riel
2010-05-26 15:27                 ` [PATCH 5/5] extend KSM refcounts to the anon_vma root Rik van Riel
2010-05-12 21:55   ` [PATCH 4/5] always lock the root (oldest) anon_vma Linus Torvalds
2010-05-12 22:18     ` Rik van Riel
2010-05-12 22:26       ` Linus Torvalds
2010-05-12 17:41 ` [PATCH 5/5] extend KSM refcounts to the anon_vma root Rik van Riel
2010-05-12 21:07   ` Mel Gorman
2010-05-12 21:09     ` Rik van Riel
2010-05-13 11:26       ` Mel Gorman
2010-05-13 13:11         ` Rik van Riel
2010-05-13 13:24           ` Mel Gorman
2010-05-13 14:34             ` [PATCH -v2 " Rik van Riel
2010-05-19  1:05               ` Andrea Arcangeli
2010-05-12 17:41 ` [PATCH 2/5] change direct call of spin_lock(anon_vma->lock) to inline function Rik van Riel
2010-05-12 20:58   ` Mel Gorman
2010-05-13  0:32   ` KAMEZAWA Hiroyuki
2010-05-20 22:42 ` [PATCH 6/5] adjust mm_take_all_locks to anon-vma-root locking Andrea Arcangeli
2010-05-20 23:07   ` Rik van Riel
  -- strict thread matches above, loose matches on Subject: below --
2010-05-26 19:38 [PATCH -v3 0/5] always lock the root anon_vma Rik van Riel
2010-05-26 19:40 ` [PATCH 4/5] always lock the root (oldest) anon_vma Rik van Riel
2010-05-26 20:36   ` Larry Woodman
2010-05-27  0:57   ` KAMEZAWA Hiroyuki
2010-05-27 13:55   ` Minchan Kim
2010-05-27 17:48   ` Mel Gorman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100513095439.GA27949@csn.ul.ie \
    --to=mel@csn.ul.ie \
    --cc=Lee.Schermerhorn@hp.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=minchan.kim@gmail.com \
    --cc=riel@redhat.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).