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

Groups > linux.kernel > #1320171 > unrolled thread

Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

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: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-28 01:40 +0100
    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-01-28 11:00 +0100
      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-28 23:40 +0100
        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-01-29 11:10 +0100
          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-29 23:40 +0100
            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-01 15:00 +0100
              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 05:00 +0100
                Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 06:20 +0100
                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 07:50 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 09:10 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 09:20 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 10:40 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 18:40 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 19:00 +0100
                              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 19:10 +0100
                                Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 20:40 +0100
                                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 21:00 +0100
                                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 20:20 +0100
                                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ingo Molnar <mingo@kernel.org> - 2016-02-03 09:40 +0100
                                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 14:40 +0100
                                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-03 20:10 +0100
                                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ingo Molnar <mingo@kernel.org> - 2016-02-09 12:30 +0100
                                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-09 12:50 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 13:10 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-02 19:00 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 23:40 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ralf Baechle <ralf@linux-mips.org> - 2016-02-02 15:50 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Måns Rullgård <mans@mansr.com> - 2016-02-02 16:00 +0100
                            Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Ralf Baechle <ralf@linux-mips.org> - 2016-02-02 16:00 +0100
                              Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Måns Rullgård <mans@mansr.com> - 2016-02-02 17:00 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Peter Zijlstra <peterz@infradead.org> - 2016-02-02 18:30 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 23:40 +0100
                  Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 12:50 +0100
                    Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 13:20 +0100
                      Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 13:30 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Boqun Feng <boqun.feng@gmail.com> - 2016-02-02 14:20 +0100
                        Re: [RFC][PATCH] mips: Fix arch_spin_unlock() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-02 18:20 +0100
                          Re: [RFC][PATCH] mips: Fix arch_spin_unlock() Will Deacon <will.deacon@arm.com> - 2016-02-02 18:40 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1325871

