linux-kernel - Re: [PATCH 2/2] tracing/core: use appropriate waiting on trace

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 10 Feb 2009 15:54:49 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: [PATCH 2/2] tracing/core: use appropriate waiting on trace_pipe

On Tue, Feb 10, 2009 at 01:02:05PM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@...il.com> 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

That's more smart indeed!
I will take advantage of this v2 to add more comments on the struct tracer.

Thanks.

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