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

Groups > linux.kernel > #1459512 > unrolled thread

[PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] locking/mutex: Prevent lock starvation when spinning is  enabled Jason Low <jason.low2@hpe.com> - 2016-08-10 20:50 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Jason Low <jason.low2@hpe.com> - 2016-08-11 00:10 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Jason Low <jason.low2@hpe.com> - 2016-08-11 04:40 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Jason Low <jason.low2@hpe.com> - 2016-08-16 21:50 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Wanpeng Li <kernellwp@gmail.com> - 2016-08-17 03:50 +0200
      Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Jason Low <jason.low2@hpe.com> - 2016-08-17 20:40 +0200
        Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Wanpeng Li <kernellwp@gmail.com> - 2016-08-18 02:40 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Peter Zijlstra <peterz@infradead.org> - 2016-08-18 16:30 +0200
      Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Peter Zijlstra <peterz@infradead.org> - 2016-08-19 02:50 +0200
        Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Peter Zijlstra <peterz@infradead.org> - 2016-08-19 02:50 +0200
    Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning  is enabled Peter Zijlstra <peterz@infradead.org> - 2016-08-18 16:50 +0200

#1459512 — [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

Imre reported an issue where threads are getting starved when trying
to acquire a mutex. Threads acquiring a mutex can get arbitrarily delayed
sleeping on a mutex because other threads can continually steal the lock
in the fastpath and/or through optimistic spinning.

Waiman has developed patches that allow waiters to return to optimistic
spinning, thus reducing the probability that starvation occurs. However,
Imre still sees this starvation problem in the workloads when optimistic
spinning is disabled.

This patch adds an additional boolean to the mutex that gets used in
the CONFIG_SMP && !CONFIG_MUTEX_SPIN_ON_OWNER cases. The flag signifies
whether or not other threads need to yield to a waiter and gets set
when a waiter spends too much time waiting for the mutex. The threshold
is currently set to 16 wakeups, and once the wakeup threshold is exceeded,
other threads must yield to the top waiter. The flag gets cleared
immediately after the top waiter acquires the mutex.

This prevents waiters from getting starved without sacrificing much
much performance, as lock stealing is still allowed and only
temporarily disabled when it is detected that a waiter has been waiting
for too long.

Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jason Low <jason.low2@hpe.com>
---
v1->v2:
- Addressed Waiman's suggestions of needing the yield_to_waiter
  flag only in the CONFIG_SMP case.

- Make sure to only clear the flag if the thread is the top waiter.

- Refactor code to clear flag into an inline function.

- Rename 'loops' variable name to 'wakeups'.

 include/linux/mutex.h  |  2 ++
 kernel/locking/mutex.c | 83 +++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 74 insertions(+), 11 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 2cb7531..5643a233 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -57,6 +57,8 @@ struct mutex {
 #endif
 #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
 	struct optimistic_spin_queue osq; /* Spinner MCS lock */
+#elif defined(CONFIG_SMP)
+	bool yield_to_waiter; /* Prevent starvation when spinning disabled */
 #endif
 #ifdef CONFIG_DEBUG_MUTEXES
 	void			*magic;
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index a70b90d..faf31a0 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -55,6 +55,8 @@ __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
 	mutex_clear_owner(lock);
 #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
 	osq_lock_init(&lock->osq);
+#elif defined(CONFIG_SMP)
+	lock->yield_to_waiter = false;
 #endif
 
 	debug_mutex_init(lock, name, key);
@@ -71,6 +73,9 @@ EXPORT_SYMBOL(__mutex_init);
  */
 __visible void __sched __mutex_lock_slowpath(atomic_t *lock_count);
 
+
+static inline bool need_yield_to_waiter(struct mutex *lock);
+
 /**
  * mutex_lock - acquire the mutex
  * @lock: the mutex to be acquired
@@ -99,7 +104,10 @@ void __sched mutex_lock(struct mutex *lock)
 	 * The locking fastpath is the 1->0 transition from
 	 * 'unlocked' into 'locked' state.
 	 */
-	__mutex_fastpath_lock(&lock->count, __mutex_lock_slowpath);
+	if (!need_yield_to_waiter(lock))
+		__mutex_fastpath_lock(&lock->count, __mutex_lock_slowpath);
+	else
+		__mutex_lock_slowpath(&lock->count);
 	mutex_set_owner(lock);
 }
 
