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
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
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
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
> > 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
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/
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/
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 asm/system.h
+
#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 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 asm/system.h
+
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/
