Messages in this thread | | | Date | Tue, 21 Apr 2009 00:25:14 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v3 1/1] ftrace, workqueuetrace: Make workqueuetracepoints use TRACE_EVENT macro |
| |
On 04/20, Ingo Molnar wrote: > > Andrew, Oleg: if you plan to make unhappy noises about this level of > instrumentation in the workqueue code, please do it sooner rather > than later, as there's quite some effort injected into this already. > A tentative non-NAK now (patches are still being sorted out) and an > Ack on the final topic tree from you (once we send it and if it's > good) and general happiness would be the ideal outcome :)
Imho, this info is useful, and the changes are fine.
But I didn't study kernel/trace/trace_workqueue.c yet... Sorry, I was going to do this, but I didn't.
At first glance, with or without these new changes some parts of trace_workqueue.c looks suspicious.
For example, I don't understand cpu_workqueue_stats->pid. Why it is needed? Why can't we just record wq_thread itself? And if we copy wq_thread->comm to cpu_workqueue_stats, we don't even need get/put task_struct, afaics.
probe_workqueue_destruction() does cpumask_first(cwq->cpus_allowed), this doesn't look right. When workqueue_cpu_callback() calls cleanup_workqueue_thread(), wq_thread is no longer affine to CPU it was created on. This means probe_workqueue_destruction() can't always find the correct cpu_workqueue_stats in workqueue_cpu_stat(cpu), no?
Oleg.
| |