Re: [PATCH] relay: use plain timer instead of delayed work

[Tom Zanussi <zanussi@comcast.net>]

On Sun, 2007-05-06 at 16:47 +0400, Oleg Nesterov wrote:
> On 05/05, Tom Zanussi wrote:
> >
> > This patch makes relay use timers instead of workqueues for reader
> > waking.
> 
> A couple of very minor nits,
> 
> > @@ -337,11 +334,11 @@ static void __relay_reset(struct rchan_buf *buf, unsigned int init)
> >  	if (init) {
> >  		init_waitqueue_head(&buf->read_wait);
> >  		kref_init(&buf->kref);
> > -		INIT_DELAYED_WORK(&buf->wake_readers, NULL);
> > -	} else {
> > -		cancel_delayed_work(&buf->wake_readers);
> > -		flush_scheduled_work();
> > -	}
> > +		init_timer(&buf->timer);
> > +		buf->timer.data = (unsigned long)buf;
> > +		buf->timer.function = wakeup_readers;
> 
> I'd suggest to use setup_timer(&buf->timer, wakeup_readers, buf);
> 
> > @@ -609,9 +605,16 @@ size_t relay_switch_subbuf(struct rchan_buf *buf, size_t length)
> >  			buf->padding[old_subbuf];
> >  		smp_mb();
> >  		if (waitqueue_active(&buf->read_wait)) {
> > -			PREPARE_DELAYED_WORK(&buf->wake_readers,
> > -					     wakeup_readers);
> > -			schedule_delayed_work(&buf->wake_readers, 1);
> > +			/*
> > +			 * Calling wake_up_interruptible() from here
> > +			 * will deadlock if we happen to be logging
> > +			 * from the scheduler (trying to re-grab
> > +			 * rq->lock), so defer it.
> > +			 */
> > +			if (!timer_pending(&buf->timer)) {
> > +				buf->timer.expires = jiffies + 1;
> > +				add_timer(&buf->timer);
> > +			}
> 
> I think it is better to use __mod_timer(&buf->timer, jiffies + 1). In that
> case this "if (!timer_pending(&buf->timer))" is not strictly needed, yes?
> 
> Imho, add_timer() is almost never should be used. The only valid usage is when
> timer->expires was already set by somebody else.
> 
> Btw, thanks for your explanation about deferred wakeup.
> 

OK, and thanks for your helpful comments.  Corrected patch to follow...

Tom