Re: [PATCH] Optimize relay_alloc_page_array() slightly by using vzalloc rather than vmalloc and memset

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* 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