Date:	Fri, 30 Nov 2012 09:17:06 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Hiraku Toyooka <hiraku.toyooka.gu@...achi.com>
Cc:	yrl.pp-manager.tt@...achi.com, linux-kernel@...r.kernel.org,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>, Jiri Olsa <jolsa@...hat.com>,
	Li Zefan <lizf@...fujitsu.com>
Subject: Re: [PATCH v2 -tip 3/4] tracing: make a snapshot feature available
 from userspace

On Fri, 2012-11-30 at 14:46 +0900, Hiraku Toyooka wrote:
> Hi, Steven,
> 
> Thank you for your review.
> 
> (2012/11/16 10:46), Steven Rostedt wrote:
> [snip]
>  > I was thinking about this some more, and I don't like the
>  > snapshot_allocate part. Also, I think the snapshot should not be
>  > allocated by default, and not used until the user explicitly asks for
>  > it.
>  >
>  > Have this:
>  >
>  > ---
>  >  # cat snapshot
>  > Snapshot is not allocated. To allocate it write the ASCII "1" into
>  > this file:
>  >
>  >   echo 1 > snapshot
>  >
>  > This will allocate the buffer for you. To free the snapshot echo "0"
>  > into this file.
>  >
>  >   echo "0" > snapshot
>  >
>  > Anything else will reset the snapshot if it is allocated, or return
>  > EINVAL if it is not allocated.
>  > ---
>  >
> 
> Your idea about "snapshot" is like following table, isn't it?
> 
>   status\input |     0      |     1      |    else    |
> --------------+------------+------------+------------+
> not allocated |   EINVAL   | alloc+swap |   EINVAL   |
> --------------+------------+------------+------------+
>    allocated   |    free    | clear+swap |   clear    |
> --------------+------------+------------+------------+

Actually, I would have:

  status\input |     0      |     1      |    else    |
 --------------+------------+------------+------------+
 not allocated |(do nothing)| alloc+swap |   EINVAL   |
 --------------+------------+------------+------------+
   allocated   |    free    |   swap     |   clear    |
 --------------+------------+------------+------------+

Perhaps we don't need to do the clear on swap, just let the trace
continue where it left off? But in case we should swap...

There's a fast way to clear the tracer. Look at what the wakeup tracer
does. We can make that generic. If you want, I can write that code up
too. Hmm, maybe I'll do that, as it will speed things up for
everyone :-)


> 
> I think it is almost OK, but there is a problem.
> When we echo "1" to the allocated snapshot, the clear operation adds
> some delay because the time cost of tracing_reset_online_cpus() is in
> proportion to the number of CPUs.
> (It takes 72ms in my 8 CPU environment.)
> 
> So, when the snapshot is already cleared by echoing "else" values, we
> can avoid the delay on echoing "1" by keeping "cleared" status
> internally. For example, we can add the "cleared" flag to struct tracer.
> What do you think about it?
> 
>  >
>  > Also we can add a "trace_snapshot" to the kernel parameters to have it
>  > allocated on boot. But I can add that if you update these patches.
>  >
> 
> OK, I'll update my patches.

This part (kernel parameter) can be a separate patch.


>  > Either test here, or better yet, put the test into
>  > tracing_reset_online_cpus().
>  >
>  >     if (!buffer)
>  >         return;
>  >
> 
> I see. I'll add the test to tracing_reset_online_cpus(). Should I make a
> separated patch?

It's a small change, you can add it to your patch or make it separate.
I'll leave that up to you.

> 
> [snip]
>  >> +static ssize_t tracing_snapshot_read(struct file *filp, char __user 
> *ubuf,
>  >> +                     size_t cnt, loff_t *ppos)
>  >> +{
>  >> +    ssize_t ret = 0;
>  >> +
>  >> +    mutex_lock(&trace_types_lock);
>  >> +    if (current_trace && current_trace->use_max_tr)
>  >> +        ret = -EBUSY;
>  >> +    mutex_unlock(&trace_types_lock);
>  >
>  > I don't like this, as it is racy. The current tracer could change after
>  > the unlock, and your back to the problem.
>  >
> 
> You're right...
> This is racy.
> 
>  > Now what we may be able to do, but it would take a little checking for
>  > lock ordering with trace_access_lock() and trace_event_read_lock(), but
>  > we could add the mutex to trace_types_lock to both s_start() and
>  > s_stop() and do the check in s_start() if iter->snapshot is true.
>  >
> 
> If I catch your meaning, do s_start() and s_stop() become like following
> code?
> (Now, s_start() is used from two files - "trace" and "snapshot", so we
> should change static "old_tracer" to per open-file.)

Actually, lets nuke all the old_tracer totally, and instead do:

if (unlikely(strcmp(iter->trace->name, current_trace->name) != 0)) {

You can make this into a separate patch. You can add a check if
current_trace is not NULL too, but I need to fix that, as current_trace
should never be NULL (except for very early boot). But don't worry about
that, I'll handle that.

Or I can write up this patch and send it to you, and you can include it
in your series.

> 
> static void *s_start(struct seq_file *m, loff_t *pos)
> {
>       struct trace_iterator *iter = m->private;
> -    static struct tracer *old_tracer;
> ...
>       /* copy the tracer to avoid using a global lock all around */
>       mutex_lock(&trace_types_lock);
> -    if (unlikely(old_tracer != current_trace && current_trace)) {
> -        old_tracer = current_trace;
> +    if (unlikely(iter->old_tracer != current_trace && current_trace)) {
> +        iter->old_tracer = current_trace;
>           *iter->trace = *current_trace;
>       }
>       mutex_unlock(&trace_types_lock);
> 
> +    if (iter->snapshot && iter->trace->use_max_tr)
> +        return ERR_PTR(-EBUSY);
> +
> ...
> }
> 
> static void s_stop(struct seq_file *m, void *p)
> {
>       struct trace_iterator *iter = m->private;
> 
> +    if (iter->snapshot && iter->trace->use_max_tr)
> +        return;

This part shouldn't be needed, as if s_start fails it wont call
s_stop(). But if you are paranoid (like I am ;-) then we can do:

	if (WARN_ON_ONCE(iter->snapshot && iter->trace->use_max_tr)
		return;



-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists