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

Groups > linux.kernel > #1321622 > unrolled thread

Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list is not NULL.

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 RFC] locking/mutexes: don't spin on owner when wait list  is not NULL. Peter Zijlstra <peterz@infradead.org> - 2016-01-29 11:00 +0100
    Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list  is not NULL. Ding Tianhong <dingtianhong@huawei.com> - 2016-01-30 02:20 +0100
      Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list  is not NULL. huang ying <huang.ying.caritas@gmail.com> - 2016-02-01 04:30 +0100
        Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list is not NULL. "Huang\, Ying" <ying.huang@intel.com> - 2016-02-01 04:40 +0100
      [PATCH] locking/mutex: Avoid spinner vs waiter starvation Peter Zijlstra <peterz@infradead.org> - 2016-02-01 11:10 +0100
        Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Davidlohr Bueso <dave@stgolabs.net> - 2016-02-02 22:20 +0100
          Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Ding Tianhong <dingtianhong@huawei.com> - 2016-02-03 08:20 +0100
            Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Davidlohr Bueso <dave@stgolabs.net> - 2016-02-03 20:30 +0100
              Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Ding Tianhong <dingtianhong@huawei.com> - 2016-02-04 02:30 +0100
          Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Waiman Long <waiman.long@hpe.com> - 2016-02-03 23:10 +0100
        Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Jason Low <jason.low2@hp.com> - 2016-02-04 02:40 +0100
          Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation huang ying <huang.ying.caritas@gmail.com> - 2016-02-04 10:00 +0100
            Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation Jason Low <jason.low2@hp.com> - 2016-02-05 00:00 +0100

#1321622 — Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list is not NULL.

On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:

> looks good to me, I will try this solution and report the result, thanks everyone.

Did you get a change to run with this?

[toc] | [next] | [standalone]

#1322224

On 2016/1/29 17:53, Peter Zijlstra wrote:
> On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:
> 
>> looks good to me, I will try this solution and report the result, thanks everyone.
> 
> Did you get a change to run with this?
> 
> .
> 

I backport this patch to 3.10 lts kernel, and didn't change any logic, Till now, the patch works fine to me, and no need to change anything,
So I think this patch is no problem, could you formal release this patch to the latest kernel? :)

Thanks.
Ding 

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

#1322804

On Sat, Jan 30, 2016 at 9:18 AM, Ding Tianhong <dingtianhong@huawei.com> wrote:
> On 2016/1/29 17:53, Peter Zijlstra wrote:
>> On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:
>>
>>> looks good to me, I will try this solution and report the result, thanks everyone.
>>
>> Did you get a change to run with this?
>>
>> .
>>
>
> I backport this patch to 3.10 lts kernel, and didn't change any logic, Till now, the patch works fine to me, and no need to change anything,
> So I think this patch is no problem, could you formal release this patch to the latest kernel? :)
>
> Thanks.
> Ding
>
>

The original patch from Tianhong triggered a performance regression
because the optimistic spinning is turned off in effect.  I tested
Peter's patch with same configuration and there show no regression.
So I think the patch keep the optimistic spinning.  Test result
details will be in the next email.

Best Regards,
Huang, YIng

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

#1322808 — Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list is not NULL.

huang ying <huang.ying.caritas@gmail.com> writes:

> On Sat, Jan 30, 2016 at 9:18 AM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>> On 2016/1/29 17:53, Peter Zijlstra wrote:
>>> On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:
>>>
>>>> looks good to me, I will try this solution and report the result, thanks everyone.
>>>
>>> Did you get a change to run with this?
>>>
>>> .
>>>
>>
>> I backport this patch to 3.10 lts kernel, and didn't change any
>> logic, Till now, the patch works fine to me, and no need to change
>> anything,
>> So I think this patch is no problem, could you formal release this patch to the latest kernel? :)
>>
>> Thanks.
>> Ding
>>
>>
>
> The original patch from Tianhong triggered a performance regression
> because the optimistic spinning is turned off in effect.  I tested
> Peter's patch with same configuration and there show no regression.
> So I think the patch keep the optimistic spinning.  Test result
> details will be in the next email.

