[PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Paul E. McKenney]

The definition of smp_mb__after_unlock_lock() is currently smp_mb()
for CONFIG_PPC and a no-op otherwise.  It would be better to instead
provide an architecture-selectable Kconfig option, and select the
strength of smp_mb__after_unlock_lock() based on that option.  This
commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
and bases the definition of smp_mb__after_unlock_lock() on this new
CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Boqun Feng <boqun.feng@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: <linuxppc-dev@lists.ozlabs.org>
---
 arch/Kconfig             | 3 +++
 arch/powerpc/Kconfig     | 1 +
 include/linux/rcupdate.h | 6 +++---
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index cd211a14a88f..adefaf344239 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -320,6 +320,9 @@ config HAVE_CMPXCHG_LOCAL
 config HAVE_CMPXCHG_DOUBLE
 	bool
 
+config ARCH_WEAK_RELEASE_ACQUIRE
+	bool
+
 config ARCH_WANT_IPC_PARSE_VERSION
 	bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 97a8bc8a095c..7a5c9b764cd2 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -99,6 +99,7 @@ config PPC
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if PPC64
 	select ARCH_WANT_IPC_PARSE_VERSION
+	select ARCH_WEAK_RELEASE_ACQUIRE
 	select BINFMT_ELF
 	select BUILDTIME_EXTABLE_SORT
 	select CLONE_BACKWARDS
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index de88b33c0974..e6146d0074f8 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1127,11 +1127,11 @@ do { \
  * if the UNLOCK and LOCK are executed by the same CPU or if the
  * UNLOCK and LOCK operate on the same lock variable.
  */
-#ifdef CONFIG_PPC
+#ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE
 #define smp_mb__after_unlock_lock()	smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
+#else /* #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
 #define smp_mb__after_unlock_lock()	do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELEASE_ACQUIRE */
 
 
 #endif /* __LINUX_RCUPDATE_H */
-- 
2.5.2

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Peter Zijlstra]

On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:

> CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Naming in the changelog and patch don't match.

> +config ARCH_WEAK_RELEASE_ACQUIRE
> +	bool
> +

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Peter Zijlstra]

On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> provide an architecture-selectable Kconfig option, and select the
> strength of smp_mb__after_unlock_lock() based on that option. 


Why is this better? Do we want to have more of this? I thought we still
wanted to convince PPC to go RCsc and eradicate all this RCpc 'fun'. But
instead now you're making it look like its OK to grow more of this pain.

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Paul E. McKenney]

On Thu, Apr 13, 2017 at 11:21:53AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> 
> > CONFIG_ARCH_WEAK_RELACQ Kconfig option.
> 
> Naming in the changelog and patch don't match.

Good eyes!  Fixed.

							Thanx, Paul

> > +config ARCH_WEAK_RELEASE_ACQUIRE
> > +	bool
> > +
> 
> 

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Paul E. McKenney]

On Thu, Apr 13, 2017 at 11:24:18AM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2017 at 10:39:47AM -0700, Paul E. McKenney wrote:
> > The definition of smp_mb__after_unlock_lock() is currently smp_mb()
> > for CONFIG_PPC and a no-op otherwise.  It would be better to instead
> > provide an architecture-selectable Kconfig option, and select the
> > strength of smp_mb__after_unlock_lock() based on that option. 
> 
> Why is this better? Do we want to have more of this? I thought we still
> wanted to convince PPC to go RCsc and eradicate all this RCpc 'fun'. But
> instead now you're making it look like its OK to grow more of this pain.

ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.

To see this, imagine some strange alternate universe in which the Power
hardware guys actually did decide to switch PPC to doing RCsc as you
suggest.  There would still be a lot of Power hardware out there that
still does RCpc.  Therefore, powerpc builds that needed to run on old
Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
built to run only on the shiny new (but mythical) alternate-universe
Power hardware would avoid selecting this Kconfig option.

