Re: [PATCH v2 2/4] ftrace: introduce workqueue_handler_exittracepoint and rename workqueue_execution to workqueue_handler_entry

[Ingo Molnar]

* Oleg Nesterov <oleg@xxxxxxxxxx> wrote:

> On 04/15, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > > > >  		lock_map_acquire(&lockdep_map);
> > > > > +		trace_workqueue_handler_entry(cwq->thread, work);
> > > > >  		f(work);
> > > > > +		trace_workqueue_handler_exit(cwq->thread, work);
> > >
> > > This doesn't look right. We must not use "work" after f(work).
> > > work->func() can kfree its work.
> >
> > We can use it as long as we use it as a 'cookie' - i.e. an
> > identifier for visualization/statistics, but dont actually
> > dereference it, right?
> 
> Yes sure.
> 
> I do not know whether this matters or not, but just in case... Of 
> course it is possible that, when trace_workqueue_handler_exit() 
> runs, this memory was already reused for another work even without 
> kfree. For example,
> 
> 	void my_work_func(struct work_struct *work)
> 	{
> 		INIT_WORK(work, another_work_func);
> 		queue_work(another_workqueue, work);
> 	}
> 
> In this case another_workqueue can report 
> trace_workqueue_handler_entry() before my_work_func() returns.
> 
> This means that trace_workqueue_handler_exit() can't use the 
> address of work as a "key" to find some data recorded by _entry(). 
> Unless this data lives in cpu_workqueue_struct.
> 
> Not sure why I am trying to explain the things which are very 
> obvious to all ;) Just because I don't really understand what 
> these patches do.

The patches try to map and instrument the life cycle of a worklet, 
and the main actions that occur in the workqueue subsystem in 
general.

The purpose is instrumentation: for debugging purposes, for 
improving kernel code and for just understanding how the system 
functions and what its dynamic actions are.

In that sense the worklet 'key' possibly getting reallocated and 
reused before the 'completed' action was traced is probably not a 
big deal - but tools have to be aware of it possibly happening (and 
most not hard-code any assumption to the contrary).

Plus the exit handler must not dereference the worklet either. 
Safest would be to make this sure in the prototype already: pass in 
a void * key, not a work structure.

Thanks,

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