Here is the detailed test result:

=========================================================================================
compiler/cpufreq_governor/kconfig/nr_task/rootfs/tbox_group/test/testcase:
  gcc-4.9/performance/x86_64-rhel/100%/debian-x86_64-2015-02-07.cgz/lkp-ivb-d01/fstime/unixbench

commit: 
  v4.4
  1db66c17114d5437c0757d6792c0d8923192ecd6

            v4.4 1db66c17114d5437c0757d6792 
---------------- -------------------------- 
         %stddev     %change         %stddev
             \          |                \  
    371269 ± 10%     -93.2%      25080 ±  4%  unixbench.time.voluntary_context_switches
    371269 ± 10%     -93.2%      25080 ±  4%  time.voluntary_context_switches
      6189 ±  8%     -76.4%       1463 ±  6%  vmstat.system.cs
      5706 ±  0%      -1.7%       5608 ±  0%  vmstat.system.in
    113680 ± 12%     -73.5%      30086 ±  1%  cpuidle.C1-IVB.usage
   1515925 ± 20%     +68.3%    2552001 ± 11%  cpuidle.C1E-IVB.time
   1227221 ± 20%     -51.7%     592695 ± 17%  cpuidle.C3-IVB.time
      2697 ± 10%     -77.8%     598.33 ± 10%  cpuidle.C3-IVB.usage
     15173 ±  6%     -23.1%      11663 ±  1%  cpuidle.C6-IVB.usage
     34.38 ± 27%     -35.0%      22.33 ±  2%  cpuidle.POLL.usage
     61.92 ±  9%     +14.3%      70.78 ±  7%  sched_debug.cfs_rq:/.load_avg.min
     40.85 ± 29%     +27.3%      52.00 ± 10%  sched_debug.cfs_rq:/.runnable_load_avg.2
     -1949 ±-37%     -64.6%    -690.19 ±-134%  sched_debug.cfs_rq:/.spread0.4
     -1773 ±-29%     -85.6%    -256.00 ±-388%  sched_debug.cfs_rq:/.spread0.7
     -2478 ±-26%     -49.5%      -1251 ±-66%  sched_debug.cfs_rq:/.spread0.min
     61.95 ±  9%     +14.8%      71.11 ±  7%  sched_debug.cfs_rq:/.tg_load_avg_contrib.min
    396962 ± 12%     +27.9%     507573 ± 10%  sched_debug.cpu.avg_idle.0
    432973 ± 18%     +45.3%     629147 ± 12%  sched_debug.cpu.avg_idle.6
    448566 ±  3%     +11.5%     499990 ±  0%  sched_debug.cpu.avg_idle.avg
     45.31 ±  5%      -9.5%      41.00 ±  3%  sched_debug.cpu.cpu_load[3].7
     52204 ± 10%     -49.9%      26173 ± 34%  sched_debug.cpu.nr_switches.0
     50383 ± 12%     -57.6%      21353 ± 15%  sched_debug.cpu.nr_switches.1
     45425 ± 16%     -68.5%      14325 ± 28%  sched_debug.cpu.nr_switches.2
     43069 ± 20%     -65.5%      14852 ± 41%  sched_debug.cpu.nr_switches.3
     40285 ± 16%     -70.4%      11905 ± 47%  sched_debug.cpu.nr_switches.4
     40732 ± 13%     -75.8%       9872 ± 39%  sched_debug.cpu.nr_switches.5
     43011 ± 19%     -80.0%       8607 ± 42%  sched_debug.cpu.nr_switches.6
     38076 ± 12%     -75.9%       9167 ± 40%  sched_debug.cpu.nr_switches.7
     44148 ±  7%     -67.1%      14532 ±  6%  sched_debug.cpu.nr_switches.avg
     59877 ±  8%     -51.3%      29146 ± 21%  sched_debug.cpu.nr_switches.max
     33672 ±  9%     -83.0%       5718 ±  4%  sched_debug.cpu.nr_switches.min
     -0.62 ±-1411%   -2212.5%      13.00 ± 16%  sched_debug.cpu.nr_uninterruptible.0
     -1.23 ±-582%   -1318.8%      15.00 ± 35%  sched_debug.cpu.nr_uninterruptible.1
      2.54 ±263%    +267.7%       9.33 ± 91%  sched_debug.cpu.nr_uninterruptible.2
      0.31 ±2966%   -5841.7%     -17.67 ±-19%  sched_debug.cpu.nr_uninterruptible.5
      9.84 ± 19%     +70.4%      16.76 ±  5%  sched_debug.cpu.nr_uninterruptible.stddev
    116287 ±  4%     -20.9%      91972 ±  9%  sched_debug.cpu.sched_count.0
     50411 ± 12%     -57.6%      21382 ± 15%  sched_debug.cpu.sched_count.1
     45453 ± 16%     -68.4%      14356 ± 28%  sched_debug.cpu.sched_count.2
     43098 ± 20%     -65.5%      14888 ± 41%  sched_debug.cpu.sched_count.3
     40314 ± 16%     -70.4%      11934 ± 47%  sched_debug.cpu.sched_count.4
     40761 ± 13%     -75.7%       9896 ± 39%  sched_debug.cpu.sched_count.5
     43041 ± 19%     -79.9%       8636 ± 42%  sched_debug.cpu.sched_count.6
     38105 ± 12%     -75.9%       9193 ± 40%  sched_debug.cpu.sched_count.7
     52184 ±  6%     -56.3%      22782 ±  4%  sched_debug.cpu.sched_count.avg
    116288 ±  4%     -20.9%      91972 ±  9%  sched_debug.cpu.sched_count.max
     33701 ±  9%     -82.9%       5746 ±  4%  sched_debug.cpu.sched_count.min
     22760 ± 10%     -63.0%       8418 ± 40%  sched_debug.cpu.sched_goidle.0
     23319 ± 13%     -60.9%       9114 ± 22%  sched_debug.cpu.sched_goidle.1
     21273 ± 17%     -80.4%       4169 ± 13%  sched_debug.cpu.sched_goidle.2
     19993 ± 19%     -67.8%       6429 ± 45%  sched_debug.cpu.sched_goidle.3
     18614 ± 17%     -85.0%       2788 ± 29%  sched_debug.cpu.sched_goidle.4
     18921 ± 12%     -86.7%       2520 ± 15%  sched_debug.cpu.sched_goidle.5
     20131 ± 17%     -82.1%       3596 ± 52%  sched_debug.cpu.sched_goidle.6
     17861 ± 12%     -86.9%       2334 ± 14%  sched_debug.cpu.sched_goidle.7
     20359 ±  8%     -75.8%       4921 ±  5%  sched_debug.cpu.sched_goidle.avg
     26477 ± 10%     -60.2%      10539 ± 21%  sched_debug.cpu.sched_goidle.max
     15845 ± 10%     -87.2%       2033 ±  4%  sched_debug.cpu.sched_goidle.min
     29043 ± 15%     -58.8%      11958 ± 26%  sched_debug.cpu.ttwu_count.0
     24191 ± 10%     -68.8%       7547 ± 27%  sched_debug.cpu.ttwu_count.1
     21313 ± 11%     -72.7%       5819 ± 24%  sched_debug.cpu.ttwu_count.2
     21487 ± 13%     -61.4%       8296 ± 43%  sched_debug.cpu.ttwu_count.3
     19644 ± 15%     -54.4%       8967 ± 79%  sched_debug.cpu.ttwu_count.4
     20786 ± 15%     -69.2%       6409 ± 58%  sched_debug.cpu.ttwu_count.5
     20435 ± 17%     -79.3%       4231 ± 58%  sched_debug.cpu.ttwu_count.6
     19293 ± 17%     -77.0%       4432 ± 55%  sched_debug.cpu.ttwu_count.7
     22024 ±  7%     -67.3%       7207 ±  6%  sched_debug.cpu.ttwu_count.avg
     31009 ±  9%     -45.2%      17008 ± 17%  sched_debug.cpu.ttwu_count.max
     16791 ± 10%     -85.1%       2494 ±  5%  sched_debug.cpu.ttwu_count.min
      3084 ±  8%     +25.5%       3870 ±  9%  sched_debug.cpu.ttwu_local.avg

