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

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

[Jan Stancek]

find_vma() can be called by multiple threads with read lock
held on mm->mmap_sem and any of them can update mm->mmap_cache.
Prevent compiler from re-fetching mm->mmap_cache, because other
readers could update it in the meantime:

               thread 1                             thread 2
                                        |
  find_vma()                            |  find_vma()
    struct vm_area_struct *vma = NULL;  |
    vma = mm->mmap_cache;               |
    if (!(vma && vma->vm_end > addr     |
        && vma->vm_start <= addr)) {    |
                                        |    mm->mmap_cache = vma;
    return vma;                         |
     ^^ compiler may optimize this      |
        local variable out and re-read  |
        mm->mmap_cache                  |

This issue can be reproduced with gcc-4.8.0-1 on s390x by running
mallocstress testcase from LTP, which triggers:
  kernel BUG at mm/rmap.c:1088!
    Call Trace:
     ([<000003d100c57000>] 0x3d100c57000)
      [<000000000023a1c0>] do_wp_page+0x2fc/0xa88
      [<000000000023baae>] handle_pte_fault+0x41a/0xac8
      [<000000000023d832>] handle_mm_fault+0x17a/0x268
      [<000000000060507a>] do_protection_exception+0x1e2/0x394
      [<0000000000603a04>] pgm_check_handler+0x138/0x13c
      [<000003fffcf1f07a>] 0x3fffcf1f07a
    Last Breaking-Event-Address:
      [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168

Thanks to Jakub Jelinek for his insight on gcc and helping to
track this down.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 mm/mmap.c  |    2 +-
 mm/nommu.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 6466699..0db0de1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 
 	/* Check the cache first. */
 	/* (Cache hit rate is typically around 35%.) */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) {
 		struct rb_node *rb_node;
 
diff --git a/mm/nommu.c b/mm/nommu.c
index e193280..2f3ea74 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 	struct vm_area_struct *vma;
 
 	/* check the cache first */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
 		return vma;
 
-- 
1.7.1

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

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

[David Rientjes]

On Tue, 2 Apr 2013, Jan Stancek wrote:

> find_vma() can be called by multiple threads with read lock
> held on mm->mmap_sem and any of them can update mm->mmap_cache.
> Prevent compiler from re-fetching mm->mmap_cache, because other
> readers could update it in the meantime:
> 

FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
mm->mmap_cache whatsoever; there is nothing that prevents this either in 
the C standard.  You'll be relying solely on gcc's implementation of how 
it dereferences volatile-qualified pointers.

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

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

[Hugh Dickins]

On Tue, 2 Apr 2013, David Rientjes wrote:
> On Tue, 2 Apr 2013, Jan Stancek wrote:
> 
> > find_vma() can be called by multiple threads with read lock
> > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > Prevent compiler from re-fetching mm->mmap_cache, because other
> > readers could update it in the meantime:
> 
> FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
> mm->mmap_cache whatsoever; there is nothing that prevents this either in 
> the C standard.  You'll be relying solely on gcc's implementation of how 
> it dereferences volatile-qualified pointers.

Jan is using ACCESS_ONCE() as it should be used, for its intended
purpose.  If the kernel's implementation of ACCESS_ONCE() is deficient,
then we should fix that, not discourage its use.

Hugh

> > 
> >                thread 1                             thread 2
> >                                         |
> >   find_vma()                            |  find_vma()
> >     struct vm_area_struct *vma = NULL;  |
> >     vma = mm->mmap_cache;               |
> >     if (!(vma && vma->vm_end > addr     |
> >         && vma->vm_start <= addr)) {    |
> >                                         |    mm->mmap_cache = vma;
> >     return vma;                         |
> >      ^^ compiler may optimize this      |
> >         local variable out and re-read  |
> >         mm->mmap_cache                  |
> > 
> > This issue can be reproduced with gcc-4.8.0-1 on s390x by running
> > mallocstress testcase from LTP, which triggers:
> >   kernel BUG at mm/rmap.c:1088!
> >     Call Trace:
> >      ([<000003d100c57000>] 0x3d100c57000)
> >       [<000000000023a1c0>] do_wp_page+0x2fc/0xa88
> >       [<000000000023baae>] handle_pte_fault+0x41a/0xac8
> >       [<000000000023d832>] handle_mm_fault+0x17a/0x268
> >       [<000000000060507a>] do_protection_exception+0x1e2/0x394
> >       [<0000000000603a04>] pgm_check_handler+0x138/0x13c
> >       [<000003fffcf1f07a>] 0x3fffcf1f07a
> >     Last Breaking-Event-Address:
> >       [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168
> > 
> > Thanks to Jakub Jelinek for his insight on gcc and helping to
> > track this down.
> > 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  mm/mmap.c  |    2 +-
> >  mm/nommu.c |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 6466699..0db0de1 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> >  
> >  	/* Check the cache first. */
> >  	/* (Cache hit rate is typically around 35%.) */
> > -	vma = mm->mmap_cache;
> > +	vma = ACCESS_ONCE(mm->mmap_cache);
> >  	if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) {
> >  		struct rb_node *rb_node;
> >  
> > diff --git a/mm/nommu.c b/mm/nommu.c
> > index e193280..2f3ea74 100644
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> >  	struct vm_area_struct *vma;
> >  
> >  	/* check the cache first */
> > -	vma = mm->mmap_cache;
> > +	vma = ACCESS_ONCE(mm->mmap_cache);
> >  	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
> >  		return vma;
> >  
> > -- 
> > 1.7.1

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

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

[David Rientjes]

On Tue, 2 Apr 2013, Hugh Dickins wrote:

> > > find_vma() can be called by multiple threads with read lock
> > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > readers could update it in the meantime:
> > 
> > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
> > mm->mmap_cache whatsoever; there is nothing that prevents this either in 
> > the C standard.  You'll be relying solely on gcc's implementation of how 
> > it dereferences volatile-qualified pointers.
> 
> Jan is using ACCESS_ONCE() as it should be used, for its intended
> purpose.  If the kernel's implementation of ACCESS_ONCE() is deficient,
> then we should fix that, not discourage its use.
> 

My comment is about the changelog, quoted above, saying "prevent compiler 
from re-fetching mm->mmap_cache..."  ACCESS_ONCE(), as implemented, does 
not prevent the compiler from re-fetching anything.  It is entirely 
plausible that in gcc's current implementation that this guarantee is 
made, but it is not prevented by the language standard and I think the 
changelog should be reworded for anybody who reads it in the future.  
There is a dependency here on gcc's implementation, it's a meaningful 
distinction.

I never discouraged its use since for gcc's current implementation it 
appears to work as desired and without gcc extensions there is no way to 
make such a guarantee by the standard.  In fact, I acked a patch from Eric 
Dumazet that fixes a NULL pointer dereference by using ACCESS_ONCE() with 
gcc in slub.

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

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

[Paul E. McKenney]

On Tue, Apr 02, 2013 at 04:55:45PM -0700, David Rientjes wrote:
> On Tue, 2 Apr 2013, Hugh Dickins wrote:
> 
> > > > find_vma() can be called by multiple threads with read lock
> > > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > > readers could update it in the meantime:
> > > 
> > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
> > > mm->mmap_cache whatsoever; there is nothing that prevents this either in 
> > > the C standard.  You'll be relying solely on gcc's implementation of how 
> > > it dereferences volatile-qualified pointers.
> > 
> > Jan is using ACCESS_ONCE() as it should be used, for its intended
> > purpose.  If the kernel's implementation of ACCESS_ONCE() is deficient,
> > then we should fix that, not discourage its use.
> > 
> 
> My comment is about the changelog, quoted above, saying "prevent compiler 
> from re-fetching mm->mmap_cache..."  ACCESS_ONCE(), as implemented, does 
> not prevent the compiler from re-fetching anything.  It is entirely 
> plausible that in gcc's current implementation that this guarantee is 
> made, but it is not prevented by the language standard and I think the 
> changelog should be reworded for anybody who reads it in the future.  
> There is a dependency here on gcc's implementation, it's a meaningful 
> distinction.
> 
> I never discouraged its use since for gcc's current implementation it 
> appears to work as desired and without gcc extensions there is no way to 
> make such a guarantee by the standard.  In fact, I acked a patch from Eric 
> Dumazet that fixes a NULL pointer dereference by using ACCESS_ONCE() with 
> gcc in slub.

This LWN comment from user "nix" is helpful here:

https://lwn.net/Articles/509731/

In particular:

	... volatile's meaning as 'minimize optimizations applied to
	things manipulating anything of volatile type, do not duplicate,
	elide, move, fold, spindle or mutilate' is of long standing.

So although I agree that the standard does not say as much as one might
like about volatile, ACCESS_ONCE()'s use of volatile should be expected
to work in a wide range of C compilers.  ACCESS_ONCE()'s use of typeof()
might not be quite so generally applicable, but a fair range of C
compilers do seem to support typeof() as well as ACCESS_ONCE()'s use
of volatile.

							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>

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

[Johannes Weiner]

On Tue, Apr 02, 2013 at 04:55:45PM -0700, David Rientjes wrote:
> On Tue, 2 Apr 2013, Hugh Dickins wrote:
> 
> > > > find_vma() can be called by multiple threads with read lock
> > > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > > readers could update it in the meantime:
> > > 
> > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
> > > mm->mmap_cache whatsoever; there is nothing that prevents this either in 
> > > the C standard.  You'll be relying solely on gcc's implementation of how 
> > > it dereferences volatile-qualified pointers.
> > 
> > Jan is using ACCESS_ONCE() as it should be used, for its intended
> > purpose.  If the kernel's implementation of ACCESS_ONCE() is deficient,
> > then we should fix that, not discourage its use.
> > 
> 
> My comment is about the changelog, quoted above, saying "prevent compiler 
> from re-fetching mm->mmap_cache..."  ACCESS_ONCE(), as implemented, does 
> not prevent the compiler from re-fetching anything.  It is entirely 
> plausible that in gcc's current implementation that this guarantee is 
> made, but it is not prevented by the language standard and I think the 
> changelog should be reworded for anybody who reads it in the future.  
> There is a dependency here on gcc's implementation, it's a meaningful 
> distinction.

The definition of ACCESS_ONCE() relies on gcc's current
implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE()
being defined.

Should it ever break you have to either fix it at the implementation
level or remove/replace the abstraction in its entirety, how does the
individual callsite matter in this case?

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

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

[David Rientjes]

On Tue, 2 Apr 2013, Paul E. McKenney wrote:

> So although I agree that the standard does not say as much as one might
> like about volatile, ACCESS_ONCE()'s use of volatile should be expected
> to work in a wide range of C compilers.  ACCESS_ONCE()'s use of typeof()
> might not be quite so generally applicable, but a fair range of C
> compilers do seem to support typeof() as well as ACCESS_ONCE()'s use
> of volatile.
> 

Agreed and I have nothing against code that uses it in that manner based 
on the implementations of those compilers.  The _only_ thing I've said in 
this thread is that ACCESS_ONCE() does not "prevent the compiler from 
re-fetching."  The only thing that is going to prevent the compiler from 
doing anything is the standard and, as you eluded, it's legal for a 
compiler to compile code such as 

	vma = ACCESS_ONCE(mm->mmap_cache);
	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
		return vma;

to be equivalent as if it had been written

	if (mm->mmap_cache && mm->mmap_cache->vm_start <= addr &&
	    mm->mmap_cache->vm_end > addr)
		return mm->mmap_cache;

and still be a conforming implementation.  We know gcc doesn't do that, so 
nobody is arguing the code in this patch as being incorrect.  In fact, to 
remove any question about it:

Acked-by: David Rientjes <rientjes@google.com>

However, as originally stated, I would prefer that the changelog be 
reworded so nobody believes ACCESS_ONCE() prevents the compiler from 
re-fetching anything.

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

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

[David Rientjes]

On Wed, 3 Apr 2013, Johannes Weiner wrote:

> The definition of ACCESS_ONCE() relies on gcc's current
> implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE()
> being defined.
> 
> Should it ever break you have to either fix it at the implementation
> level or remove/replace the abstraction in its entirety, how does the
> individual callsite matter in this case?
> 

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.  I'd much rather it refer 
to gcc's implementation, which we're counting on here, to avoid any 
confusion since I know a couple people have thought that ACCESS_ONCE() 
forces the compiler to load memory onto the stack and that belief is 
completely and utterly wrong.

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

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

[Johannes Weiner]

On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote:
> On Wed, 3 Apr 2013, Johannes Weiner wrote:
> 
> > The definition of ACCESS_ONCE() relies on gcc's current
> > implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE()
> > being defined.
> > 
> > Should it ever break you have to either fix it at the implementation
> > level or remove/replace the abstraction in its entirety, how does the
> > individual callsite matter in this case?
> > 
> 
> 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!

> I'd much rather it refer to gcc's implementation, which we're
> counting on here,

No, we really don't.  There is no

  "(*(volatile typeof(x)*)&(x))"

anywhere in this patch.

> to avoid any confusion since I know a couple people have thought
> that ACCESS_ONCE() forces the compiler to load memory onto the stack
> and that belief is completely and utterly wrong.

Maybe "forces the compiler to load memory onto the stack" should be
removed from the documentation of ACCESS_ONCE() then?

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

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

[David Rientjes]

On Wed, 3 Apr 2013, Johannes Weiner 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!
> 

The C99 and earlier C standards do not provide any way of "preventing the 
compiler from refetching accesses," and in fact C99 leaves an access to a 
volatile qualified object as implementation defined.  (If you disagree, 
then specify what exactly about ACCESS_ONCE() prevents the compiler from 
doing so.)

I agree that comment is confusing unless you specify that gcc's 
implementation provides that guarantee and I would tend to agree with 
Paul's assessment that the wide majority (all?) of compilers do the same.  
I would hesitate to say positively that gcc will continue to implement 
anything in the future other than what the standard specifies, though.  
But I do agree that the comment is confusing.

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

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

[Jakub Jelinek]

On Tue, Apr 02, 2013 at 03:33:58PM -0700, David Rientjes wrote:
> On Tue, 2 Apr 2013, Jan Stancek wrote:
> 
> > find_vma() can be called by multiple threads with read lock
> > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > Prevent compiler from re-fetching mm->mmap_cache, because other
> > readers could update it in the meantime:
> > 
> 
> FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch 
> mm->mmap_cache whatsoever; there is nothing that prevents this either in 
> the C standard.  You'll be relying solely on gcc's implementation of how 
> it dereferences volatile-qualified pointers.

FYI, the volatile access can be also unnecessarily pessimizing, other
projects like glibc use a friendlier alternative to the kernel's
ACCESS_ONCE.  In glibc it is:
# define atomic_forced_read(x) \
  ({ __typeof (x) __x; __asm ("" : "=r" (__x) : "0" (x)); __x; })
(could as well use "=g" instead).  This isn't volatile, so it can be
scheduled freely but if you store the result of it in a local variable and
use only that local variable everywhere in the function (and don't modify
it), there is a guarantee that you'll always see the same value.
The above should work fine with any GCC versions that can be used to compile
kernel.

Or of course, starting with GCC 4.7 you have also the alternative to use
# define ATOMIC_ONCE(x) __atomic_load_n (&x, __ATOMIC_RELAXED)

	Jakub

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

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

[Ian Lance Taylor]

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.

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.

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)

Ian

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

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

[Johannes Weiner]

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.

Who cares about the implementation, we are discussing a user here.
ACCESS_ONCE() isolates a problem so that the users don't have to think
about it, that's the whole point of abstraction.  ACCESS_ONCE() is an
opaque building block that says it prevents the compiler from merging
and refetching accesses.  That's all we care about right now.

It may rely on compiler-specific behavior to achieve this, and its
implementation may change if the underlying compilers change, but this
will not affect the promise that it makes and so this is off-topic.
This patch uses "ACCESS_ONCE()" and not "<compiler-specific tricks>".

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

I am operating under the assumption that people compile their kernels
with a subset of "most compilers" and not the C standard.

[ Actually, I just tried to imagine how you would compile the kernel
  using the C standard instead of a compiler and that may have popped
  a blood vessel in my eye. ]

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

This is good to know but the implementation details of ACCESS_ONCE()
are irrelevant here.

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

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

[Paul E. McKenney]

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>

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

[Paul E. McKenney]

On Tue, Apr 02, 2013 at 09:21:49PM -0700, David Rientjes wrote:
> On Tue, 2 Apr 2013, Paul E. McKenney wrote:
> 
> > So although I agree that the standard does not say as much as one might
> > like about volatile, ACCESS_ONCE()'s use of volatile should be expected
> > to work in a wide range of C compilers.  ACCESS_ONCE()'s use of typeof()
> > might not be quite so generally applicable, but a fair range of C
> > compilers do seem to support typeof() as well as ACCESS_ONCE()'s use
> > of volatile.
> > 
> 
> Agreed and I have nothing against code that uses it in that manner based 
> on the implementations of those compilers.  The _only_ thing I've said in 
> this thread is that ACCESS_ONCE() does not "prevent the compiler from 
> re-fetching."  The only thing that is going to prevent the compiler from 
> doing anything is the standard and, as you eluded, it's legal for a 
> compiler to compile code such as 
> 
> 	vma = ACCESS_ONCE(mm->mmap_cache);
> 	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
> 		return vma;
> 
> to be equivalent as if it had been written
> 
> 	if (mm->mmap_cache && mm->mmap_cache->vm_start <= addr &&
> 	    mm->mmap_cache->vm_end > addr)
> 		return mm->mmap_cache;
> 
> and still be a conforming implementation.  We know gcc doesn't do that, so 
> nobody is arguing the code in this patch as being incorrect.  In fact, to 
> remove any question about it:
> 
> Acked-by: David Rientjes <rientjes@google.com>

Thank you!

> However, as originally stated, I would prefer that the changelog be 
> reworded so nobody believes ACCESS_ONCE() prevents the compiler from 
> re-fetching anything.

If you were to instead say:

	However, as originally stated, I would prefer that the changelog
	be reworded so nobody believes that the C standard guarantees that
	volatile casts prevent the compiler from re-fetching anything.

I might agree with you.  But ACCESS_ONCE() really is defined to prevent
the compiler from refetching anything.  If a new version of gcc appears
for which volatile casts does not protect against refetching, then we
will change either (1) gcc or (2) the implementation of ACCESS_ONCE().
Whatever is needed to provide the guarantee against refetching.  The
Linux kernel absolutely needs -something- that provides this guarantee.

							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>

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

[Paul E. McKenney]

On Wed, Apr 03, 2013 at 09:33:48AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote:

[ . . . ]

> > 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 addition, even if the semantics of relaxed loads now guarantee against
load combining, note that ACCESS_ONCE() is also used to prevent combining
and splitting of stores, for example:

	ACCESS_ONCE(p) = give_me_a_pointer();

							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>

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

[Ian Lance Taylor]

On Wed, Apr 3, 2013 at 9:33 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote:
>
>> 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.

That was just a digression to explain why the standard does not need
to define the behaviour of volatile-qualified pointers.


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

Yes, I'm sorry, you are right.  It will work in practice today but
you're quite right that there is no reason to think that it will work
in principle.

This need suggests that GCC needs a new builtin function to implement
the functionality that you want.  Would you consider opening a request
for that at http://gcc.gnu.org/bugzilla/ ?

Ian

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

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

[Paul E. McKenney]

On Wed, Apr 03, 2013 at 10:47:28AM -0700, Ian Lance Taylor wrote:
> On Wed, Apr 3, 2013 at 9:33 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote:
> >
> >> 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.
> 
> That was just a digression to explain why the standard does not need
> to define the behaviour of volatile-qualified pointers.
> 
> 
> >> 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().
> 
> Yes, I'm sorry, you are right.  It will work in practice today but
> you're quite right that there is no reason to think that it will work
> in principle.
> 
> This need suggests that GCC needs a new builtin function to implement
> the functionality that you want.  Would you consider opening a request
> for that at http://gcc.gnu.org/bugzilla/ ?

How about a request for gcc to formally honor the current uses of volatile?

							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>

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

[Ian Lance Taylor]

On Wed, Apr 3, 2013 at 3:11 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> How about a request for gcc to formally honor the current uses of volatile?

Seems harder to define, but, sure, if it can be made to work.

Ian

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

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

[David Rientjes]

On Wed, 3 Apr 2013, Johannes Weiner wrote:

> Who cares about the implementation, we are discussing a user here.
> ACCESS_ONCE() isolates a problem so that the users don't have to think
> about it, that's the whole point of abstraction.  ACCESS_ONCE() is an
> opaque building block that says it prevents the compiler from merging
> and refetching accesses.  That's all we care about right now.
> 

The discussion is about the implementation of ACCESS_ONCE().  Nobody, thus 
far, has said anything about this specific patch other than me when I 
acked it.  I didn't start another thread off this patch because the 
changelog is relying on the comment above ACCESS_ONCE() to say that this 
"prevents the compiler from refetching."  Some have been confused by this 
comment and accept it at face value that it's really preventing the 
compiler from doing something; in reality, it's "using gcc's 
current implementation to prevent refetching."  It's an important 
distinction for anyone who comes away from the comment believing that 
volatile-qualified pointer dereferences are forbidden to be refetched by 
the compiler.

Others have said that I've somehow discouraged its use because its somehow 
an invalid way of preventing gcc from refetching (when I've acked patches 
that do this exact thing in slub!).  I have no idea how anyone could parse 
anything I've said about discouraging its use.  I simply noted that the 
changelog could be reworded to be clearer since we're not relying on any 
standard here but rather a compiler's current implementation.  It was 
never intended to be a long lengthy thread, but the comment I made is 
correct.  I've worked with Paul to make that clearer in the comment so 
that people aren't confused in the future.

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

[patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

The dereference of a volatile-qualified pointer does not guarantee that it
cannot be optimized by the compiler to be loaded multiple times into
memory even if assigned to a local variable by C99 or any previous C
standard.

Clarify the comment of ACCESS_ONCE() to state explicitly that its current
form relies on the compiler's implementation to work correctly.

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/compiler.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -348,6 +348,15 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  * merging, or refetching absolutely anything at any time.  Its main intended
  * use is to mediate communication between process-level code and irq/NMI
  * handlers, all running on the same CPU.
+ *
+ * The current implementation of ACCESS_ONCE() works in all mainstream C
+ * compilers (including of course gcc), but is not guaranteed by the C standard.
+ * However, there is a lot of software that depends on the semantics of volatile
+ * casts, so we have good reason to believe that there will always be a way of
+ * implementing ACCESS_ONCE().  The implementation of ACCESS_ONCE() might well
+ * change to track the C standard and the mainstream C compilers, so if you copy
+ * this definition into some other code base, be sure to check back here
+ * periodically for changes.
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[Linus Torvalds]

On Wed, Apr 3, 2013 at 5:00 PM, David Rientjes <rientjes@google.com> wrote:
> The dereference of a volatile-qualified pointer does not guarantee that it
> cannot be optimized by the compiler to be loaded multiple times into
> memory even if assigned to a local variable by C99 or any previous C
> standard.
>
> Clarify the comment of ACCESS_ONCE() to state explicitly that its current
> form relies on the compiler's implementation to work correctly.

This is utter bullshit and garbage.

Any compiler that thinks it can load something through a "volatile"
pointer multiple times is SHIT. We don't add these kinds of comments
to make excuses for crap, we call out such compilers and tell people
not to use the utter crap they are.

The fact is, "volatile" is pretty much the only thing that can do this
(and there are no sane alternate semantics that a compiler *could* use
for a volatile cast), and no amount of weasel-wording by compiler
apologists makes it not so. I'm not a huge fan of volatile as a C
feature, but I'm even less of a fan of people trying to make excuses
for bad compilers.

Is there a reason why you want to add this idiotic comment? Is there a
compiler that actually dismisses a volatile cast? If so, warn us about
that kind of crap, and I'll happily make it very clear that people
should not use the piece-of-shit compiler in question. Making excuses
for that kind of compiler behavior is absolutely the last thing we
should do.

                  Linus

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

On Wed, 3 Apr 2013, Linus Torvalds wrote:

> Any compiler that thinks it can load something through a "volatile"
> pointer multiple times is SHIT. We don't add these kinds of comments
> to make excuses for crap, we call out such compilers and tell people
> not to use the utter crap they are.
> 

How nice to have reconvened after six years when in February of 2007 you 
slammed me for relying on the implementation of a particular compiler on 
the sign of a bitfield and referred multiple times to the standard and how 
it's implementation defined.  I don't want to go down that road again, 
even though the poster of quotes we had on the wall at work was amusing 
for awhile.

The specification here says an access to this volatile quaified pointer is 
implementation defined.  It also allows for the pointer to be reloaded and 
the local variable optimized out.  Do people need to know that?  
Debatable.  But there are misconceptions of what a volatile-qualified 
pointer can do.

This patch was an attempt to clarify for those who didn't understand that 
we're relying on the implementation of the compiler to not optimize the 
loads from memory out.  The way the comment is currently written, stating 
that it is "preventing the compiler from refetching" can literally mean 
one of two things: it actually does something based on specification that 
causes these objects to not be reloaded, or it is using the way 
volatile-qualified pointers are dereferenced in gcc and other compilers to 
prevent it.  I'm pretty darn sure Paul was referring to the latter when he 
wrote it, which is completely 100% correct, but other readers of the 
comment have taken it to mean the former and it causes misconceptions of 
the keyword.

That said, I'm not interested in arguing with you on the meaning of the 
word "prevent" in the ACCESS_ONCE() comment nor in the changelog of the 
patch that sparked this discussion.  We're talking about English here, not 
C, and the goal was to make it explicit.  If you take offense to that, 
owell.

Ian has suggested that we file a request with the gcc team for the exact 
semantics we're asking for from ACCESS_ONCE() so that we don't need to 
rely on any particular compiler's implementation and can standardize this 
at least for gcc.

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[Linus Torvalds]

On Wed, Apr 3, 2013 at 6:52 PM, David Rientjes <rientjes@google.com> wrote:
>
> The specification here says an access to this volatile quaified pointer is
> implementation defined

.. and my argument is that we don't care about paper standards, we
care about QUALITY OF IMPLEMENTATION.

If a compiler messes up volatile casts, the quality of implementation
is bad. There's just no excuse.

The compiler people can talk about how the paper standard allows it
until the cows come home. Why should we care? The compiler person is
still just making excuses for a bad implementation.

There is no sane alternative semantics to "volatile" that I can come
up with. Seriously. What meaning could "volatile" ever have that would
be sensible and break this?

Now, I do repeat: I don't like volatile. I think it has many problems,
and being underspecified is just one of them (the much deeper problem
is that the C standard attaches it to the data, not to the code, and
we then have to "fix" that by mis-using it as a cast).

So if some improved standard comes along, I'd happily use that. In the
meantime, we don't have any choice, do we? Seriously, you can talk
about paper standards until you are blue in the face, but since there
is no sane alternative to the volatile cast, what's the point, really?

                    Linus

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

On Wed, 3 Apr 2013, Linus Torvalds wrote:

> .. and my argument is that we don't care about paper standards, we
> care about QUALITY OF IMPLEMENTATION.
> 
> If a compiler messes up volatile casts, the quality of implementation
> is bad. There's just no excuse.
> 

I agreed, and I agreed when we had a "discussion" about the sign of a 
bitfield six years ago, but the key here is that I'm talking about meaning 
of the comment and not the compiler.  I'm trying to clear up any 
misconception that people have, and that's why it's a patch that modifies 
a comment and not source.

Those misconceptions exist, like it or not.  People have seen this 
ACCESS_ONCE() abstraction and think they can use it to solve concurrency 
issues in their own userland code with dereferences to shared objects.

> There is no sane alternative semantics to "volatile" that I can come
> up with. Seriously. What meaning could "volatile" ever have that would
> be sensible and break this?
> 
> Now, I do repeat: I don't like volatile. I think it has many problems,
> and being underspecified is just one of them (the much deeper problem
> is that the C standard attaches it to the data, not to the code, and
> we then have to "fix" that by mis-using it as a cast).
> 

I know you do and I know you contributed to having an entire 
volatile-considered-harmful doc in the tree, which is really a lecture on 
C rather than having anything specific to do with the kernel itself.  So 
I'm a little surpised that you wouldn't opt for being 100% explicit in one 
of its rare appearances.  It's easy to see you're no longer amused.

> So if some improved standard comes along, I'd happily use that. In the
> meantime, we don't have any choice, do we? Seriously, you can talk
> about paper standards until you are blue in the face, but since there
> is no sane alternative to the volatile cast, what's the point, really?
> 

Would you convert the definition of ACCESS_ONCE() to use the resulting 
feature from the gcc folks that would actually guarantee it in the 
compiler-gcc.h files?

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[Linus Torvalds]

On Wed, Apr 3, 2013 at 7:18 PM, David Rientjes <rientjes@google.com> wrote:
>
> Would you convert the definition of ACCESS_ONCE() to use the resulting
> feature from the gcc folks that would actually guarantee it in the
> compiler-gcc.h files?

So I wouldn't object for any other reason than the fact that it makes
me feel like I'm helping somebody screw up "volatile", and then we
would help cover up that serious compiler quality regression.

So I do repeat: what kind of messed-up compiler could *possibly* do
the wrong thing for our current use of accessing a volatile pointer,
and not consider that a compiler bug? Why should be support such a
fundamentally broken agenda?

Yes, yes, I know all about how compiler people will talk about "access
to volatile pointer" vs "actual volatile *object*", as if that made
things fundamentally different. I personally don't think it makes any
difference what-so-ever, and a quality compiler shouldn't either. Yes,
the compiler can get confused about aliasing, but hey, if it doesn't
keep track of the volatile status of some pointer access, then it's
going to get confused about aliasing elsewhere when it passes down
pointer to an actual volatile object in a union or whatever. So a good
C compiler really wants to get that kind of thing right *anyway*.

IOW, I'm not seeing a huge upside, and I *am* seeing downsides.  Why
should we encourage bad C implementations? If the compiler people
understand that threading (as well as just IO accesses) do actually
need the whole "access once" semantics, and have support for that in
their compiler anyway, why aren't they just turning "volatile" into
that?

IOW, my argument is that a *good* C compiler writer would acknowledge that:

  "Yes, the language specs may allow me to quibble about what the
meaning of the word "object" is, but I also realize that people who do
threading and IO accesses need a way to specify "access once" through
a pointer without any regard to what the "underlying object" -
whatever that is - is, so I might as well interpret the meaning of
"object" to include the known needed semantics of access through a
pointer, and turn it into this internal interface that I have to
expose *anyway*".

So instead of encouraging people to rely on some strange
compiler-dependent crap, why not just admit that the C standard
*could* also be read the way the language was clearly meant to be
read, without any stupid quibbling? And instead of the
compiler-specific thing, say "Our compiler is a *good* compiler, and
we took the C standard language and read it in the most useful way
possible, and made our compiler *better*".

IOW, instead of going down the whole idiotic language-lawyer dead end,
just DTRT. What is the advantage to *anybody* - C compiler writers
included - in making "volatile" less useful, and blathering about bad
specifications.

This is my "quality of implementation" argument. A C compiler
shouldn't try to screw over its users. If there is some undefined
corner-case, try to define it as usefully as possible. And for
"volatile pointer accesses", there really are clear and unambiguous
good and useful interpretations, which just say "we consider the
volatile pointer access to be a volatile object, and we won't be
quibbling and trying to argue about the meaning of "object"".

                     Linus

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

On Wed, 3 Apr 2013, Linus Torvalds wrote:

> > Would you convert the definition of ACCESS_ONCE() to use the resulting
> > feature from the gcc folks that would actually guarantee it in the
> > compiler-gcc.h files?
> 
> So I wouldn't object for any other reason than the fact that it makes
> me feel like I'm helping somebody screw up "volatile", and then we
> would help cover up that serious compiler quality regression.
> 

I'm surprised you would object to using a new builtin with well-defined 
semantics that would actually specify what ACCESS_ONCE() wants to do.  
Owell, it seems this is becoming philosophical.

> So I do repeat: what kind of messed-up compiler could *possibly* do
> the wrong thing for our current use of accessing a volatile pointer,
> and not consider that a compiler bug? Why should be support such a
> fundamentally broken agenda?
> 

This is tangential to the issue of trying to clarify the ACCESS_ONCE() 
comment, as that discussion was tangential to the patch actually being 
discussed at the time.  You answered it yourself, though, when talking 
about the difference between dereferencing a volatile pointer and 
accessing a volatile object.  You hate the quibbling, but are asking for a 
quibble.

	unsigned long local_foo = ACCESS_ONCE(foo);
	unsigned long local_bar = ACCESS_ONCE(bar);

I believe a "sane" compiler can load bar before foo and not be a bug, and 
this is allowed from the C99 perspective since we're not talking about an 
access of a volatile object, thus no side effect.  This is the quibbling 
neither of us want to get involved in, so why discuss it?

The comment of ACCESS_ONCE() says "the compiler is also forbidden from 
reordering successive instances of ACCESS_ONCE(), but only when the 
compiler is aware of some particular ordering.  One way to make the 
compiler aware of ordering is to put the two invocations of ACCESS_ONCE() 
in different C statements."

That example is successive instances of ACCESS_ONCE() and in different C 
statements, but the compiler is not forbidden to reorder them.  Does 
anyone in the kernel do this and actually think it provides a cheap memory 
barrier?  Doubt it, but it's not a compiler bug to reorder them.

> IOW, I'm not seeing a huge upside, and I *am* seeing downsides.  Why
> should we encourage bad C implementations? If the compiler people
> understand that threading (as well as just IO accesses) do actually
> need the whole "access once" semantics, and have support for that in
> their compiler anyway, why aren't they just turning "volatile" into
> that?
> 

Nobody is encouraging gcc or any other compiler to change the semantics 
and I highly doubt anything ever would, even if we do adopt a gcc builtin.  
But I do believe, if nothing more, that it would clear up this confusion 
around the volatile in ACCESS_ONCE()'s definition.  If you'd like to do 
that with a comment instead, that'd be great.  I proposed such a comment.

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[Linus Torvalds]

On Wed, Apr 3, 2013 at 11:02 PM, David Rientjes <rientjes@google.com> wrote:
>
> I'm surprised you would object to using a new builtin with well-defined
> semantics that would actually specify what ACCESS_ONCE() wants to do.

Why? We'd still have to have the volatile for old compilers, so it's
actually adding unnecessary code. It would be gcc-only, so we'd have
to protect it for other compilers too.

And it wouldn't add any advantage, since "volatile" would still work
for any quality compiler. So why do it?

If it was some *standard* feature that was specified by a real
standards body, and likely to become real in a few years, then that in
itself might be an argument. But as long as it's some random gcc
built-in that just does the same thing as a volatile access, what's
the upside? Seriously?

>         unsigned long local_foo = ACCESS_ONCE(foo);
>         unsigned long local_bar = ACCESS_ONCE(bar);
>
> I believe a "sane" compiler can load bar before foo and not be a bug,

That's bullshit. The whole definition of volatile is about being
visible as an access in the virtual machine, and having externally
visible side effects. Two volatiles cannot be re-ordered wrt each
other, and arguably with the traditional explanation for what it's all
about, you can't even re-order them wrt global memory accesses.

In fact, one sane semantic for what "volatile" means from a compiler
standpoint is to say that a volatile access aliases with all other
accesses (including other accesses to the same thing). That's one of
the saner approaches for compilers to handle volatile, since a C
compiler wants to have a notion of aliasing anyway (and C also has the
notion of type-based aliasing in general).

So no, a "sane" compiler cannot reorder the two.

An insane one could, but it is clearly against the spirit of what
"volatile" means. I believe it's against the letter too, but as
mentioned, compiler people love to quibble about what the meaning of
"is" is.

> The comment of ACCESS_ONCE() says "the compiler is also forbidden from
> reordering successive instances of ACCESS_ONCE(), but only when the
> compiler is aware of some particular ordering.  One way to make the
> compiler aware of ordering is to put the two invocations of ACCESS_ONCE()
> in different C statements."

Correct. That's the whole "sequence point thing". If there's a
sequence point in between (and if they are in separate C statements,
there is) a sane compiler cannot re-order them.

The comment is correct, you are just confused.

                   Linus

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

On Thu, 4 Apr 2013, Linus Torvalds wrote:

> > I'm surprised you would object to using a new builtin with well-defined
> > semantics that would actually specify what ACCESS_ONCE() wants to do.
> 
> Why? We'd still have to have the volatile for old compilers, so it's
> actually adding unnecessary code. It would be gcc-only, so we'd have
> to protect it for other compilers too.
> 
> And it wouldn't add any advantage, since "volatile" would still work
> for any quality compiler. So why do it?
> 
> If it was some *standard* feature that was specified by a real
> standards body, and likely to become real in a few years, then that in
> itself might be an argument. But as long as it's some random gcc
> built-in that just does the same thing as a volatile access, what's
> the upside? Seriously?
> 

I said in the previous email that you'd do this solely to rely on a 
well-defined semantic rather than reading paragraphs of comments that 
we're developing.  I personally don't care either way, but I would really 
like to avoid this type of "why can't I use volatile? Go read 
volatile-considered-harmful" and "why can ACCESS_ONCE() use volatile, 
then? Go read this email thread" because the current comment is a bit 
confusing in a few different areas.

> >         unsigned long local_foo = ACCESS_ONCE(foo);
> >         unsigned long local_bar = ACCESS_ONCE(bar);
> >
> > I believe a "sane" compiler can load bar before foo and not be a bug,
> 
> That's bullshit. The whole definition of volatile is about being
> visible as an access in the virtual machine, and having externally
> visible side effects. Two volatiles cannot be re-ordered wrt each
> other, and arguably with the traditional explanation for what it's all
> about, you can't even re-order them wrt global memory accesses.
> 
> In fact, one sane semantic for what "volatile" means from a compiler
> standpoint is to say that a volatile access aliases with all other
> accesses (including other accesses to the same thing). That's one of
> the saner approaches for compilers to handle volatile, since a C
> compiler wants to have a notion of aliasing anyway (and C also has the
> notion of type-based aliasing in general).
> 
> So no, a "sane" compiler cannot reorder the two.
> 

This is the quibbling that we both want to avoid, but the quibble you keep 
asking to get and now we've arrived at my definition of a sane compiler vs 
insane compiler.  I don't see this as mattering since you're already 
receptive to changing the comment.  In this case, we're again talking 
about volatile-qualified pointers vs volatile objects, and this is the 
area you hate.  That's why I believe a "slightly less than sane" compiler 
with a conforming implementation can do so without being buggy.  It's not 
what gcc or any other known compiler does, and I don't think it will ever 
change, so I don't see the point in discussing these theoretical 
compilers and whether they conform or not.

That said, I know for sure that the comment can be improved because there 
does exist a misconception about it.  An illustration: there was one 
person on the to: list of the email when I replied to it initially and 
everybody else has either injected themselves or were cc'd by someone else 
(admittedly, I brought you in because we had thought the comment was 
improved but you weren't amused).  This whole long thread started with me 
saying simply "ACCESS_ONCE() does not guarantee that the compiler will not 
refetch mm->mmap_cache whatsoever; there is nothing that prevents this 
in the C standard.  You'll be relying solely on gcc's implementation of 
how it dereferences volatile-qualified pointers."  That statement is 
completely 100% correct yet there was this scary confusion about it and 
referencing of the comment to justify some idea that it's a cheap memory 
barrier.

I know you're now receptive to changing the comment in some ways and I've 
proposed such a comment to you, and I admit there may have been some 
confusion about the intention of the patch that I proposed.  You probably 
also saw compiler people cc'd and thought I was defending their ability to 
change this behavior whenever they wanted.  If you actually read the 
originating thread, though, you'll see the only goal the entire time was 
to increase the understanding.  For someone who mostly wrote 
Documentation/volatile-considered-harmful.txt, I think you'd appreciate 
that clarify for one of its rare appearances in the kernel.

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[Linus Torvalds]

On Thu, Apr 4, 2013 at 12:40 PM, David Rientjes <rientjes@google.com> wrote:
>
> I said in the previous email that you'd do this solely to rely on a
> well-defined semantic rather than reading paragraphs of comments that
> we're developing.

What's "well-defined" about it? It's implementation-defined in both cases.

IOW, why do you think "__builtin_access_once(x)" is fundamentally
different from "(*(volatile type *)&(x))"? Both would be equally
dependent on the compiler implementation, and I'd argue that it would
be much nicer if gcc just automatically turned the existing volatile
code internally into the builtin version (and then didn't even bother
to expose that builtin), since if they are willing to do the built-in,
they clearly acknowledge the need for this kind of behavior in the
first place.

See what I'm arguing? If a compiler writer is acknowledging that this
kind of "access once with good semantics through a pointer" is needed
and useful (and in the presense of IO and threading, a compiler writer
that doesn't acknowledge that is a moron), then _why_ would that same
compiler writer then argue against just doing that for volatile
pointers?

What's so magically bad about "volatile" that would be solved by a
totally new and nonstandard builtin?

                    Linus

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

Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation

[David Rientjes]

On Thu, 4 Apr 2013, Linus Torvalds wrote:

> > I said in the previous email that you'd do this solely to rely on a
> > well-defined semantic rather than reading paragraphs of comments that
> > we're developing.
> 
> What's "well-defined" about it? It's implementation-defined in both cases.
> 
> IOW, why do you think "__builtin_access_once(x)" is fundamentally
> different from "(*(volatile type *)&(x))"? Both would be equally
> dependent on the compiler implementation, and I'd argue that it would
> be much nicer if gcc just automatically turned the existing volatile
> code internally into the builtin version (and then didn't even bother
> to expose that builtin), since if they are willing to do the built-in,
> they clearly acknowledge the need for this kind of behavior in the
> first place.
> 

Agreed, and the gcc community can do that if they choose, and even better 
if they explicitly state that they are doing this for the traditional 
meaning (yet not standard defined) of volatile.  For nothing else, it 
would give me something to cite in the comment I proposed to you.

> See what I'm arguing? If a compiler writer is acknowledging that this
> kind of "access once with good semantics through a pointer" is needed
> and useful (and in the presense of IO and threading, a compiler writer
> that doesn't acknowledge that is a moron), then _why_ would that same
> compiler writer then argue against just doing that for volatile
> pointers?
> 

Absolutely, it would be great.

This is why I wanted to clarify the comment of ACCESS_ONCE(), however, 
that states it's "preventing the compiler" and "forbidding the compiler" 
from doing certain things, because it doesn't.  That's the whole 
discussion.  For someone interested in C, one of the natural places they 
are going to look in the source is compiler.h, and especially when they 
see this magical ACCESS_ONCE() floating around which is seemingly a cheap 
memory barrier with other ACCESS_ONCE() references and I think we can do 
better, as I did in my initial reply which started this thread, to say 
this isn't in the standard and we're relying on gcc's implementation for 
this behavior.

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

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

[Paul E. McKenney]

On Wed, Apr 03, 2013 at 03:28:35PM -0700, Ian Lance Taylor wrote:
> On Wed, Apr 3, 2013 at 3:11 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > How about a request for gcc to formally honor the current uses of volatile?
> 
> Seems harder to define, but, sure, if it can be made to work.

Actually, I am instead preparing to take this up with the standards
committee.  Even those who hate volatile (of which there are many) have
an interest in a good codification of the defacto definition of volatile.
After all, without a definition how can they hope to replace it?  ;-)
And the C committee cares deeply about backwards compatibility, so
getting a good definition will help there as well.

							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>

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

[Paul E. McKenney]

On Thu, Apr 04, 2013 at 12:01:52PM -0700, Hugh Dickins wrote:
> On Thu, 4 Apr 2013, Linus Torvalds wrote:
> > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
> > >
> > > find_vma() can be called by multiple threads with read lock
> > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > readers could update it in the meantime:
> > 
> > Ack. I do wonder if we should mark the unlocked update too some way
> > (also in find_vma()), although it's probably not a problem in practice
> > since there's no way the compiler can reasonably really do anything
> > odd with it. We *could* make that an ACCESS_ONCE() write too just to
> > highlight the fact that it's an unlocked write to this optimistic data
> > structure.
> 
> Hah, you beat me to it.
> 
> I wanted to get Jan's patch in first, seeing as it actually fixes his
> observed issue; and it is very nice to have such a good description of
> one of those, when ACCESS_ONCE() is usually just an insurance policy.
> 
> But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage
> (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and
> sound/firewire, but few places else).
> 
> When Paul reminded us of it yesterday, I came to wonder if actually
> every use of ACCESS_ONCE in the read form should strictly be matched
> by ACCESS_ONCE whenever modifying the location.

>From a hygiene/insurance/documentation point of view, I agree.  Of course,
it is OK to use things like cmpxchg() in place of ACCESS_ONCE().

The possible exceptions that come to mind are (1) if the access in
question is done holding a lock that excludes all other accesses to that
location, (2) if the access in question happens during initialization
before any other CPU has access to that location, and (3) if the access
in question happens during cleanup after all other CPUs have lost access
to that location.  Any others?

/me goes to look to see if the RCU code follows this good advice...

							Thanx, Paul

> My uneducated guess is that strictly it ought to, in the sense of
> insurance policy; but that (apart from that strange split writing
> issue which came up a couple of months ago) in practice our compilers
> have not "advanced" to the point of making this an issue yet.
> 
> > 
> > Anyway, applied.
> 
> Thanks,
> Hugh
> 

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

[Paul E. McKenney]

On Thu, Apr 04, 2013 at 12:01:52PM -0700, Hugh Dickins wrote:
> On Thu, 4 Apr 2013, Linus Torvalds wrote:
> > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
> > >
> > > find_vma() can be called by multiple threads with read lock
> > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > readers could update it in the meantime:
> > 
> > Ack. I do wonder if we should mark the unlocked update too some way
> > (also in find_vma()), although it's probably not a problem in practice
> > since there's no way the compiler can reasonably really do anything
> > odd with it. We *could* make that an ACCESS_ONCE() write too just to
> > highlight the fact that it's an unlocked write to this optimistic data
> > structure.
> 
> Hah, you beat me to it.
> 
> I wanted to get Jan's patch in first, seeing as it actually fixes his
> observed issue; and it is very nice to have such a good description of
> one of those, when ACCESS_ONCE() is usually just an insurance policy.
> 
> But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage
> (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and
> sound/firewire, but few places else).
> 
> When Paul reminded us of it yesterday, I came to wonder if actually
> every use of ACCESS_ONCE in the read form should strictly be matched
> by ACCESS_ONCE whenever modifying the location.

>From a hygiene/insurance/documentation point of view, I agree.  Of course,
it is OK to use things like cmpxchg() in place of ACCESS_ONCE().

The possible exceptions that come to mind are (1) if the access in
question is done holding a lock that excludes all other accesses to that
location, (2) if the access in question happens during initialization
before any other CPU has access to that location, and (3) if the access
in question happens during cleanup after all other CPUs have lost access
to that location.  Any others?

/me goes to look to see if the RCU code follows this good advice...

							Thanx, Paul

> My uneducated guess is that strictly it ought to, in the sense of
> insurance policy; but that (apart from that strange split writing
> issue which came up a couple of months ago) in practice our compilers
> have not "advanced" to the point of making this an issue yet.
> 
> > 
> > Anyway, applied.
> 
> Thanks,
> Hugh
> 

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

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

[Linus Torvalds]

On Thu, Apr 4, 2013 at 12:01 PM, Hugh Dickins <hughd@google.com> wrote:
>
> When Paul reminded us of it yesterday, I came to wonder if actually
> every use of ACCESS_ONCE in the read form should strictly be matched
> by ACCESS_ONCE whenever modifying the location.
>
> My uneducated guess is that strictly it ought to, in the sense of
> insurance policy; but that (apart from that strange split writing
> issue which came up a couple of months ago) in practice our compilers
> have not "advanced" to the point of making this an issue yet.

I don't see how a compiler could reasonably really ever do anything
different, but I do think the ACCESS_ONCE() modification version might
be a good thing just as a "documentation".

This is a good example of this issue, exactly because we have a mix of
both speculative cases (the find_vma() lookup and modification)
together with strictly exclusive locked accesses to the same field
(the ones that invalidate the cache under the write lock). So
documenting that the write in find_vma() is this kind of "optimistic
unlocked access" is actually a potentially interesting piece of
information for programmers, completely independently of whether the
compiler will then treat it really differently or not.

Of course, a plain comment would do the same, but would be less greppable.

And despite the verbiage here, I don't really have a very strong
opinion on this. I'm going to let it go, and if somebody sends me a
patch with a good explanation in the next merge window, I'll probably
apply it.

                    Linus

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

[Linus Torvalds]

On Thu, Apr 4, 2013 at 12:01 PM, Hugh Dickins <hughd@google.com> wrote:
>
> When Paul reminded us of it yesterday, I came to wonder if actually
> every use of ACCESS_ONCE in the read form should strictly be matched
> by ACCESS_ONCE whenever modifying the location.
>
> My uneducated guess is that strictly it ought to, in the sense of
> insurance policy; but that (apart from that strange split writing
> issue which came up a couple of months ago) in practice our compilers
> have not "advanced" to the point of making this an issue yet.

I don't see how a compiler could reasonably really ever do anything
different, but I do think the ACCESS_ONCE() modification version might
be a good thing just as a "documentation".

This is a good example of this issue, exactly because we have a mix of
both speculative cases (the find_vma() lookup and modification)
together with strictly exclusive locked accesses to the same field
(the ones that invalidate the cache under the write lock). So
documenting that the write in find_vma() is this kind of "optimistic
unlocked access" is actually a potentially interesting piece of
information for programmers, completely independently of whether the
compiler will then treat it really differently or not.

Of course, a plain comment would do the same, but would be less greppable.

And despite the verbiage here, I don't really have a very strong
opinion on this. I'm going to let it go, and if somebody sends me a
patch with a good explanation in the next merge window, I'll probably
apply it.

                    Linus

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

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

[Hugh Dickins]

On Thu, 4 Apr 2013, Linus Torvalds wrote:
> On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
> >
> > find_vma() can be called by multiple threads with read lock
> > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > Prevent compiler from re-fetching mm->mmap_cache, because other
> > readers could update it in the meantime:
> 
> Ack. I do wonder if we should mark the unlocked update too some way
> (also in find_vma()), although it's probably not a problem in practice
> since there's no way the compiler can reasonably really do anything
> odd with it. We *could* make that an ACCESS_ONCE() write too just to
> highlight the fact that it's an unlocked write to this optimistic data
> structure.

Hah, you beat me to it.

I wanted to get Jan's patch in first, seeing as it actually fixes his
observed issue; and it is very nice to have such a good description of
one of those, when ACCESS_ONCE() is usually just an insurance policy.

But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage
(popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and
sound/firewire, but few places else).

When Paul reminded us of it yesterday, I came to wonder if actually
every use of ACCESS_ONCE in the read form should strictly be matched
by ACCESS_ONCE whenever modifying the location.

My uneducated guess is that strictly it ought to, in the sense of
insurance policy; but that (apart from that strange split writing
issue which came up a couple of months ago) in practice our compilers
have not "advanced" to the point of making this an issue yet.

> 
> Anyway, applied.

Thanks,
Hugh

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

[Hugh Dickins]

On Thu, 4 Apr 2013, Linus Torvalds wrote:
> On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
> >
> > find_vma() can be called by multiple threads with read lock
> > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > Prevent compiler from re-fetching mm->mmap_cache, because other
> > readers could update it in the meantime:
> 
> Ack. I do wonder if we should mark the unlocked update too some way
> (also in find_vma()), although it's probably not a problem in practice
> since there's no way the compiler can reasonably really do anything
> odd with it. We *could* make that an ACCESS_ONCE() write too just to
> highlight the fact that it's an unlocked write to this optimistic data
> structure.

Hah, you beat me to it.

I wanted to get Jan's patch in first, seeing as it actually fixes his
observed issue; and it is very nice to have such a good description of
one of those, when ACCESS_ONCE() is usually just an insurance policy.

But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage
(popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and
sound/firewire, but few places else).

When Paul reminded us of it yesterday, I came to wonder if actually
every use of ACCESS_ONCE in the read form should strictly be matched
by ACCESS_ONCE whenever modifying the location.

My uneducated guess is that strictly it ought to, in the sense of
insurance policy; but that (apart from that strange split writing
issue which came up a couple of months ago) in practice our compilers
have not "advanced" to the point of making this an issue yet.

> 
> Anyway, applied.

Thanks,
Hugh

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

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

[Linus Torvalds]

On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
>
> find_vma() can be called by multiple threads with read lock
> held on mm->mmap_sem and any of them can update mm->mmap_cache.
> Prevent compiler from re-fetching mm->mmap_cache, because other
> readers could update it in the meantime:

Ack. I do wonder if we should mark the unlocked update too some way
(also in find_vma()), although it's probably not a problem in practice
since there's no way the compiler can reasonably really do anything
odd with it. We *could* make that an ACCESS_ONCE() write too just to
highlight the fact that it's an unlocked write to this optimistic data
structure.

Anyway, applied.

                  Linus

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

[Linus Torvalds]

On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote:
>
> find_vma() can be called by multiple threads with read lock
> held on mm->mmap_sem and any of them can update mm->mmap_cache.
> Prevent compiler from re-fetching mm->mmap_cache, because other
> readers could update it in the meantime:

Ack. I do wonder if we should mark the unlocked update too some way
(also in find_vma()), although it's probably not a problem in practice
since there's no way the compiler can reasonably really do anything
odd with it. We *could* make that an ACCESS_ONCE() write too just to
highlight the fact that it's an unlocked write to this optimistic data
structure.

Anyway, applied.

                  Linus

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

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

[Hugh Dickins]

From: Jan Stancek <jstancek@redhat.com>

find_vma() can be called by multiple threads with read lock
held on mm->mmap_sem and any of them can update mm->mmap_cache.
Prevent compiler from re-fetching mm->mmap_cache, because other
readers could update it in the meantime:

               thread 1                             thread 2
                                        |
  find_vma()                            |  find_vma()
    struct vm_area_struct *vma = NULL;  |
    vma = mm->mmap_cache;               |
    if (!(vma && vma->vm_end > addr     |
        && vma->vm_start <= addr)) {    |
                                        |    mm->mmap_cache = vma;
    return vma;                         |
     ^^ compiler may optimize this      |
        local variable out and re-read  |
        mm->mmap_cache                  |

This issue can be reproduced with gcc-4.8.0-1 on s390x by running
mallocstress testcase from LTP, which triggers:
  kernel BUG at mm/rmap.c:1088!
    Call Trace:
     ([<000003d100c57000>] 0x3d100c57000)
      [<000000000023a1c0>] do_wp_page+0x2fc/0xa88
      [<000000000023baae>] handle_pte_fault+0x41a/0xac8
      [<000000000023d832>] handle_mm_fault+0x17a/0x268
      [<000000000060507a>] do_protection_exception+0x1e2/0x394
      [<0000000000603a04>] pgm_check_handler+0x138/0x13c
      [<000003fffcf1f07a>] 0x3fffcf1f07a
    Last Breaking-Event-Address:
      [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168

Thanks to Jakub Jelinek for his insight on gcc and helping to
track this down.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---
This is Jan's valuable patch, posted a couple of days ago, which then
triggered "some discussion" of ACCESS_ONCE() on linux-mm.  I've marked
it for stable: should go back years, but 3.5 changed the indentation.

 mm/mmap.c  |    2 +-
 mm/nommu.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 6466699..0db0de1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 
 	/* Check the cache first. */
 	/* (Cache hit rate is typically around 35%.) */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) {
 		struct rb_node *rb_node;
 
diff --git a/mm/nommu.c b/mm/nommu.c
index e193280..2f3ea74 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 	struct vm_area_struct *vma;
 
 	/* check the cache first */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
 		return vma;
 
-- 
1.7.1

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

[Hugh Dickins]

From: Jan Stancek <jstancek@redhat.com>

find_vma() can be called by multiple threads with read lock
held on mm->mmap_sem and any of them can update mm->mmap_cache.
Prevent compiler from re-fetching mm->mmap_cache, because other
readers could update it in the meantime:

               thread 1                             thread 2
                                        |
  find_vma()                            |  find_vma()
    struct vm_area_struct *vma = NULL;  |
    vma = mm->mmap_cache;               |
    if (!(vma && vma->vm_end > addr     |
        && vma->vm_start <= addr)) {    |
                                        |    mm->mmap_cache = vma;
    return vma;                         |
     ^^ compiler may optimize this      |
        local variable out and re-read  |
        mm->mmap_cache                  |

This issue can be reproduced with gcc-4.8.0-1 on s390x by running
mallocstress testcase from LTP, which triggers:
  kernel BUG at mm/rmap.c:1088!
    Call Trace:
     ([<000003d100c57000>] 0x3d100c57000)
      [<000000000023a1c0>] do_wp_page+0x2fc/0xa88
      [<000000000023baae>] handle_pte_fault+0x41a/0xac8
      [<000000000023d832>] handle_mm_fault+0x17a/0x268
      [<000000000060507a>] do_protection_exception+0x1e2/0x394
      [<0000000000603a04>] pgm_check_handler+0x138/0x13c
      [<000003fffcf1f07a>] 0x3fffcf1f07a
    Last Breaking-Event-Address:
      [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168

Thanks to Jakub Jelinek for his insight on gcc and helping to
track this down.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---
This is Jan's valuable patch, posted a couple of days ago, which then
triggered "some discussion" of ACCESS_ONCE() on linux-mm.  I've marked
it for stable: should go back years, but 3.5 changed the indentation.

 mm/mmap.c  |    2 +-
 mm/nommu.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 6466699..0db0de1 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 
 	/* Check the cache first. */
 	/* (Cache hit rate is typically around 35%.) */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) {
 		struct rb_node *rb_node;
 
diff --git a/mm/nommu.c b/mm/nommu.c
index e193280..2f3ea74 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 	struct vm_area_struct *vma;
 
 	/* check the cache first */
-	vma = mm->mmap_cache;
+	vma = ACCESS_ONCE(mm->mmap_cache);
 	if (vma && vma->vm_start <= addr && vma->vm_end > addr)
 		return vma;
 
-- 
1.7.1

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