[RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

Now that PowerTop v2 is out, which uses the parse-event library, it no
longer is broken by the removal of the lock-depth field from every
event. Currently we add 4 bytes of empty space to every event. If we
have 1 million events, 4 million bytes are wasted in the ring buffers
(for both ftrace and perf).

But this change will break PowerTop v1. Thus my question is, how long do
we need to keep this wasted space in the ring buffers to satisfy an out
of date tool?

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5f3f3be..f96dfef 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -49,7 +49,6 @@ struct trace_entry {
 	unsigned char		flags;
 	unsigned char		preempt_count;
 	int			pid;
-	int			padding;
 };
 
 #define FTRACE_MAX_EVENT						\

[RFC][PATCH v2] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

On Wed, 2012-05-16 at 09:44 -0400, Steven Rostedt wrote:
> Now that PowerTop v2 is out, which uses the parse-event library, it no
> longer is broken by the removal of the lock-depth field from every
> event. Currently we add 4 bytes of empty space to every event. If we
> have 1 million events, 4 million bytes are wasted in the ring buffers
> (for both ftrace and perf).
> 
> But this change will break PowerTop v1. Thus my question is, how long do
> we need to keep this wasted space in the ring buffers to satisfy an out
> of date tool?
> 

Would help if I compiled it :-)

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 5f3f3be..f96dfef 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -49,7 +49,6 @@ struct trace_entry {
 	unsigned char		flags;
 	unsigned char		preempt_count;
 	int			pid;
-	int			padding;
 };
 
 #define FTRACE_MAX_EVENT						\

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 08a08ba..0960aa7 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1145,7 +1145,6 @@ tracing_generic_entry_update(struct trace_entry *entry, unsigned long flags,
 
 	entry->preempt_count		= pc & 0xff;
 	entry->pid			= (tsk) ? tsk->pid : 0;
-	entry->padding			= 0;
 	entry->flags =
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
 		(irqs_disabled_flags(flags) ? TRACE_FLAG_IRQS_OFF : 0) |
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 079a93a..5845731 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -116,7 +116,6 @@ static int trace_define_common_fields(void)
 	__common_field(unsigned char, flags);
 	__common_field(unsigned char, preempt_count);
 	__common_field(int, pid);
-	__common_field(int, padding);
 
 	return ret;
 }

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Linus Torvalds]

On Wed, May 16, 2012 at 6:44 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> But this change will break PowerTop v1. Thus my question is, how long do
> we need to keep this wasted space in the ring buffers to satisfy an out
> of date tool?

The wasted space seems of limited importance.

More important is to check which distros have the new powertop.

F16 and F17 seem to have powertop-1.98, which I assume is the new
world order already. But maybe I assume incorrectly.

F14 (which I personally still use, since it doesn't have gnome3) is 1.13.

SuSE, Ubuntu?

                    Linus

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Arjan van de Ven]

On 5/16/2012 12:33 PM, Linus Torvalds wrote:
> On Wed, May 16, 2012 at 6:44 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> But this change will break PowerTop v1. Thus my question is, how long do
>> we need to keep this wasted space in the ring buffers to satisfy an out
>> of date tool?
> 
> The wasted space seems of limited importance.
> 
> More important is to check which distros have the new powertop.
> 
> F16 and F17 seem to have powertop-1.98, which I assume is the new
> world order already. But maybe I assume incorrectly.

this is the one that will break

> 
> F14 (which I personally still use, since it doesn't have gnome3) is 1.13.

this one is fine, it does not use perf events at all.

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

[ Fixes David Sharps email. I have two versions, and picked the broken 
  one :-p ]


On Wed, 2012-05-16 at 12:36 -0700, Arjan van de Ven wrote:
> On 5/16/2012 12:33 PM, Linus Torvalds wrote:
> > On Wed, May 16, 2012 at 6:44 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >>
> >> But this change will break PowerTop v1. Thus my question is, how long do
> >> we need to keep this wasted space in the ring buffers to satisfy an out
> >> of date tool?
> > 
> > The wasted space seems of limited importance.

Depends on who you ask. For you, it's probably of no importance. For
someone trying to get as many events as possible, it's of much greater
importance. Lack of events in a report is one of the biggest issues we
have. Especially for those that are trying to debug an issue and needs
as much info as possible. (function tracing sucks up a hell of a lot)

But that said, it doesn't go against what you stated. "limited
importance". Its importance is limited to a group of people.

> > 
> > More important is to check which distros have the new powertop.
> > 
> > F16 and F17 seem to have powertop-1.98, which I assume is the new
> > world order already. But maybe I assume incorrectly.
> 
> this is the one that will break

Shh ;-)