But the real reason I queued this patch is that Ingo asked me for it:
https://lkml.org/lkml/2017/1/14/88

							Thanx, Paul

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Peter Zijlstra]

On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:

> ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> 
> To see this, imagine some strange alternate universe in which the Power
> hardware guys actually did decide to switch PPC to doing RCsc as you
> suggest.  There would still be a lot of Power hardware out there that
> still does RCpc.  Therefore, powerpc builds that needed to run on old
> Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> built to run only on the shiny new (but mythical) alternate-universe
> Power hardware would avoid selecting this Kconfig option.

Ah, but Power software guys could do it today by replacing an LWSYNC
with a SYNC in say arch_spin_unlock().

And yes, I know this isn't a popular suggestion, but it would do the
trick.

Its just that since there's one (PPC) we can sort of pressure them with
the pain of being the only ones to hit all the bugs. But the moment more
appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
does this) it will be ever so much harder to get rid of it.

Then again, maybe I should just give up and accept the Linux kernel has
RCpc locks..

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Paul E. McKenney]

On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
> 
> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> > 
> > To see this, imagine some strange alternate universe in which the Power
> > hardware guys actually did decide to switch PPC to doing RCsc as you
> > suggest.  There would still be a lot of Power hardware out there that
> > still does RCpc.  Therefore, powerpc builds that needed to run on old
> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> > built to run only on the shiny new (but mythical) alternate-universe
> > Power hardware would avoid selecting this Kconfig option.
> 
> Ah, but Power software guys could do it today by replacing an LWSYNC
> with a SYNC in say arch_spin_unlock().
> 
> And yes, I know this isn't a popular suggestion, but it would do the
> trick.

Indeed, there is a fine line between motivating people to move to new
hardware on the one hand and terminally annoying existing users on
the other.  ;-)

> Its just that since there's one (PPC) we can sort of pressure them with
> the pain of being the only ones to hit all the bugs. But the moment more
> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
> does this) it will be ever so much harder to get rid of it.
> 
> Then again, maybe I should just give up and accept the Linux kernel has
> RCpc locks..

As usual, I must defer to the powerpc maintainers on this one.

							Thanx, Paul

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Michael Ellerman]

"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:

> On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
>> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
>> 
>> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
>> > 
>> > To see this, imagine some strange alternate universe in which the Power
>> > hardware guys actually did decide to switch PPC to doing RCsc as you
>> > suggest.  There would still be a lot of Power hardware out there that
>> > still does RCpc.  Therefore, powerpc builds that needed to run on old
>> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
>> > built to run only on the shiny new (but mythical) alternate-universe
>> > Power hardware would avoid selecting this Kconfig option.
>> 
>> Ah, but Power software guys could do it today by replacing an LWSYNC
>> with a SYNC in say arch_spin_unlock().
>> 
>> And yes, I know this isn't a popular suggestion, but it would do the
>> trick.
>
> Indeed, there is a fine line between motivating people to move to new
> hardware on the one hand and terminally annoying existing users on
> the other.  ;-)
>
>> Its just that since there's one (PPC) we can sort of pressure them with
>> the pain of being the only ones to hit all the bugs. But the moment more
>> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
>> does this) it will be ever so much harder to get rid of it.
>> 
>> Then again, maybe I should just give up and accept the Linux kernel has
>> RCpc locks..
>
> As usual, I must defer to the powerpc maintainers on this one.

I reworked my locking tests a bit, to run longer, disable ASLR and a few
other things, and ran them again. They just bang repeatedly on an
uncontended lock, so nothing fancy at all.

Switching the release barrier to sync (from lwsync) slows it down by
about 18%.

So I think that pretty much rules it out, at least on current CPUs.

I'll try and get some more time to make sure I didn't do something
stupid in the test, and maybe do a version that includes some
contention.

cheers

Re: [PATCH tip/core/rcu 02/40] rcu: Make arch select smp_mb__after_unlock_lock() strength