On Wed, Feb 03, 2016 at 01:32:10PM +0000, Will Deacon wrote:
> On Wed, Feb 03, 2016 at 09:33:39AM +0100, Ingo Molnar wrote:
> > In fact I'd suggest to test this via a quick runtime hack like this in rcupdate.h:
> > 
> > 	extern int panic_timeout;
> > 
> > 	...
> > 
> > 	if (panic_timeout)
> > 		smp_load_acquire(p);
> > 	else
> > 		typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p);
> > 
> > (or so)
> 
> So the problem with this is that a LOAD <ctrl> LOAD sequence isn't an
> ordering hazard on ARM, so you're potentially at the mercy of the branch
> predictor as to whether you get an acquire. That's not to say it won't
> be discarded as soon as the conditional is resolved, but it could
> screw up the benchmarking.
> 
> I'd be better off doing some runtime patching, but that's not something
> I can knock up in a couple of minutes (so I'll add it to my list).

... so I actually got that up and running, believe it or not. Filthy stuff.

The good news is that you're right, and I'm now seeing ~1% difference
between the runs with ~0.3% noise for either of them. I still think
that's significant, but it's a lot more reassuring than 4%.

Will

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

#1330110

* Will Deacon <will.deacon@arm.com> wrote:

> On Wed, Feb 03, 2016 at 01:32:10PM +0000, Will Deacon wrote:
> > On Wed, Feb 03, 2016 at 09:33:39AM +0100, Ingo Molnar wrote:
> > > In fact I'd suggest to test this via a quick runtime hack like this in rcupdate.h:
> > > 
> > > 	extern int panic_timeout;
> > > 
> > > 	...
> > > 
> > > 	if (panic_timeout)
> > > 		smp_load_acquire(p);
> > > 	else
> > > 		typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p);
> > > 
> > > (or so)
> > 
> > So the problem with this is that a LOAD <ctrl> LOAD sequence isn't an
> > ordering hazard on ARM, so you're potentially at the mercy of the branch
> > predictor as to whether you get an acquire. That's not to say it won't
> > be discarded as soon as the conditional is resolved, but it could
> > screw up the benchmarking.
> > 
> > I'd be better off doing some runtime patching, but that's not something
> > I can knock up in a couple of minutes (so I'll add it to my list).
> 
> ... so I actually got that up and running, believe it or not. Filthy stuff.

Wow!

I tried to implement the simpler solution by hacking rcupdate.h, but got drowned 
in nasty circular header file dependencies and gave up...

If you are not overly embarrassed by posting hacky patches, mind posting your 
solution?

> The good news is that you're right, and I'm now seeing ~1% difference between 
> the runs with ~0.3% noise for either of them. I still think that's significant, 
> but it's a lot more reassuring than 4%.

hm, so for such marginal effects I think we could improve the testing method a 
bit: we could improve 'perf bench sched messaging' to allow 'steady state 
testing': to not exit+restart all the processes between test iterations, but to 
continuously measure and print out current performance figures.

I.e. every 10 seconds it could print a decaying running average of current 
throughput.

That way you could patch/unpatch the instructions without having to restart the 
tasks. If you still see an effect (in the numbers reported every 10 seconds), then 
that's a guaranteed result.

[ We have such functionality in 'perf bench numa' (the --show-convergence option), 
  for similar reasons, to allow runtime monitoring and tweaking of kernel 
  parameters. ]

Thanks,

	Ingo

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

#1330128

On Tue, Feb 09, 2016 at 12:23:58PM +0100, Ingo Molnar wrote:
> * Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Feb 03, 2016 at 01:32:10PM +0000, Will Deacon wrote:
> > > On Wed, Feb 03, 2016 at 09:33:39AM +0100, Ingo Molnar wrote:
> > > > In fact I'd suggest to test this via a quick runtime hack like this in rcupdate.h:
> > > > 
> > > > 	extern int panic_timeout;
> > > > 
> > > > 	...
> > > > 
> > > > 	if (panic_timeout)
> > > > 		smp_load_acquire(p);
> > > > 	else
> > > > 		typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p);
> > > > 
> > > > (or so)
> > > 
> > > So the problem with this is that a LOAD <ctrl> LOAD sequence isn't an
> > > ordering hazard on ARM, so you're potentially at the mercy of the branch
> > > predictor as to whether you get an acquire. That's not to say it won't
> > > be discarded as soon as the conditional is resolved, but it could
> > > screw up the benchmarking.
> > > 
> > > I'd be better off doing some runtime patching, but that's not something
> > > I can knock up in a couple of minutes (so I'll add it to my list).
> > 
> > ... so I actually got that up and running, believe it or not. Filthy stuff.
> 
> Wow!
> 
> I tried to implement the simpler solution by hacking rcupdate.h, but got drowned 
> in nasty circular header file dependencies and gave up...

I hacked linux/compiler.h, but got similar problems and ended up copying
what I need under a new namespace (posh way of saying I added _WILL to
everything until it built).

> If you are not overly embarrassed by posting hacky patches, mind posting your 
> solution?

Ok, since you asked! I'm thoroughly ashamed of the hacks, but maybe it
will keep those spammy Microsoft recruiters at bay ;)

Note that the "trigger" is changing the loglevel, but you need to do this
by echoing to /proc/sysrq-trigger, otherwise the whole thing gets invoked
in irq context which ends badly. It's also heavily arm64-specific.

> > The good news is that you're right, and I'm now seeing ~1% difference between 
> > the runs with ~0.3% noise for either of them. I still think that's significant, 
> > but it's a lot more reassuring than 4%.
> 
> hm, so for such marginal effects I think we could improve the testing method a 
> bit: we could improve 'perf bench sched messaging' to allow 'steady state 
> testing': to not exit+restart all the processes between test iterations, but to 
> continuously measure and print out current performance figures.
> 
> I.e. every 10 seconds it could print a decaying running average of current 
> throughput.
> 
> That way you could patch/unpatch the instructions without having to restart the 
> tasks. If you still see an effect (in the numbers reported every 10 seconds), then 
> that's a guaranteed result.
> 
> [ We have such functionality in 'perf bench numa' (the --show-convergence option), 
>   for similar reasons, to allow runtime monitoring and tweaking of kernel 
>   parameters. ]

That sounds handy.

Will

--->8

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 8f271b83f910..1a1e353983be 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -30,8 +30,8 @@
 #define ARM64_HAS_LSE_ATOMICS			5
 #define ARM64_WORKAROUND_CAVIUM_23154		6
 #define ARM64_WORKAROUND_834220			7
