Re: smp_rmb in mm/memory.c in 2.6.10

[Andrea Arcangeli <andrea@suse.de>]

Added Andrew and Linus since they were the other signers of the buggy
changeset, so they can review I'm not missing something too.

	http://linux.bkbits.net:8080/linux-2.5/cset%401.2247.12.78?nav=index.html|ChangeSet@-7d

Thanks.

On Fri, Jan 14, 2005 at 10:25:33PM +0100, Andrea Arcangeli wrote:
> On Fri, Jan 14, 2005 at 08:37:58PM +0000, Hugh Dickins wrote:
> > On Thu, 13 Jan 2005, Kanoj Sarcar wrote:
> > > 
> > > Thanks, I think this explains it. IE, if do_no_page()
> > > reads truncate_count, and then later goes on to
> > > acquire a lock in nopage(), the smp_rmb() is
> > > guaranteeing that the read of truncate_count completes
> > > before nopage() starts executing. 
> > > 
> > > For x86 at least, it seems to me that since the
> > > spin_lock (in nopage()) uses a "lock" instruction,
> > > that itself guarantees that the truncate_count read is
> > > completed, even without the smp_rmb(). (Refer to IA32
> > > SDM Vol 3 section 7.2.4 last para page 7-11). Thus for
> > > x86, the smp_rmb is superfluous.
> > 
> > You're making me nervous.  If you look at 2.6.11-rc1 you'll find
> > that I too couldn't see the point of that smp_rmb(), on any architecture,
> > and so removed it; while also removing the "atomicity" of truncate_count.
> > 
> > Here was my comment to that patch:
> > > Why is mapping->truncate_count atomic?  It's incremented inside
> > > i_mmap_lock (and i_sem), and the reads don't need it to be atomic.
> > > 
> > > And why smp_rmb() before call to ->nopage?  The compiler cannot reorder
> > > the initial assignment of sequence after the call to ->nopage, and no
> > > cpu (yet!) can read from the future, which is all that matters there.
> > 
> > Now I'm not so convinced by that "no cpu can read from the future".
> > 
> > I don't entirely follow your remarks above, but I do think people
> > on this thread have a better grasp of these matters than I have:
> > does anyone now think that smp_rmb() needs to be restored?
> 
> You could have asked even before breaking mainline ;).
> 
> The rmb serializes the read of truncate_count with the read of
> inode->i_size. The rmb is definitely required, and I would leave it an
> atomic op to be sure gcc doesn't outsmart unmap_mapping_range_list (gcc
> can see the internals of unmap_mapping_range_list). I mean just in case.
> We must increase that piece of ram before we truncate the ptes and after
> we updated the i_size.
> 
> Infact it seems to me right now that we miss a smp_wmb() right before
> atomic_inc(&mapping->truncate_count): the spin_lock has inclusive
> semantics on ia64, and in turn the i_size update could happen after the
> atomic_inc without a smp_wmb().
> 
> So please backout the buggy changes and add the smp_wmb() to fix this
> ia64 altix race.
--
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:"aart@kvack.org"> aart@kvack.org </a>