Re: [PATCH v3 1/1] ftrace, workqueuetrace: Make workqueuetracepoints use TRACE_EVENT macro

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Apr 21, 2009 at 12:25:14AM +0200, Oleg Nesterov wrote:
> 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.


We record the pid because of a race condition that can happen
against the stat tracing framework.

For example,

- the user opens the stat file
- then the entries are loaded and sorted into memory
- one of the workqueues is destroyed
- the user reads the file. We try to get the task struct
  of each workqueues that were recorded using their pid.
  If the task is destroyed then this is fine, we don't get
  any struct. If we were using the task_struct as an identifier,
  me would have derefenced a freed pointer.

 
> 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?


Ah, at this stage of cpu_down() the cpu is cleared on
tsk->cpu_allowed ?

Thanks for looking at this.

Frederic.


> Oleg.
>