[PATCH] ring_buffer: fix ring_buffer_event_length()

[PATCH] ring_buffer: fix ring_buffer_event_length()

[Steven Rostedt]

Hi Ingo,

I added the Impact line. This probably should go into 2.6.29.

The following patch is in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: tip/devel


Robert Richter (1):
      ring_buffer: fix ring_buffer_event_length()

----
 kernel/trace/ring_buffer.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
---------------------------
commit 49c86f2104c25d36f1a44e569c08968f388da75d
Author: Robert Richter <robert.richter@amd.com>
Date:   Wed Jan 7 23:45:23 2009 +0100

    ring_buffer: fix ring_buffer_event_length()
    
    Impact: fix to ring_buffer_event_length()
    
    Function ring_buffer_event_length() provides an interface to detect
    the length of data stored in an entry. However, the length contains
    offsets depending on the internal usage. This makes it unusable. This
    patch fixes this and now ring_buffer_event_length() returns the
    alligned length that has been used in ring_buffer_lock_reserve().
    
    Signed-off-by: Robert Richter <robert.richter@amd.com>
    Signed-off-by: Steven Rostedt <srostedt@redhat.com>

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index a9d9760..8b0daf0 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -168,7 +168,13 @@ rb_event_length(struct ring_buffer_event *event)
  */
 unsigned ring_buffer_event_length(struct ring_buffer_event *event)
 {
-	return rb_event_length(event);
+	unsigned length = rb_event_length(event);
+	if (event->type != RINGBUF_TYPE_DATA)
+		return length;
+	length -= RB_EVNT_HDR_SIZE;
+	if (length > RB_MAX_SMALL_DATA + sizeof(event->array[0]))
+                length -= sizeof(event->array[0]);
+	return length;
 }
 EXPORT_SYMBOL_GPL(ring_buffer_event_length);
 

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Andrew Morton]

On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:

>  kernel/trace/ring_buffer.c |    8 +++++++-

<looks>

heavens, what a lot of inlining.  Looks like something from 1997 :)

Prove me wrong!


From: Andrew Morton <akpm@linux-foundation.org>

          text    data     bss     dec     hex filename
before:  11320     228       8   11556    2d24 kernel/trace/ring_buffer.o
after:   10592     228       8   10828    2a4c kernel/trace/ring_buffer.o

Also: free_page(0) is legal.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/trace/ring_buffer.c |   23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff -puN kernel/trace/ring_buffer.c~kernel-trace-ring_bufferc-reduce-inlining kernel/trace/ring_buffer.c
--- a/kernel/trace/ring_buffer.c~kernel-trace-ring_bufferc-reduce-inlining
+++ a/kernel/trace/ring_buffer.c
@@ -133,7 +133,7 @@ enum {
 };
 
 /* inline for ring buffer fast paths */
