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

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

Message-ID: <49D22FB0.2040605@us.ibm.com>
Date:	Tue, 31 Mar 2009 07:58:56 -0700
From:	Darren Hart <dvhltc@...ibm.com>
To:	Thomas Gleixner <tglx@...utronix.de>
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>
Subject: Re: [tip PATCH v6 1/8] RFC: futex: futex_wait_queue_me()

Thomas Gleixner wrote:
> 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.

How about something like this:

	/* add_wait_queue is the barrier after __set_current_state. */
	__set_current_state(TASK_INTERRUPTIBLE);

	/*
	 * Add current as the futex_q waiter.  We don't remove ourselves from
	 * the wait_queue because we are the only user of it.
	 */
	add_wait_queue(&q->waiter, &wait);


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

Done.

> 
>> +			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 ?

Hrm, right, so long as I'm changing futex_wait I might as well correct this.  Done (and voids the missing new line comment above).

Thanks,

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
--
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/