[PATCH] make raid5 checksums preempt-safe

[PATCH] make raid5 checksums preempt-safe

[Robert Love]

Linus,

The raid5 xor checksums use MMX/SSE state and are not preempt-safe.

Attached patch disables preemption in FPU_SAVE and XMMS_SAVE and
restores it in FPU_RESTORE and XMMS_RESTORE - preventing preemption
while in fp mode.

Please, apply.

	Robert Love

diff -urN linux-2.5.31/include/asm-i386/xor.h linux/include/asm-i386/xor.h
--- linux-2.5.31/include/asm-i386/xor.h	Sat Aug 10 21:41:20 2002
+++ linux/include/asm-i386/xor.h	Sat Aug 24 20:14:43 2002
@@ -20,6 +20,7 @@
 
 #define FPU_SAVE							\
   do {									\
+	preempt_disable();						\
 	if (!test_thread_flag(TIF_USEDFPU))				\
 		__asm__ __volatile__ (" clts;\n");			\
 	__asm__ __volatile__ ("fsave %0; fwait": "=m"(fpu_save[0]));	\
@@ -30,6 +31,7 @@
 	__asm__ __volatile__ ("frstor %0": : "m"(fpu_save[0]));		\
 	if (!test_thread_flag(TIF_USEDFPU))				\
 		stts();							\
+	preempt_enable();						\
   } while (0)
 
 #define LD(x,y)		"       movq   8*("#x")(%1), %%mm"#y"   ;\n"
@@ -543,6 +545,7 @@
  */
 
 #define XMMS_SAVE				\
+	preempt_disable();			\
 	__asm__ __volatile__ ( 			\
 		"movl %%cr0,%0		;\n\t"	\
 		"clts			;\n\t"	\
@@ -564,7 +567,8 @@
 		"movl 	%0,%%cr0	;\n\t"	\
 		:				\
 		: "r" (cr0), "r" (xmm_save)	\
-		: "memory")
+		: "memory")			\
+	preempt_enable();
 
 #define ALIGN16 __attribute__((aligned(16)))
 
diff -urN linux-2.5.31/include/asm-x86_64/xor.h linux/include/asm-x86_64/xor.h
--- linux-2.5.31/include/asm-x86_64/xor.h	Sat Aug 10 21:41:23 2002
+++ linux/include/asm-x86_64/xor.h	Sat Aug 24 20:05:41 2002
@@ -38,7 +38,8 @@
 /* Doesn't use gcc to save the XMM registers, because there is no easy way to 
    tell it to do a clts before the register saving. */
 #define XMMS_SAVE				\
-	asm volatile ( 			\
+	preempt_disable();			\
+	asm volatile (				\
 		"movq %%cr0,%0		;\n\t"	\
 		"clts			;\n\t"	\
 		"movups %%xmm0,(%1)	;\n\t"	\
@@ -59,7 +60,8 @@
 		"movq 	%0,%%cr0	;\n\t"	\
 		:				\
 		: "r" (cr0), "r" (xmm_save)	\
-		: "memory")
+		: "memory")			\
+	preempt_enable();
 
 #define OFFS(x)		"16*("#x")"
 #define PF_OFFS(x)	"256+16*("#x")"

Re: [PATCH] make raid5 checksums preempt-safe

[Thunder from the hill]

Hi,

On 26 Aug 2002, Robert Love wrote:
> @@ -543,6 +545,7 @@
>  #define XMMS_SAVE				\
> +	preempt_disable();			\
>  	__asm__ __volatile__ ( 			\
>  		"movl %%cr0,%0		;\n\t"	\
>  		"clts			;\n\t"	\
> @@ -564,7 +567,8 @@
>  		"movl 	%0,%%cr0	;\n\t"	\
>  		:				\
>  		: "r" (cr0), "r" (xmm_save)	\
> -		: "memory")
> +		: "memory")			\
> +	preempt_enable();
> @@ -38,7 +38,8 @@
>  #define XMMS_SAVE				\
> -	asm volatile ( 			\
> +	preempt_disable();			\
> +	asm volatile (				\
>  		"movq %%cr0,%0		;\n\t"	\
>  		"clts			;\n\t"	\
>  		"movups %%xmm0,(%1)	;\n\t"	\
> @@ -59,7 +60,8 @@
>  		"movq 	%0,%%cr0	;\n\t"	\
>  		:				\
>  		: "r" (cr0), "r" (xmm_save)	\
> -		: "memory")
> +		: "memory")			\
> +	preempt_enable();

