Re: Bug in haifa scheduler ?

[Franz Sirl <Franz dot Sirl-kernel at lauterbach dot com>]

Am Sun, 28 Jun 1998 schrieb Gabriel Paubert:
>On Sat, 27 Jun 1998, Franz Sirl wrote:
>
>> I think this is not enough. The eieio (Old MacDonald had a farm, I love this
>> instruction :-))  ) instruction (if we want to avoid the general memory 
>> clobber for in/out's) has to be integrated into the in/out volatile asm,
>> otherwise it can again be reordered around the in/out's by the compiler, which
>> is not what we want.
>
>I disagree, a series of 2 asm volatile should not be reordered
>relative to each other, this simply does not make sense. And if it
>does happen, it is a compiler bug. 


Huh???? This is even explicitly stated in the docs:

---
   Note that even a volatile `asm' instruction can be moved in ways
that appear insignificant to the compiler, such as across jump
instructions.  You can't expect a sequence of volatile `asm'
instructions to remain perfectly consecutive.  If you want consecutive
output, use a single `asm'.
---

I think this is one of the few statements in there that is perfectly clear. So
it is perfectly legal for the compiler to move the asm around, since it doesn't
clobber anything.


>BTW: I made a detailed comparison of the assembly output of platinum.c
>with both versions of eieio. I have not been able to see any difference in
>the order of I/O port accesses and eieio instructions. The generated code
>is different simply because one variable (plat_regs) is repeatedly
>reloaded because of the memory clobber.  Note that I also tried to change
>the eieio macro to be a simple asm("eieio"), without any colon to specify
>parameters. It generates still a different assembly code but once again I
>fail to see any significant difference. I might have to look closer,
>however... 

Simple asm is seen by the compiler as a so-called "old-style asm" and turns on
some switches (memory clobber?).


>Anyway the platinum.c code basically does not use any of the in/out
>macros, but I have found 2 places where the code is at the very least
>doubtful. One is in read_platinum_sense, all writes to the regs[23].r
>are followed by eieio and a delay except the last one. This is however
>a minor point IMHO. 

I've tried that one, no success.

>The other one is the code which determines the frame buffer size, this
>code is extremely fragile. Some memory mapped as write-through is written
>to and later read from to know if the address effectively corresponds to
>physical storage.  This will fail is the data happens for any reason to be
>in the cache (L1 or L2), the eieio which is beteen the read and write
>accesses could better be replaced by a true memory barrier and the cache
>lines should be invalidated just in case. This does not explain the
>observed problems however.

Hmm, I will look at that.


>Finally, why is plat_regs declared volatile and cmap_regs is not. This
>might be a problem: there are places with an out_8(cmap_regs->addr) 
>followed by a test of another cmap_regs like: 
>
>	out_8(&cmap_regs->addr, 3+32);
>	if (cmap_regs->d2 == 2) {
>		STORE_D2(7, clksel[0]);
>		STORE_D2(8, clksel[1]);
>		STORE_D2(3, 3);
>
>
>since cmap_regs is a normal pointer (not to volatile), the compiler
>may reorder the read before the out_8 if it seems fit for scheduling 
>purposes. Adding the memory clobber to eieio prevents this, but IMHO
>declaring cmap_regs as a pointer to volatile should have the same effect.

I will try that too.

>> 
>> So it should look something like that:
>> 
>> outw():
>> asm volatile("eieio; sthbrx %1,0,%2" : "=m" (*port): "r" (data), "r" (port));
>> 
>> inw():
>> asm volatile("eieio; lhbrx %0,0,%1" : "=r" (data): "r" (port), "m" (*port));
>> 
>
>Right now we do the eieio after, but it should not matter. 
>
>> And for eieio() uses indepedent of the in/out inlines we should keep the
>> general memory clobber.
>> 
>> eieio():
>> asm volatile("eieio" : : : "memory");
>> 
>
>Once again I'm not so sure. 

Hm, this probably needs some discussion for the best solution. At least somebody
else see's what's going on here :-) But with my understanding of the
documentation, I think the combined asm is the best solution. What arguments do
you have against it?

Franz.