-
-#define ARM64_NCAPS				8
+#define ARM64_RCU_USES_ACQUIRE			8
+#define ARM64_NCAPS				9
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index d2ee1b21a10d..edbf188a8541 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -143,6 +143,66 @@ static int __apply_alternatives_multi_stop(void *unused)
 	return 0;
 }
 
+static void __apply_alternatives_rcu(void *alt_region)
+{
+	struct alt_instr *alt;
+	struct alt_region *region = alt_region;
+	u32 *origptr, *replptr;
+
+	for (alt = region->begin; alt < region->end; alt++) {
+		u32 insn, orig_insn;
+		int nr_inst;
+
+		if (alt->cpufeature != ARM64_RCU_USES_ACQUIRE)
+			continue;
+
+		BUG_ON(alt->alt_len != alt->orig_len);
+
+		origptr = ALT_ORIG_PTR(alt);
+		replptr = ALT_REPL_PTR(alt);
+		nr_inst = alt->alt_len / sizeof(insn);
+
+		BUG_ON(nr_inst != 1);
+
+		insn = le32_to_cpu(*replptr);
+		orig_insn = le32_to_cpu(*origptr);
+		*(origptr) = cpu_to_le32(insn);
+		*replptr = cpu_to_le32(orig_insn);
+
+		flush_icache_range((uintptr_t)origptr, (uintptr_t)(origptr + 1));
+		pr_info_ratelimited("%p: 0x%x => 0x%x\n", origptr, orig_insn, insn);
+	}
+}
+
+static int will_patched;
+
+static int __apply_alternatives_rcu_multi_stop(void *unused)
+{
+	struct alt_region region = {
+		.begin	= __alt_instructions,
+		.end	= __alt_instructions_end,
+	};
+
+	/* We always have a CPU 0 at this point (__init) */
+	if (smp_processor_id()) {
+		while (!READ_ONCE(will_patched))
+			cpu_relax();
+		isb();
+	} else {
+		__apply_alternatives_rcu(&region);
+		/* Barriers provided by the cache flushing */
+		WRITE_ONCE(will_patched, 1);
+	}
+
+	return 0;
+}
+
+void apply_alternatives_rcu(void)
+{
+	will_patched = 0;
+	stop_machine(__apply_alternatives_rcu_multi_stop, NULL, cpu_online_mask);
+}
+
 void __init apply_alternatives_all(void)
 {
 	/* better not try code patching on a live SMP system */
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index e3928f578891..98c132c3b018 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -141,7 +141,10 @@ SECTIONS
 
 	PERCPU_SECTION(L1_CACHE_BYTES)
 
-	. = ALIGN(4);
+	__init_end = .;
+	. = ALIGN(PAGE_SIZE);
+
+
 	.altinstructions : {
 		__alt_instructions = .;
 		*(.altinstructions)
@@ -151,8 +154,7 @@ SECTIONS
 		*(.altinstr_replacement)
 	}
 
-	. = ALIGN(PAGE_SIZE);
-	__init_end = .;
+	. = ALIGN(4);
 
 	_data = .;
 	_sdata = .;
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index e5139402e7f8..3eb7193fcf88 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -80,6 +80,7 @@ static int __init sysrq_always_enabled_setup(char *str)
 
 __setup("sysrq_always_enabled", sysrq_always_enabled_setup);
 
+extern void apply_alternatives_rcu(void);
 
 static void sysrq_handle_loglevel(int key)
 {
@@ -89,6 +90,7 @@ static void sysrq_handle_loglevel(int key)
 	console_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
 	pr_info("Loglevel set to %d\n", i);
 	console_loglevel = i;
+	apply_alternatives_rcu();
 }
 static struct sysrq_key_op sysrq_loglevel_op = {
 	.handler	= sysrq_handle_loglevel,
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 00b042c49ccd..75e29d61fedd 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -218,6 +218,61 @@ void __read_once_size(const volatile void *p, void *res, int size)
 	__READ_ONCE_SIZE;
 }
 
+extern void panic(const char *fmt, ...);
+
+#define __stringify_1_will(x...)	#x
+#define __stringify_will(x...)	__stringify_1_will(x)
+
+#define ALTINSTR_ENTRY_WILL(feature)						      \
+	" .word 661b - .\n"				/* label           */ \
+	" .word 663f - .\n"				/* new instruction */ \
+	" .hword " __stringify_will(feature) "\n"		/* feature bit     */ \
+	" .byte 662b-661b\n"				/* source len      */ \
+	" .byte 664f-663f\n"				/* replacement len */
+
+#define __ALTERNATIVE_CFG_WILL(oldinstr, newinstr, feature, cfg_enabled)	\
+	".if "__stringify_will(cfg_enabled)" == 1\n"				\
+	"661:\n\t"							\
+	oldinstr "\n"							\
+	"662:\n"							\
+	".pushsection .altinstructions,\"a\"\n"				\
+	ALTINSTR_ENTRY_WILL(feature)						\
+	".popsection\n"							\
+	".pushsection .altinstr_replacement, \"a\"\n"			\
+	"663:\n\t"							\
+	newinstr "\n"							\
+	"664:\n\t"							\
+	".popsection\n\t"						\
+	".org	. - (664b-663b) + (662b-661b)\n\t"			\
+	".org	. - (662b-661b) + (664b-663b)\n"			\
+	".endif\n"
+
+#define _ALTERNATIVE_CFG_WILL(oldinstr, newinstr, feature, cfg, ...)	\
+	__ALTERNATIVE_CFG_WILL(oldinstr, newinstr, feature, 1)
+
+#define ALTERNATIVE_WILL(oldinstr, newinstr, ...)   \
+	_ALTERNATIVE_CFG_WILL(oldinstr, newinstr, __VA_ARGS__, 1)
+
+#define __READ_ONCE_SIZE_WILL						\
+({									\
+	__u64 tmp;							\
+									\
+	switch (size) {							\
+	case 8: asm volatile(						\
+		ALTERNATIVE_WILL("ldr %0, %1", "ldar %0, %1", 8)	\
+		: "=r" (tmp) : "Q" (*(volatile __u64 *)p));		\
+		*(__u64 *)res = tmp; break;				\
+	default:							\
+		panic("that's no pointer, yo");				\
+	}								\
+})
+
+static __always_inline
+void __read_once_size_will(const volatile void *p, void *res, int size)
+{
+	__READ_ONCE_SIZE_WILL;
+}
+
 #ifdef CONFIG_KASAN
 /*
  * This function is not 'inline' because __no_sanitize_address confilcts
@@ -537,10 +592,17 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  * object's lifetime is managed by something other than RCU.  That
  * "something other" might be reference counting or simple immortality.
  */
+
+#define READ_ONCE_WILL(x)						\
+({									\
+	union { typeof(x) __val; char __c[1]; } __u;			\
+	__read_once_size_will(&(x), __u.__c, sizeof(x));		\
+	__u.__val;							\
+})
+
 #define lockless_dereference(p) \
 ({ \
-	typeof(p) _________p1 = READ_ONCE(p); \
-	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
+	typeof(p) _________p1 = READ_ONCE_WILL(p); \
 	(_________p1); \
 })
 

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

#1324022

On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 12:07 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > So we *absolutely* should say that *OF COURSE* these things work:
> >
> >  - CPU A:
> >
> >    .. initialize data structure -> smp_wmb() -> WRITE_ONCE(ptr);
> >
> >  - CPU B:
> >
> >    smp_load_acquire(ptr)  - we can rely on things behind "ptr" being initialized
> 
> That's a bad example, btw. I shouldn't have made it be a "pointer",
> because then we get the whole address dependency chain ordering
> anyway.
> 
> So instead of "ptr", read "state flag". It might just be an "int" that
> says "data has been initialized".
> 
> So
> 
>     .. initialize memory ..
>     smp_wmb();
>     WRITE_ONCE(&is_initialized, 1);
> 
> should pair with
> 
>     if (smp_load_acquire(&is_initialized))
>         ... we can read and write the data, knowing it has been initialized ..
> 
> exactly because "smp_wmb()" (cheap write barrier) might be cheaper
> than "smp_store_release()" (expensive full barrier) and thus
> preferred.
> 
> So mixing ordering metaphors actually does make sense, and should be
> entirely well-defined.

I don't believe that anyone is arguing that this particular example
should not work the way that you want it to.

> There's likely less reason to do it the other way (ie
> "smp_store_release()" on one side pairing with "LOAD_ONCE() +
> smp_rmb()" on the other) since there likely isn't the same kind of
> performance reason for that pairing. But even if we would never
> necessarily want to do it, I think our memory ordering rules would be
> *much* better for strongly stating that it has to work, than being
> timid and trying to make the rules weak.
> 
> Memory ordering is confusing enough as it is. We should not make
> people worry more than they already have to. Strong rules are good.

The sorts of things I am really worried about are abominations like this
(and far worse):

	void thread0(void)
	{
		r1 = smp_load_acquire(&a);
		smp_store_release(&b, 1);
	}

	void thread1(void)
	{
		r2 = smp_load_acquire(&b);
		smp_store_release(&c, 1);
	}

	void thread2(void)
	{
		WRITE_ONCE(c, 2);
		smp_mb();
		r3 = READ_ONCE(d);
	}

	void thread3(void)
	{
		WRITE_ONCE(d, 1);
		smp_store_release(&a, 1);
	}

	r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???

I advise discouraging this sort of thing.  But it is your kernel, so
what is your preference?

							Thanx, Paul

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

#1324339

On Tue, Feb 2, 2016 at 4:02 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> The sorts of things I am really worried about are abominations like this
> (and far worse):

That one doesn't have any causal chain that I can see, so I agree that
it's an abomination, but it also doesn't act as an argument.

>  r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???

What do you see as the problem here? The above can happen in a
strictly ordered situation: thread2 runs first (c == 2, r3 = 0), then
thread3 runs (d = 1, a = 1) then thread0 runs (r1 = 1) and then
thread1 starts running but the store to c doesn't complete (now r2 =
1).