> 
> > 
> > F14 (which I personally still use, since it doesn't have gnome3) is 1.13.
> 
> this one is fine, it does not use perf events at all.

Oo oo! We can change it and Linus won't know.

-- Steve

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Linus Torvalds]

On Wed, May 16, 2012 at 12:36 PM, Arjan van de Ven
<arjan@linux.intel.com> wrote:
>
> this is the one that will break

Oh, ok. So we need to wait at *least* for F16/F17 to upgrade. I assume
that SuSE/Ubuntu are in the same boat.

>> F14 (which I personally still use, since it doesn't have gnome3) is 1.13.
>
> this one is fine, it does not use perf events at all.

Ok. Sadly, my actual laptop (which is where I've used it) runs F17
these days, since F14 can't handle the SNB graphics. So I'd actually
notice.

Steven is foiled again.

                  Linus

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

On Wed, 2012-05-16 at 12:33 -0700, Linus Torvalds wrote:

> F16 and F17 seem to have powertop-1.98, which I assume is the new
> world order already. But maybe I assume incorrectly.

Seriously though. What's your take on changing the kernel that will
break an older distro. Obviously, this change is too early to apply. But
because an old distro has one app that will break if we make a change in
the kernel, is that enough to keep that change out?

Lets say we are at F23, and F18+ have the new powertop tools. Lets even
go to say that F16 and F17 have updated their powertop to powertop v2.

Because someone may be running a F17 without updates, which has powertop
that will break if they update their kernel, rational to keeping that
change out?

I want to know if this change will ever go in. Otherwise, I have to make
workarounds for it. I have no problem with that, as I will be adding
workarounds anyway, but this will continue to punish the tools that use
the non-workaround methods even though they don't break with the change.

Now if you say that it's OK to break and old distro if the affected
tools in the new distros work. What's the timeframe of that? One year?

-- Steve

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

On Wed, 2012-05-16 at 12:51 -0700, Linus Torvalds wrote:
> On Wed, May 16, 2012 at 12:36 PM, Arjan van de Ven
> <arjan@linux.intel.com> wrote:
> >
> > this is the one that will break
> 
> Oh, ok. So we need to wait at *least* for F16/F17 to upgrade. I assume
> that SuSE/Ubuntu are in the same boat.

Ah, so you just answered the email I sent while you sent this one. You
must be psychic. 

> 
> >> F14 (which I personally still use, since it doesn't have gnome3) is 1.13.
> >
> > this one is fine, it does not use perf events at all.
> 
> Ok. Sadly, my actual laptop (which is where I've used it) runs F17
> these days, since F14 can't handle the SNB graphics. So I'd actually
> notice.
> 
> Steven is foiled again.

/me curses! That damn rascal Linus!

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Linus Torvalds]

On Wed, May 16, 2012 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Seriously though. What's your take on changing the kernel that will
> break an older distro. Obviously, this change is too early to apply. But
> because an old distro has one app that will break if we make a change in
> the kernel, is that enough to keep that change out?

I suspect that powertop is enough of a developer thing that if that's
the only thing that breaks, we don't have to worry too much.

I don't want to break *everybody*, so new distro's should be
up-to-date. But breaking something like a F14-15 timeframe distro or
something staid like a SLES (or "Debian Stale" or whatever they call
that thing that only takes crazy-old binaries)? It's fine. We don't
want to *rush* into it, but no, if those distros are basically not
updating, we can't care about them forever for something like
powertop.

Things that break *normal* applications are different. There the rule
really must be "never".

                   Linus

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Arjan van de Ven]

On 5/16/2012 1:04 PM, Linus Torvalds wrote:
> On Wed, May 16, 2012 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> Seriously though. What's your take on changing the kernel that will
>> break an older distro. Obviously, this change is too early to apply. But
>> because an old distro has one app that will break if we make a change in
>> the kernel, is that enough to keep that change out?
> 
> I suspect that powertop is enough of a developer thing that if that's
> the only thing that breaks, we don't have to worry too much.
> 
> I don't want to break *everybody*, so new distro's should be
> up-to-date. But breaking something like a F14-15 timeframe distro or
> something staid like a SLES (or "Debian Stale" or whatever they call
> that thing that only takes crazy-old binaries)? It's fine. We don't
> want to *rush* into it, but no, if those distros are basically not
> updating, we can't care about them forever for something like
> powertop.

agreed.

I would say something like "6 months" (e.g. 3.6); anybody who's likely
to update anything will have done the powertop upgrade from his distro
by then, and anybody who isn't isn't going to update the kernel either.

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

On Wed, 2012-05-16 at 13:07 -0700, Arjan van de Ven wrote:
> On 5/16/2012 1:04 PM, Linus Torvalds wrote:
> > On Wed, May 16, 2012 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >>
> >> Seriously though. What's your take on changing the kernel that will
> >> break an older distro. Obviously, this change is too early to apply. But
> >> because an old distro has one app that will break if we make a change in
> >> the kernel, is that enough to keep that change out?
> > 
> > I suspect that powertop is enough of a developer thing that if that's
> > the only thing that breaks, we don't have to worry too much.
> > 
> > I don't want to break *everybody*, so new distro's should be
> > up-to-date. But breaking something like a F14-15 timeframe distro or
> > something staid like a SLES (or "Debian Stale" or whatever they call
> > that thing that only takes crazy-old binaries)? It's fine. We don't
> > want to *rush* into it, but no, if those distros are basically not
> > updating, we can't care about them forever for something like
> > powertop.
> 
> agreed.
> 
> I would say something like "6 months" (e.g. 3.6); anybody who's likely
> to update anything will have done the powertop upgrade from his distro
> by then, and anybody who isn't isn't going to update the kernel either.
> 

Thanks for the clarification. I'll put the patch in my 3.6 or 3.7 queue.

-- Steve

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Dave Jones]

On Wed, May 16, 2012 at 01:07:14PM -0700, Arjan van de Ven wrote:
 > On 5/16/2012 1:04 PM, Linus Torvalds wrote:
 > > On Wed, May 16, 2012 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
 > >>
 > >> Seriously though. What's your take on changing the kernel that will
 > >> break an older distro. Obviously, this change is too early to apply. But
 > >> because an old distro has one app that will break if we make a change in
 > >> the kernel, is that enough to keep that change out?
 > > 
 > > I suspect that powertop is enough of a developer thing that if that's
 > > the only thing that breaks, we don't have to worry too much.
 > > 
 > > I don't want to break *everybody*, so new distro's should be
 > > up-to-date. But breaking something like a F14-15 timeframe distro or
 > > something staid like a SLES (or "Debian Stale" or whatever they call
 > > that thing that only takes crazy-old binaries)? It's fine. We don't
 > > want to *rush* into it, but no, if those distros are basically not
 > > updating, we can't care about them forever for something like
 > > powertop.
 > 
 > agreed.
 > 
 > I would say something like "6 months" (e.g. 3.6); anybody who's likely
 > to update anything will have done the powertop upgrade from his distro
 > by then, and anybody who isn't isn't going to update the kernel either.

One possibility would be a config symbol to enable the deprecated field
(kind of like what we did with compat vdso).  Distros that have a new
enough powertop can then disable it.

	Dave

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Arjan van de Ven]

