Re: [PATCH 2/2] tracing/core: use appropriate waiting on trace_pipe

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:

>  static int tracing_wait_pipe(struct file *filp)
>  {
> +	DEFINE_WAIT(wait);
>  	struct trace_iterator *iter = filp->private_data;
>  
>  	while (trace_empty(iter)) {
> -
>  		if ((filp->f_flags & O_NONBLOCK)) {
>  			return -EAGAIN;
>  		}
>  
> -		/*
> -		 * This is a make-shift waitqueue. The reason we don't use
> -		 * an actual wait queue is because:
> -		 *  1) we only ever have one waiter
> -		 *  2) the tracing, traces all functions, we don't want
> -		 *     the overhead of calling wake_up and friends
> -		 *     (and tracing them too)
> -		 *     Anyway, this is really very primitive wakeup.
> -		 */
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		iter->tr->waiter = current;
> -
>  		mutex_unlock(&trace_types_lock);
>  
> -		/* sleep for 100 msecs, and try again. */
> -		schedule_timeout(HZ/10);
> +		if (might_hold_runqueue_lock(iter->trace)) {
> +			/*
> +			 * This is a make-shift waitqueue. The reason we don't
> +			 * use an actual wait queue is because:
> +			 *  1) we only ever have one waiter
> +			 *  2) the tracing, traces all functions, we don't want
> +			 *     the overhead of calling wake_up and friends
> +			 *     (and tracing them too)
> +			 *     Anyway, this is really very primitive wakeup.
> +			 */
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule_timeout(HZ / 10);

Instead of adding this ugly dynamic switch in the middle of tracing_wait_pipe(), i'd 
suggest to restructure this along the following lines:

1) move the new waiting waitqueue based function into default_wait_pipe() function

2) add a poll_wait_pipe() function as well that does the old 100 msecs polling 
   method

3) add a iter->wait_pipe() method that is called by tracing_wait_pipe()

4) make register_tracer() fill in default_wait_pipe() for plugins that do not 
   register an explicit ->wait_pipe method.

That way the 'special', intrusive tracers (like sched and function tracer) can still 
specify poll_wait_pipe() - while the others will default to the waitqueue based 
tracing_wait_pipe() method.

	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/