So there's no reason for your case to not happen, but the real issue
is that there is no causal relationship that your example describes,
so it's not even interesting.

Causality breaking is what really screws with peoples minds. The
reason transitivity is important (and why smp_read_barrier_depends()
is so annoying) is because causal breaks make peoples minds twist in
bad ways.

Sadly, memory orderings are very seldom described as honoring
causality, and instead people have the crazy litmus tests.

                 Linus

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

#1324696

On Tue, Feb 02, 2016 at 09:56:14AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 4:02 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > The sorts of things I am really worried about are abominations like this
> > (and far worse):
> 
> That one doesn't have any causal chain that I can see, so I agree that
> it's an abomination, but it also doesn't act as an argument.
> 
> >  r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???
> 
> What do you see as the problem here? The above can happen in a
> strictly ordered situation: thread2 runs first (c == 2, r3 = 0), then
> thread3 runs (d = 1, a = 1) then thread0 runs (r1 = 1) and then
> thread1 starts running but the store to c doesn't complete (now r2 =
> 1).

Apologies, I should have added that the condition does not get evaluated
until all the dust settles.  At that point both stores to c would have
completed, so that c == 1.

> So there's no reason for your case to not happen, but the real issue
> is that there is no causal relationship that your example describes,
> so it's not even interesting.

