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

[Oleg Nesterov]

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.

Oleg.

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