Re: [PATCH 2/2] tracing/workqueue: Rename workqueue_execute toworklet_entry and add worklet_exit

[Frederic Weisbecker]

On Tue, Sep 22, 2009 at 12:42:32PM +1000, Anton Blanchard wrote:
> 
> Keep a common naming convention for tracing the latency of events such as
> softirq_entry/softirq_exit.
> 
> Based on a patch from KOSAKI Motohiro.
> 
> Signed-off-by: Anton Blanchard <anton@xxxxxxxxx>


Ah. Kosaki Motohiro sent a very similar patch few monthes ago:

http://git.kernel.org/?p=linux/kernel/git/frederic/random-tracing.git;a=commit;h=5c11f399da166ff3bd8cf823add5fff7d036b67e

I haven't proposed it because of the debate about workqueue profiling at this
time.

But now I think this should make its way, as only the trace events are necessary
for the kernel part of such profiling.

Some comments below:

> ---
> 
> Index: linux.trees.git/kernel/workqueue.c
> ===================================================================
> --- linux.trees.git.orig/kernel/workqueue.c	2009-09-14 09:43:00.000000000 +1000
> +++ linux.trees.git/kernel/workqueue.c	2009-09-14 09:45:45.000000000 +1000
> @@ -279,7 +279,6 @@ static void run_workqueue(struct cpu_wor
>  		 */
>  		struct lockdep_map lockdep_map = work->lockdep_map;
>  #endif
> -		trace_workqueue_execution(cwq->thread, work);
>  		cwq->current_work = work;
>  		list_del_init(cwq->worklist.next);
>  		spin_unlock_irq(&cwq->lock);
> @@ -288,7 +287,9 @@ static void run_workqueue(struct cpu_wor
>  		work_clear_pending(work);
>  		lock_map_acquire(&cwq->wq->lockdep_map);
>  		lock_map_acquire(&lockdep_map);
> +		trace_worklet_entry(cwq->thread, work);
>  		f(work);
> +		trace_worklet_exit(cwq->thread, work);
>  		lock_map_release(&lockdep_map);
>  		lock_map_release(&cwq->wq->lockdep_map);
>  
> Index: linux.trees.git/include/trace/events/workqueue.h
> ===================================================================
> --- linux.trees.git.orig/include/trace/events/workqueue.h	2009-09-14 09:45:41.000000000 +1000
> +++ linux.trees.git/include/trace/events/workqueue.h	2009-09-14 09:45:45.000000000 +1000
> @@ -30,7 +30,7 @@ TRACE_EVENT(workqueue_insertion,
>  		__entry->thread_pid, __entry->func)
>  );
>  
> -TRACE_EVENT(workqueue_execution,
> +TRACE_EVENT(worklet_entry,


In Kosaki's patch, it's named workqueue_handler_entry, but worklet_entry
looks sufficient and more concise.


>  
>  	TP_PROTO(struct task_struct *wq_thread, struct work_struct *work),
>  
> @@ -52,6 +52,27 @@ TRACE_EVENT(workqueue_execution,
>  		__entry->thread_pid, __entry->func)
>  );
>  
> +/* Declare work as void *, because we can't use work->... in after f(work) */
> +TRACE_EVENT(worklet_exit,
> +
> +	TP_PROTO(struct task_struct *wq_thread, void *work),
> +
> +	TP_ARGS(wq_thread, work),
> +
> +	TP_STRUCT__entry(
> +		__array(char,		thread_comm,	TASK_COMM_LEN)
> +		__field(pid_t,		thread_pid)
> +	),
> +
> +	TP_fast_assign(
> +		memcpy(__entry->thread_comm, wq_thread->comm, TASK_COMM_LEN);
> +		__entry->thread_pid	= wq_thread->pid;
> +	),
> +
> +	TP_printk("thread=%s:%d", __entry->thread_comm,
> +		  __entry->thread_pid)



In Kosaki's patch, we had the work struct address displayed too.
Your version is supposed to be sufficient because we know a workqueue
serializes its works. Then we know that an exit event will always follow
and match the previous entry event from the same workqueue thread.
The workqueue pid then provides a sufficient key for that.

That said, we should worry about possible lost events from
perf in some circumstances. And userspace profiling needs something
to ensure the accuracy about this entry/exit pair.

We could have:
	entry work1
	exit work 1 <--- lost event
	entry work2 <--- lost event
	exit work2

And then the pair would be misinterpreted.

(Although we could have even other misinterpretation
with other kind of scenarios, even if we have this work
address. But that's still more safety).

So I'd prefer to keep Kosaki's idea about these keys.
But I prefer your event naming.

May be I can unearth Kosaki's patch, change it with your naming
and add your Signed-off-by?

Kosaki, no problem about it?

Thanks,
Frederic.

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