Because of the write-to-write relationship between thread1() and
thread2(), yes.  And I am very glad that you find this one uninteresting,
because including it would make things -really- complicated.

> Causality breaking is what really screws with peoples minds. The
> reason transitivity is important (and why smp_read_barrier_depends()
> is so annoying) is because causal breaks make peoples minds twist in
> bad ways.

Agreed, which means it is very important that the various flavors of
release-acquire chains be transitive.

In addition, smp_mb() is transitive, as are synchronize_rcu() and friends.

> Sadly, memory orderings are very seldom described as honoring
> causality, and instead people have the crazy litmus tests.

Indeed, a memory model defined solely by litmus tests would qualify as
an exotic form of torture.  What we do instead is use sets of litmus
tests as test cases for the prototype memory model under consideration.
It is all too easy to create a set of rules that look good and sound
good, but which mess something up.  The litmus tests help catch these
sorts of errors.

							Thanx, Paul

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

#1324140

On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:

> Memory ordering is confusing enough as it is. We should not make
> people worry more than they already have to. Strong rules are good.

Confusing and the resulting bugs can be very hard to debug.

One of the problems I've experienced is that Linux does support liberal
memory ordering, even as extreme as the Alpha.  And every once in a
while a hardware guy is asking me if Linux their preferred variant of
weak ordering and answering honestly I have to say yes.  Even advising
against it in strong words against it, guess what will happen - another
weakly ordered core implementation.

