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