[Paul E. McKenney]

On Wed, Apr 19, 2017 at 11:38:22PM +1000, Michael Ellerman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > On Thu, Apr 13, 2017 at 06:37:57PM +0200, Peter Zijlstra wrote:
> >> On Thu, Apr 13, 2017 at 09:26:51AM -0700, Paul E. McKenney wrote:
> >> 
> >> > ARCH_WEAK_RELEASE_ACQUIRE actually works both ways.
> >> > 
> >> > To see this, imagine some strange alternate universe in which the Power
> >> > hardware guys actually did decide to switch PPC to doing RCsc as you
> >> > suggest.  There would still be a lot of Power hardware out there that
> >> > still does RCpc.  Therefore, powerpc builds that needed to run on old
> >> > Power hardware would select ARCH_WEAK_RELEASE_ACQUIRE, while kernels
> >> > built to run only on the shiny new (but mythical) alternate-universe
> >> > Power hardware would avoid selecting this Kconfig option.
> >> 
> >> Ah, but Power software guys could do it today by replacing an LWSYNC
> >> with a SYNC in say arch_spin_unlock().
> >> 
> >> And yes, I know this isn't a popular suggestion, but it would do the
> >> trick.
> >
> > Indeed, there is a fine line between motivating people to move to new
> > hardware on the one hand and terminally annoying existing users on
> > the other.  ;-)
> >
> >> Its just that since there's one (PPC) we can sort of pressure them with
> >> the pain of being the only ones to hit all the bugs. But the moment more
> >> appear (and I'm afraid it'll be MIPS, with the excuse that PPC already
> >> does this) it will be ever so much harder to get rid of it.
> >> 
> >> Then again, maybe I should just give up and accept the Linux kernel has
> >> RCpc locks..
> >
> > As usual, I must defer to the powerpc maintainers on this one.
> 
> I reworked my locking tests a bit, to run longer, disable ASLR and a few
> other things, and ran them again. They just bang repeatedly on an
> uncontended lock, so nothing fancy at all.
> 
> Switching the release barrier to sync (from lwsync) slows it down by
> about 18%.

Ouch!!!

> So I think that pretty much rules it out, at least on current CPUs.
> 
> I'll try and get some more time to make sure I didn't do something
> stupid in the test, and maybe do a version that includes some
> contention.

Looking forward to seeing what you come up with...

							Thanx, Paul

powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Michael Ellerman]

"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:

> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index f2b3bd6c6bc2..0400e211aa44 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -24,25 +24,75 @@
...
> +/*
> + * Per-SRCU-domain structure, similar in function to rcu_state.
> + */
>  struct srcu_struct {
> -	unsigned long completed;
> -	unsigned long srcu_gp_seq;
> -	atomic_t srcu_exp_cnt;
> -	struct srcu_array __percpu *per_cpu_ref;
> -	spinlock_t queue_lock; /* protect ->srcu_cblist */
> -	struct rcu_segcblist srcu_cblist;
> +	struct srcu_node node[NUM_RCU_NODES];	/* Combining tree. */
> +	struct srcu_node *level[RCU_NUM_LVLS + 1];
> +						/* First node at each level. */
> +	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
> +	spinlock_t gp_lock;			/* protect ->srcu_cblist */
> +	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
> +	unsigned int srcu_idx;			/* Current rdr array element. */
> +	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
> +	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
> +	atomic_t srcu_exp_cnt;			/* # ongoing expedited GPs. */
> +	struct srcu_data __percpu *sda;		/* Per-CPU srcu_data array. */
> +	unsigned long srcu_barrier_seq;		/* srcu_barrier seq #. */
> +	struct mutex srcu_barrier_mutex;	/* Serialize barrier ops. */
> +	struct completion srcu_barrier_completion;
> +						/* Awaken barrier rq at end. */
> +	atomic_t srcu_barrier_cpu_cnt;		/* # CPUs not yet posting a */
> +						/*  callback for the barrier */
> +						/*  operation. */

This change seems to have had the non-obvious effect of breaking the
powerpc KVM build.

Because struct kvm contains two srcu_structs which are located
before the kvm_arch struct, the increase in size of srcu_struct has
caused the offset from the start of struct kvm to kvm_arch to be too big
for some of our asm.

struct kvm {
	spinlock_t mmu_lock;
	struct mutex slots_lock;
	struct mm_struct *mm; /* userspace tied to this vm */
	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
	struct srcu_struct srcu;
	struct srcu_struct irq_srcu;
        ...
	struct kvm_arch arch;


Example error:
  arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)

Where line 617 is:
  lwz	r7,KVM_LPID(r9)

And the KVM_LPID constant comes from asm-offsets.s. The diff of old vs
new is:

  -->KVM_LPID 17752 offsetof(struct kvm, arch.lpid)	 #
  +->KVM_LPID 45864 offsetof(struct kvm, arch.lpid)	 #


We can probably just fix it by changing the asm to keep the address of
kvm_arch in a register, and then offset from that.

cheers

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paul E. McKenney]

