x86: OLPC: speed up device tree creation during boot (v2)

arch/x86/platform/olpc/olpc_dt.c +2 -5

 	void *res;
 
-	res = alloc_bootmem(size);
-	if (res)
-		memset(res, 0, size);
-
-	prom_early_allocated += size;
+	if (free_mem < size) {
+		const size_t chunk_size = max(PAGE_SIZE, size);

arch/x86/platform/olpc/olpc_dt.c static unsigned int prom_early_allocated · line 130

Quoted 10 lines · Andres Salomon

(quoted diff) +0 -0

b/arch/x86/platform/olpc/olpc_dt.c index b8c8ff9..0ab824d 100644

arch/x86/platform/olpc/olpc_dt.c +2 -0

@@ -126,14 +126,31 @@  static unsigned int prom_early_allocated
 __initdata; 
 void * __init prom_early_alloc(unsigned long size)
 {
+	static u8 *mem = NULL;
+	static size_t free_mem = 0;

Static variables are implicitly 0 and NULL

Quoted 19 lines

arch/x86/platform/olpc/olpc_dt.c +5 -5

 	void *res;
 
-	res = alloc_bootmem(size);
-	if (res)
-		memset(res, 0, size);
-
-	prom_early_allocated += size;
+	if (free_mem < size) {
+		const size_t chunk_size = max(PAGE_SIZE, size);
+
+		/*
+		 * To mimimize the number of allocations, grab at

least 4k of

arch/x86/platform/olpc/olpc_dt.c +1 -0

+		 * memory (that's an arbitrary choice that matches

PAGE_SIZE on

arch/x86/platform/olpc/olpc_dt.c +1 -0

+		 * the platforms we care about, and minimizes

wasted bootmem)

arch/x86/platform/olpc/olpc_dt.c +1 -0

+		 * and hand off chunks of it to callers.

arch/x86/platform/olpc/olpc_dt.c static unsigned int prom_early_allocated __initdata; · line 144

Quoted 4 lines

arch/x86/platform/olpc/olpc_dt.c +4 -0

+		 */
+		res = mem = alloc_bootmem(chunk_size);
+		if (!res)
+			return NULL;

Oops. If alloc_bootmem fails, we loose mem but don't reset free_mem, so a later call (possibly for a smaller chunk) may return memory starting at NULL.

I suggest just assinging res above and then add mem = res inside this if.

Oh, this is alloc_bootmem not alloc_bootmem_nopainc ... should it be?

arch/x86/platform/olpc/olpc_dt.c static unsigned int prom_early_allocated __initdata; · line 155

Quoted 12 lines

arch/x86/platform/olpc/olpc_dt.c +8 -0

+		prom_early_allocated += chunk_size;
+		memset(res, 0, chunk_size);
+		free_mem = chunk_size;
+	}
 
+	/* allocate from the local cache */
+	free_mem -= size;
+	res = mem;
+	mem += size;
 	return res;
 }

milton

Quoted 13 lines · On Fri, 12 Nov 2010 01:48:30 -0600 Milton Miller <[email protected]> wrote:

arch/x86/platform/olpc/olpc_dt.c static unsigned int prom_early_allocated · line 130

Quoted 10 lines · Andres Salomon

(quoted diff) +0 -0

b/arch/x86/platform/olpc/olpc_dt.c index b8c8ff9..0ab824d 100644

arch/x86/platform/olpc/olpc_dt.c +2 -0

@@ -126,14 +126,31 @@  static unsigned int prom_early_allocated
 __initdata; 
 void * __init prom_early_alloc(unsigned long size)
 {
+	static u8 *mem = NULL;
+	static size_t free_mem = 0;

Static variables are implicitly 0 and NULL

That's true for global static variables; is it also true for static variables that are local to a function?

It's being frowned upon btw, because it's easy to overlook (and for other reasons) - please dont use statics inside functions - please put them in front of the function or further up into file scope.

What? What is wrong with static variables in functions? It really doesn't seem to be a good idea to make them file-scope if they don't need to be.

Quoted 2 lines · H. Peter Anvin

What? What is wrong with static variables in functions? It really doesn't seem to be a good idea to make them file-scope if they don't need to be.

They are very easy to overlook and mix up with regular stack variables and i've seen (and introduced myself) a number of bugs due to them.

They also often are used in buggy ways (with SMP not taken into consideration), so overlooking them during review compounds their negative effects. Putting them in front of the function isnt a big deal in exchange.

There are people who never overlook them (like yourself), but my brain is wired up differently.

Quoted 9 lines · Ingo Molnar

Quoted 2 lines · H. Peter Anvin

What? What is wrong with static variables in functions? It really doesn't seem to be a good idea to make them file-scope if they don't need to be.

They are very easy to overlook and mix up with regular stack variables and i've seen (and introduced myself) a number of bugs due to them.

Quoted 7 lines

They also often are used in buggy ways (with SMP not taken into consideration), so overlooking them during review compounds their negative effects. Putting them in front of the function isnt a big deal in exchange.

There are people who never overlook them (like yourself), but my brain is wired up differently.

However, I have to vehemently object to putting them in a wider scope than is otherwise necessary. I agree that static variables should be used sparsely if at all (there really are vary few uses of them that are valid), but putting them in a larger scope screams "I'm used in more than one function", and that is *not* a good thing.

Quoted 18 lines · H. Peter Anvin

Quoted 9 lines · Ingo Molnar

Quoted 2 lines · H. Peter Anvin

What? What is wrong with static variables in functions? It really doesn't seem to be a good idea to make them file-scope if they don't need to be.

They are very easy to overlook and mix up with regular stack variables and i've seen (and introduced myself) a number of bugs due to them.

Quoted 7 lines

They also often are used in buggy ways (with SMP not taken into consideration), so overlooking them during review compounds their negative effects. Putting them in front of the function isnt a big deal in exchange.

There are people who never overlook them (like yourself), but my brain is wired up differently.

Quoted 5 lines

However, I have to vehemently object to putting them in a wider scope than is otherwise necessary. I agree that static variables should be used sparsely if at all (there really are vary few uses of them that are valid), but putting them in a larger scope screams "I'm used in more than one function", and that is *not* a good thing.

That's why we sometimes use the (imperfect) compromise to put them in front of that function, not at the top of the file.

Look at the general balance of hardship: very little harm is done (it's not a big deal if a variable is only used in a single function) but having it with local variables can be _really_ harmful - for example i overlooked them when i reviewed this patch. I dont like important details obscured - i like them to be apparent. Again, this is something that some people can parse immediately on the visual level
- me and many others cannot.

Quoted 7 lines · Ingo Molnar

Look at the general balance of hardship: very little harm is done (it's not a big deal if a variable is only used in a single function) but having it with local variables can be _really_ harmful - for example i overlooked them when i reviewed this patch. I dont like important details obscured - i like them to be apparent. Again, this is something that some people can parse immediately on the visual level
- me and many others cannot.

What about:

int foo(void)

{
	static int bar;

	struct thing_struct *thing;
	int other_var;
	char *p;

	...
}

I think the visual wrongness of that formatting would be enough for me to stop and look twice. Though I guess it doesn't work if you have few, or no other variables other than the statics to declare.

Quoted 11 lines · Michael Ellerman

Quoted 9 lines · Ingo Molnar

Look at the general balance of hardship: very little harm is done (it's not a big deal if a variable is only used in a single function) but having it with local variables can be _really_ harmful - for example i overlooked them when i reviewed this patch. I dont like important details obscured - i like them to be apparent. Again, this is something that some people can parse immediately on the visual level
- me and many others cannot.

No, sorry, this sounds like a personal preference that is well out of line with the vast majority of C programmers I've ever come across, not just in the Linux kernel world but outside of it.

Quoted 4 lines

No, sorry, this sounds like a personal preference that is well out of line with the vast majority of C programmers I've ever come across, not just in the Linux kernel world but outside of it.

This is actually one of the reasons I specifically like initialized static variables (inside of functions). Take the following code:

int foo(void)

{
	static char *frob = NULL;
	int p;

	if (frob) {
		...
	}

Upon seeing that and thinking "whoa, how could frob be initialized and then checked?", I realize that it's either a bug or I look back at the initialization and realize that frob is static. It's less obvious (to me) with non-explicit initialization.

Quoted 19 lines · Andres Salomon

Quoted 4 lines

No, sorry, this sounds like a personal preference that is well out of line with the vast majority of C programmers I've ever come across, not just in the Linux kernel world but outside of it.

This is actually one of the reasons I specifically like initialized static variables (inside of functions). Take the following code:

int foo(void)

{
	static char *frob = NULL;
	int p;

	if (frob) {
		...
	}

Quoted 4 lines

Upon seeing that and thinking "whoa, how could frob be initialized and then checked?", I realize that it's either a bug or I look back at the initialization and realize that frob is static. It's less obvious (to me) with non-explicit initialization.

I have to agree with this one. In general I dislike relying on an implicit (even well-defined) initialized value; unfortunately we ripped out explicit initializations across the Linux kernel, not due to readability but due to the fact that long-since-obsolete versions of gcc would put explicitly-initialized variables in data rather than bss even if the initial value is zero.

-hpa

Quoted 5 lines

However, I have to vehemently object to putting them in a wider scope than is otherwise necessary. I agree that static variables should be used sparsely if at all (there really are vary few uses of them that are valid), but putting them in a larger scope screams "I'm used in more than one function", and that is *not* a good thing.

That's why we sometimes use the (imperfect) compromise to put them in front of that function, not at the top of the file.

Look at the general balance of hardship: very little harm is done (it's not a big deal if a variable is only used in a single function) but having it with local variables can be _really_ harmful - for example i overlooked them when i reviewed this patch. I dont like important details obscured - i like them to be apparent. Again, this is something that some people can parse immediately on the visual level - me and many others cannot.

From 3cb50ddf97a0a1ca4c68bc12fa1e727a6b45fbf2 Mon Sep 17 00:00:00 2001