linux-kernel - Re: [tip PATCH v6 1/8] RFC: futex: futex_wait_queue

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.0903310836500.12916@localhost.localdomain>
Date:	Tue, 31 Mar 2009 08:44:45 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Darren Hart <dvhltc@...ibm.com>
cc:	linux-kernel@...r.kernel.org, Sripathi Kodi <sripathik@...ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	John Stultz <johnstul@...ibm.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Dinakar Guniguntala <dino@...ibm.com>,
	Ulrich Drepper <drepper@...hat.com>,
	Eric Dumazet <dada1@...mosbay.com>,
	Ingo Molnar <mingo@...e.hu>, Jakub Jelinek <jakub@...hat.com>,
	Sripathi Kodi <sripathik@...ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	John Stultz <johnstul@...ibm.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Dinakar Guniguntala <dino@...ibm.com>,
	Ulrich Drepper <drepper@...hat.com>,
	Eric Dumazet <dada1@...mosbay.com>,
	Ingo Molnar <mingo@...e.hu>, Jakub Jelinek <jakub@...hat.com>
Subject: Re: [tip PATCH v6 1/8] RFC: futex: futex_wait_queue_me()

On Mon, 30 Mar 2009, Darren Hart wrote:
> +
> +	/* add_wait_queue is the barrier after __set_current_state. */
> +	__set_current_state(TASK_INTERRUPTIBLE);
> +	add_wait_queue(&q->waiter, &wait);
> +	/*
> +	 * NOTE: we don't remove ourselves from the waitqueue because
> +	 * we are the only user of it.
> +	 */

  This comment, while correct is at an odd place.

> +	/* Arm the timer */
> +	if (timeout) {
> +		hrtimer_start_expires(&timeout->timer, HRTIMER_MODE_ABS);
> +		if (!hrtimer_active(&timeout->timer))
> +			timeout->task = NULL;
> +	}
> +
> +	/*
> +	 * !plist_node_empty() is safe here without any lock.
> +	 * q.lock_ptr != 0 is not safe, because of ordering against wakeup.
> +	 */
> +	if (likely(!plist_node_empty(&q->list))) {
> +		/*
> +		 * If the timer has already expired, current will already be
> +		 * flagged for rescheduling. Only call schedule if there
> +		 * is no timeout, or if it has yet to expire.
> +		 */
> +		if (!timeout || likely(timeout->task))

  Remove the likely(). It does not make sense

> +			schedule();
> +	}
> +	__set_current_state(TASK_RUNNING);
> +}
> +
>  static int futex_wait(u32 __user *uaddr, int fshared,
>  		      u32 val, ktime_t *abs_time, u32 bitset, int clockrt)
>  {
> -	struct task_struct *curr = current;
> +	struct hrtimer_sleeper timeout, *to = NULL;
>  	struct restart_block *restart;
> -	DECLARE_WAITQUEUE(wait, curr);
>  	struct futex_hash_bucket *hb;
>  	struct futex_q q;
>  	u32 uval;
>  	int ret;
> -	struct hrtimer_sleeper t;
> -	int rem = 0;
>  
>  	if (!bitset)
>  		return -EINVAL;
>  
>  	q.pi_state = NULL;
>  	q.bitset = bitset;
> +
> +	if (abs_time) {
> +		unsigned long slack;

  missing new line

> +		to = &timeout;
> +		slack = current->timer_slack_ns;
> +		if (rt_task(current))
> +			slack = 0;

  Hmm. I thought we would use current->timer_slack_ns ?

Thanks,

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