Proposal: make RAID6 code optional

Proposal: make RAID6 code optional

[Prakash Punnoor]

[-- Attachment #1: Type: text/plain, Size: 7803 bytes --]

Hi,

as I am using only RAID5 I wonder why the RAID6 code also needs to be built.
Here is a rough patch of making RAID6 optional (but depending on raid456)
without reording of functions to minimize ifdef scattering.
(I also haven't checked yet who needs ASYNC_MEMCPY and ASYNC_XOR...)
It would probably be nicer to make RAID4/5 and RAID6 independently selectable
of each other. But that requires more refactoring, as I can see.

I am willing to do more cleanups, so please provide some comments. I am not a
kernel hacker so please bear with me. ;-)

Greetings,

Prakash

diff --git a/drivers/md/Kconfig b/drivers/md/Kconfig
index 36e0675..0d98b3a 100644
--- a/drivers/md/Kconfig
+++ b/drivers/md/Kconfig
@@ -121,7 +121,6 @@ config MD_RAID10
 config MD_RAID456
 	tristate "RAID-4/RAID-5/RAID-6 mode"
 	depends on BLK_DEV_MD
-	select MD_RAID6_PQ
 	select ASYNC_MEMCPY
 	select ASYNC_XOR
 	---help---
@@ -152,8 +151,15 @@ config MD_RAID456
 
 	  If unsure, say Y.
 
-config MD_RAID6_PQ
-	tristate
+config MD_RAID6
+	boolean "Include RAID-6 support"
+	depends on MD_RAID456
+	select ASYNC_MEMCPY
+	select ASYNC_XOR
+	---help---
+	  If you want to use such a RAID-6 set, say Y.
+
+	  If unsure, say Y.
 
 config MD_MULTIPATH
 	tristate "Multipath I/O support"
diff --git a/drivers/md/Makefile b/drivers/md/Makefile
index 45cc595..5084b23 100644
--- a/drivers/md/Makefile
+++ b/drivers/md/Makefile
@@ -27,7 +27,7 @@ obj-$(CONFIG_MD_LINEAR)		+= linear.o
 obj-$(CONFIG_MD_RAID0)		+= raid0.o
 obj-$(CONFIG_MD_RAID1)		+= raid1.o
 obj-$(CONFIG_MD_RAID10)		+= raid10.o
-obj-$(CONFIG_MD_RAID6_PQ)	+= raid6_pq.o
+obj-$(CONFIG_MD_RAID6)		+= raid6_pq.o
 obj-$(CONFIG_MD_RAID456)	+= raid456.o
 obj-$(CONFIG_MD_MULTIPATH)	+= multipath.o
 obj-$(CONFIG_MD_FAULTY)		+= faulty.o
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 3bbc6d6..fc29b4b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -128,6 +128,7 @@ static inline void raid5_set_bi_hw_segments(struct bio *bio, unsigned int cnt)
 	bio->bi_phys_segments = raid5_bi_phys_segments(bio) || (cnt << 16);
 }
 
