Re: [PATCH] Use correct macros in raid code, not raw asm
Neil Brown wrote: So does this look right (no, I haven't compiled it yet) No, the whole raid6_*_save_t should be removed. I'll try to have a patch for you later. -hpa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
On Thursday February 8, [EMAIL PROTECTED] wrote: > Andi Kleen wrote: > > > > It should use kernel_fpu_begin() imho. If someone wants to test > > it in user space again they can add dummy definitions of that > > to their user space header. > > I hadn't seen this thread until now, when Neil pointed me to the thread. > > Using kernel_fpu_begin() ... kernel_fpu_end() is probably indeed the > best option. > So does this look right (no, I haven't compiled it yet) NeilBrown ### Diffstat output ./drivers/md/raid6x86.h | 56 ++-- 1 file changed, 12 insertions(+), 44 deletions(-) diff .prev/drivers/md/raid6x86.h ./drivers/md/raid6x86.h --- .prev/drivers/md/raid6x86.h 2007-02-09 12:30:32.0 +1100 +++ ./drivers/md/raid6x86.h 2007-02-09 12:36:01.0 +1100 @@ -25,20 +25,17 @@ typedef struct { unsigned int fsave[27]; - unsigned long cr0; } raid6_mmx_save_t __attribute__((aligned(16))); /* N.B.: For SSE we only save %xmm0-%xmm7 even for x86-64, since the code doesn't know about the additional x86-64 registers */ typedef struct { unsigned int sarea[8*4+2]; - unsigned long cr0; } raid6_sse_save_t __attribute__((aligned(16))); /* This is for x86-64-specific code which uses all 16 XMM registers */ typedef struct { unsigned int sarea[16*4+2]; - unsigned long cr0; } raid6_sse16_save_t __attribute__((aligned(16))); /* On x86-64 the stack *SHOULD* be 16-byte aligned, but currently this @@ -50,7 +47,6 @@ typedef struct { typedef struct { unsigned int fsave[27]; - unsigned long cr0; } raid6_mmx_save_t; /* On i386, the stack is only 8-byte aligned, but SSE requires 16-byte @@ -58,7 +54,6 @@ typedef struct { a properly-sized area correctly. */ typedef struct { unsigned int sarea[8*4+3]; - unsigned long cr0; } raid6_sse_save_t; /* Find the 16-byte aligned save area */ @@ -66,56 +61,29 @@ typedef struct { #endif -#ifdef __KERNEL__ /* Real code */ - -/* Note: %cr0 is 32 bits on i386 and 64 bits on x86-64 */ - -static inline unsigned long raid6_get_fpu(void) -{ - unsigned long cr0; - - preempt_disable(); - asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0)); - return cr0; -} - -static inline void raid6_put_fpu(unsigned long cr0) -{ - asm volatile("mov %0,%%cr0" : : "r" (cr0)); - preempt_enable(); -} - -#else /* Dummy code for user space testing */ - -static inline unsigned long raid6_get_fpu(void) -{ - return 0xf00ba6; -} - -static inline void raid6_put_fpu(unsigned long cr0) -{ - (void)cr0; -} - +#ifndef __KERNEL__ +/* for user-space testing */ +#define kernel_fpu_begin() +#define kernel_fpu_end(); #endif static inline void raid6_before_mmx(raid6_mmx_save_t *s) { - s->cr0 = raid6_get_fpu(); + kernel_fpu_begin(); asm volatile("fsave %0 ; fwait" : "=m" (s->fsave[0])); } static inline void raid6_after_mmx(raid6_mmx_save_t *s) { asm volatile("frstor %0" : : "m" (s->fsave[0])); - raid6_put_fpu(s->cr0); + kernel_fpu_end(); } static inline void raid6_before_sse(raid6_sse_save_t *s) { unsigned int *rsa = SAREA(s); - s->cr0 = raid6_get_fpu(); + kernel_fpu_begin(); asm volatile("movaps %%xmm0,%0" : "=m" (rsa[0])); asm volatile("movaps %%xmm1,%0" : "=m" (rsa[4])); @@ -140,14 +108,14 @@ static inline void raid6_after_sse(raid6 asm volatile("movaps %0,%%xmm6" : : "m" (rsa[24])); asm volatile("movaps %0,%%xmm7" : : "m" (rsa[28])); - raid6_put_fpu(s->cr0); + kernel_fpu_end(); } static inline void raid6_before_sse2(raid6_sse_save_t *s) { unsigned int *rsa = SAREA(s); - s->cr0 = raid6_get_fpu(); + kernel_fpu_begin(); asm volatile("movdqa %%xmm0,%0" : "=m" (rsa[0])); asm volatile("movdqa %%xmm1,%0" : "=m" (rsa[4])); @@ -172,7 +140,7 @@ static inline void raid6_after_sse2(raid asm volatile("movdqa %0,%%xmm6" : : "m" (rsa[24])); asm volatile("movdqa %0,%%xmm7" : : "m" (rsa[28])); - raid6_put_fpu(s->cr0); + kernel_fpu_end(); } #ifdef __x86_64__ @@ -181,7 +149,7 @@ static inline void raid6_before_sse16(ra { unsigned int *rsa = SAREA(s); - s->cr0 = raid6_get_fpu(); + kernel_fpu_begin(); asm volatile("movdqa %%xmm0,%0" : "=m" (rsa[0])); asm volatile("movdqa %%xmm1,%0" : "=m" (rsa[4])); @@ -222,7 +190,7 @@ static inline void raid6_after_sse16(rai asm volatile("movdqa %0,%%xmm14" : : "m" (rsa[56])); asm volatile("movdqa %0,%%xmm15" : : "m" (rsa[60])); - raid6_put_fpu(s->cr0); + kernel_fpu_end(); } #endif /* __x86_64__ */ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
Andi Kleen wrote: The code looks like it's designed to be included from userspace for testing; as it compiles without this include (and has no other includes), I chose not to add it. Linus makes a good point, but someone who actually knows the code should, y'know, test it and stuff... It should use kernel_fpu_begin() imho. If someone wants to test it in user space again they can add dummy definitions of that to their user space header. I hadn't seen this thread until now, when Neil pointed me to the thread. Using kernel_fpu_begin() ... kernel_fpu_end() is probably indeed the best option. -hpa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
> > The code looks like it's designed to be included from userspace for > testing; as it compiles without this include (and has no other > includes), I chose not to add it. > > Linus makes a good point, but someone who actually knows the code > should, y'know, test it and stuff... It should use kernel_fpu_begin() imho. If someone wants to test it in user space again they can add dummy definitions of that to their user space header. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
On Thu, 2006-12-28 at 15:56 -0800, Andrew Morton wrote: > On Fri, 29 Dec 2006 10:34:21 +1100 > Rusty Russell <[EMAIL PROTECTED]> wrote: > > > This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y. > > > > Signed-off-by: Rusty Russell <[EMAIL PROTECTED]> > > > > diff -r 4ff048622391 drivers/md/raid6x86.h > > --- a/drivers/md/raid6x86.h Thu Dec 28 16:52:54 2006 +1100 > > +++ b/drivers/md/raid6x86.h Fri Dec 29 10:09:38 2006 +1100 > > @@ -75,13 +75,14 @@ static inline unsigned long raid6_get_fp > > unsigned long cr0; > > > > preempt_disable(); > > - asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0)); > > + cr0 = read_cr0(); > > + clts(); > > return cr0; > > } > > > > static inline void raid6_put_fpu(unsigned long cr0) > > { > > - asm volatile("mov %0,%%cr0" : : "r" (cr0)); > > + write_cr0(cr0); > > preempt_enable(); > > } > > > > Perhaps we also need: > > --- > a/drivers/md/raid6x86.h~use-correct-macros-in-raid-code-not-raw-asm-include > +++ a/drivers/md/raid6x86.h > @@ -21,6 +21,8 @@ > > #if defined(__i386__) || defined(__x86_64__) > > +#include > + The code looks like it's designed to be included from userspace for testing; as it compiles without this include (and has no other includes), I chose not to add it. Linus makes a good point, but someone who actually knows the code should, y'know, test it and stuff... Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
On Fri, 29 Dec 2006 10:34:21 +1100 Rusty Russell <[EMAIL PROTECTED]> wrote: > This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y. > > Signed-off-by: Rusty Russell <[EMAIL PROTECTED]> > > diff -r 4ff048622391 drivers/md/raid6x86.h > --- a/drivers/md/raid6x86.h Thu Dec 28 16:52:54 2006 +1100 > +++ b/drivers/md/raid6x86.h Fri Dec 29 10:09:38 2006 +1100 > @@ -75,13 +75,14 @@ static inline unsigned long raid6_get_fp > unsigned long cr0; > > preempt_disable(); > - asm volatile("mov %%cr0,%0 ; clts" : "=r" (cr0)); > + cr0 = read_cr0(); > + clts(); > return cr0; > } > > static inline void raid6_put_fpu(unsigned long cr0) > { > - asm volatile("mov %0,%%cr0" : : "r" (cr0)); > + write_cr0(cr0); > preempt_enable(); > } > Perhaps we also need: --- a/drivers/md/raid6x86.h~use-correct-macros-in-raid-code-not-raw-asm-include +++ a/drivers/md/raid6x86.h @@ -21,6 +21,8 @@ #if defined(__i386__) || defined(__x86_64__) +#include + #ifdef __x86_64__ typedef struct { _ ? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Use correct macros in raid code, not raw asm
On Fri, 29 Dec 2006, Rusty Russell wrote: > > This make sure it's paravirtualized correctly when CONFIG_PARAVIRT=y. Why doesn't this code use "kernel_fpu_begin()" and "kernel_fpu_end()"? The raid6 code is crap, and slower. It does "fsave/frstor" or movaps or other crud, and the thing is, it shouldn't. It should just do kernel_fpu_begin/end(), which does it all right, and avoids saving any state at all unless it's being used by the user RIGHT NOW. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/