On Thu, Apr 20, 2017 at 01:40:13PM +1000, Michael Ellerman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> > index f2b3bd6c6bc2..0400e211aa44 100644
> > --- a/include/linux/srcutree.h
> > +++ b/include/linux/srcutree.h
> > @@ -24,25 +24,75 @@
> ...
> > +/*
> > + * Per-SRCU-domain structure, similar in function to rcu_state.
> > + */
> >  struct srcu_struct {
> > -	unsigned long completed;
> > -	unsigned long srcu_gp_seq;
> > -	atomic_t srcu_exp_cnt;
> > -	struct srcu_array __percpu *per_cpu_ref;
> > -	spinlock_t queue_lock; /* protect ->srcu_cblist */
> > -	struct rcu_segcblist srcu_cblist;
> > +	struct srcu_node node[NUM_RCU_NODES];	/* Combining tree. */
> > +	struct srcu_node *level[RCU_NUM_LVLS + 1];
> > +						/* First node at each level. */
> > +	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
> > +	spinlock_t gp_lock;			/* protect ->srcu_cblist */
> > +	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
> > +	unsigned int srcu_idx;			/* Current rdr array element. */
> > +	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
> > +	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
> > +	atomic_t srcu_exp_cnt;			/* # ongoing expedited GPs. */
> > +	struct srcu_data __percpu *sda;		/* Per-CPU srcu_data array. */
> > +	unsigned long srcu_barrier_seq;		/* srcu_barrier seq #. */
> > +	struct mutex srcu_barrier_mutex;	/* Serialize barrier ops. */
> > +	struct completion srcu_barrier_completion;
> > +						/* Awaken barrier rq at end. */
> > +	atomic_t srcu_barrier_cpu_cnt;		/* # CPUs not yet posting a */
> > +						/*  callback for the barrier */
> > +						/*  operation. */
> 
> This change seems to have had the non-obvious effect of breaking the
> powerpc KVM build.
> 
> Because struct kvm contains two srcu_structs which are located
> before the kvm_arch struct, the increase in size of srcu_struct has
> caused the offset from the start of struct kvm to kvm_arch to be too big
> for some of our asm.
> 
> struct kvm {
> 	spinlock_t mmu_lock;
> 	struct mutex slots_lock;
> 	struct mm_struct *mm; /* userspace tied to this vm */
> 	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> 	struct srcu_struct srcu;
> 	struct srcu_struct irq_srcu;
>         ...
> 	struct kvm_arch arch;
> 
> 
> Example error:
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)
> 
> Where line 617 is:
>   lwz	r7,KVM_LPID(r9)
> 
> And the KVM_LPID constant comes from asm-offsets.s. The diff of old vs
> new is:
> 
>   -->KVM_LPID 17752 offsetof(struct kvm, arch.lpid)	 #
>   +->KVM_LPID 45864 offsetof(struct kvm, arch.lpid)	 #
> 
> 
> We can probably just fix it by changing the asm to keep the address of
> kvm_arch in a register, and then offset from that.

Ouch!

What do you need from me?

							Thanx, Paul

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paolo Bonzini]

