Re: [PATCH] power, trace: add tracing for device_resume

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, May 18, 2012 at 03:17:32PM -0700, Sameer Nanda wrote:
> On Fri, May 18, 2012 at 2:46 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, May 18, 2012, Sameer Nanda wrote:
> >> On Fri, May 18, 2012 at 2:19 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> > On Fri, 2012-05-18 at 13:58 -0700, Sameer Nanda wrote:
> >> >> On Fri, May 18, 2012 at 12:14 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >> >> > On Fri, 2012-05-18 at 11:57 -0700, Sameer Nanda wrote:
> >> >> >
> >> >> >> AFAICT, they are used for something completely different -- help solve
> >> >> >> suspend/resume issues by saving a hash in the RTC of the last device
> >> >> >> that suspended/resumed.  They don't use the perf tracing mechanism at
> >> >> >> all.
> >> >> >>
> >> >> >
> >> >> > Also note that all tracepoints have timestamps attached to them. You do
> >> >> > not need to add deltas. Do that in the userspace tools that read the
> >> >> > timestamps and events. This way you can have one DECLARE_EVENT_CLASS and
> >> >> > three DEFINE_EVENTs. This will save space.
> >> >>
> >> >> Agreed on the space savings.  However, with the time_delta in the
> >> >> trace message itself, a one line shell script [1] that sorts on the
> >> >> time_delta field is sufficient to quickly spot the devices that take a
> >> >> long time to resume.  Without the time_delta field, the user tool is
> >> >> more complex since it needs to first match up the device_resume_in,
> >> >> device_resume_waited and device_resume_out traces and then calculate
> >> >> time deltas.
> >> >>
> >> >> Seems like a worthwhile trade-off to me but I can take out the
> >> >> time_delta if the general consensus is otherwise.
> >> >
> >> > Just note that every TRACE_EVENT() adds around 5k or more code. Every
> >> > DEFINE_EVENT adds just about 300 bytes.
> >>
> >> Ok, let me respin the patch.  I am thinking of adding time_delta to
> >> all three traces.  That way we should get the space saving while still
> >> allowing quick spotting of devices that take long time to resume.
> >
> > Well, what's wrong with the code in drivers/base/power/main.c that
> > is activated by adding initcall_debug to the kernel command line?
> 
> Mostly that I hadn't looked closely at initcall_debug before writing my patch :)
> 
> Now that I have taken a look at it, the main issue is that the kernel
> command line needs to be modified to activate it.  We cannot do this
> for our automated regression suites since the kernel command line is
> protected on Chrome OS systems.

You are kidding, right?  You have control over your test systems, don't
bloat everyone's kernel by 5k just because your infrastructure is
somehow something that you feel you can't change.

> Does it make sense to merge these tracepoints with initcall_debug
> then?  The initcall_debug prints will still be conditional on the
> kernel command line flag but the tracepoints wouldn't be.
> 
> Another possibility is to convert initcall_debug to use tracepoints
> instead of printks.  But I am not sure if this would break any
> existing tools that depend on initcall_debug on resume path?

We have tools that use the existing printks, don't change them.

greg k-h