Re: [PATCH] mm: prevent mmap_cache race in find_vma()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote:
> On Tue, Apr 2, 2013 at 9:58 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote:
> >
> >> As stated, it doesn't.  I made the comment "for what it's worth" that
> >> ACCESS_ONCE() doesn't do anything to "prevent the compiler from
> >> re-fetching" as the changelog insists it does.
> >
> > That's exactly what it does:
> >
> > /*
> >  * Prevent the compiler from merging or refetching accesses.
> >
> > This is the guarantee ACCESS_ONCE() gives, users should absolutely be
> > allowed to rely on this literal definition.  The underlying gcc
> > implementation does not matter one bit.  That's the whole point of
> > abstraction!
> 
> If the definition of ACCESS_ONCE is indeed
> 
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> 
> then its behaviour is compiler-specific.

That is the implementation of ACCESS_ONCE().  As Johannes noted,
in the unlikely event that this implementation ever fails to provide
the semantics required of ACCESS_ONCE(), something will be changed.
This has already happened at least once.  A recent version of gcc allowed
volatile stores of certain constants to be split, but gcc was changed
to avoid this behavior, while of course preserving this optimization
for non-volatile stores.  If we later need to change the ACCESS_ONCE()
macro, we will make that change.

> The C language standard only describes how access to
> volatile-qualified objects behave.  In this case x is (presumably) not
> a volatile-qualifed object.  The standard never defines the behaviour
> of volatile-qualified pointers.  That might seem like an oversight,
> but it is not: using a non-volatile-qualified pointer to access a
> volatile-qualified object is undefined behaviour.
>
> In short, casting a pointer to a non-volatile-qualified object to a
> volatile-qualified pointer has no specific meaning in C.  It's true
> that most compilers will behave as you wish, but there is no
> guarantee.

But we are not using a non-volatile-qualified pointer to access a
volatile-qualified object.  We are doing the opposite.  I therefore
don't understand the relevance of your comment about undefined behavior.

> If using a sufficiently recent version of GCC, you can get the
> behaviour that I think you want by using
>     __atomic_load(&x, __ATOMIC_RELAXED)

If this maps to the memory_order_relaxed token defined in earlier versions
of the C11 standard, then this absolutely does -not-, repeat -not-, work
for ACCESS_ONCE().  The relaxed load instead guarantees is that the load
will be atomic with respect to other atomic stores to that same variable,
in other words, it will prevent "load tearing" and "store tearing".  I
also believe that it prevents reloading, in other words, preventing this:

	tmp = __atomic_load(&x, __ATOMIC_RELAXED);
	do_something_with(tmp);
	do_something_else_with(tmp);

from being optimized into something like this:

	do_something_with(__atomic_load(&x, __ATOMIC_RELAXED));
	do_something_else_with(__atomic_load(&x, __ATOMIC_RELAXED));

It says nothing about combining nearby loads from that same variable.
As I understand it, the compiler would be within its rights to do the
reverse optimization from this:

	do_something_with(__atomic_load(&x, __ATOMIC_RELAXED));
	do_something_else_with(__atomic_load(&x, __ATOMIC_RELAXED));

into this:

	tmp = __atomic_load(&x, __ATOMIC_RELAXED);
	do_something_with(tmp);
	do_something_else_with(tmp);

It is only permitted to do finite combining, so that it is prohibited
from turning this:

	while (__atomic_load(&x, __ATOMIC_RELAXED) != 0)
		do_some_other_thing();

into this:

	tmp = __atomic_load(&x, __ATOMIC_RELAXED);
	while (tmp)
		do_some_other_thing();

and thus into this:

	tmp = __atomic_load(&x, __ATOMIC_RELAXED);
	for (;;)
		do_some_other_thing();

But it would be within its rights to unroll the original loop into
something like this:

	while (__atomic_load(&x, __ATOMIC_RELAXED) != 0) {
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
		do_some_other_thing();
	}

This could of course destroy the response-time characteristics of the
resulting program, so we absolutely must have a way to prevent the
compiler from doing this.  One way to prevent it from doing this is in
fact a volatile cast:

	while (__atomic_load((volatile typeof(x) *)&x, __ATOMIC_RELAXED) != 0)
		do_some_other_thing();

The last time I went through this with the C/C++ standards committee
members, they agreed with my interpretation.  Perhaps the standard has
been changed to allow volatile to be dispensed with, but I have not
seen any such change.  So, if you believe differently, please show me
the wording in the standard that supports your view.

							Thanx, Paul

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