Message-ID: <20101031183913.GA26160@Krystal>
Date:	Sun, 31 Oct 2010 14:39:14 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tom Zanussi <zanussi@...ibm.com>,
	Karim Yaghmour <karim@...rsys.com>,
	Paul Mundt <lethal@...ux-sh.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Jens Axboe <axboe@...nel.dk>
Subject: Re: [PATCH] Optimize relay_alloc_page_array() slightly by using
	vzalloc rather than vmalloc and memset

* Jesper Juhl (jj@...osbits.net) wrote:
> On Sat, 30 Oct 2010, Mathieu Desnoyers wrote:
> 
> > * Jesper Juhl (jj@...osbits.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.

> 
> 
> > 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.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists