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

Groups > linux.kernel > #1468537 > unrolled thread

[RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-23 15:00 +0200
    [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid starvation Peter Zijlstra <peterz@infradead.org> - 2016-08-23 15:00 +0200
      Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid  starvation Peter Zijlstra <peterz@infradead.org> - 2016-08-23 15:00 +0200
      Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid  starvation Peter Zijlstra <peterz@infradead.org> - 2016-08-23 22:50 +0200
        Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid  starvation Peter Zijlstra <peterz@infradead.org> - 2016-08-25 10:20 +0200
    [RFC][PATCH 2/3] locking/mutex: Allow MUTEX_SPIN_ON_OWNER when DEBUG_MUTEXES Peter Zijlstra <peterz@infradead.org> - 2016-08-23 15:00 +0200
    Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Davidlohr Bueso <dave@stgolabs.net> - 2016-08-23 18:20 +0200
      Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Jason Low <jason.low2@hpe.com> - 2016-08-23 18:40 +0200
        Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-23 19:00 +0200
          Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-23 22:50 +0200
        Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Jason Low <jason.low2@hpe.com> - 2016-08-24 03:30 +0200
          Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-25 14:40 +0200
          Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-25 18:40 +0200
            Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Ingo Molnar <mingo@kernel.org> - 2016-08-27 20:30 +0200
          Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex huang ying <huang.ying.caritas@gmail.com> - 2016-08-25 21:20 +0200
            Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-25 22:00 +0200
      Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Peter Zijlstra <peterz@infradead.org> - 2016-08-24 00:00 +0200
      Re: [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex Linus Torvalds <torvalds@linux-foundation.org> - 2016-08-24 03:30 +0200
    Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Peter Zijlstra <peterz@infradead.org> - 2016-08-23 22:40 +0200
    Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Tim Chen <tim.c.chen@linux.intel.com> - 2016-08-23 23:00 +0200
      Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Peter Zijlstra <peterz@infradead.org> - 2016-08-23 23:10 +0200
    Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Peter Zijlstra <peterz@infradead.org> - 2016-08-23 23:20 +0200
    Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Will Deacon <will.deacon@arm.com> - 2016-08-24 12:00 +0200
      Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Will Deacon <will.deacon@arm.com> - 2016-08-24 19:00 +0200
      Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Peter Zijlstra <peterz@infradead.org> - 2016-08-24 19:00 +0200
      Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner Peter Zijlstra <peterz@infradead.org> - 2016-08-24 19:20 +0200

Page 1 of 2  [1] 2  Next page →

#1468537 — [RFC][PATCH 0/3] locking/mutex: Rewrite basic mutex

... might eat your pets and set your house on fire ...

But they seem to boot and build kernels on my x86_64.

---
 arch/alpha/include/asm/mutex.h      |   9 --
 arch/arc/include/asm/mutex.h        |  18 ---
 arch/arm/include/asm/mutex.h        |  21 ---
 arch/arm64/include/asm/Kbuild       |   1 -
 arch/avr32/include/asm/mutex.h      |   9 --
 arch/blackfin/include/asm/Kbuild    |   1 -
 arch/c6x/include/asm/mutex.h        |   6 -
 arch/cris/include/asm/mutex.h       |   9 --
 arch/frv/include/asm/mutex.h        |   9 --
 arch/h8300/include/asm/mutex.h      |   9 --
 arch/hexagon/include/asm/mutex.h    |   8 -
 arch/ia64/include/asm/mutex.h       |  90 -----------
 arch/m32r/include/asm/mutex.h       |   9 --
 arch/m68k/include/asm/Kbuild        |   1 -
 arch/metag/include/asm/Kbuild       |   1 -
 arch/microblaze/include/asm/mutex.h |   1 -
 arch/mips/include/asm/Kbuild        |   1 -
 arch/mn10300/include/asm/mutex.h    |  16 --
 arch/nios2/include/asm/mutex.h      |   1 -
 arch/openrisc/include/asm/mutex.h   |  27 ----
 arch/parisc/include/asm/Kbuild      |   1 -
 arch/powerpc/include/asm/mutex.h    | 132 ---------------
 arch/s390/include/asm/mutex.h       |   9 --
 arch/score/include/asm/mutex.h      |   6 -
 arch/sh/include/asm/mutex-llsc.h    | 109 -------------
 arch/sh/include/asm/mutex.h         |  12 --
 arch/sparc/include/asm/Kbuild       |   1 -
 arch/tile/include/asm/Kbuild        |   1 -
 arch/um/include/asm/Kbuild          |   1 -
 arch/unicore32/include/asm/mutex.h  |  20 ---
 arch/x86/include/asm/mutex.h        |   5 -
 arch/x86/include/asm/mutex_32.h     | 110 -------------
 arch/x86/include/asm/mutex_64.h     | 127 ---------------
 arch/xtensa/include/asm/mutex.h     |   9 --
 include/asm-generic/mutex-dec.h     |  88 ----------
 include/asm-generic/mutex-null.h    |  19 ---
 include/asm-generic/mutex-xchg.h    | 120 --------------
 include/asm-generic/mutex.h         |   9 --
 include/linux/mutex-debug.h         |  24 ---
 include/linux/mutex.h               |  44 +++--
 kernel/Kconfig.locks                |   2 +-
 kernel/locking/mutex-debug.c        |  13 --
 kernel/locking/mutex-debug.h        |  10 --
 kernel/locking/mutex.c              | 311 +++++++++++++++---------------------
 kernel/locking/mutex.h              |  26 ---
 kernel/sched/core.c                 |   2 +-
 46 files changed, 160 insertions(+), 1298 deletions(-)

[toc] | [next] | [standalone]

#1468539 — [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid starvation

Now that we have an atomic owner field, we can do explicit lock
handoff. Use this to avoid starvation.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/locking/mutex.c |   44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -53,6 +53,7 @@ __mutex_init(struct mutex *lock, const c
 EXPORT_SYMBOL(__mutex_init);
 
 #define MUTEX_FLAG_WAITERS	0x01
+#define MUTEX_FLAG_HANDOFF	0x02
 
 #define MUTEX_FLAG_ALL		0x03
 
@@ -84,6 +85,29 @@ static inline void __mutex_clear_flag(st
 	atomic_long_andnot(flag, &lock->owner);
 }
 
+static inline bool __mutex_waiter_is_first(struct mutex *lock, struct mutex_waiter *waiter)
+{
+	return list_first_entry(&lock->wait_list, struct mutex_waiter, list) == waiter;
+}
+
+static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
+{
+	unsigned long owner = atomic_long_read(&lock->owner);
+
+	for (;;) {
+		unsigned long old, new;
+
+		new = (owner & MUTEX_FLAG_WAITERS);
+		new |= (unsigned long)task;
+
+		old = atomic_long_cmpxchg(&lock->owner, owner, new);
+		if (old == owner)
+			break;
+
+		owner = old;
+	}
+}
+
 #ifndef CONFIG_DEBUG_LOCK_ALLOC
 /*
  * We split the mutex lock/unlock logic into separate fastpath and
@@ -414,7 +438,7 @@ static bool mutex_optimistic_spin(struct
 }
 #endif
 
-static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock);
+static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigned long owner);
 
 /**
  * mutex_unlock - release the mutex
@@ -439,6 +463,9 @@ void __sched mutex_unlock(struct mutex *
 	for (;;) {
 		unsigned long old;
 
+		if (owner & MUTEX_FLAG_HANDOFF)
+			break;
+
 		old = atomic_long_cmpxchg_release(&lock->owner, owner, owner & 0x03);
 		if (old == owner)
 			break;
@@ -447,7 +474,7 @@ void __sched mutex_unlock(struct mutex *
 	}
 
 	if (owner & 0x03);
-		__mutex_unlock_slowpath(lock);
+		__mutex_unlock_slowpath(lock, owner);
 }
 EXPORT_SYMBOL(mutex_unlock);
 
@@ -545,7 +572,7 @@ __mutex_lock_common(struct mutex *lock,
 	list_add_tail(&waiter.list, &lock->wait_list);
 	waiter.task = task;
 
-	if (list_first_entry(&lock->wait_list, struct mutex_waiter, list) == &waiter)
+	if (__mutex_waiter_is_first(lock, &waiter))
 		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
 
 	lock_contended(&lock->dep_map, ip);
@@ -573,8 +600,14 @@ __mutex_lock_common(struct mutex *lock,
 		schedule_preempt_disabled();
 		spin_lock_mutex(&lock->wait_lock, flags);
 
+		if (__mutex_owner(lock) == current)
+			break;
+
 		if (__mutex_trylock(lock))
 			break;
+
+		if (__mutex_waiter_is_first(lock, &waiter))
+			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 	}
 	__set_task_state(task, TASK_RUNNING);
 
@@ -707,7 +740,7 @@ EXPORT_SYMBOL_GPL(__ww_mutex_lock_interr
 /*
  * Release the lock, slowpath:
  */
-static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock)
+static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigned long owner)
 {
 	unsigned long flags;
 	WAKE_Q(wake_q);
@@ -722,6 +755,9 @@ static noinline void __sched __mutex_unl
 				list_entry(lock->wait_list.next,
 					   struct mutex_waiter, list);
 
+		if (owner & MUTEX_FLAG_HANDOFF)
+			__mutex_handoff(lock, waiter->task);
+
 		debug_mutex_wake_waiter(lock, waiter);
 		wake_q_add(&wake_q, waiter->task);
 	}

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

#1468543 — Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid starvation

On Tue, Aug 23, 2016 at 02:46:20PM +0200, Peter Zijlstra wrote:
> @@ -573,8 +600,14 @@ __mutex_lock_common(struct mutex *lock,
>  		schedule_preempt_disabled();
>  		spin_lock_mutex(&lock->wait_lock, flags);
>  
> +		if (__mutex_owner(lock) == current)
> +			break;
> +
>  		if (__mutex_trylock(lock))
>  			break;
> +
> +		if (__mutex_waiter_is_first(lock, &waiter))
> +			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
>  	}
>  	__set_task_state(task, TASK_RUNNING);
>  

And 'obviously' we can add a spin-on-owner loop in there as well, as
Waiman's patches did, but I didn't bother pulling that in for now.

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

#1468837 — Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid starvation

On Tue, Aug 23, 2016 at 03:47:53PM -0400, Waiman Long wrote:
> On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
> >N
> >@@ -573,8 +600,14 @@ __mutex_lock_common(struct mutex *lock,
> >  		schedule_preempt_disabled();
> >  		spin_lock_mutex(&lock->wait_lock, flags);
> >
> >+		if (__mutex_owner(lock) == current)
> >+			break;
> >+
> >  		if (__mutex_trylock(lock))
> >  			break;
> >+
> >+		if (__mutex_waiter_is_first(lock,&waiter))
> >+			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> >  	}
> >  	__set_task_state(task, TASK_RUNNING);
> >
> >
> 
> You may want to think about doing some spinning while the owner is active
> instead of going back to sleep again here.

For sure; I just didn't bother pulling in your patches. I didn't want to
sink in more time in case people really hated on 1/3 ;-)

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

#1469951 — Re: [RFC][PATCH 3/3] locking/mutex: Add lock handoff to avoid starvation

On Wed, Aug 24, 2016 at 03:50:10PM -0400, Waiman Long wrote:

> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -97,6 +97,8 @@ static void __mutex_handoff(struct mutex *lock, struct
> task_st
>         for (;;) {
>                 unsigned long old, new;
> 
> +               if ((owner & ~MUTEX_FLAG_ALL) != current)
> +                       break;
>                 new = (owner & MUTEX_FLAG_WAITERS);
>                 new |= (unsigned long)task;
> 
> I also think that the MUTEX_FLAG_HANDOFF bit needs to be cleared if the list
> is empty.
> 
> @@ -614,7 +633,7 @@ __mutex_lock_common(struct mutex *lock, long state,
> unsigned
>         mutex_remove_waiter(lock, &waiter, task);
>         /* set it to 0 if there are no waiters left: */
>         if (likely(list_empty(&lock->wait_list)))
> -               __mutex_clear_flag(lock, MUTEX_FLAG_WAITERS);
> +               __mutex_clear_flag(lock,
> MUTEX_FLAG_WAITERS|MUTEX_FLAG_HANDOFF);
> 
> Or we should try to reset the handoff bit after the while loop exit if the
> bit is still set.

Yes, I think you're right. I've also found another issue wrt WAITERS in
patch 1.

I'm not trying to get aim7 running to see if I can reproduce Jason's
results and verify things.

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

#1468542 — [RFC][PATCH 2/3] locking/mutex: Allow MUTEX_SPIN_ON_OWNER when DEBUG_MUTEXES

Now that mutex::count and mutex::owner are the same field, we can
allow SPIN_ON_OWNER while DEBUG_MUTEX.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/Kconfig.locks |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -225,7 +225,7 @@ config ARCH_SUPPORTS_ATOMIC_RMW
 
 config MUTEX_SPIN_ON_OWNER
 	def_bool y
-	depends on SMP && !DEBUG_MUTEXES && ARCH_SUPPORTS_ATOMIC_RMW
+	depends on SMP && ARCH_SUPPORTS_ATOMIC_RMW
 
 config RWSEM_SPIN_ON_OWNER
        def_bool y

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

#1468677

What's the motivation here? Is it just to unify counter and owner for
the starvation issue? If so, is this really the path we wanna take for
a small debug corner case?

I have not looked at the patches yet, but are there any performance minutia
to be aware of?

> 46 files changed, 160 insertions(+), 1298 deletions(-)

Oh my.

Thanks,
Davidlohr

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

#1468692

On Tue, 2016-08-23 at 09:17 -0700, Davidlohr Bueso wrote:
> What's the motivation here? Is it just to unify counter and owner for
> the starvation issue? If so, is this really the path we wanna take for
> a small debug corner case?

And we thought our other patch was a bit invasive  :-)

> I have not looked at the patches yet, but are there any performance minutia
> to be aware of?

This would remove all of the mutex architecture specific optimizations
in the (common) fastpath, so that is one thing that could reduce
performance. I'll run some benchmarks to see what some of the
performance impacts of these patches would be.

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

#1468718

On Tue, Aug 23, 2016 at 09:35:03AM -0700, Jason Low wrote:
> On Tue, 2016-08-23 at 09:17 -0700, Davidlohr Bueso wrote:
> > What's the motivation here? Is it just to unify counter and owner for
> > the starvation issue? If so, is this really the path we wanna take for
> > a small debug corner case?
> 
> And we thought our other patch was a bit invasive  :-)

So I've wanted to do something like this for a while now, and Linus
saying he wanted to always enable the spinning and basically reduce
special cases made me bite the bullet and just do it to see what it
would look like.

So it not only unifies counter and owner for the starvation case, it
does so to allow spinning and debug as well as lock handoff.
It collapses the whole count+owner+yield_to_owner into a single
variable.

It obviously is a tad invasive, but it does make things more similar to
rt-mutex and pi futex, both of which track the owner and pending in the
primary 'word'.

That said, I don't particularly like the new mutex_unlock() code, its
rather more heavy than I would like, although typically the word is
uncontended at unlock and we'd only need a single go at the
cmpxchg-loop.

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

#1468836

On Tue, Aug 23, 2016 at 03:36:17PM -0400, Waiman Long wrote:
> I think this is the right way to go. There isn't any big change in the
> slowpath, so the contended performance should be the same. The fastpath,
> however, will get a bit slower as a single atomic op plus a jump instruction
> (a single cacheline load) is replaced by a read-and-test and compxchg
> (potentially 2 cacheline loads) which will be somewhat slower than the
> optimized assembly code.

Yeah, I'll try and run some workloads tomorrow if you and Jason don't
beat me to it ;-)

> Alternatively, you can replace the
> __mutex_trylock() in mutex_lock() by just a blind cmpxchg to optimize the
> fastpath further. 

Problem with that is that we need to preserve the flag bits, so we need
the initial load.

Or were you thinking of: cmpxchg(&lock->owner, 0UL, (unsigned
long)current), which only works on uncontended locks?

> A cmpxhcg will still be a tiny bit slower than other
> atomic ops, but it will be more acceptable, I think.

I don't think cmpxchg is much slower than say xadd or xchg, the typical
problem with cmpxchg is the looping part, but single instruction costs
should be similar.

> BTW, I got the following compilation warning when I tried your patch:
> 
> drivers/gpu/drm/i915/i915_gem_shrinker.c: In function ‘mutex_is_locked_by’:
> drivers/gpu/drm/i915/i915_gem_shrinker.c:44:22: error: invalid operands to
> binary == (have ‘atomic_long_t’ and ‘struct task_struct *’)
> return mutex->owner == task;
> ^
> CC [M] drivers/gpu/drm/i915/intel_psr.o
> drivers/gpu/drm/i915/i915_gem_shrinker.c:49:1: warning: control reaches end
> of non-void function [-Wreturn-type]
> }
> ^
> make[4]: *** [drivers/gpu/drm/i915/i915_gem_shrinker.o] Error 1
> 
> Apparently, you may need to look to see if there are other direct access of
> the owner field in the other code.