These will suck when on if, I guess... Anyway, will this compile at all? 
There seems no semicolon after the asm volatile ()

			Thunder
-- 
--./../...-/. -.--/---/..-/.-./..././.-../..-. .---/..-/.../- .-
--/../-./..-/-/./--..-- ../.----./.-../.-.. --./../...-/. -.--/---/..-
.- -/---/--/---/.-./.-./---/.--/.-.-.-
--./.-/-.../.-./.././.-../.-.-.-

Re: [PATCH] make raid5 checksums preempt-safe

[Robert Love]

On Mon, 2002-08-26 at 17:09, Thunder from the hill wrote:

> These will suck when on if, I guess... 

hm?

> Anyway, will this compile at all?  There seems no semicolon after the
> asm volatile ()

Ah yes, curses.  Thanks.

	Robert Love

diff -urN linux-2.5.31/include/asm-i386/xor.h linux/include/asm-i386/xor.h
--- linux-2.5.31/include/asm-i386/xor.h	Sat Aug 10 21:41:20 2002
+++ linux/include/asm-i386/xor.h	Sat Aug 24 20:14:43 2002
@@ -20,6 +20,7 @@
 
 #define FPU_SAVE							\
   do {									\
+	preempt_disable();						\
 	if (!test_thread_flag(TIF_USEDFPU))				\
 		__asm__ __volatile__ (" clts;\n");			\
 	__asm__ __volatile__ ("fsave %0; fwait": "=m"(fpu_save[0]));	\
@@ -30,6 +31,7 @@
 	__asm__ __volatile__ ("frstor %0": : "m"(fpu_save[0]));		\
 	if (!test_thread_flag(TIF_USEDFPU))				\
 		stts();							\
+	preempt_enable();						\
   } while (0)
 
 #define LD(x,y)		"       movq   8*("#x")(%1), %%mm"#y"   ;\n"
@@ -543,6 +545,7 @@
  */
 
 #define XMMS_SAVE				\
+	preempt_disable();			\
 	__asm__ __volatile__ ( 			\
 		"movl %%cr0,%0		;\n\t"	\
 		"clts			;\n\t"	\
@@ -564,7 +567,8 @@
 		"movl 	%0,%%cr0	;\n\t"	\
 		:				\
 		: "r" (cr0), "r" (xmm_save)	\
-		: "memory")
+		: "memory");			\
+	preempt_enable()
 
 #define ALIGN16 __attribute__((aligned(16)))
 
diff -urN linux-2.5.31/include/asm-x86_64/xor.h linux/include/asm-x86_64/xor.h
--- linux-2.5.31/include/asm-x86_64/xor.h	Sat Aug 10 21:41:23 2002
+++ linux/include/asm-x86_64/xor.h	Sat Aug 24 20:05:41 2002
@@ -38,7 +38,8 @@
 /* Doesn't use gcc to save the XMM registers, because there is no easy way to 
    tell it to do a clts before the register saving. */
 #define XMMS_SAVE				\
-	asm volatile ( 			\
+	preempt_disable();			\
+	asm volatile (				\
 		"movq %%cr0,%0		;\n\t"	\
 		"clts			;\n\t"	\
 		"movups %%xmm0,(%1)	;\n\t"	\
@@ -59,7 +60,8 @@
 		"movq 	%0,%%cr0	;\n\t"	\
 		:				\
 		: "r" (cr0), "r" (xmm_save)	\
-		: "memory")
+		: "memory");			\
+	preempt_enable()
 
 #define OFFS(x)		"16*("#x")"
 #define PF_OFFS(x)	"256+16*("#x")"

Re: [PATCH] make raid5 checksums preempt-safe