-static inline unsigned
+static unsigned
 rb_event_length(struct ring_buffer_event *event)
 {
 	unsigned length;
@@ -173,7 +173,7 @@ unsigned ring_buffer_event_length(struct
 EXPORT_SYMBOL_GPL(ring_buffer_event_length);
 
 /* inline for ring buffer fast paths */
-static inline void *
+static void *
 rb_event_data(struct ring_buffer_event *event)
 {
 	BUG_ON(event->type != RINGBUF_TYPE_DATA);
@@ -223,10 +223,9 @@ static void rb_init_page(struct buffer_d
  * Also stolen from mm/slob.c. Thanks to Mathieu Desnoyers for pointing
  * this issue out.
  */
-static inline void free_buffer_page(struct buffer_page *bpage)
+static void free_buffer_page(struct buffer_page *bpage)
 {
-	if (bpage->page)
-		free_page((unsigned long)bpage->page);
+	free_page((unsigned long)bpage->page);
 	kfree(bpage);
 }
 
@@ -805,7 +804,7 @@ rb_event_index(struct ring_buffer_event 
 	return (addr & ~PAGE_MASK) - (PAGE_SIZE - BUF_PAGE_SIZE);
 }
 
-static inline int
+static int
 rb_is_commit(struct ring_buffer_per_cpu *cpu_buffer,
 	     struct ring_buffer_event *event)
 {
@@ -819,7 +818,7 @@ rb_is_commit(struct ring_buffer_per_cpu 
 		rb_commit_index(cpu_buffer) == index;
 }
 
-static inline void
+static void
 rb_set_commit_event(struct ring_buffer_per_cpu *cpu_buffer,
 		    struct ring_buffer_event *event)
 {
@@ -844,7 +843,7 @@ rb_set_commit_event(struct ring_buffer_p
 	local_set(&cpu_buffer->commit_page->page->commit, index);
 }
 
-static inline void
+static void
 rb_set_commit_to_write(struct ring_buffer_per_cpu *cpu_buffer)
 {
 	/*
@@ -890,7 +889,7 @@ static void rb_reset_reader_page(struct 
 	cpu_buffer->reader_page->read = 0;
 }
 
-static inline void rb_inc_iter(struct ring_buffer_iter *iter)
+static void rb_inc_iter(struct ring_buffer_iter *iter)
 {
 	struct ring_buffer_per_cpu *cpu_buffer = iter->cpu_buffer;
 
@@ -920,7 +919,7 @@ static inline void rb_inc_iter(struct ri
  * and with this, we can determine what to place into the
  * data field.
  */
-static inline void
+static void
 rb_update_event(struct ring_buffer_event *event,
 			 unsigned type, unsigned length)
 {
@@ -958,7 +957,7 @@ rb_update_event(struct ring_buffer_event
 	}
 }
 
-static inline unsigned rb_calculate_event_length(unsigned length)
+static unsigned rb_calculate_event_length(unsigned length)
 {
 	struct ring_buffer_event event; /* Used only for sizeof array */
 
@@ -1432,7 +1431,7 @@ int ring_buffer_write(struct ring_buffer
 }
 EXPORT_SYMBOL_GPL(ring_buffer_write);
 
-static inline int rb_per_cpu_empty(struct ring_buffer_per_cpu *cpu_buffer)
+static int rb_per_cpu_empty(struct ring_buffer_per_cpu *cpu_buffer)
 {
 	struct buffer_page *reader = cpu_buffer->reader_page;
 	struct buffer_page *head = cpu_buffer->head_page;
_

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  kernel/trace/ring_buffer.c |    8 +++++++-
> 
> <looks>
> 
> heavens, what a lot of inlining.  Looks like something from 1997 :)
> 
> Prove me wrong!
> 
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> 
>           text    data     bss     dec     hex filename
> before:  11320     228       8   11556    2d24 kernel/trace/ring_buffer.o
> after:   10592     228       8   10828    2a4c kernel/trace/ring_buffer.o

You are wrong :-)

With x86 defconfig and gcc 4.3.2 i get zero change in size:

  kernel/trace/ring_buffer.o:

     text	   data	    bss	    dec	    hex	filename
    11485	    228	      8	  11721	   2dc9	ring_buffer.o.before
    11485	    228	      8	  11721	   2dc9	ring_buffer.o.after

  md5:
     55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.before.asm
     55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.after.asm

(i took out the free_page() bit to only measure the inlining)

That is the same with and without CONFIG_OPTIMIZE_INLINING - i.e. recent 
GCC gets the inlining right.

Really, we should stop bothering about inlines on the source code level 
(the kernel has 20,000 inlines and around 100,000 functions - do we really 
want to maintain inlining information on a per function basis?) - and we 
should tell the GCC folks when the compiler messes up some detail.

Or if GCC messes up inlining so much in the future that we cannot live 
with it, we can go back to "always inline" and manual annotations again. 
Or write a new compiler. (the latter is probably less work ;-)

	Ingo

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Andrew Morton]

On Thu, 8 Jan 2009 12:55:30 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > >  kernel/trace/ring_buffer.c |    8 +++++++-
> > 
> > <looks>
> > 
> > heavens, what a lot of inlining.  Looks like something from 1997 :)
> > 
> > Prove me wrong!
> > 
> > 
> > From: Andrew Morton <akpm@linux-foundation.org>
> > 
> >           text    data     bss     dec     hex filename
> > before:  11320     228       8   11556    2d24 kernel/trace/ring_buffer.o
> > after:   10592     228       8   10828    2a4c kernel/trace/ring_buffer.o
> 
> You are wrong :-)

Not.

> With x86 defconfig and gcc 4.3.2 i get zero change in size:

With my config and my gcc I see a large change in size.  So those
`inline' statements in that C file are *wrong*.

>   kernel/trace/ring_buffer.o:
> 
>      text	   data	    bss	    dec	    hex	filename
>     11485	    228	      8	  11721	   2dc9	ring_buffer.o.before
>     11485	    228	      8	  11721	   2dc9	ring_buffer.o.after
> 
>   md5:
>      55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.before.asm
>      55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.after.asm
> 
> (i took out the free_page() bit to only measure the inlining)
> 
> That is the same with and without CONFIG_OPTIMIZE_INLINING - i.e. recent 
> GCC gets the inlining right.
> 
> Really, we should stop bothering about inlines on the source code level 
> (the kernel has 20,000 inlines and around 100,000 functions - do we really 
> want to maintain inlining information on a per function basis?) - and we 
> should tell the GCC folks when the compiler messes up some detail.
> 
> Or if GCC messes up inlining so much in the future that we cannot live 
> with it, we can go back to "always inline" and manual annotations again. 
> Or write a new compiler. (the latter is probably less work ;-)

None of that makes the inline statements in ring_buffer.c less wrong. 
It says that with some configs and some gcc versions, their damage is
lessened.

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 8 Jan 2009 12:55:30 +0100 Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> > > 
> > > >  kernel/trace/ring_buffer.c |    8 +++++++-
> > > 
> > > <looks>
> > > 
> > > heavens, what a lot of inlining.  Looks like something from 1997 :)
> > > 
> > > Prove me wrong!
> > > 
> > > 
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > >           text    data     bss     dec     hex filename
> > > before:  11320     228       8   11556    2d24 kernel/trace/ring_buffer.o
> > > after:   10592     228       8   10828    2a4c kernel/trace/ring_buffer.o
> > 
> > You are wrong :-)
> 
> Not.
> 
> > With x86 defconfig and gcc 4.3.2 i get zero change in size:
> 
> With my config and my gcc I see a large change in size.  So those
> `inline' statements in that C file are *wrong*.
> 
> >   kernel/trace/ring_buffer.o:
> > 
> >      text	   data	    bss	    dec	    hex	filename
> >     11485	    228	      8	  11721	   2dc9	ring_buffer.o.before
> >     11485	    228	      8	  11721	   2dc9	ring_buffer.o.after
> > 
> >   md5:
> >      55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.before.asm
> >      55447563cd459bbb02c6234b2544fcc2  ring_buffer.o.after.asm
> > 
> > (i took out the free_page() bit to only measure the inlining)
> > 
> > That is the same with and without CONFIG_OPTIMIZE_INLINING - i.e. recent 
> > GCC gets the inlining right.
> > 
> > Really, we should stop bothering about inlines on the source code level 
> > (the kernel has 20,000 inlines and around 100,000 functions - do we really 
> > want to maintain inlining information on a per function basis?) - and we 
> > should tell the GCC folks when the compiler messes up some detail.
> > 
> > Or if GCC messes up inlining so much in the future that we cannot live 
> > with it, we can go back to "always inline" and manual annotations 
> > again. Or write a new compiler. (the latter is probably less work ;-)
> 
> None of that makes the inline statements in ring_buffer.c less wrong. It 
> says that with some configs and some gcc versions, their damage is 
> lessened.

It's not 'some configs' - it's the "make the kernel smaller via inlining" 
config.

Here's the stats with gcc 4.3.2:

   text  filename
  11502  kernel/trace/ring_buffer.o [always-inline]
  .....
  11466  kernel/trace/ring_buffer.o +optimize-inlining
  11461  kernel/trace/ring_buffer.o +your-patch

i.e. the compiler was able to get within 0.043% of the manual tuning that 
you did - without the need of any patch.

Lets assume you needed 15 minutes to create, test and send that patch. 
ring_buffer.c is 1 file with 2500 lines of code.

In this cycle alone we changed this much kernel code:

   9046 files changed, 1214357 insertions(+), 461447 deletions(-)

Lets assume that you can spend 8 hours a day just to re-validate the 
inlining of that code. Only that - nothing else. It would need ~100 hours 
of your time per kernel cycle (about two weeks if sleep time is counted as 
well) - or two hours per day, just to keep the inlines maintained.

The numbers are probably far worse for non-akpm coders and if we count the 
inefficiency of distributing this amongst many coders who dont generally 
do this kind of activities.

And that's for something that can be done by a tool to within ~0.043% 
efficiency.

Is it really worth the trouble? Is the payoff proportional? Is it a wise 
use of development resources?

( And i've applied your patch of course - it's a good patch - i'm just
  asking whether we humans should be in the business of inline 
  annotations. )

	Ingo

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Steven Rostedt]

On Wed, 7 Jan 2009, Andrew Morton wrote:

> On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> >  kernel/trace/ring_buffer.c |    8 +++++++-
> 
> <looks>
> 
> heavens, what a lot of inlining.  Looks like something from 1997 :)
> 
> Prove me wrong!

I'm working on it ;-)

-- Steve

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 7 Jan 2009, Andrew Morton wrote:
> 
> > On Wed, 7 Jan 2009 23:58:39 -0500 (EST) Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > >  kernel/trace/ring_buffer.c |    8 +++++++-
> > 
> > <looks>
> > 
> > heavens, what a lot of inlining.  Looks like something from 1997 :)
> > 
> > Prove me wrong!
> 
> I'm working on it ;-)

i really think that inlines in .c code are uninteresting. They are 
basically inserted randomly when a function starts out 'looking simple' - 
then are forgotten about when the function grows a bit.

They have some very mild documentation value ('look, this function is 
supposed to be simple'), but otherwise it just increases review noise. And 
latest GCC seems pretty ambivalent to their presence - so they dont help 
nor hurt in any direction.

So i think we can afford to enter the year 2009 and can stop bothering 
about [.c file] inlines ;-)

[ The only interesting inlines are the __always_inline instances - that's 
  where we _must_ have inlining for some subtle reason. But those should 
  almost never be used in .c code. ]

	Ingo

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> Hi Ingo,
> 
> I added the Impact line. This probably should go into 2.6.29.
> 
> The following patch is in:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> 
>     branch: tip/devel
> 
> 
> Robert Richter (1):
>       ring_buffer: fix ring_buffer_event_length()

applied to tip/tracing/urgent, thanks guys!

sidenote:

>     ring_buffer: fix ring_buffer_event_length()
>     
>     Impact: fix to ring_buffer_event_length()

That is a pretty useless duplication of the subject line, so i dropped it. 
When the subject line explains the practical impact already, there's no 
need to add the impact line.

	Ingo

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Steven Rostedt]

On Thu, 8 Jan 2009, Ingo Molnar wrote:
> 
> That is a pretty useless duplication of the subject line, so i dropped it. 
> When the subject line explains the practical impact already, there's no 
> need to add the impact line.

Actually, Frederic and I had a discussion about a duplicate Impact line. 
Perhaps someone is searching the git log on "Impact:" looking for all 
fixes. Perhaps just a simple:

Impact: fix

would be sufficient?

-- Steve

Re: [PATCH] ring_buffer: fix ring_buffer_event_length()

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Thu, 8 Jan 2009, Ingo Molnar wrote:
> > 
> > That is a pretty useless duplication of the subject line, so i dropped it. 
> > When the subject line explains the practical impact already, there's no 
> > need to add the impact line.
> 
> Actually, Frederic and I had a discussion about a duplicate Impact line. 
> Perhaps someone is searching the git log on "Impact:" looking for all 
> fixes. Perhaps just a simple:
> 
> Impact: fix
> 
> would be sufficient?

hm, okay - but it should be somewhat different. The impact line should 
really be a 'description of impact/risk'. That overlaps subject lines some 
of the time - but often not. So we could do something more specific like:

  Impact: change/fix (unused) ring-buffer API to be more useful

(For future commits.)

	Ingo