[PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[Boaz Harrosh]

At some past instance Linus Trovalds wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> commit a84a79e4d369a73c0130b5858199e949432da4c6 upstream.
>
> The size is always valid, but variable-length arrays generate worse code
> for no good reason (unless the function happens to be inlined and the
> compiler sees the length for the simple constant it is).
>
> Also, there seems to be some code generation problem on POWER, where
> Henrik Bakken reports that register r28 can get corrupted under some
> subtle circumstances (interrupt happening at the wrong time?).  That all
> indicates some seriously broken compiler issues, but since variable
> length arrays are bad regardless, there's little point in trying to
> chase it down.
>
> "Just don't do that, then".

Since then any use of "variable length arrays" has become blasphemous.
Even in perfectly good, beautiful, perfectly safe code like the one
below where the variable length arrays are only used as a sizeof()
parameter, for type-safe dynamic structure allocations. GCC is not
executing any stack allocation code.

I have produced a small file which defines two functions main1(unsigned numdevs)
and main2(unsigned numdevs). main1 uses code as before with call to malloc
and main2 uses code as of after this patch. I compiled it as:
	gcc -O2 -S see_asm.c
and here is what I get:

<see_asm.s>
main1:
.LFB7:
	.cfi_startproc
	mov	%edi, %edi
	leaq	4(%rdi,%rdi), %rdi
	salq	$3, %rdi
	jmp	malloc
	.cfi_endproc
.LFE7:
	.size	main1, .-main1
	.p2align 4,,15
	.globl	main2
	.type	main2, @function
main2:
.LFB8:
	.cfi_startproc
	mov	%edi, %edi
	addq	$2, %rdi
	salq	$4, %rdi
	jmp	malloc
	.cfi_endproc
.LFE8:
	.size	main2, .-main2
	.section	.text.startup,"ax",@progbits
	.p2align 4,,15
</see_asm.s>

*Exact* same code !!!

So please seriously consider not accepting this patch and leave the
perfectly good code intact.

CC: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/nfs/objlayout/objio_osd.c |   39 +++++++++++++++++++++++++--------------
 1 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
index 55d0128..37a9269 100644
--- a/fs/nfs/objlayout/objio_osd.c
+++ b/fs/nfs/objlayout/objio_osd.c
@@ -205,25 +205,36 @@ static void copy_single_comp(struct ore_components *oc, unsigned c,
 int __alloc_objio_seg(unsigned numdevs, gfp_t gfp_flags,
 		       struct objio_segment **pseg)
 {
-	struct __alloc_objio_segment {
-		struct objio_segment olseg;
-		struct ore_dev *ods[numdevs];
-		struct ore_comp	comps[numdevs];
-	} *aolseg;
-
-	aolseg = kzalloc(sizeof(*aolseg), gfp_flags);
-	if (unlikely(!aolseg)) {
+/*	This is the in memory structure of the objio_segment
+ *
+ *	struct __alloc_objio_segment {
+ *		struct objio_segment olseg;
+ *		struct ore_dev *ods[numdevs];
+ *		struct ore_comp	comps[numdevs];
+ *	} *aolseg;
+ *	NOTE: The code as above compiles and runs perfectly. It is elegant,
+ *	type safe and compact. At some Past time Linus has decided he does not
+ *	like variable length arrays, For the sake of this principal we uglify
+ *	the code as below.
+ */
+	struct objio_segment *lseg;
+	size_t lseg_size = sizeof(*lseg) +
+			numdevs * sizeof(lseg->oc.ods[0]) +
+			numdevs * sizeof(*lseg->oc.comps);
+
+	lseg = kzalloc(lseg_size, gfp_flags);
+	if (unlikely(!lseg)) {
 		dprintk("%s: Faild allocation numdevs=%d size=%zd\n", __func__,
-			numdevs, sizeof(*aolseg));
+			numdevs, lseg_size);
 		return -ENOMEM;
 	}
 
-	aolseg->olseg.oc.numdevs = numdevs;
-	aolseg->olseg.oc.single_comp = EC_MULTPLE_COMPS;
-	aolseg->olseg.oc.comps = aolseg->comps;
-	aolseg->olseg.oc.ods = aolseg->ods;
+	lseg->oc.numdevs = numdevs;
+	lseg->oc.single_comp = EC_MULTPLE_COMPS;
+	lseg->oc.ods = (void *)(lseg + 1);
+	lseg->oc.comps = (void *)(lseg->oc.ods + numdevs);
 
-	*pseg = &aolseg->olseg;
+	*pseg = lseg;
 	return 0;
 }
 
-- 
1.7.6.5

Re: [PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[Linus Torvalds]

On Tue, Mar 13, 2012 at 8:44 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> Since then any use of "variable length arrays" has become blasphemous.
> Even in perfectly good, beautiful, perfectly safe code like the one
> below where the variable length arrays are only used as a sizeof()
> parameter, for type-safe dynamic structure allocations. GCC is not
> executing any stack allocation code.

If it's a compile-time constant, just make it one. Stop the whining.
No VLA's required. You could, for example, make a trivial wrapper
macro that just declares that array as a constant array in the caller,
and it really *is* a constant sized array with no questions asked.

Instead, you waste more time and effort *whining* than doing that
technical solution.

You do realize that VLA's have caused real problems, since you even
quote some of the background. Not to mention all the problems they
cause for semantics analysis and static checkers.

VLA's really aren't a very good thing in the kernel. And the
work-around I suggest above is actually *simpler* than using them and
then spednign the effort explaining "but they are actually constants
at compile time after all, and aren't the bad kinds of VLAs".

Just don't do them. They are a mistake.

                  Linus

Re: [PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[Boaz Harrosh]

On 03/13/2012 09:37 PM, Linus Torvalds wrote:
> On Tue, Mar 13, 2012 at 8:44 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> Since then any use of "variable length arrays" has become blasphemous.
>> Even in perfectly good, beautiful, perfectly safe code like the one
>> below where the variable length arrays are only used as a sizeof()
>> parameter, for type-safe dynamic structure allocations. GCC is not
>> executing any stack allocation code.
> 
> If it's a compile-time constant, just make it one. Stop the whining.
> No VLA's required. You could, for example, make a trivial wrapper
> macro that just declares that array as a constant array in the caller,
> and it really *is* a constant sized array with no questions asked.
> 

You have not read the code. I do not have a "constant sized array"
I have a function that receives num_entries as parameter.

In turn, the function declares an array *type* of variable size,
just a type, not an actual array. Then uses that type as a
sizeof() parameter to take the size and call kalloc for actual
object allocation.

So all I'm doing is using the compiler generated math to
multiply the record size by num_entries. No other code is
generated.

The reason I like them a lot is because I'm now not doing any
*casting* like I do with the new code. After I did the wrong
math and stumped all over Kernel memory a couple of times I
like the compiler to check me out.

> Instead, you waste more time and effort *whining* than doing that
> technical solution.
> 

Well I also did the change which is not that bad. So since I sent
in a fix, I'm entitled to  a *whine*.

Specially in light of the assembly proof I sent. Which you did
not quote.

> You do realize that VLA's have caused real problems, since you even
> quote some of the background. Not to mention all the problems they
> cause for semantics analysis and static checkers.
> 

Sure, that's true

> VLA's really aren't a very good thing in the kernel. And the
> work-around I suggest above is actually *simpler* than using them and
> then spednign the effort explaining "but they are actually constants
> at compile time after all, and aren't the bad kinds of VLAs".
> 

Again, I am actually using real VLAs but only as a type definition
not as an object declaration. And that part works very well.

(Again see assembler attached)

> Just don't do them. They are a mistake.
> 

Yes! So I've sent in a patch. Assembler wise it's identical.
static checkers should now be happy. So here it is.

Just that I don't have to like it or agree with you.

>                   Linus

Thanks for your reply, always a pleasure
Boaz

Re: [PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[Al Viro]

On Tue, Mar 13, 2012 at 08:44:26PM -0700, Boaz Harrosh wrote:
> +/*	This is the in memory structure of the objio_segment
> + *
> + *	struct __alloc_objio_segment {
> + *		struct objio_segment olseg;
> + *		struct ore_dev *ods[numdevs];
> + *		struct ore_comp	comps[numdevs];
> + *	} *aolseg;
> + *	NOTE: The code as above compiles and runs perfectly. It is elegant,
> + *	type safe and compact.

In which type system?  Not C99 - there variably-modified types are not
allowed as structure or union members...

Re: [PATCH] pnfs-obj: Uglify objio_segment allocation for the sake of the principle :-(

[Boaz Harrosh]

On 03/13/2012 10:15 PM, Al Viro wrote:
> On Tue, Mar 13, 2012 at 08:44:26PM -0700, Boaz Harrosh wrote:
>> +/*	This is the in memory structure of the objio_segment
>> + *
>> + *	struct __alloc_objio_segment {
>> + *		struct objio_segment olseg;
>> + *		struct ore_dev *ods[numdevs];
>> + *		struct ore_comp	comps[numdevs];
>> + *	} *aolseg;
>> + *	NOTE: The code as above compiles and runs perfectly. It is elegant,
>> + *	type safe and compact.
> 
> In which type system?  Not C99 - there variably-modified types are not
> allowed as structure or union members...

Yes. It's not C99. It's a GCC extension that's also supported by
some other compilers.

But the Kernel is full of GCC extensions. Not like this is the
first one.

Thanks
Boaz