On 5/16/2012 1:14 PM, Dave Jones wrote:
> On Wed, May 16, 2012 at 01:07:14PM -0700, Arjan van de Ven wrote:
>  > On 5/16/2012 1:04 PM, Linus Torvalds wrote:
>  > > On Wed, May 16, 2012 at 1:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
>  > >>
>  > >> Seriously though. What's your take on changing the kernel that will
>  > >> break an older distro. Obviously, this change is too early to apply. But
>  > >> because an old distro has one app that will break if we make a change in
>  > >> the kernel, is that enough to keep that change out?
>  > > 
>  > > I suspect that powertop is enough of a developer thing that if that's
>  > > the only thing that breaks, we don't have to worry too much.
>  > > 
>  > > I don't want to break *everybody*, so new distro's should be
>  > > up-to-date. But breaking something like a F14-15 timeframe distro or
>  > > something staid like a SLES (or "Debian Stale" or whatever they call
>  > > that thing that only takes crazy-old binaries)? It's fine. We don't
>  > > want to *rush* into it, but no, if those distros are basically not
>  > > updating, we can't care about them forever for something like
>  > > powertop.
>  > 
>  > agreed.
>  > 
>  > I would say something like "6 months" (e.g. 3.6); anybody who's likely
>  > to update anything will have done the powertop upgrade from his distro
>  > by then, and anybody who isn't isn't going to update the kernel either.
> 
> One possibility would be a config symbol to enable the deprecated field
> (kind of like what we did with compat vdso).  Distros that have a new
> enough powertop can then disable it.