[Brian Gerst]

Robert Love wrote:
> Linus,
> 
> The raid5 xor checksums use MMX/SSE state and are not preempt-safe.
> 
> Attached patch disables preemption in FPU_SAVE and XMMS_SAVE and
> restores it in FPU_RESTORE and XMMS_RESTORE - preventing preemption
> while in fp mode.
> 
> Please, apply.
> 
> 	Robert Love

Use kernel_fpu_begin() and kernel_fpu_end() instead of reinventing the 
wheel.  The current code is broken wrt SSE as well.

--
				Brian Gerst

Re: [PATCH] make raid5 checksums preempt-safe

[Horst von Brand]

Robert Love <rml@tech9.net> said:
> On Mon, 2002-08-26 at 17:09, Thunder from the hill wrote:
> 
> > These will suck when on if, I guess... 
> 
> hm?
> 
> > Anyway, will this compile at all?  There seems no semicolon after the
> > asm volatile ()
> 
> Ah yes, curses.  Thanks.

Also, kindly place "do { ... } while(0)" around XMMS_SAVE, if only for
symmetry.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [PATCH] make raid5 checksums preempt-safe

[Thunder from the hill]

Hi,

On Mon, 26 Aug 2002, Horst von Brand wrote:
> Also, kindly place "do { ... } while(0)" around XMMS_SAVE, if only for
> symmetry.

We've already had that talked about. Will appear in one of the following 
patches.

			Thunder
-- 
--./../...-/. -.--/---/..-/.-./..././.-../..-. .---/..-/.../- .-
--/../-./..-/-/./--..-- ../.----./.-../.-.. --./../...-/. -.--/---/..-
.- -/---/--/---/.-./.-./---/.--/.-.-.-
--./.-/-.../.-./.././.-../.-.-.-

How can a process easily get a list of all it's open fd?

[Mark Atwood]

I need to close all the none std[in|out|err] open fd's.

I've been told to do it like so:

  {
    int i;
    for (i=3; i<OPEN_MAX; i++)
      close(i);
  }

This is very slow, plus I have discovered that I can have open fd's with
values greater than OPEN_MAX.

I thought about getting the max fd from rlimit, but that doesn't work
either.  Say I have a rlimit of 1024 open fd's, and I open numbers 3
thru 1023, then I close 3 thru 1022, then I set the rlimit down to
16. rlimit then returns 16, but the largest open fd is still 1023.

So that doesn't work.

And I still have the problem that looping between 3 and whatever I pick
as the top and calling close on each in turn is very slow.

So what's the "right way" to do it?

I would *love* for there to be an ioctl or some syscall that I could
pass a pointer to an int and a pointer to an int array, and it would
come back telling me how many open fd's I've got, and fill in the
array with those fd's.

-- 
Mark Atwood   | Well done is better than well said.
mra@pobox.com | 
http://www.pobox.com/~mra

Re: How can a process easily get a list of all it's open fd?

[Alex Riesen]

tricky. You can use /proc/<getpid>/fd, and close all
handles listed here, but this has some caveats:
it's _very_ slow if you have many open files.
it's not portable.
it's not safe if you have a thread/signal handler running.

i never heard of a right way to do this.

-alex

int close_all_fd()
{
    char fdpath[PATH_MAX];
    DIR * dp;
    struct dirent * de;
    int fd;

    sprintf(fdpath, "/proc/%d/fd", getpid());
    dp = opendir(fdpath);
    if ( !dp )
	return -errno;
    while ( (de = readdir(dp)) )
    {
	if ( !strcmp(de->d_name, ".") || !strcmp(de->d_name, "..") )
	    continue;
	fd = strtol(de->d_name, 0, 10);
	if ( fd == dirfd(dp) || fd == 0 || fd == 1 || fd == 2 )
	    continue;

	if ( close(fd) < 0 )
	    fprintf(stderr, "%s: %s\n", de->d_name, strerror(errno));
    }
    closedir(dp);
    return 0;
}


