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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20070506124742.GA102@tv-sign.ru>
Date:	Sun, 6 May 2007 16:47:42 +0400
From:	Oleg Nesterov <oleg@...sign.ru>
To:	Tom Zanussi <zanussi@...cast.net>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] relay: use plain timer instead of delayed work

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