this isn't really about distro kernels; a distro that pushes a 3.7
kernel to Fedora 15 might as well do a powertop upgrade.. the later is
much simpler and lighter (the main difference between 1.98 and 2.0 is
the use of the perfevent library anyway)

it's about people who compile their own upstream kernel...

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Ingo Molnar]

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

> > Ok. Sadly, my actual laptop (which is where I've used it) 
> > runs F17 these days, since F14 can't handle the SNB 
> > graphics. So I'd actually notice.
> > 
> > Steven is foiled again.
> 
> /me curses! That damn rascal Linus!

Binary compatibility's a bitch!

But yeah, since this is instrumentation, the rules are somewhat 
relaxed to current widespread installations of tools, because 
few people are running old tools on old kernels to produce new 
code against new kernels, right?

Thanks,

	Ingo

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> > One possibility would be a config symbol to enable the 
> > deprecated field (kind of like what we did with compat 
> > vdso).  Distros that have a new enough powertop can then 
> > disable it.
> 
> this isn't really about distro kernels; a distro that pushes a 
> 3.7 kernel to Fedora 15 might as well do a powertop upgrade.. 
> the later is much simpler and lighter (the main difference 
> between 1.98 and 2.0 is the use of the perfevent library 
> anyway)
> 
> it's about people who compile their own upstream kernel...

Nor should we waste too much time over these 4 bytes really. Is 
the kernel really in such a good shape that we must spend our 
time trying to break working apps, over a mostly cosmetic detail 
in an ABI which will soon be messed up with our next set up 
mistakes anyway? ;-)

Thanks,

	Ingo

Re: [RFC][PATCH] tracing: Remove useless 4 bytes of padding from every event

[Steven Rostedt]

On Thu, 2012-05-17 at 10:06 +0200, Ingo Molnar wrote:

> Nor should we waste too much time over these 4 bytes really. Is 
> the kernel really in such a good shape that we must spend our 
> time trying to break working apps, over a mostly cosmetic detail 
> in an ABI which will soon be messed up with our next set up 
> mistakes anyway? ;-)
> 

4 bytes is not cosmetic for a 32 byte event. That's 1/8th overhead. If
we could get rid of 4 bytes from struct page, would we do that? It's
only just 4 bytes for ever 4096 bytes. Just a 1/1024 overhead. Of course
perf events are much bigger than 32 bytes. It's one of the biggest
complaints I hear about perf, the size of its events. We should be
trying hard to fix that.

And we are not spending time trying to break tools. The tools were
already broken. The main motivator for getting parse-events into
powertop was that the older version (due to this binary interface) could
not be used on a system running a 32bit userspace on a 64bit kernel.

With the proper parsing, it not only was able to run on a 32/64
environment, it can also parse the events like perf does and not need
direct padding of space.

-- Steve