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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
