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

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.

---
 fs/nfs/objlayout/objio_osd.c |   39 +++++++++++++++++++++++++--------------
 1 files changed, 25 insertions(+), 14 deletions(-)

index 55d0128..37a9269 100644
@@ -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

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

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

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.

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.

Sure, that's true

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)

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.

Thanks for your reply, always a pleasure
Boaz

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