[U-Boot] [PATCH] ARM926: Add mb to the cache invalidate/flush

[Albert ARIBAUD]

Hi Scott,

On Thu, 11 Oct 2012 15:21:28 -0500, Scott Wood
<scottwood at freescale.com> wrote:

> > Scott, I think you should not mistake as condescension what is just
> > humo(u)r. What I wrote above is a quotation from a (light, quite
> > humorous and above all, self-mocking) movie, meant to be read by, but  
> > in
> > no way directed against, Marex.
> 
> Sure, it just seemed an odd way to resume a conversation that had  
> already begun on the topic.

It felt odd to you obviously. But does oddity imply condescension?
Seems to me like you're assuming a lot on what or how I think.

> > > > A memory barrier's effect is only that all of the volatile  
> > accesses
> > > > placed before it in the source code finish when the barrier  
> > executes,
> > > > and that none of the volatile accesses placed after it in the  
> > source
> > > > code starts before the barrier has executed.
> > >
> > > Cite from official GCC documentation please, or example code that  
> > shows
> > > a problem.
> > 
> > http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm
> > 
> > "If your assembler instructions access memory in an unpredictable
> > fashion, add `memory' to the list of clobbered registers. This will
> > cause GCC to not keep memory values cached in registers across the
> > assembler instruction and not optimize stores or loads to that memory.
> > You will also want to add the volatile keyword if the memory affected
> > is not listed in the inputs or outputs of the asm, as the `memory'
> > clobber does not count as a side-effect of the asm".
> 
> "and not optimize stores or loads to that memory".  It's not clear what  
> "that" refers to, since the memory clobber does not refer to specific  
> memory, but given that the purpose is "if your assembler instructions  
> access memory in an unpredictable fashion", I don't see how it could be  
> interpreted as anything other than "any memory which could possibly be  
> modified by the program".  So it excludes constant data, but that's  
> about it.

It does not necessarily include "all memory". Besides, "that"  -- to me
-- cleary means "the memory mentioned in the statement, for instance in
the inputs or outputs.

> The only reference to volatile is to tell you to add it to the asm  
> statement (not to other memory accesses) so that the asm statement does  
> not get removed altogether.

The memory clobber definition says no memory values are kept cached
in registers across the instruction, that implies that if a volatile
access was prepared (a memory value was cached in a register) it is
finished before the asm statement executes, and similarly, since the
desciption says "across" the instruction, volatiles reads or writes
located after the instruction won't have been started before ithe
instruction executest, or they would have needed to cache the value,
which is contrary to the memory clobber definition.

This is not to say that only volatiles are affected by the barrier; but
volatiles certainly are.

> > > We've use memory barriers like this all the time.  It works and is
> > > standard practice.  If it doesn't work like that it needs to be  
> > fixed.
> > 
> > I have used memory barriers too, and I've already seen some weird
> > things happening because they were used in ways that did not match
> > their effects. Particularly, we did not use memory clobbers on cache
> > flush or invalidate operations, we used them on actual barrier
> > operations -- dsb, dmb and their cp15 incarnations.
> 
> What specifically did you see the compiler do?

I'll have to look up the repositories. I'll keep you posted once I find
back examples.

> > > That AVR/ARM example you showed on IRC is special because it's  
> > calling
> > > a libgcc function and GCC knows that the function doesn't access  
> > memory
> > > (loading constant data for the argument doesn't count).  I couldn't  
> > get
> > > the same thing to happen with a normal function, even when declared
> > > with __attribute__((const)).  Yes, it's a problem for ordering code  
> > in
> > > general and thus keeping slow stuff out of critical sections, but it
> > > shouldn't be a problem for ordering memory accesses.
> > 
> > Can you *guarantee* that no valid C code will ever let a non-volatile
> > write slip across a memory clobber?
> > 
> > Memory clobbers do not guarantee this, at least not explicitly in  
> > their
> > description, whereas C sequence points do. For instance, the call to a
> > function is a sequence point, reached only after its arguments have
> > been evaluated.
> 
> I don't know GCC internals, so I personally can't guarantee anything.   
> What I know is that they're used for this purpose all over the place,  
> and if there really is a problem it needs to be fixed.  If the use in  
> this patch is wrong, then so are Linux synchronization primitives, for  
> example.  How would you make a spinlock?  Certainly you can't insist  
> that all the variables protected by the lock be volatile.

My point is that memory clobbers have some effect, but just because a
memory clobber appears somewhere does not mean it is responsible for
all that happens there.

Regarding Linux spinlocks vs these patches: it's not the same
situation. spinlock functions are inlined, as you noted, thus a code
sequence that takes a spinlock, does some accesses, then releases the
spinlock ends up as a long sequence of instructions. On the contrary,
the cache functions (which are not going to be inlined any time soon as
they are strong versions of weak symbols, incompatible with inlining)
contain a single asm statement, thus adding a memory clobber to this
statement won't have any effect for lack of preceding or following
instructions to (not) reorder.

> -Scott

Amicalement,
-- 
Albert.