The URL of the original regression report email:

https://lists.01.org/pipermail/lkp/2016-January/003442.html

Best Regards,
Huang, Ying

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

#1322957 — [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On Sat, Jan 30, 2016 at 09:18:44AM +0800, Ding Tianhong wrote:
> On 2016/1/29 17:53, Peter Zijlstra wrote:
> > On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:
> > 
> >> looks good to me, I will try this solution and report the result, thanks everyone.
> > 
> > Did you get a change to run with this?
> > 
> > .
> > 
> 
> I backport this patch to 3.10 lts kernel, and didn't change any logic,
> Till now, the patch works fine to me, and no need to change anything,
> So I think this patch is no problem, could you formal release this
> patch to the latest kernel? :)

Thanks for testing, I've queued the below patch.

---
Subject: locking/mutex: Avoid spinner vs waiter starvation
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri, 22 Jan 2016 12:06:53 +0100

Ding Tianhong reported that under his load the optimistic spinners
would totally starve a task that ended up on the wait list.

Fix this by ensuring the top waiter also partakes in the optimistic
spin queue.

There are a few subtle differences between the assumed state of
regular optimistic spinners and those already on the wait list, which
result in the @acquired complication of the acquire path.

Most notable are:

 - waiters are on the wait list and need to be taken off
 - mutex_optimistic_spin() sets the lock->count to 0 on acquire
   even though there might be more tasks on the wait list.

