All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
To: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@saeurebad.de>,
	torvalds@linux-foundation.org, kosaki.motohiro@jp.fujitsu.com,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [patch v2] vmscan: protect zone rotation stats by lru lock
Date: Mon, 01 Dec 2008 17:09:45 -0500	[thread overview]
Message-ID: <1228169385.18834.136.camel@lts-notebook> (raw)
In-Reply-To: <49345B3B.30703@redhat.com>

On Mon, 2008-12-01 at 16:46 -0500, Rik van Riel wrote:
> Andrew Morton wrote:
> > On Mon, 01 Dec 2008 03:00:35 +0100
> > Johannes Weiner <hannes@saeurebad.de> wrote:
> > 
> >> The zone's rotation statistics must not be accessed without the
> >> corresponding LRU lock held.  Fix an unprotected write in
> >> shrink_active_list().
> >>
> > 
> > I don't think it really matters.  It's quite common in that code to do
> > unlocked, racy update to statistics such as this.  Because on those
> > rare occasions where a race does happen, there's a small glitch in the
> > reclaim logic which nobody will notice anyway.
> > 
> > Of course, this does need to be done with some care, to ensure the
> > glitch _will_ be small.
> 
> Processing at most SWAP_CLUSTER_MAX pages at once probably
> ensures that glitches will be small most of the time.
> 
> The only way this could be a big problem is if we end up
> racing with the divide-by-two logic in get_scan_ratio,
> leaving the rotated pages a factor two higher than they
> should be.
> 
> Putting all the writes to the stats under the LRU lock
> should ensure that never happens.

And he's not actually adding a lock.  Just moving the exiting one up to
include the stats update.  The intervening pagevec, pgmoved and lru
initializations don't need to be under the lock, but that's probably not
a big deal?

Lee


WARNING: multiple messages have this Message-ID (diff)
From: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
To: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Johannes Weiner <hannes@saeurebad.de>,
	torvalds@linux-foundation.org, kosaki.motohiro@jp.fujitsu.com,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [patch v2] vmscan: protect zone rotation stats by lru lock
Date: Mon, 01 Dec 2008 17:09:45 -0500	[thread overview]
Message-ID: <1228169385.18834.136.camel@lts-notebook> (raw)
In-Reply-To: <49345B3B.30703@redhat.com>

On Mon, 2008-12-01 at 16:46 -0500, Rik van Riel wrote:
> Andrew Morton wrote:
> > On Mon, 01 Dec 2008 03:00:35 +0100
> > Johannes Weiner <hannes@saeurebad.de> wrote:
> > 
> >> The zone's rotation statistics must not be accessed without the
> >> corresponding LRU lock held.  Fix an unprotected write in
> >> shrink_active_list().
> >>
> > 
> > I don't think it really matters.  It's quite common in that code to do
> > unlocked, racy update to statistics such as this.  Because on those
> > rare occasions where a race does happen, there's a small glitch in the
> > reclaim logic which nobody will notice anyway.
> > 
> > Of course, this does need to be done with some care, to ensure the
> > glitch _will_ be small.
> 
> Processing at most SWAP_CLUSTER_MAX pages at once probably
> ensures that glitches will be small most of the time.
> 
> The only way this could be a big problem is if we end up
> racing with the divide-by-two logic in get_scan_ratio,
> leaving the rotated pages a factor two higher than they
> should be.
> 
> Putting all the writes to the stats under the LRU lock
> should ensure that never happens.

And he's not actually adding a lock.  Just moving the exiting one up to
include the stats update.  The intervening pagevec, pgmoved and lru
initializations don't need to be under the lock, but that's probably not
a big deal?

Lee

--
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:[~2008-12-01 22:10 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-12-01  2:00 [patch v2] vmscan: protect zone rotation stats by lru lock Johannes Weiner
2008-12-01  2:00 ` Johannes Weiner
2008-12-01 21:41 ` Andrew Morton
2008-12-01 21:41   ` Andrew Morton
2008-12-01 21:46   ` Rik van Riel
2008-12-01 21:46     ` Rik van Riel
2008-12-01 22:09     ` Lee Schermerhorn [this message]
2008-12-01 22:09       ` Lee Schermerhorn
2008-12-02 12:34       ` Johannes Weiner
2008-12-02 12:34         ` Johannes Weiner
2008-12-02 18:17         ` Lee Schermerhorn
2008-12-02 18:17           ` Lee Schermerhorn

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=1228169385.18834.136.camel@lts-notebook \
    --to=lee.schermerhorn@hp.com \
    --cc=akpm@linux-foundation.org \
    --cc=hannes@saeurebad.de \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.