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

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

[Jesper Juhl]

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.

Cuts down nicely on both source and object-code size.


Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
Compile tested only.

 relay.c |   15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/kernel/relay.c b/kernel/relay.c
index c7cf397..859ea5a 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -70,17 +70,10 @@ static const struct vm_operations_struct relay_file_mmap_ops = {
  */
 static struct page **relay_alloc_page_array(unsigned int n_pages)
 {
-	struct page **array;
-	size_t pa_size = n_pages * sizeof(struct page *);
-
-	if (pa_size > PAGE_SIZE) {
-		array = vmalloc(pa_size);
-		if (array)
-			memset(array, 0, pa_size);
-	} else {
-		array = kzalloc(pa_size, GFP_KERNEL);
-	}
-	return array;
+	const size_t pa_size = n_pages * sizeof(struct page *);
+	if (pa_size > PAGE_SIZE)
+		return vzalloc(pa_size);
+	return kzalloc(pa_size, GFP_KERNEL);
 }
 
 /*



-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

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

[Mathieu Desnoyers]

* 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 ?

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.

Thanks,

Mathieu

> 
> Cuts down nicely on both source and object-code size.
> 
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
> Compile tested only.
> 
>  relay.c |   15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/relay.c b/kernel/relay.c
> index c7cf397..859ea5a 100644
> --- a/kernel/relay.c
> +++ b/kernel/relay.c
> @@ -70,17 +70,10 @@ static const struct vm_operations_struct relay_file_mmap_ops = {
>   */
>  static struct page **relay_alloc_page_array(unsigned int n_pages)
>  {
> -	struct page **array;
> -	size_t pa_size = n_pages * sizeof(struct page *);
> -
> -	if (pa_size > PAGE_SIZE) {
> -		array = vmalloc(pa_size);
> -		if (array)
> -			memset(array, 0, pa_size);
> -	} else {
> -		array = kzalloc(pa_size, GFP_KERNEL);
> -	}
> -	return array;
> +	const size_t pa_size = n_pages * sizeof(struct page *);
> +	if (pa_size > PAGE_SIZE)
> +		return vzalloc(pa_size);
> +	return kzalloc(pa_size, GFP_KERNEL);
>  }
>  
>  /*
> 
> 
> 
> -- 
> Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
> Plain text mails only, please      http://www.expita.com/nomime.html
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

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

[Jesper Juhl]

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


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


-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

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

[Mathieu Desnoyers]

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

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

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

[Andrew Morton]

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.

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.

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

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!  

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

[Pekka Enberg]

On Mon, Nov 1, 2010 at 2:46 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> aw, c'mon, read the code.  The patch is good and improves that function
> so much it ain't funny.
>
> 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!

Looks good to me too.

Acked-by: Pekka Enberg <penberg@kernel.org>

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

[Mathieu Desnoyers]

* 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

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

[Américo Wang]

On Sun, Oct 31, 2010 at 02:39:14PM -0400, Mathieu Desnoyers 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."
>


This is not enforced by C99. This is C, not C++. :)

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

That compiler would be broken if it exists. Also, I doubt linux kernel
could be compiled with other compilers than gcc (except icc?).

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

[Américo Wang]

On Mon, Nov 01, 2010 at 07:27:55AM -0400, Mathieu Desnoyers wrote:
>
>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.
>

blktrace is still using it, AFAIK.

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

[Jens Axboe]

On 2010-10-30 17:47, Mathieu Desnoyers wrote:
> 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.

The blktrace user bits is still (by far) the most wide spread way that
blktrace is used in the field, and those still rely on relayfs. So no,
we can't kill it now.

-- 
Jens Axboe

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

[el es]

Américo Wang <xiyou.wangcong <at> gmail.com> writes:


> That compiler would be broken if it exists. Also, I doubt linux kernel
> could be compiled with other compilers than gcc (except icc?).
> 

Not longer than a few days ago, somebody from llvm claimed he got it compiling
the kernel, and also that he got it self-hosting, on bare metal and in virtual
box (virtual box <> VirtualBox, lol)

el es

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

[Mathieu Desnoyers]

* Jens Axboe (axboe@kernel.dk) wrote:
> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
> > 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.
> 
> The blktrace user bits is still (by far) the most wide spread way that
> blktrace is used in the field, and those still rely on relayfs. So no,
> we can't kill it now.

What I am proposing is that the Generic Ring Buffer Library could
replace relayfs without changing any of the interfaces blktrace exposes
to user-space. Indeed, I would not remove relayfs unless there was a
replacement.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

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

[Jens Axboe]

On 2010-11-01 09:08, Mathieu Desnoyers wrote:
> * Jens Axboe (axboe@kernel.dk) wrote:
>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
>>> 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.
>>
>> The blktrace user bits is still (by far) the most wide spread way that
>> blktrace is used in the field, and those still rely on relayfs. So no,
>> we can't kill it now.
> 
> What I am proposing is that the Generic Ring Buffer Library could
> replace relayfs without changing any of the interfaces blktrace exposes
> to user-space. Indeed, I would not remove relayfs unless there was a
> replacement.

Sure, I'm open to such a solution as long as it doesn't slow anything
down for blktrace (primary concern, certainly) and retains feature
parity. I'm not married to relayfs, it's just what was available easily
when I added blktrace originally.

-- 
Jens Axboe

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

[Mathieu Desnoyers]

* 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:
[...]
> > 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.

Actually, "object" in the C99 standard refers to global variables, not
local variables. The misunderstanding was on my part.

Sorry about that,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

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

[Pekka Enberg]

* Jens Axboe (axboe@kernel.dk) wrote:
>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
>> > 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.
>>
>> The blktrace user bits is still (by far) the most wide spread way that
>> blktrace is used in the field, and those still rely on relayfs. So no,
>> we can't kill it now.

On Mon, Nov 1, 2010 at 3:08 PM, Mathieu Desnoyers
<mathieu.desnoyers@polymtl.ca> wrote:
> What I am proposing is that the Generic Ring Buffer Library could
> replace relayfs without changing any of the interfaces blktrace exposes
> to user-space. Indeed, I would not remove relayfs unless there was a
> replacement.

We don't in general NAK cleanups because of future features or
removals that may or may not happen.

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

[Jens Axboe]

On 2010-11-01 09:41, Pekka Enberg wrote:
> * Jens Axboe (axboe@kernel.dk) wrote:
>>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
>>>> 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.
>>>
>>> The blktrace user bits is still (by far) the most wide spread way that
>>> blktrace is used in the field, and those still rely on relayfs. So no,
>>> we can't kill it now.
> 
> On Mon, Nov 1, 2010 at 3:08 PM, Mathieu Desnoyers
> <mathieu.desnoyers@polymtl.ca> wrote:
>> What I am proposing is that the Generic Ring Buffer Library could
>> replace relayfs without changing any of the interfaces blktrace exposes
>> to user-space. Indeed, I would not remove relayfs unless there was a
>> replacement.
> 
> We don't in general NAK cleanups because of future features or
> removals that may or may not happen.

Agree, this is parallel to whether or not we can transition blktrace to
using the generic ring buffer library or not. If the current patch
proposed makes sense, then it should go in regardless of
potential/future plans.

-- 
Jens Axboe

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

[Mathieu Desnoyers]

* Jens Axboe (axboe@kernel.dk) wrote:
> On 2010-11-01 09:41, Pekka Enberg wrote:
> > * Jens Axboe (axboe@kernel.dk) wrote:
> >>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
> >>>> 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.
> >>>
> >>> The blktrace user bits is still (by far) the most wide spread way that
> >>> blktrace is used in the field, and those still rely on relayfs. So no,
> >>> we can't kill it now.
> > 
> > On Mon, Nov 1, 2010 at 3:08 PM, Mathieu Desnoyers
> > <mathieu.desnoyers@polymtl.ca> wrote:
> >> What I am proposing is that the Generic Ring Buffer Library could
> >> replace relayfs without changing any of the interfaces blktrace exposes
> >> to user-space. Indeed, I would not remove relayfs unless there was a
> >> replacement.
> > 
> > We don't in general NAK cleanups because of future features or
> > removals that may or may not happen.
> 
> Agree, this is parallel to whether or not we can transition blktrace to
> using the generic ring buffer library or not. If the current patch
> proposed makes sense, then it should go in regardless of
> potential/future plans.

Agreed. Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

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

[Jesper Juhl]

On Mon, 1 Nov 2010, Mathieu Desnoyers wrote:

> * Jens Axboe (axboe@kernel.dk) wrote:
> > On 2010-11-01 09:41, Pekka Enberg wrote:
> > > * Jens Axboe (axboe@kernel.dk) wrote:
> > >>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
> > >>>> 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.
> > >>>
> > >>> The blktrace user bits is still (by far) the most wide spread way that
> > >>> blktrace is used in the field, and those still rely on relayfs. So no,
> > >>> we can't kill it now.
> > > 
> > > On Mon, Nov 1, 2010 at 3:08 PM, Mathieu Desnoyers
> > > <mathieu.desnoyers@polymtl.ca> wrote:
> > >> What I am proposing is that the Generic Ring Buffer Library could
> > >> replace relayfs without changing any of the interfaces blktrace exposes
> > >> to user-space. Indeed, I would not remove relayfs unless there was a
> > >> replacement.
> > > 
> > > We don't in general NAK cleanups because of future features or
> > > removals that may or may not happen.
> > 
> > Agree, this is parallel to whether or not we can transition blktrace to
> > using the generic ring buffer library or not. If the current patch
> > proposed makes sense, then it should go in regardless of
> > potential/future plans.
> 
> Agreed. Thanks,
> 

As I read the discussion over the last day or so, since I last checked my 
mail, there are no longer any objections to the patch. Correct?
If that's the case, who will merge it? and should I resend it with various 
peoples Acked-by: lines? or?


-- 
Jesper Juhl <jj@chaosbits.net>             http://www.chaosbits.net/
Plain text mails only, please      http://www.expita.com/nomime.html
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html

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

[Mathieu Desnoyers]

* Jesper Juhl (jj@chaosbits.net) wrote:
> On Mon, 1 Nov 2010, Mathieu Desnoyers wrote:
> 
> > * Jens Axboe (axboe@kernel.dk) wrote:
> > > On 2010-11-01 09:41, Pekka Enberg wrote:
> > > > * Jens Axboe (axboe@kernel.dk) wrote:
> > > >>> On 2010-10-30 17:47, Mathieu Desnoyers wrote:
> > > >>>> 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.
> > > >>>
> > > >>> The blktrace user bits is still (by far) the most wide spread way that
> > > >>> blktrace is used in the field, and those still rely on relayfs. So no,
> > > >>> we can't kill it now.
> > > > 
> > > > On Mon, Nov 1, 2010 at 3:08 PM, Mathieu Desnoyers
> > > > <mathieu.desnoyers@polymtl.ca> wrote:
> > > >> What I am proposing is that the Generic Ring Buffer Library could
> > > >> replace relayfs without changing any of the interfaces blktrace exposes
> > > >> to user-space. Indeed, I would not remove relayfs unless there was a
> > > >> replacement.
> > > > 
> > > > We don't in general NAK cleanups because of future features or
> > > > removals that may or may not happen.
> > > 
> > > Agree, this is parallel to whether or not we can transition blktrace to
> > > using the generic ring buffer library or not. If the current patch
> > > proposed makes sense, then it should go in regardless of
> > > potential/future plans.
> > 
> > Agreed. Thanks,
> > 
> 
> As I read the discussion over the last day or so, since I last checked my 
> mail, there are no longer any objections to the patch. Correct?

Yes,

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> If that's the case, who will merge it? and should I resend it with various 
> peoples Acked-by: lines? or?

Yep, this might make the maintainer's work easier, I think. As for who
will merge it, good question. Andrew ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com