On 20/04/2017 05:40, Michael Ellerman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
>> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
>> index f2b3bd6c6bc2..0400e211aa44 100644
>> --- a/include/linux/srcutree.h
>> +++ b/include/linux/srcutree.h
>> @@ -24,25 +24,75 @@
> ...
>> +/*
>> + * Per-SRCU-domain structure, similar in function to rcu_state.
>> + */
>>  struct srcu_struct {
>> -	unsigned long completed;
>> -	unsigned long srcu_gp_seq;
>> -	atomic_t srcu_exp_cnt;
>> -	struct srcu_array __percpu *per_cpu_ref;
>> -	spinlock_t queue_lock; /* protect ->srcu_cblist */
>> -	struct rcu_segcblist srcu_cblist;
>> +	struct srcu_node node[NUM_RCU_NODES];	/* Combining tree. */
>> +	struct srcu_node *level[RCU_NUM_LVLS + 1];
>> +						/* First node at each level. */
>> +	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
>> +	spinlock_t gp_lock;			/* protect ->srcu_cblist */
>> +	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
>> +	unsigned int srcu_idx;			/* Current rdr array element. */
>> +	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
>> +	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
>> +	atomic_t srcu_exp_cnt;			/* # ongoing expedited GPs. */
>> +	struct srcu_data __percpu *sda;		/* Per-CPU srcu_data array. */
>> +	unsigned long srcu_barrier_seq;		/* srcu_barrier seq #. */
>> +	struct mutex srcu_barrier_mutex;	/* Serialize barrier ops. */
>> +	struct completion srcu_barrier_completion;
>> +						/* Awaken barrier rq at end. */
>> +	atomic_t srcu_barrier_cpu_cnt;		/* # CPUs not yet posting a */
>> +						/*  callback for the barrier */
>> +						/*  operation. */
> 
> This change seems to have had the non-obvious effect of breaking the
> powerpc KVM build.
> 
> Because struct kvm contains two srcu_structs which are located
> before the kvm_arch struct, the increase in size of srcu_struct has
> caused the offset from the start of struct kvm to kvm_arch to be too big
> for some of our asm.
> 
> struct kvm {
> 	spinlock_t mmu_lock;
> 	struct mutex slots_lock;
> 	struct mm_struct *mm; /* userspace tied to this vm */
> 	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> 	struct srcu_struct srcu;
> 	struct srcu_struct irq_srcu;
>         ...
> 	struct kvm_arch arch;
> 
> 
> Example error:
>   arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)
> 
> Where line 617 is:
>   lwz	r7,KVM_LPID(r9)
> 
> And the KVM_LPID constant comes from asm-offsets.s. The diff of old vs
> new is:
> 
>   -->KVM_LPID 17752 offsetof(struct kvm, arch.lpid)	 #
>   +->KVM_LPID 45864 offsetof(struct kvm, arch.lpid)	 #
> 
> 
> We can probably just fix it by changing the asm to keep the address of
> kvm_arch in a register, and then offset from that.

Michael, if you want to move the two srcu structs at the end of struct
kvm, that would be fine by me.  Please send a patch yourself so you can
test it on PPC.  Thanks,

Paolo

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paul E. McKenney]