+#ifdef CONFIG_MD_RAID6
 /* Find first data disk in a raid6 stripe */
 static inline int raid6_d0(struct stripe_head *sh)
 {
@@ -163,6 +164,7 @@ static int raid6_idx_to_slot(int idx, struct stripe_head *sh,
 	slot = (*count)++;
 	return slot;
 }
+#endif
 
 static void return_io(struct bio *return_bi)
 {
@@ -1594,7 +1596,7 @@ static sector_t compute_blocknr(struct stripe_head *sh, int i, int previous)
 }
 
 
-
+#ifdef CONFIG_MD_RAID6
 /*
  * Copy data between a page in the stripe cache, and one or more bion
  * The page could align with the middle of the bio, or there could be
@@ -1738,7 +1740,6 @@ static void compute_parity6(struct stripe_head *sh, int method)
 	}
 }
 
-
 /* Compute one missing block */
 static void compute_block_1(struct stripe_head *sh, int dd_idx, int nozero)
 {
@@ -1840,6 +1841,7 @@ static void compute_block_2(struct stripe_head *sh, int dd_idx1, int dd_idx2)
 	set_bit(R5_UPTODATE, &sh->dev[dd_idx1].flags);
 	set_bit(R5_UPTODATE, &sh->dev[dd_idx2].flags);
 }
+#endif
 
 static void
 schedule_reconstruction5(struct stripe_head *sh, struct stripe_head_state *s,
@@ -1986,12 +1988,14 @@ static int add_stripe_bio(struct stripe_head *sh, struct bio *bi, int dd_idx, in
 
 static void end_reshape(raid5_conf_t *conf);
 
+#ifdef CONFIG_MD_RAID6
 static int page_is_zero(struct page *p)
 {
 	char *a = page_address(p);
 	return ((*(u32*)a) == 0 &&
 		memcmp(a, a+4, STRIPE_SIZE-4)==0);
 }
+#endif
 
 static void stripe_set_idx(sector_t stripe, raid5_conf_t *conf, int previous,
 			    struct stripe_head *sh)
@@ -2174,6 +2178,7 @@ static void handle_stripe_fill5(struct stripe_head *sh,
 	set_bit(STRIPE_HANDLE, &sh->state);
 }
 
+#ifdef CONFIG_MD_RAID6
 static void handle_stripe_fill6(struct stripe_head *sh,
 			struct stripe_head_state *s, struct r6_state *r6s,
 			int disks)
@@ -2231,7 +2236,7 @@ static void handle_stripe_fill6(struct stripe_head *sh,
 	}
 	set_bit(STRIPE_HANDLE, &sh->state);
 }
-
+#endif
 
 /* handle_stripe_clean_event
  * any written block on an uptodate or failed drive can be returned.
@@ -2373,6 +2378,7 @@ static void handle_stripe_dirtying5(raid5_conf_t *conf,
 		schedule_reconstruction5(sh, s, rcw == 0, 0);
 }
 
+#ifdef CONFIG_MD_RAID6
 static void handle_stripe_dirtying6(raid5_conf_t *conf,
 		struct stripe_head *sh,	struct stripe_head_state *s,
 		struct r6_state *r6s, int disks)
@@ -2472,6 +2478,7 @@ static void handle_stripe_dirtying6(raid5_conf_t *conf,
 		}
 	}
 }
+#endif
 
 static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 				struct stripe_head_state *s, int disks)
@@ -2559,7 +2566,7 @@ static void handle_parity_checks5(raid5_conf_t *conf, struct stripe_head *sh,
 	}
 }
 
-
+#ifdef CONFIG_MD_RAID6
 static void handle_parity_checks6(raid5_conf_t *conf, struct stripe_head *sh,
 				struct stripe_head_state *s,
 				struct r6_state *r6s, struct page *tmp_page,
@@ -2652,6 +2659,7 @@ static void handle_parity_checks6(raid5_conf_t *conf, struct stripe_head *sh,
 		set_bit(STRIPE_INSYNC, &sh->state);
 	}
 }
+#endif
 
 static void handle_stripe_expansion(raid5_conf_t *conf, struct stripe_head *sh,
 				struct r6_state *r6s)
@@ -3003,6 +3011,7 @@ static bool handle_stripe5(struct stripe_head *sh)
 	return blocked_rdev == NULL;
 }
 
+#ifdef CONFIG_MD_RAID6
 static bool handle_stripe6(struct stripe_head *sh, struct page *tmp_page)
 {
 	raid5_conf_t *conf = sh->raid_conf;
@@ -3239,13 +3248,16 @@ static bool handle_stripe6(struct stripe_head *sh, struct page *tmp_page)
 
 	return blocked_rdev == NULL;
 }
+#endif
 
 /* returns true if the stripe was handled */
 static bool handle_stripe(struct stripe_head *sh, struct page *tmp_page)
 {
+#ifdef CONFIG_MD_RAID6
 	if (sh->raid_conf->level == 6)
 		return handle_stripe6(sh, tmp_page);
 	else
+#endif
 		return handle_stripe5(sh);
 }
 
@@ -5190,6 +5202,7 @@ static int raid5_reconfig(mddev_t *mddev, int new_layout, int new_chunk)
 	return 0;
 }
 
+#ifdef CONFIG_MD_RAID6
 static int raid6_reconfig(mddev_t *mddev, int new_layout, int new_chunk)
 {
 	if (new_layout >= 0 && !algorithm_valid_raid6(new_layout))
@@ -5214,7 +5227,7 @@ static int raid6_reconfig(mddev_t *mddev, int new_layout, int new_chunk)
 
 	return 0;
 }
-
+#endif
 static void *raid5_takeover(mddev_t *mddev)
 {
 	/* raid5 can take over:
@@ -5242,6 +5255,7 @@ static void *raid5_takeover(mddev_t *mddev)
 
 static struct mdk_personality raid5_personality;
 
+#ifdef CONFIG_MD_RAID6
 static void *raid6_takeover(mddev_t *mddev)
 {
 	/* Currently can only take over a raid5.  We map the
@@ -5288,7 +5302,6 @@ static void *raid6_takeover(mddev_t *mddev)
 	return setup_conf(mddev);
 }
 
-
 static struct mdk_personality raid6_personality =
 {
 	.name		= "raid6",
@@ -5312,6 +5325,7 @@ static struct mdk_personality raid6_personality =
 	.takeover	= raid6_takeover,
 	.reconfig	= raid6_reconfig,
 };
+#endif
 static struct mdk_personality raid5_personality =
 {
 	.name		= "raid5",
@@ -5360,7 +5374,9 @@ static struct mdk_personality raid4_personality =
 
 static int __init raid5_init(void)
 {
+#ifdef CONFIG_MD_RAID6
 	register_md_personality(&raid6_personality);
+#endif
 	register_md_personality(&raid5_personality);
 	register_md_personality(&raid4_personality);
 	return 0;
@@ -5368,7 +5384,9 @@ static int __init raid5_init(void)
 
 static void raid5_exit(void)
 {
+#ifdef CONFIG_MD_RAID6
 	unregister_md_personality(&raid6_personality);
+#endif
 	unregister_md_personality(&raid5_personality);
 	unregister_md_personality(&raid4_personality);
 }


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Proposal: make RAID6 code optional

[Michael Tokarev]

Prakash Punnoor wrote:
> Hi,
> 
> as I am using only RAID5 I wonder why the RAID6 code also needs to be built.
> Here is a rough patch of making RAID6 optional (but depending on raid456)
> without reording of functions to minimize ifdef scattering.
> (I also haven't checked yet who needs ASYNC_MEMCPY and ASYNC_XOR...)
> It would probably be nicer to make RAID4/5 and RAID6 independently selectable
> of each other. But that requires more refactoring, as I can see.

Hm.  In "old good days" there were 3 independent kernel modules,
named raid4, raid5 and raid6.  Later on, they got merged into one
since they share quite alot of the code, and has only a few specific
parts.  Now you're trying to separate them back somewhat....

What's your goal?  What's the problem you're trying to solve?

/mjt

Re: Proposal: make RAID6 code optional

[Prakash Punnoor]

[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]

On Samstag 18 April 2009 10:09:54 Michael Tokarev wrote:
> Prakash Punnoor wrote:
> > Hi,
> >
> > as I am using only RAID5 I wonder why the RAID6 code also needs to be
> > built. Here is a rough patch of making RAID6 optional (but depending on
> > raid456) without reording of functions to minimize ifdef scattering.
> > (I also haven't checked yet who needs ASYNC_MEMCPY and ASYNC_XOR...)
> > It would probably be nicer to make RAID4/5 and RAID6 independently
> > selectable of each other. But that requires more refactoring, as I can
> > see.
>
> Hm.  In "old good days" there were 3 independent kernel modules,
> named raid4, raid5 and raid6.  Later on, they got merged into one
> since they share quite alot of the code, and has only a few specific
> parts.  Now you're trying to separate them back somewhat....
>
> What's your goal?  What's the problem you're trying to solve?

Having duplicate code is not good, of course. But unused code is also not 
good. As I said, I only use RAID5, so I don't need RAID6 support. The RAID6 
support enlarges kernel (the built-in.o in drivers/md grows from 325kb to 
414kb in my case), making boot time and compile time longer - admittedly not 
by a big margin. But then again I could argue: Why not put RAID0,1,10,4,5,6 
into one big module? Makes no sense, huh? For me putting 5 and 6 into one 
monolithic module makes no sense. A proper architecture would be to have some 
common shared code (in a separate module?), not a monolithic big one.

Regards,

Prakash

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Proposal: make RAID6 code optional

[Jesper Juhl]

On Sat, 18 Apr 2009, Prakash Punnoor wrote:

> On Samstag 18 April 2009 10:09:54 Michael Tokarev wrote:
> > Prakash Punnoor wrote:
> > > Hi,
> > >
> > > as I am using only RAID5 I wonder why the RAID6 code also needs to be
> > > built. Here is a rough patch of making RAID6 optional (but depending on
> > > raid456) without reording of functions to minimize ifdef scattering.
> > > (I also haven't checked yet who needs ASYNC_MEMCPY and ASYNC_XOR...)
> > > It would probably be nicer to make RAID4/5 and RAID6 independently
> > > selectable of each other. But that requires more refactoring, as I can
> > > see.
> >
> > Hm.  In "old good days" there were 3 independent kernel modules,
> > named raid4, raid5 and raid6.  Later on, they got merged into one
> > since they share quite alot of the code, and has only a few specific
> > parts.  Now you're trying to separate them back somewhat....
> >
> > What's your goal?  What's the problem you're trying to solve?
> 
> Having duplicate code is not good, of course. But unused code is also not 
> good. As I said, I only use RAID5, so I don't need RAID6 support. The RAID6 
> support enlarges kernel (the built-in.o in drivers/md grows from 325kb to 
> 414kb in my case), making boot time and compile time longer 

By a few ms perhaps - nothing that you'd ever notice in real life... A 
small price to pay for the shared code. If you were to split them all 
again, the combined total size would be greater still.

> - admittedly not 
> by a big margin. But then again I could argue: Why not put RAID0,1,10,4,5,6 
> into one big module? Makes no sense, huh? 

Makes perfect sense to me. Just modprobe raid.o and you have all 
raid levels available. That would make a lot of sense. 

> For me putting 5 and 6 into one 
> monolithic module makes no sense. A proper architecture would be to have some 
> common shared code (in a separate module?), not a monolithic big one.
> 
That's also a way, and certainly better than just splitting out raid6.

-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

Re: Proposal: make RAID6 code optional

[Matti Aarnio]

On Sat, Apr 18, 2009 at 03:56:17PM +0200, Jesper Juhl wrote:
> On Sat, 18 Apr 2009, Prakash Punnoor wrote:
> > On Samstag 18 April 2009 10:09:54 Michael Tokarev wrote:
> > > Prakash Punnoor wrote:
.....
> > > What's your goal?  What's the problem you're trying to solve?
> > 
> > Having duplicate code is not good, of course. But unused code is also not 
> > good. As I said, I only use RAID5, so I don't need RAID6 support. The RAID6 
> > support enlarges kernel (the built-in.o in drivers/md grows from 325kb to 
> > 414kb in my case), making boot time and compile time longer 
> 
> By a few ms perhaps - nothing that you'd ever notice in real life... A 
> small price to pay for the shared code. If you were to split them all 
> again, the combined total size would be greater still.


I did quick "sum of symbol sizes" lookup of the   raid.ko, and got
it like this:

nm -t d -n -S /lib/modules/2.6.27.21-170.2.56.fc10.x86_64/kernel/drivers/md/raid456.ko | grep raid4|awk '{print $2}'|sed -e 's/^0*//g'|awk '{sum+=$1}END{print sum}'
  ...

raid4:   152
raid5:  7165
raid6: 75558

Entire 64kB of that raid6 is single pre-initialized r/o datablock:  raid6_gfmul

So yes, having RAID6 personality as separate module would be appropriate for
systems that are only interested in RAID4 or RAID5.  Separating the RAID4
personality wastes space, separating RAID5 ...  barely 2 of 4k memory pages.

There are perhaps a few kB more of codes for RAID5 and RAID6 classes - not all
local functions at each are named with relevant prefix,  but overall I would
consider extracting RAID6 as a reasonable goal with common codes on RAID4/5.

> > - admittedly not 
> > by a big margin. But then again I could argue: Why not put RAID0,1,10,4,5,6 
> > into one big module? Makes no sense, huh? 
> 
> Makes perfect sense to me. Just modprobe raid.o and you have all 
> raid levels available. That would make a lot of sense. 

Also, systems with so many disks that they run RAID4/5/6 to begin with are
likely to have enough memory so that "wasted" 75-80 kB does not matter.

> > For me putting 5 and 6 into one 
> > monolithic module makes no sense. A proper architecture would be to have some 
> > common shared code (in a separate module?), not a monolithic big one.
> 
> That's also a way, and certainly better than just splitting out raid6.
> -- 
> Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
> Plain text mails only, please      http://www.expita.com/nomime.html

/Matti Aarnio

Re: Proposal: make RAID6 code optional

[H. Peter Anvin]

Matti Aarnio wrote:
> 
> I did quick "sum of symbol sizes" lookup of the   raid.ko, and got
> it like this:
> 
> nm -t d -n -S /lib/modules/2.6.27.21-170.2.56.fc10.x86_64/kernel/drivers/md/raid456.ko | grep raid4|awk '{print $2}'|sed -e 's/^0*//g'|awk '{sum+=$1}END{print sum}'
>   ...
> 
> raid4:   152
> raid5:  7165
> raid6: 75558
> 
> Entire 64kB of that raid6 is single pre-initialized r/o datablock:  raid6_gfmul
> 
> So yes, having RAID6 personality as separate module would be appropriate for
> systems that are only interested in RAID4 or RAID5.  Separating the RAID4
> personality wastes space, separating RAID5 ...  barely 2 of 4k memory pages.
> 

RAID 4 is really just another layout scheme for RAID 5.  But yes, moving
RAID 6 to a separate module makes sense.  The amount of RAID 5 code not
used by RAID 6 is fairly trivial, so the right way to do this is to have
 the raid6 module depend on the raid5 module.

There used to be a raid6 module which was forked from raid5, with a lot
of duplicate code.  That really made really no sense.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Proposal: make RAID6 code optional

[NeilBrown]

On Sun, April 19, 2009 12:07 pm, H. Peter Anvin wrote:
> Matti Aarnio wrote:
>>
>> I did quick "sum of symbol sizes" lookup of the   raid.ko, and got
>> it like this:
>>
>> nm -t d -n -S
>> /lib/modules/2.6.27.21-170.2.56.fc10.x86_64/kernel/drivers/md/raid456.ko
>> | grep raid4|awk '{print $2}'|sed -e 's/^0*//g'|awk '{sum+=$1}END{print
>> sum}'
>>   ...
>>
>> raid4:   152
>> raid5:  7165
>> raid6: 75558
>>
>> Entire 64kB of that raid6 is single pre-initialized r/o datablock:
>> raid6_gfmul
>>
>> So yes, having RAID6 personality as separate module would be appropriate
>> for
>> systems that are only interested in RAID4 or RAID5.  Separating the
>> RAID4
>> personality wastes space, separating RAID5 ...  barely 2 of 4k memory
>> pages.
>>
>
> RAID 4 is really just another layout scheme for RAID 5.  But yes, moving
> RAID 6 to a separate module makes sense.  The amount of RAID 5 code not
> used by RAID 6 is fairly trivial, so the right way to do this is to have
>  the raid6 module depend on the raid5 module.

In 2.6.30, the Q syndrome code has been moved into a separate module,
so raid456.ko should be quite a bit smaller.

Of the remaining code, much is common, and some have raid5 and raid6
versions.
Part of the reason for that is that we support async xor offload for
raid5 but not for raid6, so raid6 doesn't use the async paths.

In 2.6.31, we should get async offload of the Q syndrome, so there
is a good chance that we could unify a lot of the code that currently
has separate raid5 and raid6 paths.

In code terms, the different between raid5 and raid6 is quite small
 - raid6 has an extra 'parity' block
 - raid6 cannot do read-modify-write cycles (at least with the current
   implementation)

That is not reason enough to have separate code.

>
> There used to be a raid6 module which was forked from raid5, with a lot
> of duplicate code.  That really made really no sense.
>

It was a good way to get raid6 implemented without risking raid5,
but long term I fully agree.

NeilBrown

Re: Proposal: make RAID6 code optional

[Neil Brown]

On Sunday April 19, neilb@suse.de wrote:
> 
> In 2.6.30, the Q syndrome code has been moved into a separate module,
> so raid456.ko should be quite a bit smaller.

Of course that doesn't really help as there will be a dependency
between raid456.ko and pq.ko so you cannot avoid having pq.ko loaded
while using raid5.

It might make sense to create a raid6 module that just contains 
The call to register_md_personality(raid6_personality) and some
linkage so that the code in raid456.ko can get to the code in pq.ko.

This is probably worth trying one the raid6 async offload stuff
stabilises.

NeilBrown

Re: Proposal: make RAID6 code optional

[Neil Brown]

On Sunday April 19, neilb@suse.de wrote:
> 
> In 2.6.30, the Q syndrome code has been moved into a separate module,
> so raid456.ko should be quite a bit smaller.

Of course that doesn't really help as there will be a dependency
between raid456.ko and pq.ko so you cannot avoid having pq.ko loaded
while using raid5.

It might make sense to create a raid6 module that just contains 
The call to register_md_personality(raid6_personality) and some
linkage so that the code in raid456.ko can get to the code in pq.ko.

This is probably worth trying one the raid6 async offload stuff
stabilises.

NeilBrown

Re: Proposal: make RAID6 code optional

[Bill Davidsen]

Matti Aarnio wrote:
> On Sat, Apr 18, 2009 at 03:56:17PM +0200, Jesper Juhl wrote:
>   
>> On Sat, 18 Apr 2009, Prakash Punnoor wrote:
>>     
>>> On Samstag 18 April 2009 10:09:54 Michael Tokarev wrote:
>>>       
>>>> Prakash Punnoor wrote:
>>>>         
> .....
>   
>>>> What's your goal?  What's the problem you're trying to solve?
>>>>         
>>> Having duplicate code is not good, of course. But unused code is also not 
>>> good. As I said, I only use RAID5, so I don't need RAID6 support. The RAID6 
>>> support enlarges kernel (the built-in.o in drivers/md grows from 325kb to 
>>> 414kb in my case), making boot time and compile time longer 
>>>       
>> By a few ms perhaps - nothing that you'd ever notice in real life... A 
>> small price to pay for the shared code. If you were to split them all 
>> again, the combined total size would be greater still.
>>     
>
>
> I did quick "sum of symbol sizes" lookup of the   raid.ko, and got
> it like this:
>
> nm -t d -n -S /lib/modules/2.6.27.21-170.2.56.fc10.x86_64/kernel/drivers/md/raid456.ko | grep raid4|awk '{print $2}'|sed -e 's/^0*//g'|awk '{sum+=$1}END{print sum}'
>   ...
>
> raid4:   152
> raid5:  7165
> raid6: 75558
>
> Entire 64kB of that raid6 is single pre-initialized r/o datablock:  raid6_gfmul
>
>   
It would seem that that space could be allocated and populated when 
raid6 was first used, as part of the initialization. I haven't looked at 
that code since it was new, so I might be optimistic about doing it that 
way.

> So yes, having RAID6 personality as separate module would be appropriate for
> systems that are only interested in RAID4 or RAID5.  Separating the RAID4
> personality wastes space, separating RAID5 ...  barely 2 of 4k memory pages.
>
> There are perhaps a few kB more of codes for RAID5 and RAID6 classes - not all
> local functions at each are named with relevant prefix,  but overall I would
> consider extracting RAID6 as a reasonable goal with common codes on RAID4/5.
>
>   
>>> - admittedly not 
>>> by a big margin. But then again I could argue: Why not put RAID0,1,10,4,5,6 
>>> into one big module? Makes no sense, huh? 
>>>       
>> Makes perfect sense to me. Just modprobe raid.o and you have all 
>> raid levels available. That would make a lot of sense. 
>>     
>
> Also, systems with so many disks that they run RAID4/5/6 to begin with are
> likely to have enough memory so that "wasted" 75-80 kB does not matter.
>   

Everything matters. "Take care of the pennies and the dollars will take 
care of themselves" is not just an old German proverb.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc

"You are disgraced professional losers. And by the way, give us our money back."
    - Representative Earl Pomeroy,  Democrat of North Dakota
on the A.I.G. executives who were paid bonuses  after a federal bailout.

Re: Proposal: make RAID6 code optional

[H. Peter Anvin]

Bill Davidsen wrote:
> It would seem that that space could be allocated and populated when
> raid6 was first used, as part of the initialization. I haven't looked at
> that code since it was new, so I might be optimistic about doing it that
> way.

We could use vmalloc() and generate the tables at initialization time.
However, having a separate module which exports the raid6 declaration
and uses the raid5 module as a subroutine library seems easier.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Proposal: make RAID6 code optional

[Goswin von Brederlow]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Bill Davidsen wrote:
>> It would seem that that space could be allocated and populated when
>> raid6 was first used, as part of the initialization. I haven't looked at
>> that code since it was new, so I might be optimistic about doing it that
>> way.
>
> We could use vmalloc() and generate the tables at initialization time.
> However, having a separate module which exports the raid6 declaration
> and uses the raid5 module as a subroutine library seems easier.
>
> 	-hpa

Combine the two.

The raid6 module initializes the tables for raid6 and uses the raid5
module as subroutine library.

MfG
        Goswin

Re: Proposal: make RAID6 code optional

[Bill Davidsen]

Goswin von Brederlow wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
>
>   
>> Bill Davidsen wrote:
>>     
>>> It would seem that that space could be allocated and populated when
>>> raid6 was first used, as part of the initialization. I haven't looked at
>>> that code since it was new, so I might be optimistic about doing it that
>>> way.
>>>       
>> We could use vmalloc() and generate the tables at initialization time.
>> However, having a separate module which exports the raid6 declaration
>> and uses the raid5 module as a subroutine library seems easier.
>>
>> 	-hpa
>>     
>
> Combine the two.
>
> The raid6 module initializes the tables for raid6 and uses the raid5
> module as subroutine library.
>   

My thought was that by saving almost all of the increased size of the 
raid6 capability it greatly reduces the need to have yet another module. 
It doesn't look as if the actual added code for raid6 is all that large.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc

"You are disgraced professional losers. And by the way, give us our money back."
    - Representative Earl Pomeroy,  Democrat of North Dakota
on the A.I.G. executives who were paid bonuses  after a federal bailout.

Re: Proposal: make RAID6 code optional

[H. Peter Anvin]

Goswin von Brederlow wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
> 
>> Bill Davidsen wrote:
>>> It would seem that that space could be allocated and populated when
>>> raid6 was first used, as part of the initialization. I haven't looked at
>>> that code since it was new, so I might be optimistic about doing it that
>>> way.
>> We could use vmalloc() and generate the tables at initialization time.
>> However, having a separate module which exports the raid6 declaration
>> and uses the raid5 module as a subroutine library seems easier.
>>
>> 	-hpa
> 
> Combine the two.
> 
> The raid6 module initializes the tables for raid6 and uses the raid5
> module as subroutine library.
> 

It really doesn't make sense at all.  It's easier at that point to
retain the static tables.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Proposal: make RAID6 code optional

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 10:23, H. Peter Anvin wrote:

> We could use vmalloc() and generate the tables at initialization time.
> However, having a separate module which exports the raid6 declaration
> and uses the raid5 module as a subroutine library seems easier.

Really? Easier than keeping only two 256-byte arrays for exp() and
log() and use these at runtime to populate the (dynamically allocated)
64K GF multiplication table? That seems to be really simple and would
still shave off 64K of kernel memory for raid5-only users.

Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Proposal: make RAID6 code optional

[Goswin von Brederlow]

Andre Noll <maan@systemlinux.org> writes:

> On 10:23, H. Peter Anvin wrote:
>
>> We could use vmalloc() and generate the tables at initialization time.
>> However, having a separate module which exports the raid6 declaration
>> and uses the raid5 module as a subroutine library seems easier.
>
> Really? Easier than keeping only two 256-byte arrays for exp() and
> log() and use these at runtime to populate the (dynamically allocated)
> 64K GF multiplication table? That seems to be really simple and would
> still shave off 64K of kernel memory for raid5-only users.
>
> Andre

Oh, you mean when the first raid6 device is started and not when the
module is loaded. That would work.

MfG
        Goswin

Re: Proposal: make RAID6 code optional

[H. Peter Anvin]

Andre Noll wrote:
> On 10:23, H. Peter Anvin wrote:
> 
>> We could use vmalloc() and generate the tables at initialization time.
>> However, having a separate module which exports the raid6 declaration
>> and uses the raid5 module as a subroutine library seems easier.
> 
> Really? Easier than keeping only two 256-byte arrays for exp() and
> log() and use these at runtime to populate the (dynamically allocated)
> 64K GF multiplication table? That seems to be really simple and would
> still shave off 64K of kernel memory for raid5-only users.
> 

Yes, I believe it would be easier than having dynamically allocated 
arrays.  Dynamically generated arrays using static memory allocations 
(bss) is one thing, but that would only reduce size of the module on 
disk, which I don't think anyone considers a problem.

	-hpa

Re: Proposal: make RAID6 code optional

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

On 20:31, Goswin von Brederlow wrote:
> > Really? Easier than keeping only two 256-byte arrays for exp() and
> > log() and use these at runtime to populate the (dynamically allocated)
> > 64K GF multiplication table? That seems to be really simple and would
> > still shave off 64K of kernel memory for raid5-only users.
> >
> > Andre
> 
> Oh, you mean when the first raid6 device is started and not when the
> module is loaded.

Yes, that's what I was trying to say.

Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Proposal: make RAID6 code optional

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On 11:39, H. Peter Anvin wrote:
> Yes, I believe it would be easier than having dynamically allocated 
> arrays.  Dynamically generated arrays using static memory allocations 
> (bss) is one thing, but that would only reduce size of the module on 
> disk, which I don't think anyone considers a problem.

We would save 64K of RAM in the raid5-only case if we'd defer the
allocation of the multiplication table until the first raid6 array
is about to be started.


Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Proposal: make RAID6 code optional

[H. Peter Anvin]

Andre Noll wrote:
> On 11:39, H. Peter Anvin wrote:
>> Yes, I believe it would be easier than having dynamically allocated 
>> arrays.  Dynamically generated arrays using static memory allocations 
>> (bss) is one thing, but that would only reduce size of the module on 
>> disk, which I don't think anyone considers a problem.
> 
> We would save 64K of RAM in the raid5-only case if we'd defer the
> allocation of the multiplication table until the first raid6 array
> is about to be started.

Yes, and we'd have to access it through a pointer for the rest of eternity.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: Proposal: make RAID6 code optional

[Andre Noll]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

On 18:35, H. Peter Anvin wrote:
> Andre Noll wrote:
> > On 11:39, H. Peter Anvin wrote:
> >> Yes, I believe it would be easier than having dynamically allocated 
> >> arrays.  Dynamically generated arrays using static memory allocations 
> >> (bss) is one thing, but that would only reduce size of the module on 
> >> disk, which I don't think anyone considers a problem.
> > 
> > We would save 64K of RAM in the raid5-only case if we'd defer the
> > allocation of the multiplication table until the first raid6 array
> > is about to be started.
> 
> Yes, and we'd have to access it through a pointer for the rest of eternity.

True. You put a lot of effort into raid6 to make it fast, so you know
best how much that would slow down the code. If using a pointer instead
of an array would have a measurable impact on the raid6 performance,
then we should indeed avoid using dynamically allocated memory for
the table.

As this slowdown likely depends on the arch, it is not easy to measure.
So I guess the best way to decrease memory usage for the raid5-only
case is to put the raid6-specific code into a separate module as you
suggested earlier.

Thanks
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]