AArggghh.. that is horrible horrible code.

It tries to do a recursive mutex and pokes at the innards of the mutex.
that so deserves to break.

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

#1469009

On Tue, 2016-08-23 at 09:35 -0700, Jason Low wrote:
> On Tue, 2016-08-23 at 09:17 -0700, Davidlohr Bueso wrote:
> > I have not looked at the patches yet, but are there any performance minutia
> > to be aware of?
> 
> This would remove all of the mutex architecture specific optimizations
> in the (common) fastpath, so that is one thing that could reduce
> performance. I'll run some benchmarks to see what some of the
> performance impacts of these patches would be.

I tested this patch on an 8 socket system with the high_systime AIM7
workload with diskfs. The patch provided big performance improvements in
terms of throughput in the highly contended cases.

-------------------------------------------------
|  users      | avg throughput | avg throughput |
              | without patch  | with patch     |
-------------------------------------------------
| 10 - 90     |   13,943 JPM   |   14,432 JPM   |
-------------------------------------------------
| 100 - 900   |   75,475 JPM   |  102,922 JPM   |
-------------------------------------------------
| 1000 - 1900 |   77,299 JPM   |  115,271 JPM   |
-------------------------------------------------

Unfortunately, at 2000 users, the modified kernel locked up.

# INFO: task reaim:<#> blocked for more than 120 seconds.

