Re: PATCH [0/4] perf: clean-up of power events API

Steven Rostedt
2010-10-10T15:11:38Z

On Sun, 2010-10-10 at 08:41 +0200, Peter Zijlstra wrote:
> On Sat, 2010-10-09 at 21:39 -0400, Steven Rostedt wrote:
> > I've been hesitant in the pass from doing the TRACE_EVENT_ABI()
> > before, because Peter Zijlstra (who is currently MIA) has been strongly
> > against it.
>
> I see no point in the TRACE_EVENT_ABI() because if I need to change such
> a tracepoint to reflect changes in the kernel then I will freely do so.
>
> Even seemingly stable points like sched_switch(), which we all agree
> will stay around forever (gotta have context switches on a multi-tasking
> OS) will not stay stable when we add/change scheduling policies.
>
> Sure, the prev and next task thing will stay the same, but the meaning
> and interpretation of things like the prio field will not, esp when we
> go add something like a deadline scheduler that isn't priority based.
>
> So one possibility is to simply remove all that information from the
> tracepoints, remove the prio and state fields, but how useful is that?
>
> I guess what I'm saying is that even if we were to provide _ABI I see us
> getting into this very same argument over and over again, making me want
> to remove all this trace event muck right now before it gets worse.

Then how's this as a compromise. We do not add a TRACE_EVENT_ABI(), but
instead manually add the ABI interface to existing tracepoints. Let's
use the sched example you shown above.

We can connect to the sched_switch() tracepoint manually in something
perhaps called kernel/abi_trace.c or trace_abi.c (whatever).

Here we create the directories manually there:

/sys/kernel/event/sched/sched_switch/

But this sched_switch will only include the prev and next pids, comms,
and perhaps even run state. But not the prio (since we see that
changing).

It would then need the code to enable the trace point with:

register_trace_sched_switch(sched_switch_abi_probe, NULL);

Where we have

static void
sched_switch_abi_probe(void *ignore,
struct task_switch *prev,
struct task_struct *next)
{
/* code to grab just the ABI stuff */
}

And this code can then record to what ever hooked to it.

Making this a manual effort will make it easier to control what becomes
an ABI. We can have long discussions and flames over what goes here. But
that's good since debates before an ABI is created is much better than
debates after one is created.

I'm afraid that a easy macro called TRACE_EVENT_ABI() would have the
same issue. ABIs may be created too quickly before they are thought
through.

Thoughts?

-- Steve