To this point we've only fixed theoretical memory ordering issues on MIPS
but it's only a matter of time until we get bitten by a bug that does
actual damage.

SGI and a few others have managed to build large systems with significant
memory latencies yet managed to get decent performance with strong
ordering.

  Ralf

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

#1324172

Ralf Baechle <ralf@linux-mips.org> writes:

> On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:
>
>> Memory ordering is confusing enough as it is. We should not make
>> people worry more than they already have to. Strong rules are good.
>
> Confusing and the resulting bugs can be very hard to debug.
>
> One of the problems I've experienced is that Linux does support liberal
> memory ordering, even as extreme as the Alpha.

We don't really have much choice given the reality of existing hardware.

-- 
Måns Rullgård

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

#1324174

On Tue, Feb 02, 2016 at 02:54:06PM +0000, Måns Rullgård wrote:

> We don't really have much choice given the reality of existing hardware.

No, of course not - but I want us discourage new weakly ordered
platforms as much as possible.

  Ralf

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

#1324218

Ralf Baechle <ralf@linux-mips.org> writes:

> On Tue, Feb 02, 2016 at 02:54:06PM +0000, Måns Rullgård wrote:
>
>> We don't really have much choice given the reality of existing hardware.
>
> No, of course not - but I want us discourage new weakly ordered
> platforms as much as possible.

Where do you draw the line though?  You can't exactly go and impose some
kind of global ordering in a multi-master system.

-- 
Måns Rullgård

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

#1324307

On 2 February 2016 07:44:33 CET, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

>
>> If so, do we also need to take the following pairing into
>consideration?
>> 
>> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>


We rely on this with smp_cond_aquire() to extend the chain.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1324698

On Tue, Feb 02, 2016 at 06:23:37PM +0100, Peter Zijlstra wrote:
> 
> 
> On 2 February 2016 07:44:33 CET, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> >
> >> If so, do we also need to take the following pairing into
> >consideration?
> >> 
> >> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> 
> 
> We rely on this with smp_cond_aquire() to extend the chain.

We do indeed!  And it is not possible to use smp_load_acquire() given
this API becauseof the "!".  Hmmm...

If this one turns out to be problematic, there are some ways of dealing
with it.  But thank you for reminding me of it.  I think, anyway.  ;-)

							Thanx, Paul

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

#1324003

