Messages in this thread | | | Date | Mon, 1 Nov 2010 07:27:55 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] Optimize relay_alloc_page_array() slightly by using vzalloc rather than vmalloc and memset |
| |
* Andrew Morton (akpm@linux-foundation.org) wrote: > On Sun, 31 Oct 2010 14:39:14 -0400 Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote: > > > * Jesper Juhl (jj@chaosbits.net) wrote: > > > On Sat, 30 Oct 2010, Mathieu Desnoyers wrote: > > > > > > > * Jesper Juhl (jj@chaosbits.net) wrote: > > > > > Hi, > > > > > > > > > > We can optimize kernel/relay.c::relay_alloc_page_array() slightly by using > > > > > vzalloc. The patch makes these changes: > > > > > > > > > > - use vzalloc instead of vmalloc+memset. > > > > > - remove redundant local variable 'array'. > > > > > - declare local 'pa_size' as const. > > > > > > > > Hrm ? How does declaring a local variable as const helps the compiler in > > > > any way ? > > > > > > > > > > Hmm, probably not very much in this case (but it doesn't hurt either ;) - > > > actually, removing the const yielded the exact same result, so it's > > > "not at all" in this case). > > > That bit came from my "build-in" tendency to declare stuff const when it > > > obviously doesn't change/nor should. It's a habbit.. > > > > Which looks to me like a misunderstanding of the C99 standard. What you > > do is: > > > > static struct page **relay_alloc_page_array(unsigned int n_pages) > > { > > const size_t pa_size = n_pages * sizeof(struct page *); > > ... > > } > > > > So the compiler has no choice but to emit code that will fill in the > > value of pa_size at runtime, because it depends on "n_pages", a > > parameter received by the function. So pa_size is everything but > > constant. > > > > The C99 standard, section 6.7.3 (Type qualifiers) states: > > > > "The implementation may place a const object that is not volatile in a > > read-only region of storage. Moreover, the implementation need not > > allocate storage for such an object if its address is never used." > > > > So maybe gcc is kind here and it just removes this const specifier > > without complaining, but a different compiler might be more strict and > > fail to compile because you would be dynamically assigning a value to a > > variable placed in read-only storage. > > Such a compiler would be pretty darn useless - that's a quite common > thing to do.
Just for fun, I grepped kernel/ and arch/x86/ for instances of "[tab]const". Removing all the "const char *", which is a pointer to const data (so it's entirely different), I ended up with only a handful of instances of a similar scenario (const function variable initialized depending on function arguments). Most of these were in a single function in vmi_32.c.
> It's also a very *useful* thing to do. In a long function it's easy to > lose track of what variable has what value where, and it's easy to add > bugs by modifying a variable which you didn't realise gets used later > on. If the definition has a "const" in front of it then great, that > settled everything.
I can see it being useful. I was just concerned about whether or not it respects the standard, which seems like a non-issue to you. I can therefore only provide my input, feel free to discard it.
> > > > > > > > > > > Moreover, is there anyone still using this code ? LTTng uses the Generic > > > > Ring Buffer library which completely deprecates relay.c. Perf and Ftrace > > > > each have their own ring buffers, without dependency on relay.c. > > > > > > > > BLK_DEV_IO_TRACE seems to still select RELAY. Has it completed its > > > > transition to either Ftrace or Perf ? Depending on Jens, moving blktrace > > > > relay dependency to the Generic Ring Buffer Library might be a good > > > > option to consider. > > > > > > > > > > I admit I have no idea if this code is actually still used, but as long as > > > it's in the kernel I think we should strive to make it as good as possible > > > - no? If there are still users this is an improvement, if there are no > > > users (who knows if there are out-of-tree ones?) then it should probably > > > just go away (but even then this patch does no harm - except for a bit of > > > churn). > > > > Another point that I don't like about your patch is the comment: > > > > "Compile tested only." > > > > Please don't break unused code for the sake of trying to slightly > > optimize it, especially if you don't bother testing your modifications. > > > > So as far as I am concerned, I am Nack-ing this patch. I might possibly > > nack any further change to relay.c, and hereby propose its replacement > > by the generic ring buffer library, unless someone comes up with a good > > reason for keeping it. > > aw, c'mon, read the code. The patch is good and improves that function > so much it ain't funny.
I agree that the improvements to the allocation+zeroing are welcome. As you point out, these are trivial.
> > It's a non-runtime-tested, obviously-correct cleanup. Yes, it would be > better if it was runtime tested. But we merge patches on this basis > all the time and it works out OK. > > If, amazingly, there is some bug in it then someone will hit that bug in > -next or -rc testing and we'll fix it. Shrug. If you're that worried then > *you* could runtime test it!
I was just concerned about eventual conflicts if relay.c gets removed, given that there does not seem to be many (any ?) users left. But I won't be managing the conflicts, so, again, feel free to merge it.
Best regards,
Mathieu
-- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com
| |