<linux/linkage.h> generates incorrect cache alignments for 486 and above

[Chris Sears]

Mike,

here is the code from <linux/linkage.h>

        #if !defined(__i486__) && !defined(__i586__)
        #define __ALIGN .align 4,0x90
        #define __ALIGN_STR ".align 4,0x90"
        #else  /* __i486__/__i586__ */
        #define __ALIGN .align 16,0x90
        #define __ALIGN_STR ".align 16,0x90"
        #endif /* __i486__/__i586__ */

You can see that __i686__ is missing from the list
so it defaults to the i386 case.

The 16 should be 32 as cache.h points out.

Here is the quotation from the gas manual:

        .align abs-expr , abs-expr

        Pad the location counter (in the current subsection) to a
particular storage boundary.
        The first expression (which must be absolute) is the number of
low-order zero bits
        the location counter must have after advancement. For example
`.align 3' advances
        the location counter until it a multiple of 8. If the location
counter is already a multiple
        of 8, no change is needed.

Well now, this is a docubug. The 16 is aligning to 16 byte boundaries,
I've looked at the
readelf output. Also, setting this to 5 generates assembler complaints and
2^16 is large
even for a disk page, let alone a cache line.  The comment at the
beginning of entry.S
gets it wrong as well.

         * I changed all the .align's to 4 (16 byte alignment), as that's
faster
         * on a 486.

> afaik, __ALIGN has nothing to do with cache-line size.. L1_CACHE_BYTES.
> (hmm.. am I out to lunch or should __ALIGN be set to 0 for pentia?)

I'm definitely not with you on setting __ALIGN to 0.
ALIGN represents cache line alignment.  It really does.
It is used sparingly in entry.S and head.S

Here is asm-i386/cache.h

        /*
         * include/asm-i386/cache.h
         */
        #ifndef __ARCH_I386_CACHE_H
        #define __ARCH_I386_CACHE_H

        /* bytes per L1 cache line */
        #if    CPU==586 || CPU==686
        #define        L1_CACHE_BYTES  32
        #else
        #define        L1_CACHE_BYTES  16
        #endif

        #endif

You are very right. I think it makes excellent sense to use
L1_CACHE_BYTES in <linux/linkage.h>
But note that this is inconsistant with <linux/linkage.h>
which needs to get fixed.

> That's done in specs:predefines.  __i386__ is always defined, and as
> soon as you tell it which cpu you're using, (see cpp_cpu) cpp adds
> to it's defines accordingly.

I don't know what you mean but I'm guessing that this is a gcc
configuration file.  Just where I want to look to know about linux.
I think Alan Cox is always railing about being conservative about
compiler releases.  You want to insulate yourself from gcc's whims.

> I don't see anything at all wrong with using __ix86__ because the
> kernel isn't going to be compiled by anything other than gcc without
> doing a truckload of rewriting.

I did NOT say that the kernel would/should/could be compiled on something
other than gcc.  But that doesn't mean that it should rely on gcc for
configuration.  make xconfig creates <linux/autoconf.h>
The defines are consistant and I know where to look and what they mean.
Why use TWO mechanisms?  Why not THREE?

In summary, in addition to my posted patch, linkage.h should use cache.h
Thanks for forcing me to work through it one more time.  It definitely
improved it.  I'll post a mo' better patch in the morning.

thanks,

Chris Sears
cbsears en ix.netcom.com


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo en vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/