On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > Locally transitive chain termination:
> > > 
> > > (i.e. these can't be used to extend a chain)
> > 
> > Agreed.
> > 
> > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > o	smp_store_release() -> READ_ONCE(); if
> 
> Just want to make sure, this one is actually:
> 
> o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> 
> right? Because control dependency only orders READ->WRITE.
> 
> If so, do we also need to take the following pairing into consideration?
> 
> o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> 
> > 
> > I am OK with the first and last, but I believe that the middle one
> > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > case needs to be locally transitive.
> > 
> 
> Hmm... I don't think we should differ rcu_dereference() and
> lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> lockless_dereference() right now, which means we used to think
> rcu_dereference() and lockless_dereference() are interchangeable, right?
> 
> Besides, Will, what's the reason of having a locally transitive chain
> termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> not be locally transitive?

Well, the following ISA2 test is permitted on ARM:


P0:
Wx=1
WyRel=1	// rcu_assign_pointer

P1:
Ry=1	// rcu_dereference
WzRel=1 // rcu_assign_pointer

P2:
Rz=1	// rcu_dereference
<addr>
Rx=0


Make one of the rcu_dereferences an ACQUIRE and the behaviour is
forbidden.

Paul: what use-cases did you have in mind?

Will

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

#1324029

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > Locally transitive chain termination:
> > > > 
> > > > (i.e. these can't be used to extend a chain)
> > > 
> > > Agreed.
> > > 
> > > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > > o	smp_store_release() -> READ_ONCE(); if
> > 
> > Just want to make sure, this one is actually:
> > 
> > o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> > 
> > right? Because control dependency only orders READ->WRITE.
> > 
> > If so, do we also need to take the following pairing into consideration?
> > 
> > o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> > 
> > > 
> > > I am OK with the first and last, but I believe that the middle one
> > > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > > case needs to be locally transitive.
> > > 
> > 
> > Hmm... I don't think we should differ rcu_dereference() and
> > lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> > lockless_dereference() right now, which means we used to think
> > rcu_dereference() and lockless_dereference() are interchangeable, right?
> > 
> > Besides, Will, what's the reason of having a locally transitive chain
> > termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> > not be locally transitive?
> 
> Well, the following ISA2 test is permitted on ARM:
> 
> 
> P0:
> Wx=1
> WyRel=1	// rcu_assign_pointer
> 
> P1:
> Ry=1	// rcu_dereference

What if a <addr> dependency is added here? Same result?

> WzRel=1 // rcu_assign_pointer
> 
> P2:
> Rz=1	// rcu_dereference
> <addr>
> Rx=0
> 
> 
> Make one of the rcu_dereferences an ACQUIRE and the behaviour is
> forbidden.
> 
> Paul: what use-cases did you have in mind?
> 
> Will

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

#1324034

On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > > On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > > > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > > Locally transitive chain termination:
> > > > > 
> > > > > (i.e. these can't be used to extend a chain)
> > > > 
> > > > Agreed.
> > > > 
> > > > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > > > o	smp_store_release() -> READ_ONCE(); if
> > > 
> > > Just want to make sure, this one is actually:
> > > 
> > > o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> > > 
> > > right? Because control dependency only orders READ->WRITE.
> > > 
> > > If so, do we also need to take the following pairing into consideration?
> > > 
> > > o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> > > 
> > > > 
> > > > I am OK with the first and last, but I believe that the middle one
> > > > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > > > case needs to be locally transitive.
> > > > 
> > > 
> > > Hmm... I don't think we should differ rcu_dereference() and
> > > lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> > > lockless_dereference() right now, which means we used to think
> > > rcu_dereference() and lockless_dereference() are interchangeable, right?
> > > 
> > > Besides, Will, what's the reason of having a locally transitive chain
> > > termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> > > not be locally transitive?
> > 
> > Well, the following ISA2 test is permitted on ARM:
> > 
> > 
> > P0:
> > Wx=1
> > WyRel=1	// rcu_assign_pointer
> > 
> > P1:
> > Ry=1	// rcu_dereference
> 
> What if a <addr> dependency is added here? Same result?

Right, that fixes it. So if we're only considering things like:

  rcu_dereference
  <addr>
  RELEASE

then local transitivity should be preserved.

I think the same applies to <ctrl>, which seems to match your later
example.

Will

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

#1324070

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
[...]
> > > > 
> > > > Besides, Will, what's the reason of having a locally transitive chain
> > > > termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> > > > not be locally transitive?
> > > 
> > > Well, the following ISA2 test is permitted on ARM:
> > > 
> > > 
> > > P0:
> > > Wx=1
> > > WyRel=1	// rcu_assign_pointer
> > > 
> > > P1:
> > > Ry=1	// rcu_dereference
> > 
> > What if a <addr> dependency is added here? Same result?
> 
> Right, that fixes it. So if we're only considering things like:
> 
>   rcu_dereference
>   <addr>
>   RELEASE
> 
> then local transitivity should be preserved.
> 
> I think the same applies to <ctrl>, which seems to match your later
> example.
> 

Thank you ;-)

Now I understand why you want thoses pairings to be locally transitive
chain terminations, they have more subtle requirements to extend locally
transitive chains and slight different behaviors on different
architectures. It's better for us to put them aside until we figure out
thoses subtle requirements and different behaviors.

Regards,
Boqun

> Will

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

#1324302

On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
> On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> > On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > > On Tue, Feb 02, 2016 at 01:19:04PM +0800, Boqun Feng wrote:
> > > > On Mon, Feb 01, 2016 at 07:54:58PM -0800, Paul E. McKenney wrote:
> > > > > On Mon, Feb 01, 2016 at 01:56:22PM +0000, Will Deacon wrote:
> > > > > > On Fri, Jan 29, 2016 at 02:22:53AM -0800, Paul E. McKenney wrote:
> > > > > > > On Fri, Jan 29, 2016 at 09:59:59AM +0000, Will Deacon wrote:
> > > > > > Locally transitive chain termination:
> > > > > > 
> > > > > > (i.e. these can't be used to extend a chain)
> > > > > 
> > > > > Agreed.
> > > > > 
> > > > > > > o	smp_store_release() -> lockless_dereference() (???)
> > > > > > > o	rcu_assign_pointer() -> rcu_dereference()
> > > > > > > o	smp_store_release() -> READ_ONCE(); if
> > > > 
> > > > Just want to make sure, this one is actually:
> > > > 
> > > > o	smp_store_release() -> READ_ONCE(); if ;<WRITE_ONCE()>
> > > > 
> > > > right? Because control dependency only orders READ->WRITE.
> > > > 
> > > > If so, do we also need to take the following pairing into consideration?
> > > > 
> > > > o	smp_store_release() -> READ_ONCE(); if ;smp_rmb(); <ACCESS_ONCE()>
> > > > 
> > > > > 
> > > > > I am OK with the first and last, but I believe that the middle one
> > > > > has real use cases.  So the rcu_assign_pointer() -> rcu_dereference()
> > > > > case needs to be locally transitive.
> > > > > 
> > > > 
> > > > Hmm... I don't think we should differ rcu_dereference() and
> > > > lockless_dereference(). One reason: list_for_each_entry_rcu() are using
> > > > lockless_dereference() right now, which means we used to think
> > > > rcu_dereference() and lockless_dereference() are interchangeable, right?
> > > > 
> > > > Besides, Will, what's the reason of having a locally transitive chain
> > > > termination? Because on some architectures RELEASE->DEPENDENCY pairs may
> > > > not be locally transitive?
> > > 
> > > Well, the following ISA2 test is permitted on ARM:
> > > 
> > > 
> > > P0:
> > > Wx=1
> > > WyRel=1	// rcu_assign_pointer
> > > 
> > > P1:
> > > Ry=1	// rcu_dereference
> > 
> > What if a <addr> dependency is added here? Same result?
> 
> Right, that fixes it. So if we're only considering things like:
> 
>   rcu_dereference
>   <addr>
>   RELEASE
> 
> then local transitivity should be preserved.

Whew!!!  ;-)

> I think the same applies to <ctrl>, which seems to match your later
> example.

Could you please check?

							Thanx, Paul

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

#1324326

On Tue, Feb 02, 2016 at 09:12:37AM -0800, Paul E. McKenney wrote:
> On Tue, Feb 02, 2016 at 12:20:25PM +0000, Will Deacon wrote:
> > On Tue, Feb 02, 2016 at 08:12:30PM +0800, Boqun Feng wrote:
> > > On Tue, Feb 02, 2016 at 11:45:59AM +0000, Will Deacon wrote:
> > > > Well, the following ISA2 test is permitted on ARM:
> > > > 
> > > > 
> > > > P0:
> > > > Wx=1
> > > > WyRel=1	// rcu_assign_pointer
> > > > 
> > > > P1:
> > > > Ry=1	// rcu_dereference
> > > 
> > > What if a <addr> dependency is added here? Same result?
> > 
> > Right, that fixes it. So if we're only considering things like:
> > 
> >   rcu_dereference
> >   <addr>
> >   RELEASE
> > 
> > then local transitivity should be preserved.
> 
> Whew!!!  ;-)
> 
> > I think the same applies to <ctrl>, which seems to match your later
> > example.
> 
> Could you please check?

I should've been more concrete here: it does indeed apply if you replace
the <addr> with a <ctrl> in the snippet above, I just hadn't got Boqun's
example paged in and didn't want to commit on the examples being identical.

Will

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web