On Thu, Apr 20, 2017 at 05:28:32PM +0200, Paolo Bonzini wrote:
> 
> 
> On 20/04/2017 05:40, Michael Ellerman wrote:
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> > 
> >> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> >> index f2b3bd6c6bc2..0400e211aa44 100644
> >> --- a/include/linux/srcutree.h
> >> +++ b/include/linux/srcutree.h
> >> @@ -24,25 +24,75 @@
> > ...
> >> +/*
> >> + * Per-SRCU-domain structure, similar in function to rcu_state.
> >> + */
> >>  struct srcu_struct {
> >> -	unsigned long completed;
> >> -	unsigned long srcu_gp_seq;
> >> -	atomic_t srcu_exp_cnt;
> >> -	struct srcu_array __percpu *per_cpu_ref;
> >> -	spinlock_t queue_lock; /* protect ->srcu_cblist */
> >> -	struct rcu_segcblist srcu_cblist;
> >> +	struct srcu_node node[NUM_RCU_NODES];	/* Combining tree. */
> >> +	struct srcu_node *level[RCU_NUM_LVLS + 1];
> >> +						/* First node at each level. */
> >> +	struct mutex srcu_cb_mutex;		/* Serialize CB preparation. */
> >> +	spinlock_t gp_lock;			/* protect ->srcu_cblist */
> >> +	struct mutex srcu_gp_mutex;		/* Serialize GP work. */
> >> +	unsigned int srcu_idx;			/* Current rdr array element. */
> >> +	unsigned long srcu_gp_seq;		/* Grace-period seq #. */
> >> +	unsigned long srcu_gp_seq_needed;	/* Latest gp_seq needed. */
> >> +	atomic_t srcu_exp_cnt;			/* # ongoing expedited GPs. */
> >> +	struct srcu_data __percpu *sda;		/* Per-CPU srcu_data array. */
> >> +	unsigned long srcu_barrier_seq;		/* srcu_barrier seq #. */
> >> +	struct mutex srcu_barrier_mutex;	/* Serialize barrier ops. */
> >> +	struct completion srcu_barrier_completion;
> >> +						/* Awaken barrier rq at end. */
> >> +	atomic_t srcu_barrier_cpu_cnt;		/* # CPUs not yet posting a */
> >> +						/*  callback for the barrier */
> >> +						/*  operation. */
> > 
> > This change seems to have had the non-obvious effect of breaking the
> > powerpc KVM build.
> > 
> > Because struct kvm contains two srcu_structs which are located
> > before the kvm_arch struct, the increase in size of srcu_struct has
> > caused the offset from the start of struct kvm to kvm_arch to be too big
> > for some of our asm.
> > 
> > struct kvm {
> > 	spinlock_t mmu_lock;
> > 	struct mutex slots_lock;
> > 	struct mm_struct *mm; /* userspace tied to this vm */
> > 	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> > 	struct srcu_struct srcu;
> > 	struct srcu_struct irq_srcu;
> >         ...
> > 	struct kvm_arch arch;
> > 
> > 
> > Example error:
> >   arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)
> > 
> > Where line 617 is:
> >   lwz	r7,KVM_LPID(r9)
> > 
> > And the KVM_LPID constant comes from asm-offsets.s. The diff of old vs
> > new is:
> > 
> >   -->KVM_LPID 17752 offsetof(struct kvm, arch.lpid)	 #
> >   +->KVM_LPID 45864 offsetof(struct kvm, arch.lpid)	 #
> > 
> > 
> > We can probably just fix it by changing the asm to keep the address of
> > kvm_arch in a register, and then offset from that.
> 
> Michael, if you want to move the two srcu structs at the end of struct
> kvm, that would be fine by me.  Please send a patch yourself so you can
> test it on PPC.  Thanks,

On the off-chance that it is at all helpful, I have added the commit
shown below to -rcu.  If it helps, I am happy to push this, but am
just as happy to drop it in favor of some other fix.  If at all possible,
I would like to get this into the upcoming merge window.

							Thanx, Paul

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Michael Ellerman]

