lkml.org 
[lkml]   [2010]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2010-11-01 12:41    [W:0.109 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site