Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1246098 > unrolled thread

Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt Peter Zijlstra <peterz@infradead.org> - 2015-10-13 22:00 +0200
    Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt Waiman Long <waiman.long@hpe.com> - 2015-10-13 23:00 +0200
      Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt Peter Zijlstra <peterz@infradead.org> - 2015-10-14 11:30 +0200
        Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt Waiman Long <waiman.long@hpe.com> - 2015-10-15 23:10 +0200

#1246098 — Re: [PATCH v7 4/5] locking/pvqspinlock: Allow 1 lock stealing attempt

On Tue, Sep 22, 2015 at 04:50:43PM -0400, Waiman Long wrote:

> +gotlock:
>  	/*
> +	 * We now have the lock. We need to either clear the tail code or
> +	 * notify the next one in queue as the new queue head.
>  	 */
> +	old = atomic_read(&lock->val);
> +	while ((old & _Q_TAIL_MASK) == tail) {
> +		int val;
> +		int new = old & ~_Q_TAIL_MASK;
> +
> +		/*
> +		 * We are the only one in the queue, so clear the tail code
> +		 * and return.
> +		 */
> +		val = atomic_cmpxchg(&lock->val, old, new);
> +		if (old == val)
> +			goto done;
> +		old = val;
> +	}
> +

This i need to think about a wee bit; its almost the same...


So the below is exactly duplicated from the normal slowpath, so why
don't you keep that there?

It would get you something like:

	if (pv_wait_head_or_steal(..))
		goto stolen;


stolen:
> +	/*
> +	 * contended path; wait for next, release.
> +	 */
> +	while (!(next = READ_ONCE(node->next)))
> +		cpu_relax();
> +
> +	arch_mcs_spin_unlock_contended(&next->locked);
> +	pv_kick_node(lock, next);

release:
	...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1246141

On 10/13/2015 03:56 PM, Peter Zijlstra wrote:
> On Tue, Sep 22, 2015 at 04:50:43PM -0400, Waiman Long wrote:
>
>> +gotlock:
>>   	/*
>> +	 * We now have the lock. We need to either clear the tail code or
>> +	 * notify the next one in queue as the new queue head.
>>   	 */
>> +	old = atomic_read(&lock->val);
>> +	while ((old&  _Q_TAIL_MASK) == tail) {
>> +		int val;
>> +		int new = old&  ~_Q_TAIL_MASK;
>> +
>> +		/*
>> +		 * We are the only one in the queue, so clear the tail code
>> +		 * and return.
>> +		 */
>> +		val = atomic_cmpxchg(&lock->val, old, new);
>> +		if (old == val)
>> +			goto done;
>> +		old = val;
>> +	}
>> +
> This i need to think about a wee bit; its almost the same...
>
>
> So the below is exactly duplicated from the normal slowpath, so why
> don't you keep that there?
>
> It would get you something like:
>
> 	if (pv_wait_head_or_steal(..))
> 		goto stolen;
>
>
> stolen:
>> +	/*
>> +	 * contended path; wait for next, release.
>> +	 */
>> +	while (!(next = READ_ONCE(node->next)))
>> +		cpu_relax();
>> +
>> +	arch_mcs_spin_unlock_contended(&next->locked);
>> +	pv_kick_node(lock, next);
> release:
> 	...

Yes, it is largely the same. I thought that you don't like too much 
change in the logic flow of the generic qspinlock code. I will make the 
change in the next revision.

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1246456

On Tue, Oct 13, 2015 at 04:50:25PM -0400, Waiman Long wrote:
> On 10/13/2015 03:56 PM, Peter Zijlstra wrote:

> >So the below is exactly duplicated from the normal slowpath, so why
> >don't you keep that there?
> >
> >It would get you something like:
> >
> >	if (pv_wait_head_or_steal(..))
> >		goto stolen;
> >
> >
> >stolen:
> >>+	/*
> >>+	 * contended path; wait for next, release.
> >>+	 */
> >>+	while (!(next = READ_ONCE(node->next)))
> >>+		cpu_relax();
> >>+
> >>+	arch_mcs_spin_unlock_contended(&next->locked);
> >>+	pv_kick_node(lock, next);
> >release:
> >	...
> 
> Yes, it is largely the same. I thought that you don't like too much change
> in the logic flow of the generic qspinlock code. I will make the change in
> the next revision.

Well, you already put the branch in there, the only difference here is
an 'extra' label. OTOH that extra label avoids duplicating some hairy
code. So over all I would say its a definite win.

And its easy to see it will compile away on the native case where:

 #define pv_wait_head_or_steal(l, n, t) (false)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248182

On 10/14/2015 05:28 AM, Peter Zijlstra wrote:
> On Tue, Oct 13, 2015 at 04:50:25PM -0400, Waiman Long wrote:
>> On 10/13/2015 03:56 PM, Peter Zijlstra wrote:
>>> So the below is exactly duplicated from the normal slowpath, so why
>>> don't you keep that there?
>>>
>>> It would get you something like:
>>>
>>> 	if (pv_wait_head_or_steal(..))
>>> 		goto stolen;
>>>
>>>
>>> stolen:
>>>> +	/*
>>>> +	 * contended path; wait for next, release.
>>>> +	 */
>>>> +	while (!(next = READ_ONCE(node->next)))
>>>> +		cpu_relax();
>>>> +
>>>> +	arch_mcs_spin_unlock_contended(&next->locked);
>>>> +	pv_kick_node(lock, next);
>>> release:
>>> 	...
>> Yes, it is largely the same. I thought that you don't like too much change
>> in the logic flow of the generic qspinlock code. I will make the change in
>> the next revision.
> Well, you already put the branch in there, the only difference here is
> an 'extra' label. OTOH that extra label avoids duplicating some hairy
> code. So over all I would say its a definite win.
>
> And its easy to see it will compile away on the native case where:
>
>   #define pv_wait_head_or_steal(l, n, t) (false)
>
>

I have done it in a different way to avoid duplicating code. I think I 
am not going to use the pv_wait_head_or_steal name after all as I don't 
consider acquiring the lock by the queue head CPU is lock stealing. I 
will skip "and" and name it as pv_wait_head_lock instead. Please let me 
know if you have a better idea.

Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web