All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Rientjes <rientjes@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Ian Lance Taylor <iant@google.com>,
	Hugh Dickins <hughd@google.com>,
	Jan Stancek <jstancek@redhat.com>,
	Paul McKenney <paulmck@linux.vnet.ibm.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	linux-mm <linux-mm@kvack.org>
Subject: Re: [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation
Date: Thu, 4 Apr 2013 12:40:55 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.02.1304041209330.19501@chino.kir.corp.google.com> (raw)
In-Reply-To: <CA+55aFw_krUFFpocLOcvuANODoCJiF=QSUQeED9Xa_fLt3Gv1w@mail.gmail.com>

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>

  reply	other threads:[~2013-04-04 19:40 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-02 21:59 [PATCH] mm: prevent mmap_cache race in find_vma() Jan Stancek
2013-04-02 22:33 ` David Rientjes
2013-04-02 23:09   ` Hugh Dickins
2013-04-02 23:55     ` David Rientjes
2013-04-03  3:19       ` Paul E. McKenney
2013-04-03  4:21         ` David Rientjes
2013-04-03 16:38           ` Paul E. McKenney
2013-04-03  4:14       ` Johannes Weiner
2013-04-03  4:25         ` David Rientjes
2013-04-03  4:58           ` Johannes Weiner
2013-04-03  5:13             ` David Rientjes
2013-04-03 13:45             ` Ian Lance Taylor
2013-04-03 14:33               ` Johannes Weiner
2013-04-03 23:59                 ` David Rientjes
2013-04-04  0:00                   ` [patch] compiler: clarify ACCESS_ONCE() relies on compiler implementation David Rientjes
2013-04-04  0:38                     ` Linus Torvalds
2013-04-04  1:52                       ` David Rientjes
2013-04-04  2:00                         ` Linus Torvalds
2013-04-04  2:18                           ` David Rientjes
2013-04-04  2:37                             ` Linus Torvalds
2013-04-04  6:02                               ` David Rientjes
2013-04-04 14:23                                 ` Linus Torvalds
2013-04-04 19:40                                   ` David Rientjes [this message]
2013-04-04 19:53                                     ` Linus Torvalds
2013-04-04 20:02                                       ` David Rientjes
2013-04-03 16:33               ` [PATCH] mm: prevent mmap_cache race in find_vma() Paul E. McKenney
2013-04-03 16:41                 ` Paul E. McKenney
2013-04-03 17:47                 ` Ian Lance Taylor
2013-04-03 22:11                   ` Paul E. McKenney
2013-04-03 22:28                     ` Ian Lance Taylor
2013-04-12 18:05                       ` Paul E. McKenney
2013-04-03  9:37   ` Jakub Jelinek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.02.1304041209330.19501@chino.kir.corp.google.com \
    --to=rientjes@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=hannes@cmpxchg.org \
    --cc=hughd@google.com \
    --cc=iant@google.com \
    --cc=jstancek@redhat.com \
    --cc=linux-mm@kvack.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.