@@ -398,12 +406,51 @@ done:
 
 	return false;
 }
+
+static inline void do_yield_to_waiter(struct mutex *lock, int *wakeups)
+{
+	return;
+}
+
+static inline void clear_yield_to_waiter(struct mutex *lock)
+{
+	return;
+}
+
+static inline bool need_yield_to_waiter(struct mutex *lock)
+{
+	return false;
+}
+
 #else
 static bool mutex_optimistic_spin(struct mutex *lock,
 				  struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
 {
 	return false;
 }
+
+#define MUTEX_WAKEUP_THRESHOLD 16
+
+static inline void do_yield_to_waiter(struct mutex *lock, int *wakeups)
+{
+	*wakeups += 1;
+
+	if (*wakeups < MUTEX_WAKEUP_THRESHOLD)
+		return;
+
+	if (lock->yield_to_waiter != true)
+		lock->yield_to_waiter = true;
+}
+
+static inline void clear_yield_to_waiter(struct mutex *lock)
+{
+	lock->yield_to_waiter = false;
+}
+
+static inline bool need_yield_to_waiter(struct mutex *lock)
+{
+	return lock->yield_to_waiter;
+}
 #endif
 
 __visible __used noinline
@@ -510,6 +557,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	struct mutex_waiter waiter;
 	unsigned long flags;
 	int ret;
+	int wakeups = 0;
 
 	if (use_ww_ctx) {
 		struct ww_mutex *ww = container_of(lock, struct ww_mutex, base);
@@ -532,7 +580,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 	 * Once more, try to acquire the lock. Only try-lock the mutex if
 	 * it is unlocked to reduce unnecessary xchg() operations.
 	 */
-	if (!mutex_is_locked(lock) &&
+	if (!need_yield_to_waiter(lock) && !mutex_is_locked(lock) &&
 	    (atomic_xchg_acquire(&lock->count, 0) == 1))
 		goto skip_wait;
 
@@ -556,8 +604,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		 * other waiters. We only attempt the xchg if the count is
 		 * non-negative in order to avoid unnecessary xchg operations:
 		 */
-		if (atomic_read(&lock->count) >= 0 &&
+		if ((!need_yield_to_waiter(lock) || wakeups > 1) &&
+		    atomic_read(&lock->count) >= 0 &&
 		    (atomic_xchg_acquire(&lock->count, -1) == 1))
+			if (wakeups > 1)
+				clear_yield_to_waiter(lock);
+
 			break;
 
 		/*
@@ -581,6 +633,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
 		spin_lock_mutex(&lock->wait_lock, flags);
+		do_yield_to_waiter(lock, &wakeups);
 	}
 	__set_task_state(task, TASK_RUNNING);
 
@@ -789,10 +842,13 @@ __mutex_lock_interruptible_slowpath(struct mutex *lock);
  */
 int __sched mutex_lock_interruptible(struct mutex *lock)
 {
-	int ret;
+	int ret = 1;
 
 	might_sleep();
-	ret =  __mutex_fastpath_lock_retval(&lock->count);
+
+	if (!need_yield_to_waiter(lock))
+		ret =  __mutex_fastpath_lock_retval(&lock->count);
+
 	if (likely(!ret)) {
 		mutex_set_owner(lock);
 		return 0;
@@ -804,10 +860,13 @@ EXPORT_SYMBOL(mutex_lock_interruptible);
 
 int __sched mutex_lock_killable(struct mutex *lock)
 {
-	int ret;
+	int ret = 1;
 
 	might_sleep();
-	ret = __mutex_fastpath_lock_retval(&lock->count);
+
+	if (!need_yield_to_waiter(lock))
+		ret = __mutex_fastpath_lock_retval(&lock->count);
+
 	if (likely(!ret)) {
 		mutex_set_owner(lock);
 		return 0;
@@ -917,11 +976,12 @@ EXPORT_SYMBOL(mutex_trylock);
 int __sched
 __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 {
-	int ret;
+	int ret = 1;
 
 	might_sleep();
 
-	ret = __mutex_fastpath_lock_retval(&lock->base.count);
+	if (!need_yield_to_waiter(lock))
+		ret = __mutex_fastpath_lock_retval(&lock->base.count);
 
 	if (likely(!ret)) {
 		ww_mutex_set_context_fastpath(lock, ctx);
@@ -935,11 +995,12 @@ EXPORT_SYMBOL(__ww_mutex_lock);
 int __sched
 __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
 {
-	int ret;
+	int ret = 1;
 
 	might_sleep();
 
-	ret = __mutex_fastpath_lock_retval(&lock->base.count);
+	if (!need_yield_to_waiter(lock))
+		ret = __mutex_fastpath_lock_retval(&lock->base.count);
 
 	if (likely(!ret)) {
 		ww_mutex_set_context_fastpath(lock, ctx);
-- 
2.1.4

[toc] | [next] | [standalone]

#1460043 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Wed, 2016-08-10 at 11:44 -0700, Jason Low wrote:
> Imre reported an issue where threads are getting starved when trying
> to acquire a mutex. Threads acquiring a mutex can get arbitrarily delayed
> sleeping on a mutex because other threads can continually steal the lock
> in the fastpath and/or through optimistic spinning.
> 
> Waiman has developed patches that allow waiters to return to optimistic
> spinning, thus reducing the probability that starvation occurs. However,
> Imre still sees this starvation problem in the workloads when optimistic
> spinning is disabled.
> 
> This patch adds an additional boolean to the mutex that gets used in
> the CONFIG_SMP && !CONFIG_MUTEX_SPIN_ON_OWNER cases. The flag signifies
> whether or not other threads need to yield to a waiter and gets set
> when a waiter spends too much time waiting for the mutex. The threshold
> is currently set to 16 wakeups, and once the wakeup threshold is exceeded,
> other threads must yield to the top waiter. The flag gets cleared
> immediately after the top waiter acquires the mutex.

Just noticed that the patch title mentions "when spinning is enabled".

The title should really be:

"locking/mutex: Prevent lock starvation when spinning is disabled"

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

#1460144 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Wed, 2016-08-10 at 11:44 -0700, Jason Low wrote:
> @@ -917,11 +976,12 @@ EXPORT_SYMBOL(mutex_trylock);
>  int __sched
>  __ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	might_sleep();
>  
> -	ret = __mutex_fastpath_lock_retval(&lock->base.count);
> +	if (!need_yield_to_waiter(lock))
> +		ret = __mutex_fastpath_lock_retval(&lock->base.count);
>  
>  	if (likely(!ret)) {
>  		ww_mutex_set_context_fastpath(lock, ctx);
> @@ -935,11 +995,12 @@ EXPORT_SYMBOL(__ww_mutex_lock);
>  int __sched
>  __ww_mutex_lock_interruptible(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	might_sleep();
>  
> -	ret = __mutex_fastpath_lock_retval(&lock->base.count);
> +	if (!need_yield_to_waiter(lock))

And we would need to pass &lock->base instead of lock since lock is
struct ww_mutex * here.

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

#1464029 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Thu, 2016-08-11 at 11:40 -0400, Waiman Long wrote:
> On 08/10/2016 02:44 PM, Jason Low wrote:
> > +static inline void do_yield_to_waiter(struct mutex *lock, int *wakeups)
> > +{
> > +	return;
> > +}
> > +
> > +static inline void clear_yield_to_waiter(struct mutex *lock)
> > +{
> > +	return;
> > +}
> > +
> > +static inline bool need_yield_to_waiter(struct mutex *lock)
> > +{
> > +	return false;
> > +}
> > +
> >   #else
> >   static bool mutex_optimistic_spin(struct mutex *lock,
> >   				  struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
> >   {
> >   	return false;
> >   }
> > +
> > +#define MUTEX_WAKEUP_THRESHOLD 16
> > +
> > +static inline void do_yield_to_waiter(struct mutex *lock, int *wakeups)
> > +{
> > +	*wakeups += 1;
> > +
> > +	if (*wakeups<  MUTEX_WAKEUP_THRESHOLD)
> > +		return;
> > +
> > +	if (lock->yield_to_waiter != true)
> > +		lock->yield_to_waiter = true;
> > +}
> > +
> > +static inline void clear_yield_to_waiter(struct mutex *lock)
> > +{
> > +	lock->yield_to_waiter = false;
> > +}
> > +
> > +static inline bool need_yield_to_waiter(struct mutex *lock)
> > +{
> > +	return lock->yield_to_waiter;
> > +}
> >   #endif
> >
> >   _
> 
> The *yield* helper functions should be in a separate conditional 
> compilation block as the declaration of yield_to_waiter may not match 
> the helper functions with certain combination of config variables.
> 
> Something like
> 
> #if !defined(CONFIG_MUTEX_SPIN_ON_OWNER) && defined(CONFIG_SMP)
> ...
> #else
> ...
> #endif

Right, we will need to incorporate the CONFIG_SMP logic when defining
these functions here, otherwise they would be undefined in the !SMP
case. 

Thanks,
Jason

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

#1464277 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

2016-08-11 2:44 GMT+08:00 Jason Low <jason.low2@hpe.com>:
> Imre reported an issue where threads are getting starved when trying
> to acquire a mutex. Threads acquiring a mutex can get arbitrarily delayed
> sleeping on a mutex because other threads can continually steal the lock
> in the fastpath and/or through optimistic spinning.
>
> Waiman has developed patches that allow waiters to return to optimistic
> spinning, thus reducing the probability that starvation occurs. However,
> Imre still sees this starvation problem in the workloads when optimistic
> spinning is disabled.
>
> This patch adds an additional boolean to the mutex that gets used in
> the CONFIG_SMP && !CONFIG_MUTEX_SPIN_ON_OWNER cases. The flag signifies
> whether or not other threads need to yield to a waiter and gets set
> when a waiter spends too much time waiting for the mutex. The threshold
> is currently set to 16 wakeups, and once the wakeup threshold is exceeded,
> other threads must yield to the top waiter. The flag gets cleared
> immediately after the top waiter acquires the mutex.

There is a subtle difference between this patch and Waiman's. Waiman's
patch will boost any waiter-spinner which is woken up, however, this
patch will boost the top waiter once the number of any waiter-spinners
woken up reaches the threshold. We can't get any benefit if the
resource holder which top waiter is waiting for still not release the
resource.

Regards,
Wanpeng Li

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

#1464713 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

Hi Wanpeng,

On Wed, 2016-08-17 at 09:41 +0800, Wanpeng Li wrote:
> 2016-08-11 2:44 GMT+08:00 Jason Low <jason.low2@hpe.com>:
> > Imre reported an issue where threads are getting starved when trying
> > to acquire a mutex. Threads acquiring a mutex can get arbitrarily delayed
> > sleeping on a mutex because other threads can continually steal the lock
> > in the fastpath and/or through optimistic spinning.
> >
> > Waiman has developed patches that allow waiters to return to optimistic
> > spinning, thus reducing the probability that starvation occurs. However,
> > Imre still sees this starvation problem in the workloads when optimistic
> > spinning is disabled.
> >
> > This patch adds an additional boolean to the mutex that gets used in
> > the CONFIG_SMP && !CONFIG_MUTEX_SPIN_ON_OWNER cases. The flag signifies
> > whether or not other threads need to yield to a waiter and gets set
> > when a waiter spends too much time waiting for the mutex. The threshold
> > is currently set to 16 wakeups, and once the wakeup threshold is exceeded,
> > other threads must yield to the top waiter. The flag gets cleared
> > immediately after the top waiter acquires the mutex.
> 
> There is a subtle difference between this patch and Waiman's. Waiman's
> patch will boost any waiter-spinner which is woken up, however, this
> patch will boost the top waiter once the number of any waiter-spinners
> woken up reaches the threshold.

Correct, since when spinning is disabled, we still want to generally
allow other threads to steal the lock even if there are waiters in order
to keep performance good, and only yield the lock when a waiter is
getting 'starved'.

> We can't get any benefit if the
> resource holder which top waiter is waiting for still not release the
> resource.

If the resource holder does not release the resource, that sounds like
an issue with the lock holder.

Unless you're referring to how this doesn't provide immediate benefit to
the top waiter, in which case, I think that is okay since the goal of
the patch is to prevent starvation. We tried disabling 'lock stealing'
anytime there are waiters and that proved to reduce performance by quite
a bit in some workloads.

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

#1464859 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

2016-08-18 2:30 GMT+08:00 Jason Low <jason.low2@hpe.com>:
> Hi Wanpeng,
>
> On Wed, 2016-08-17 at 09:41 +0800, Wanpeng Li wrote:
>> 2016-08-11 2:44 GMT+08:00 Jason Low <jason.low2@hpe.com>:
>> > Imre reported an issue where threads are getting starved when trying
>> > to acquire a mutex. Threads acquiring a mutex can get arbitrarily delayed
>> > sleeping on a mutex because other threads can continually steal the lock
>> > in the fastpath and/or through optimistic spinning.
>> >
>> > Waiman has developed patches that allow waiters to return to optimistic
>> > spinning, thus reducing the probability that starvation occurs. However,
>> > Imre still sees this starvation problem in the workloads when optimistic
>> > spinning is disabled.
>> >
>> > This patch adds an additional boolean to the mutex that gets used in
>> > the CONFIG_SMP && !CONFIG_MUTEX_SPIN_ON_OWNER cases. The flag signifies
>> > whether or not other threads need to yield to a waiter and gets set
>> > when a waiter spends too much time waiting for the mutex. The threshold
>> > is currently set to 16 wakeups, and once the wakeup threshold is exceeded,
>> > other threads must yield to the top waiter. The flag gets cleared
>> > immediately after the top waiter acquires the mutex.
>>
>> There is a subtle difference between this patch and Waiman's. Waiman's
>> patch will boost any waiter-spinner which is woken up, however, this
>> patch will boost the top waiter once the number of any waiter-spinners
>> woken up reaches the threshold.
>
> Correct, since when spinning is disabled, we still want to generally
> allow other threads to steal the lock even if there are waiters in order
> to keep performance good, and only yield the lock when a waiter is
> getting 'starved'.
>
>> We can't get any benefit if the
>> resource holder which top waiter is waiting for still not release the
>> resource.
>
> If the resource holder does not release the resource, that sounds like
> an issue with the lock holder.
>
> Unless you're referring to how this doesn't provide immediate benefit to
> the top waiter,

Yes.

> in which case, I think that is okay since the goal of
> the patch is to prevent starvation. We tried disabling 'lock stealing'
> anytime there are waiters and that proved to reduce performance by quite
> a bit in some workloads.

Thanks for the clarification. :)

Regards,
Wanpeng Li

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

#1465513 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Wed, Aug 10, 2016 at 11:44:08AM -0700, Jason Low wrote:
> @@ -556,8 +604,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>  		 * other waiters. We only attempt the xchg if the count is
>  		 * non-negative in order to avoid unnecessary xchg operations:
>  		 */
> -		if (atomic_read(&lock->count) >= 0 &&
> +		if ((!need_yield_to_waiter(lock) || wakeups > 1) &&
> +		    atomic_read(&lock->count) >= 0 &&
>  		    (atomic_xchg_acquire(&lock->count, -1) == 1))
> +			if (wakeups > 1)
> +				clear_yield_to_waiter(lock);
> +
>  			break;
>  
>  		/*

There's some { } gone missing there...

Also, I think I'll change it to avoid that extra wakeups > 1 condition..

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

#1465662 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Thu, Aug 18, 2016 at 04:27:35PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 10, 2016 at 11:44:08AM -0700, Jason Low wrote:
> > @@ -556,8 +604,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
> >  		 * other waiters. We only attempt the xchg if the count is
> >  		 * non-negative in order to avoid unnecessary xchg operations:
> >  		 */
> > -		if (atomic_read(&lock->count) >= 0 &&
> > +		if ((!need_yield_to_waiter(lock) || wakeups > 1) &&
> > +		    atomic_read(&lock->count) >= 0 &&
> >  		    (atomic_xchg_acquire(&lock->count, -1) == 1))
> > +			if (wakeups > 1)
> > +				clear_yield_to_waiter(lock);
> > +
> >  			break;
> >  
> >  		/*
> 
> There's some { } gone missing there...
> 
> Also, I think I'll change it to avoid that extra wakeups > 1 condition..

Also, its broken, even if we should not trylock, we should still very
much xchg(-1) to mark the lock as having waiters.

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

#1465667 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Thu, Aug 18, 2016 at 05:18:43PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 18, 2016 at 04:27:35PM +0200, Peter Zijlstra wrote:
> > On Wed, Aug 10, 2016 at 11:44:08AM -0700, Jason Low wrote:
> > > @@ -556,8 +604,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
> > >  		 * other waiters. We only attempt the xchg if the count is
> > >  		 * non-negative in order to avoid unnecessary xchg operations:
> > >  		 */
> > > -		if (atomic_read(&lock->count) >= 0 &&
> > > +		if ((!need_yield_to_waiter(lock) || wakeups > 1) &&
> > > +		    atomic_read(&lock->count) >= 0 &&
> > >  		    (atomic_xchg_acquire(&lock->count, -1) == 1))
> > > +			if (wakeups > 1)
> > > +				clear_yield_to_waiter(lock);
> > > +
> > >  			break;
> > >  
> > >  		/*
> > 
> > There's some { } gone missing there...
> > 
> > Also, I think I'll change it to avoid that extra wakeups > 1 condition..
> 
> Also, its broken, even if we should not trylock, we should still very
> much xchg(-1) to mark the lock as having waiters.

Ah, no. Since need_yield_to_waiter() can only be true if there is an
actual waiter, at which point count must already be -1. /me adds a
comment.

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

#1465612 — Re: [PATCH v2] locking/mutex: Prevent lock starvation when spinning is enabled

On Wed, Aug 10, 2016 at 11:44:08AM -0700, Jason Low wrote:
> diff --git a/include/linux/mutex.h b/include/linux/mutex.h
> index 2cb7531..5643a233 100644
> --- a/include/linux/mutex.h
> +++ b/include/linux/mutex.h
> @@ -57,6 +57,8 @@ struct mutex {
>  #endif
>  #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
>  	struct optimistic_spin_queue osq; /* Spinner MCS lock */
> +#elif defined(CONFIG_SMP)
> +	bool yield_to_waiter; /* Prevent starvation when spinning disabled */
>  #endif
>  #ifdef CONFIG_DEBUG_MUTEXES
>  	void			*magic;

Isn't this also possible on !SMP && PREEMPT ?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web