linux-kernel - Re: [PATCH 2/2] tracing/workqueue: Rename workqueue_execute to worklet_entry and add worklet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 22 Sep 2009 22:34:16 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Anton Blanchard <anton@...ba.org>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	Li Zefan <lizf@...fujitsu.com>, Zhaolei <zhaolei@...fujitsu.com>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Ingo Molnar <mingo@...e.hu>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] tracing/workqueue: Rename workqueue_execute to
	worklet_entry and add worklet_exit

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@...ba.org>


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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/