"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> On Thu, Apr 20, 2017 at 05:28:32PM +0200, Paolo Bonzini wrote:
>> On 20/04/2017 05:40, Michael Ellerman wrote:
>> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
>> > 
>> > This change seems to have had the non-obvious effect of breaking the
>> > powerpc KVM build.
...
>> 
>> Michael, if you want to move the two srcu structs at the end of struct
>> kvm, that would be fine by me.  Please send a patch yourself so you can
>> test it on PPC.  Thanks,
>
> On the off-chance that it is at all helpful, I have added the commit
> shown below to -rcu.

As shown below :)

> If it helps, I am happy to push this, but am just as happy to drop it
> in favor of some other fix. If at all possible, I would like to get
> this into the upcoming merge window.

Thanks, this looks perfect to me, and if you're happy to put it on top
of your tree that would limit the breakage to a smaller history window,
so that would be ideal.

Tested-by: Michael Ellerman <mpe@ellerman.id.au>

cheers


>From a19a6617e2817e485ccc2f7cc5a97bd7ff769b87 Mon Sep 17 00:00:00 2001
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Thu, 20 Apr 2017 17:30:06 -0700
Subject: kvm: Move srcu_struct fields to end of struct kvm

Parallelizing SRCU callback handling increased the size of srcu_struct,
which moved the kvm_arch field within the kvm struct out of reach of
powerpc's current assembly code, resulting in the following sort of
build error:

arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)