On Tue, Aug 27, 2002 at 08:34:04AM -0700, Mark Atwood wrote:
> 
> I need to close all the none std[in|out|err] open fd's.
> 
> I've been told to do it like so:
> 
>   {
>     int i;
>     for (i=3; i<OPEN_MAX; i++)
>       close(i);
>   }
> 
> This is very slow, plus I have discovered that I can have open fd's with
> values greater than OPEN_MAX.
> 
> I thought about getting the max fd from rlimit, but that doesn't work
> either.  Say I have a rlimit of 1024 open fd's, and I open numbers 3
> thru 1023, then I close 3 thru 1022, then I set the rlimit down to
> 16. rlimit then returns 16, but the largest open fd is still 1023.
> 
> So that doesn't work.
> 
> And I still have the problem that looping between 3 and whatever I pick
> as the top and calling close on each in turn is very slow.
> 
> So what's the "right way" to do it?
> 
> I would *love* for there to be an ioctl or some syscall that I could
> pass a pointer to an int and a pointer to an int array, and it would
> come back telling me how many open fd's I've got, and fill in the
> array with those fd's.
> 
> -- 
> Mark Atwood   | Well done is better than well said.
> mra@pobox.com | 
> http://www.pobox.com/~mra
> -

Re: How can a process easily get a list of all it's open fd?

[Mike Touloumtzis]

On Tue, Aug 27, 2002 at 06:08:42PM +0200, Alex Riesen wrote:
> tricky. You can use /proc/<getpid>/fd, and close all
> handles listed here, but this has some caveats:
> it's _very_ slow if you have many open files.
> it's not portable.
> it's not safe if you have a thread/signal handler running.
> 
> i never heard of a right way to do this.
> 
> -alex
> 
> int close_all_fd()
> {
>     char fdpath[PATH_MAX];
>     DIR * dp;
>     struct dirent * de;
>     int fd;
> 
>     sprintf(fdpath, "/proc/%d/fd", getpid());
>     dp = opendir(fdpath);

This can just be:

	dp = opendir("/proc/self/fd/");

then you don't need fdpath.

You can use sigprocmask() if you're worried about signals coming
in during this operation.

miket

Re: How can a process easily get a list of all it's open fd?

[DervishD]

    Hi Mark :)

>So what's the "right way" to do it?

    AFAIK, the for loop, with getdtablesize() instead of 'OPEN_MAX'.
I do it that way, but I don't really know if it is the 'right way'(tm).

>I would *love* for there to be an ioctl or some syscall that I could
>pass a pointer to an int and a pointer to an int array, and it would
>come back telling me how many open fd's I've got, and fill in the
>array with those fd's.

    The array should be allocated by the kernel, or the syscall won't
work as expected ;) If you have 2000 fd open and the array whose
address you pass to the ioctl has an smaller size... Anyway, you can
call the ioctl a few times ;)

    Your proposal seems reasonable (unless there is any other way of
doing this portably), but portability will be an issue...

    Raúl

Re: How can a process easily get a list of all it's open fd?

[Alex Riesen]

On Tue, Aug 27, 2002 at 02:26:38PM -0700, Mike Touloumtzis wrote:
> On Tue, Aug 27, 2002 at 06:08:42PM +0200, Alex Riesen wrote:
> > tricky. You can use /proc/<getpid>/fd, and close all
> > handles listed here, but this has some caveats:
> > it's _very_ slow if you have many open files.
> > it's not portable.
> > it's not safe if you have a thread/signal handler running.
> > 
> > i never heard of a right way to do this.
> > 
> > -alex
> > 
> > int close_all_fd()
> > {
> >     char fdpath[PATH_MAX];
> >     DIR * dp;
> >     struct dirent * de;
> >     int fd;
> > 
> >     sprintf(fdpath, "/proc/%d/fd", getpid());
> >     dp = opendir(fdpath);
> 
> This can just be:
> 
> 	dp = opendir("/proc/self/fd/");
> 
> then you don't need fdpath.

Oh, indeed.

> You can use sigprocmask() if you're worried about signals coming
> in during this operation.

I, personally, strongly dislike this option.
You never know which signals are to be blocked.

Besides, it's still not portable and not safe agains threads.


-alex