So something appears to be buggy.

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

#1470116

On Tue, Aug 23, 2016 at 06:13:43PM -0700, Jason Low wrote:

> I tested this patch on an 8 socket system with the high_systime AIM7
> workload with diskfs. The patch provided big performance improvements in
> terms of throughput in the highly contended cases.
> 
> -------------------------------------------------
> |  users      | avg throughput | avg throughput |
>               | without patch  | with patch     |
> -------------------------------------------------
> | 10 - 90     |   13,943 JPM   |   14,432 JPM   |
> -------------------------------------------------
> | 100 - 900   |   75,475 JPM   |  102,922 JPM   |
> -------------------------------------------------
> | 1000 - 1900 |   77,299 JPM   |  115,271 JPM   |
> -------------------------------------------------
> 
> Unfortunately, at 2000 users, the modified kernel locked up.
> 
> # INFO: task reaim:<#> blocked for more than 120 seconds.
> 
> So something appears to be buggy.

Right, so like said I think I found the reason for the lockup and Waiman
appears to have found the reason for your insane performance increase.

Running AIM7 takes ludicrous amounts of time though, so I hacked it up
like below.

That changes two things, it uses log10(rl->runnum) as scale factor and
allows overriding chld_alrm. I run it with -O60, which gets semi decent
runtimes.



