Hi, Steven,
(2012/11/30 23:17), Steven Rostedt wrote:
[snip]
>
> 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...
>
I think we don't need the clear on swap too.
I'll update my patches like this table.
> 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 looked over the wakeup tracer, but I couldn't find that code...)
>
>> 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.
>
Yes.
>
>> > 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.
>
I see. Perhaps I'll make it separate.
>> [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.
>
O.K. I'll change all the old_tracer checking to the strcmp.
> 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;
I think that seq_read() calls s_stop() even if s_start() failed.
seq_read()@fs/seq_file.c:
p = m->op->start(m, &pos);
while (1) {
err = PTR_ERR(p);
if (!p || IS_ERR(p))
break;
...
}
m->op->stop(m, p);
So, I think we need the check in s_stop(), don't we?
Thanks,
Hiraku Toyooka
--
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/