This commit moves the srcu_struct fields in the kvm structure to follow
the kvm_arch field, which again allows powerpc's assembly code to
reach it.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Reported-by: Michael Ellerman <michaele@au1.ibm.com>
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/kvm_host.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2c14ad9..96c8e29 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -375,8 +375,6 @@ struct kvm {
 	struct mutex slots_lock;
 	struct mm_struct *mm; /* userspace tied to this vm */
 	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
-	struct srcu_struct srcu;
-	struct srcu_struct irq_srcu;
 	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
 
 	/*
@@ -429,6 +427,8 @@ struct kvm {
 	struct list_head devices;
 	struct dentry *debugfs_dentry;
 	struct kvm_stat_data **debugfs_stat_data;
+	struct srcu_struct srcu;
+	struct srcu_struct irq_srcu;
 };
 
 #define kvm_err(fmt, ...) \
-- 
cgit v1.1

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Michael Ellerman]

Paolo Bonzini <pbonzini@redhat.com> writes:
> Michael, if you want to move the two srcu structs at the end of struct
> kvm, that would be fine by me.  Please send a patch yourself so you can
> test it on PPC.  Thanks,

Paul beat me to it :)

I've confirmed it fixes the build break and the resulting kernel can
boot a KVM guest happily.

cheers

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paul E. McKenney]

On Fri, Apr 21, 2017 at 11:42:01AM +1000, Michael Ellerman wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> > On Thu, Apr 20, 2017 at 05:28:32PM +0200, Paolo Bonzini wrote:
> >> On 20/04/2017 05:40, Michael Ellerman wrote:
> >> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> >> > 
> >> > This change seems to have had the non-obvious effect of breaking the
> >> > powerpc KVM build.
> ...
> >> 
> >> Michael, if you want to move the two srcu structs at the end of struct
> >> kvm, that would be fine by me.  Please send a patch yourself so you can
> >> test it on PPC.  Thanks,
> >
> > On the off-chance that it is at all helpful, I have added the commit
> > shown below to -rcu.
> 
> As shown below :)
> 
> > If it helps, I am happy to push this, but am just as happy to drop it
> > in favor of some other fix. If at all possible, I would like to get
> > this into the upcoming merge window.
> 
> Thanks, this looks perfect to me, and if you're happy to put it on top
> of your tree that would limit the breakage to a smaller history window,
> so that would be ideal.
> 
> Tested-by: Michael Ellerman <mpe@ellerman.id.au>

Thank you, Michael!

Paolo, does this look good to you?  My upstream maintainer will be much
more likely to take this with your ack.  ;-)

							Thanx, Paul

> cheers
> 
> 
> >From a19a6617e2817e485ccc2f7cc5a97bd7ff769b87 Mon Sep 17 00:00:00 2001
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Thu, 20 Apr 2017 17:30:06 -0700
> Subject: kvm: Move srcu_struct fields to end of struct kvm
> 
> Parallelizing SRCU callback handling increased the size of srcu_struct,
> which moved the kvm_arch field within the kvm struct out of reach of
> powerpc's current assembly code, resulting in the following sort of
> build error:
> 
> arch/powerpc/kvm/book3s_hv_rmhandlers.S:617: Error: operand out of range (0x000000000000b328 is not between 0xffffffffffff8000 and 0x0000000000007fff)
> 
> This commit moves the srcu_struct fields in the kvm structure to follow
> the kvm_arch field, which again allows powerpc's assembly code to
> reach it.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Reported-by: Michael Ellerman <michaele@au1.ibm.com>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  include/linux/kvm_host.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2c14ad9..96c8e29 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -375,8 +375,6 @@ struct kvm {
>  	struct mutex slots_lock;
>  	struct mm_struct *mm; /* userspace tied to this vm */
>  	struct kvm_memslots *memslots[KVM_ADDRESS_SPACE_NUM];
> -	struct srcu_struct srcu;
> -	struct srcu_struct irq_srcu;
>  	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> 
>  	/*
> @@ -429,6 +427,8 @@ struct kvm {
>  	struct list_head devices;
>  	struct dentry *debugfs_dentry;
>  	struct kvm_stat_data **debugfs_stat_data;
> +	struct srcu_struct srcu;
> +	struct srcu_struct irq_srcu;
>  };
> 
>  #define kvm_err(fmt, ...) \
> -- 
> cgit v1.1

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paolo Bonzini]

On 21/04/2017 06:17, Paul E. McKenney wrote:
>> Thanks, this looks perfect to me, and if you're happy to put it on top
>> of your tree that would limit the breakage to a smaller history window,
>> so that would be ideal.
>>
>> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
> Thank you, Michael!
> 
> Paolo, does this look good to you?  My upstream maintainer will be much
> more likely to take this with your ack.  ;-)

Sure,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

since I don't expect conflicts between whoever applies this patch and
the KVM tree.

Paolo

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Paul E. McKenney]

On Fri, Apr 21, 2017 at 09:27:59AM +0200, Paolo Bonzini wrote:
> 
> 
> On 21/04/2017 06:17, Paul E. McKenney wrote:
> >> Thanks, this looks perfect to me, and if you're happy to put it on top
> >> of your tree that would limit the breakage to a smaller history window,
> >> so that would be ideal.
> >>
> >> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
> > Thank you, Michael!
> > 
> > Paolo, does this look good to you?  My upstream maintainer will be much
> > more likely to take this with your ack.  ;-)
> 
> Sure,
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> since I don't expect conflicts between whoever applies this patch and
> the KVM tree.

Applied, thank you!

							Thanx, Paul

Re: powerpc KVM build break in linux-next (was Re: [PATCH tip/core/rcu 40/40] srcu: Parallelize callback handling)

[Michael Ellerman]

"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> On Fri, Apr 21, 2017 at 09:27:59AM +0200, Paolo Bonzini wrote:
>> On 21/04/2017 06:17, Paul E. McKenney wrote:
>> >> Thanks, this looks perfect to me, and if you're happy to put it on top
>> >> of your tree that would limit the breakage to a smaller history window,
>> >> so that would be ideal.
>> >>
>> >> Tested-by: Michael Ellerman <mpe@ellerman.id.au>
>> > Thank you, Michael!
>> > 
>> > Paolo, does this look good to you?  My upstream maintainer will be much
>> > more likely to take this with your ack.  ;-)
>> 
>> Sure,
>> 
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> since I don't expect conflicts between whoever applies this patch and
>> the KVM tree.
>
> Applied, thank you!

Thanks all.

cheers