---
diff --git a/osdl-aim-7/src/driver.c b/osdl-aim-7/src/driver.c
index 306e23b..03be655 100644
--- a/osdl-aim-7/src/driver.c
+++ b/osdl-aim-7/src/driver.c
@@ -98,6 +98,8 @@ struct runloop_input *rl_vars;
 struct disk_data *my_disk;
 struct _aimList *global_list;
 
+int alarm_timeout = 0;
+
 int flag = 0;
 /* for getopt */
 int opt_num = 0;
@@ -222,13 +224,14 @@ int main(int argc, char **argv)
 			{"config", 1, NULL, 'c'},
 			{"nosync", 0, NULL, 'y'},  /* Remove the sync'y behavior */
 			{"guesspeak", 0, NULL, 'g'}, /* terrible, but we've exhausted the alphabet */
+			{"timeout", 1, NULL, 'O'},
  			{0, 0, 0, 0}
 		};
 
-		c = getopt_long(argc, argv, "bvs:e:i:j:d::f:l:p:r:c:Z:z:mqothxyg",
+		c = getopt_long(argc, argv, "bvs:e:i:j:d::f:l:p:r:c:Z:z:O:mqothxyg",
 				long_options, &option_index);
 #elif hpux
-		c = getopt(argc, argv, "bvs:e:i:j:d::f:l:p:r:c:Z:z:mqothxyg");
+		c = getopt(argc, argv, "bvs:e:i:j:d::f:l:p:r:c:Z:z:O:mqothxyg");
 #endif
 
 		if (c == -1)
@@ -325,6 +328,9 @@ int main(int argc, char **argv)
 			print_usage();
 			exit(1);
 			break;
+		case 'O':
+			alarm_timeout = atoi(optarg);
+			break;
 /* MARCIA - DAN z: pass config file, Z: pass tool/script name (default perf_tools.sh) */
 		case 'Z':
 			tool_name = optarg;
@@ -909,7 +915,7 @@ int runloop(struct _aimList *tlist, struct runloop_input *rl)
 		long start_tick;
 		long delta = 0;
 		int chld_alrm = 0;
-
+		int timo;
 
 		close(umbilical[0]);
 		/* Step 1: seed random number generators
@@ -945,7 +951,15 @@ int runloop(struct _aimList *tlist, struct runloop_input *rl)
 			chld_alrm = 10;
 		}
 		/* now we set a timeout alarm */
-		alarm(rl->runnum * chld_alrm);
+
+		if (alarm_timeout > 0)
+			chld_alrm = alarm_timeout;
+
+		timo = (unsigned int)(log10((double)rl->runnum) * chld_alrm);
+
+		fprintf(stderr, "alarm: %d = log10(%d) * %d\n", timo, rl->runnum, chld_alrm);
+
+		alarm(timo);
 		/*
 		 * Step 4: Set up mechanism for random 
 		 * selection of directory for writes during tests

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

#1470267

On Thu, Aug 25, 2016 at 12:33:04PM -0400, Waiman Long wrote:
> On 08/25/2016 11:43 AM, Peter Zijlstra wrote:
> >On Tue, Aug 23, 2016 at 06:13:43PM -0700, Jason Low wrote:
> >>I tested this patch on an 8 socket system with the high_systime AIM7
> >>workload with diskfs. The patch provided big performance improvements in
> >>terms of throughput in the highly contended cases.
> >>
> >>-------------------------------------------------
> >>|  users      | avg throughput | avg throughput |
> >>               | without patch  | with patch     |
> >>-------------------------------------------------
> >>| 10 - 90     |   13,943 JPM   |   14,432 JPM   |
> >>-------------------------------------------------
> >>| 100 - 900   |   75,475 JPM   |  102,922 JPM   |
> >>-------------------------------------------------
> >>| 1000 - 1900 |   77,299 JPM   |  115,271 JPM   |
> >>-------------------------------------------------
> >>
> >>Unfortunately, at 2000 users, the modified kernel locked up.
> >>
> >># INFO: task reaim:<#>  blocked for more than 120 seconds.
> >>
> >>So something appears to be buggy.
> >So with the previously given changes to reaim, I get the below results
> >on my 4 socket Haswell with the new version of 1/3 (also below).
> >
> >I still need to update 3/3..
> >
> >Note that I think my reaim change wrecked the jobs/min calculation
> >somehow, as it keeps increasing. I do think however that the numbers are
> >comparable between runs, since they're wrecked the same way.
> 
> The performance data for the 2 kernels were roughly the same. This was what
> I had been expecting as there was no change in algorithm in how the slowpath
> was being handled. So I was surprised by Jason's result yesterday showing
> such a big difference.

Its because the mutex wasn't quite exclusive enough :-) If you let in
multiple owner, like with that race you found, you get big gains in
throughput ...

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

#1471235

* Peter Zijlstra <peterz@infradead.org> wrote:

> Its because the mutex wasn't quite exclusive enough :-) If you let in multiple 
> owner, like with that race you found, you get big gains in throughput ...

Btw., do we know which mutex that was?

That it didn't crash with a full AIM run suggests that whatever it is protecting 
it could probably be parallelized some more while still having a mostly working 
kernel! ;-)

Thanks,

	Ingo

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

#1470348

Hi, Peter,

Do you have a git tree branch for this patchset? We want to test it in
0day performance test.  That will make it a little easier.

Best Regards,
Huang, Ying

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

#1470364

On Thu, Aug 25, 2016 at 12:11:25PM -0700, huang ying wrote:
> Hi, Peter,
> 
> Do you have a git tree branch for this patchset? We want to test it in
> 0day performance test.  That will make it a little easier.

I just pushed it out to:

git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/rfc

But as stated, there's still a number of known issues, like lockdep
builds will not work.

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

#1468913

On Tue, Aug 23, 2016 at 02:53:07PM -0400, Linus Torvalds wrote:
> On Tue, Aug 23, 2016 at 12:17 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
> >
> >> 46 files changed, 160 insertions(+), 1298 deletions(-)
> >
> > Oh my.
> 
> Yeah, that looks like a pretty compelling argument right there, if
> there isn't any other really major downside to this...
> 
> Peter, is there some downside that isn't obvious? Like "Well, this
> does regress performance because it now always does X"?

The biggest difference is the mutex fast paths, where they were a single
atomic and branch they're now a bit bigger. How much that matters in
practise is something that we'll have to benchmark a bit.

Esp. the mutex_lock() fast-path now also needs to load current, which
at least should be fairly hot but can still be a number of dependent
loads on some archs.

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

#1469008

On Tue, Aug 23, 2016 at 12:17 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> 46 files changed, 160 insertions(+), 1298 deletions(-)
>
> Oh my.

Yeah, that looks like a pretty compelling argument right there, if
there isn't any other really major downside to this...

Peter, is there some downside that isn't obvious? Like "Well, this
does regress performance because it now always does X"?

             Linus

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

#1468826 — Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner

On Tue, Aug 23, 2016 at 04:17:54PM -0400, Waiman Long wrote:
> On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
> >  /*
> >   * Simple, straightforward mutexes with strict semantics:
> >@@ -48,13 +49,9 @@
> >   *   locks and tasks (and only those tasks)
> >   */
> >  struct mutex {
> >-	/* 1: unlocked, 0: locked, negative: locked, possible waiters */
> >-	atomic_t		count;
> >+	atomic_long_t		owner;
> >  	spinlock_t		wait_lock;
> >  	struct list_head	wait_list;
> >-#if defined(CONFIG_DEBUG_MUTEXES) || defined(CONFIG_MUTEX_SPIN_ON_OWNER)
> >-	struct task_struct	*owner;
> >-#endif
> >  #ifdef CONFIG_MUTEX_SPIN_ON_OWNER
> >  	struct optimistic_spin_queue osq; /* Spinner MCS lock */
> >  #endif
> 
> I think you should put the wait_lock and osq next to each other to save 8
> bytes in space on 64-bit machines.

Right you are.. didn't get around to looking at layout yet. Just barely
got it to compile and boot :-)

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

#1468848 — Re: [RFC][PATCH 1/3] locking/mutex: Rework mutex::owner

On Tue, 2016-08-23 at 15:55 -0400, Waiman Long wrote:
> On 08/23/2016 08:46 AM, Peter Zijlstra wrote:
> 
> I have 2 more comments about the code.
> 1) There are a couple of places where you only use 0x3 in mutex.c. They 
> should be replaced by the symbolic name instead.

May be easier to read if (owner & 0x3) and
(owner & ~0x3) are changed to something like 
_owner_flag(owner) and _owner_task(owner).

Tim

> 2) We should make __mutex_lock_slowpath() a noinline function just like 
> __mutex_lock_killable_slowpath() or __mutex_lock_interruptible_slowpath().
> 
> Cheers,
> Longman

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web