LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nick Piggin <npiggin@suse.de>
To: Hugh Dickins <hugh@veritas.com>
Cc: Andrew Morton <akpm@osdl.org>,
	Linux Memory Management <linux-mm@kvack.org>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [patch 0/3] 2.6.20 fix for PageUptodate memorder problem (try 2)
Date: Fri, 9 Feb 2007 02:31:16 +0100	[thread overview]
Message-ID: <20070209013115.GA17334@wotan.suse.de> (raw)
In-Reply-To: <Pine.LNX.4.64.0702090027580.29905@blonde.wat.veritas.com>

On Fri, Feb 09, 2007 at 12:41:51AM +0000, Hugh Dickins wrote:
> On Thu, 8 Feb 2007, Nick Piggin wrote:
> > Still no independent confirmation as to whether this is a problem or not.
> 
> I'm trying to convince myself none of your patch is necessary.  Probably
> shall fail.  But how come we've survived for years with such an issue?

Well I'm almost sure that the POWER guys hit this with anonymous pages
being zeroed out then added to process address space -- they would
occasionally see junk (via another thread, I presume).

If that were the case, then I think that a read-vs-read over a hole
(for example) should be buggy in the same way.

> > Updated some comments, added diffstats to patches, don't use
> > __SetPageUptodate as an internal page-flags.h private function.
> 
> Depressed by profusion of PageUptodate_UpperAndlowercasevariants.
> Those rmbs, you really only want them when it says "yes", don't you?

Yeah, any help with naming suggestions would be appreciated. I think
we can get rid of the SetPageUptodate_nowarn variant, by making
SetNewPageUptodate simply do an smp_wmb + __set_bit on all architectures?
This frees us from the extra atomic ops I'd added into the page fault
fastpaths as well.

Yes, we do only need the rmb if it is uptodate, that's a good point.

> > I would like to eventually get an ack from Hugh regarding the anon memory
> > and especially swap side of the equation,
> 
> Plea noted.  I'm pondering.  "Eventually" indeed.  OTOH I expect you're
> right to criticize anon/swap PageUptodate being set where it was needed
> to get by, rather than where it was natural to do so.

Well if I can get you to warm to that aspect of the patch... :) I expect
that using non-atomic bitops might help.

Should I make that change into its own patch?

> > and a glance from whoever put the
> > smp_wmb()s into the copy functions (Was it Ben H or Anton maybe?)
> 
> From: Linus Torvalds <torvalds@ppc970.osdl.org>
> Date: Thu, 14 Oct 2004 04:00:06 +0000 (-0700)
> Subject: Fix threaded user page write memory ordering
> X-Git-Tag: v2.6.9-final~3
> X-Git-Url: http://127.0.0.1:1234/?p=.git;a=commitdiff_plain;h=538ce05c0ef4055cf29a92a4abcdf139d180a0f9;hp=8c225dbc5a7b13801a8254aae0ccebab8e4bece7
> 
> Fix threaded user page write memory ordering

Thanks, I did see that, but I'm sure it must have been prompted by a
discussion or another proposed patch from IBM. Maybe I'm wrong
though.

Thanks,
Nick

  reply	other threads:[~2007-02-09  1:31 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-08 13:26 Nick Piggin
2007-02-08 13:27 ` [patch 1/3] mm: fix PageUptodate memorder Nick Piggin
2007-02-08 13:27 ` [patch 2/3] fs: buffer don't PageUptodate without page locked Nick Piggin
2007-02-08 13:27 ` [patch 3/3] mm: make read_cache_page synchronous Nick Piggin
2007-02-08 22:21 ` [patch 0/3] 2.6.20 fix for PageUptodate memorder problem (try 2) Benjamin Herrenschmidt
2007-02-09  0:41 ` Hugh Dickins
2007-02-09  1:31   ` Nick Piggin [this message]
2007-02-09  1:44     ` Benjamin Herrenschmidt
2007-02-09  1:41   ` Benjamin Herrenschmidt

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=20070209013115.GA17334@wotan.suse.de \
    --to=npiggin@suse.de \
    --cc=akpm@osdl.org \
    --cc=benh@kernel.crashing.org \
    --cc=hugh@veritas.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --subject='Re: [patch 0/3] 2.6.20 fix for PageUptodate memorder problem (try 2)' \
    /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

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).