Cc: Jason Low <jason.low2@hp.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Waiman Long <waiman.long@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Will Deacon <Will.Deacon@arm.com>
Reported-by: Ding Tianhong <dingtianhong@huawei.com>
Tested-by: Ding Tianhong <dingtianhong@huawei.com>
Tested-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Waiman Long <Waiman.Long@hp.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20160122110653.GF6375@twins.programming.kicks-ass.net
---
 kernel/locking/mutex.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -512,6 +512,7 @@ __mutex_lock_common(struct mutex *lock,
 	struct task_struct *task = current;
 	struct mutex_waiter waiter;
 	unsigned long flags;
+	bool acquired;
 	int ret;
 
 	preempt_disable();
@@ -543,6 +544,7 @@ __mutex_lock_common(struct mutex *lock,
 	lock_contended(&lock->dep_map, ip);
 
 	for (;;) {
+		acquired = false;
 		/*
 		 * Lets try to take the lock again - this is needed even if
 		 * we get here for the first time (shortly after failing to
@@ -577,7 +579,16 @@ __mutex_lock_common(struct mutex *lock,
 		/* didn't get the lock, go to sleep: */
 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
+
+		if (mutex_is_locked(lock))
+			acquired = mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx);
+
 		spin_lock_mutex(&lock->wait_lock, flags);
+
+		if (acquired) {
+			atomic_set(&lock->count, -1);
+			break;
+		}
 	}
 	__set_task_state(task, TASK_RUNNING);
 
@@ -587,6 +598,9 @@ __mutex_lock_common(struct mutex *lock,
 		atomic_set(&lock->count, 0);
 	debug_mutex_free_waiter(&waiter);
 
+	if (acquired)
+		goto unlock;
+
 skip_wait:
 	/* got the lock - cleanup and rejoice! */
 	lock_acquired(&lock->dep_map, ip);
@@ -597,6 +611,7 @@ __mutex_lock_common(struct mutex *lock,
 		ww_mutex_set_context_slowpath(ww, ww_ctx);
 	}
 
+unlock:
 	spin_unlock_mutex(&lock->wait_lock, flags);
 	preempt_enable();
 	return 0;

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

#1324636 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On Mon, 01 Feb 2016, Peter Zijlstra wrote:

>Subject: locking/mutex: Avoid spinner vs waiter starvation
>From: Peter Zijlstra <peterz@infradead.org>
>Date: Fri, 22 Jan 2016 12:06:53 +0100
>
>Ding Tianhong reported that under his load the optimistic spinners
>would totally starve a task that ended up on the wait list.
>
>Fix this by ensuring the top waiter also partakes in the optimistic
>spin queue.
>
>There are a few subtle differences between the assumed state of
>regular optimistic spinners and those already on the wait list, which
>result in the @acquired complication of the acquire path.
>
>Most notable are:
>
> - waiters are on the wait list and need to be taken off
> - mutex_optimistic_spin() sets the lock->count to 0 on acquire
>   even though there might be more tasks on the wait list.

Right, the main impact I see with these complications are that the
window of when a waiter takes the lock via spinning and then acquires
the wait_lock to remove itself from the list, will allow an unlock
thread to set the lock as available in the fastpath which could in
turn allow a third thread the steal the lock. With high contention,
this window will be come obviously larger as we contend for the
wait_lock.

CPU-0	      	 	       	    CPU-1			CPU-3
__mutex_lock_common		    
   mutex_optimistic_spin
   (->count now 0)
			__mutex_fastpath_unlock
			(->count now 1)				 __mutex_fastpath_lock
				     				 (stolen)
														
spin_lock_mutex(&lock->wait_lock, flags);

But we've always been bad when it comes to counter and waiters.

Thanks,
Davidlohr

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

#1324983 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On 2016/2/3 5:19, Davidlohr Bueso wrote:
> On Mon, 01 Feb 2016, Peter Zijlstra wrote:
> 
>> Subject: locking/mutex: Avoid spinner vs waiter starvation
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Fri, 22 Jan 2016 12:06:53 +0100
>>
>> Ding Tianhong reported that under his load the optimistic spinners
>> would totally starve a task that ended up on the wait list.
>>
>> Fix this by ensuring the top waiter also partakes in the optimistic
>> spin queue.
>>
>> There are a few subtle differences between the assumed state of
>> regular optimistic spinners and those already on the wait list, which
>> result in the @acquired complication of the acquire path.
>>
>> Most notable are:
>>
>> - waiters are on the wait list and need to be taken off
>> - mutex_optimistic_spin() sets the lock->count to 0 on acquire
>>   even though there might be more tasks on the wait list.
> 
> Right, the main impact I see with these complications are that the
> window of when a waiter takes the lock via spinning and then acquires
> the wait_lock to remove itself from the list, will allow an unlock
> thread to set the lock as available in the fastpath which could in
> turn allow a third thread the steal the lock. With high contention,
> this window will be come obviously larger as we contend for the
> wait_lock.
> 
> CPU-0                                  CPU-1            CPU-3
> __mutex_lock_common              mutex_optimistic_spin
>   (->count now 0)
>             __mutex_fastpath_unlock
>             (->count now 1)                 __mutex_fastpath_lock
>                                       (stolen)
>                                                        
> spin_lock_mutex(&lock->wait_lock, flags);
> 
> But we've always been bad when it comes to counter and waiters.
> 

Agree, but this patch is going to help the waiter in the wait list to get the lock, your scene probability looks more
too low and I don't think it is a problem.

Thanks
Ding


> Thanks,
> Davidlohr
> 
> .
> 

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

#1325887 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On Wed, 03 Feb 2016, Ding Tianhong wrote:

>Agree, but this patch is going to help the waiter in the wait list to get the lock, your scene probability looks more
>too low and I don't think it is a problem.

Sure, I was in fact implying its not the end of the world,
although it will be interesting to see the impact on different
(non pathological) workloads, even if it only affects a single
waiter. Also, technically this issue can also affect rwsems if
only using writers, but that's obviously pretty idiotic, so I
wouldn't worry about it.

Thanks,
Davidlohr

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

#1326333 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On 2016/2/4 3:24, Davidlohr Bueso wrote:
> On Wed, 03 Feb 2016, Ding Tianhong wrote:
> 
>> Agree, but this patch is going to help the waiter in the wait list to get the lock, your scene probability looks more
>> too low and I don't think it is a problem.
> 
> Sure, I was in fact implying its not the end of the world,
> although it will be interesting to see the impact on different
> (non pathological) workloads, even if it only affects a single
> waiter. Also, technically this issue can also affect rwsems if
> only using writers, but that's obviously pretty idiotic, so I
> wouldn't worry about it.
> 
> Thanks,
> Davidlohr
> 

Hi Davidlohr, Peter:

According Davidlohr's suggestion, I use several VM to test Peter's patch, and sadly I found one VM
still happen Hung Task for this problem, so I think we still need to think more about this solution.

Thanks.
Ding

> .
> 

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

#1325997 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On 02/02/2016 04:19 PM, Davidlohr Bueso wrote:
> On Mon, 01 Feb 2016, Peter Zijlstra wrote:
>
>> Subject: locking/mutex: Avoid spinner vs waiter starvation
>> From: Peter Zijlstra <peterz@infradead.org>
>> Date: Fri, 22 Jan 2016 12:06:53 +0100
>>
>> Ding Tianhong reported that under his load the optimistic spinners
>> would totally starve a task that ended up on the wait list.
>>
>> Fix this by ensuring the top waiter also partakes in the optimistic
>> spin queue.
>>
>> There are a few subtle differences between the assumed state of
>> regular optimistic spinners and those already on the wait list, which
>> result in the @acquired complication of the acquire path.
>>
>> Most notable are:
>>
>> - waiters are on the wait list and need to be taken off
>> - mutex_optimistic_spin() sets the lock->count to 0 on acquire
>>   even though there might be more tasks on the wait list.
>
> Right, the main impact I see with these complications are that the
> window of when a waiter takes the lock via spinning and then acquires
> the wait_lock to remove itself from the list, will allow an unlock
> thread to set the lock as available in the fastpath which could in
> turn allow a third thread the steal the lock. With high contention,
> this window will be come obviously larger as we contend for the
> wait_lock.
>
> CPU-0                                  CPU-1            CPU-3
> __mutex_lock_common              mutex_optimistic_spin
>   (->count now 0)
>             __mutex_fastpath_unlock
>             (->count now 1)                 __mutex_fastpath_lock
>                                       (stolen)
>
> spin_lock_mutex(&lock->wait_lock, flags);
>
> But we've always been bad when it comes to counter and waiters.
>
> Thanks,
> Davidlohr

I don't quite get how that can happen. CPU0 cannot change the count to 0 
unless CPU1, the lock holder, does the unlock first. Once CPU0 sees a 
count of 1 and change it to 0, it is the lock holder and there can be no 
other CPU that can do the unlock.

Cheers,
Longman

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

#1326338 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On Mon, 2016-02-01 at 11:08 +0100, Peter Zijlstra wrote:
> On Sat, Jan 30, 2016 at 09:18:44AM +0800, Ding Tianhong wrote:
> > On 2016/1/29 17:53, Peter Zijlstra wrote:
> > > On Sun, Jan 24, 2016 at 04:03:50PM +0800, Ding Tianhong wrote:
> > > 
> > >> looks good to me, I will try this solution and report the result, thanks everyone.
> > > 
> > > Did you get a change to run with this?
> > > 
> > > .
> > > 
> > 
> > I backport this patch to 3.10 lts kernel, and didn't change any logic,
> > Till now, the patch works fine to me, and no need to change anything,
> > So I think this patch is no problem, could you formal release this
> > patch to the latest kernel? :)
> 
> Thanks for testing, I've queued the below patch.
> 
> ---
> Subject: locking/mutex: Avoid spinner vs waiter starvation
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri, 22 Jan 2016 12:06:53 +0100
> 
> Ding Tianhong reported that under his load the optimistic spinners
> would totally starve a task that ended up on the wait list.
> 
> Fix this by ensuring the top waiter also partakes in the optimistic
> spin queue.
> 
> There are a few subtle differences between the assumed state of
> regular optimistic spinners and those already on the wait list, which
> result in the @acquired complication of the acquire path.
> 
> Most notable are:
> 
>  - waiters are on the wait list and need to be taken off
>  - mutex_optimistic_spin() sets the lock->count to 0 on acquire
>    even though there might be more tasks on the wait list.
> 
> Cc: Jason Low <jason.low2@hp.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Waiman Long <waiman.long@hpe.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Paul E. McKenney" <paulmck@us.ibm.com>
> Cc: Davidlohr Bueso <dave@stgolabs.net>
> Cc: Will Deacon <Will.Deacon@arm.com>
> Reported-by: Ding Tianhong <dingtianhong@huawei.com>
> Tested-by: Ding Tianhong <dingtianhong@huawei.com>
> Tested-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Waiman Long <Waiman.Long@hp.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/r/20160122110653.GF6375@twins.programming.kicks-ass.net

I've done some testing with this patch with some of the AIM7 workloads
and found that this reduced throughput by about 10%. The reduction in
throughput is expected since spinning as a waiter is less efficient.

Another observation I made is that the top waiter spinners would often
times require needing to reschedule before being able to acquire the
lock from spinning when there was high contention. A waiter can go into
the cycle of spin -> reschedule -> spin -> reschedule. So although the
chance of starvation is reduced, this patch doesn't fully address the
issue of waiter starvation.

Jason

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

#1326496 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

Hi, Low,

On Thu, Feb 4, 2016 at 9:35 AM, Jason Low <jason.low2@hp.com> wrote:
> I've done some testing with this patch with some of the AIM7 workloads
> and found that this reduced throughput by about 10%. The reduction in
> throughput is expected since spinning as a waiter is less efficient.
>
> Another observation I made is that the top waiter spinners would often
> times require needing to reschedule before being able to acquire the
> lock from spinning when there was high contention. A waiter can go into
> the cycle of spin -> reschedule -> spin -> reschedule. So although the
> chance of starvation is reduced, this patch doesn't fully address the
> issue of waiter starvation.

Could you share your workload?  I want to reproduce it in 0day/LKP+ environment.

Best Regards,
Huang, Ying

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

#1327290 — Re: [PATCH] locking/mutex: Avoid spinner vs waiter starvation

On Thu, 2016-02-04 at 16:55 +0800, huang ying wrote:
> Hi, Low,
> 
> On Thu, Feb 4, 2016 at 9:35 AM, Jason Low <jason.low2@hp.com> wrote:
> > I've done some testing with this patch with some of the AIM7 workloads
> > and found that this reduced throughput by about 10%. The reduction in
> > throughput is expected since spinning as a waiter is less efficient.
> >
> > Another observation I made is that the top waiter spinners would often
> > times require needing to reschedule before being able to acquire the
> > lock from spinning when there was high contention. A waiter can go into
> > the cycle of spin -> reschedule -> spin -> reschedule. So although the
> > chance of starvation is reduced, this patch doesn't fully address the
> > issue of waiter starvation.
> 
> Could you share your workload?  I want to reproduce it in 0day/LKP+ environment.

CC'ing Scott, who wrote the automation scripts.

Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web