| Message ID | 1467049290-32359-3-git-send-email-xinhui.pan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote: > diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h > index 523673d..ae938ee 100644 > --- a/arch/powerpc/include/asm/spinlock.h > +++ b/arch/powerpc/include/asm/spinlock.h > @@ -52,6 +52,21 @@ > #define SYNC_IO > #endif > > +/* For fixing some spinning issues in a guest. > + * kernel would check if vcpu is preempted during a spin loop. > + * we support that. > + */ If you look around in that file you'll notice that the above comment style is inconsistent. Nor is the comment really clarifying things, for one you fail to mention the problem by its known name. You also forget to explain how this interface will help. How about something like this: /* * In order to deal with a various lock holder preemption issues provide * an interface to see if a vCPU is currently running or not. * * This allows us to terminate optimistic spin loops and block, * analogous to the native optimistic spin heuristic of testing if the * lock owner task is running or not. */ Also, since you now have a useful comment, which is not architecture specific, I would place it with the common vcpu_is_preempted() definition in sched.h. Hmm? > +#define arch_vcpu_is_preempted arch_vcpu_is_preempted > +static inline bool arch_vcpu_is_preempted(int cpu) > +{ > + struct lppaca *lp = &lppaca_of(cpu); > + > + if (unlikely(!(lppaca_shared_proc(lp) || > + lppaca_dedicated_proc(lp)))) > + return false; > + return !!(be32_to_cpu(lp->yield_count) & 1); > +} > + > static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock) > { > return lock.slock == 0; > -- > 2.4.11 >
Hi Xinhui, On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote: > This is to fix some holder preemption issues. Spinning at one > vcpu which is preempted is meaningless. > > Kernel need such interfaces, So lets support it. > > We also should suooprt both the shared and dedicated mode. > So add lppaca_dedicated_proc macro in lppaca.h > > Suggested-by: Boqun Feng <boqun.feng@gmail.com> > Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/lppaca.h | 6 ++++++ > arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h > index d0a2a2f..0a263d3 100644 > --- a/arch/powerpc/include/asm/lppaca.h > +++ b/arch/powerpc/include/asm/lppaca.h > @@ -111,12 +111,18 @@ extern struct lppaca lppaca[]; > * we will have to transition to something better. > */ > #define LPPACA_OLD_SHARED_PROC 2 > +#define LPPACA_OLD_DEDICATED_PROC (1 << 6) > I think you should describe a little bit about the magic number here, i.e. what document/specification says this should work, and how this works. > static inline bool lppaca_shared_proc(struct lppaca *l) > { > return !!(l->__old_status & LPPACA_OLD_SHARED_PROC); > } > > +static inline bool lppaca_dedicated_proc(struct lppaca *l) > +{ > + return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC); > +} > + > /* > * SLB shadow buffer structure as defined in the PAPR. The save_area > * contains adjacent ESID and VSID pairs for each shadowed SLB. The > diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h > index 523673d..ae938ee 100644 > --- a/arch/powerpc/include/asm/spinlock.h > +++ b/arch/powerpc/include/asm/spinlock.h > @@ -52,6 +52,21 @@ > #define SYNC_IO > #endif > > +/* For fixing some spinning issues in a guest. > + * kernel would check if vcpu is preempted during a spin loop. > + * we support that. > + */ > +#define arch_vcpu_is_preempted arch_vcpu_is_preempted > +static inline bool arch_vcpu_is_preempted(int cpu) This function should be guarded by #ifdef PPC_PSERIES .. #endif, right? Because if the kernel is not compiled with guest support, vcpu_is_preempted() should always be false, right? > +{ > + struct lppaca *lp = &lppaca_of(cpu); > + > + if (unlikely(!(lppaca_shared_proc(lp) || > + lppaca_dedicated_proc(lp)))) Do you want to detect whether we are running in a guest(ie. pseries kernel) here? Then I wonder whether "machine_is(pseries)" works here. Regards, Boqun > + return false; > + return !!(be32_to_cpu(lp->yield_count) & 1); > +} > + > static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock) > { > return lock.slock == 0; > -- > 2.4.11 >
On 2016年06月27日 22:17, Peter Zijlstra wrote: > On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote: >> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h >> index 523673d..ae938ee 100644 >> --- a/arch/powerpc/include/asm/spinlock.h >> +++ b/arch/powerpc/include/asm/spinlock.h >> @@ -52,6 +52,21 @@ >> #define SYNC_IO >> #endif >> >> +/* For fixing some spinning issues in a guest. >> + * kernel would check if vcpu is preempted during a spin loop. >> + * we support that. >> + */ > > If you look around in that file you'll notice that the above comment > style is inconsistent. > > Nor is the comment really clarifying things, for one you fail to mention > the problem by its known name. You also forget to explain how this > interface will help. How about something like this: > > /* > * In order to deal with a various lock holder preemption issues provide > * an interface to see if a vCPU is currently running or not. > * > * This allows us to terminate optimistic spin loops and block, > * analogous to the native optimistic spin heuristic of testing if the > * lock owner task is running or not. > */ thanks!!! > > Also, since you now have a useful comment, which is not architecture > specific, I would place it with the common vcpu_is_preempted() > definition in sched.h. > agree with you. Will do that. I will also add Suggested-by with you. thanks > Hmm? > >> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted >> +static inline bool arch_vcpu_is_preempted(int cpu) >> +{ >> + struct lppaca *lp = &lppaca_of(cpu); >> + >> + if (unlikely(!(lppaca_shared_proc(lp) || >> + lppaca_dedicated_proc(lp)))) >> + return false; >> + return !!(be32_to_cpu(lp->yield_count) & 1); >> +} >> + >> static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock) >> { >> return lock.slock == 0; >> -- >> 2.4.11 >> >
On 2016年06月27日 22:58, Boqun Feng wrote: > Hi Xinhui, > > On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote: >> This is to fix some holder preemption issues. Spinning at one >> vcpu which is preempted is meaningless. >> >> Kernel need such interfaces, So lets support it. >> >> We also should suooprt both the shared and dedicated mode. >> So add lppaca_dedicated_proc macro in lppaca.h >> >> Suggested-by: Boqun Feng <boqun.feng@gmail.com> >> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> >> --- >> arch/powerpc/include/asm/lppaca.h | 6 ++++++ >> arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h >> index d0a2a2f..0a263d3 100644 >> --- a/arch/powerpc/include/asm/lppaca.h >> +++ b/arch/powerpc/include/asm/lppaca.h >> @@ -111,12 +111,18 @@ extern struct lppaca lppaca[]; >> * we will have to transition to something better. >> */ >> #define LPPACA_OLD_SHARED_PROC 2 >> +#define LPPACA_OLD_DEDICATED_PROC (1 << 6) >> > > I think you should describe a little bit about the magic number here, right. > i.e. what document/specification says this should work, and how this > works. > yep, I need add some comments here. for example, this bit is firmware reserved... thanks, will do that. >> static inline bool lppaca_shared_proc(struct lppaca *l) >> { >> return !!(l->__old_status & LPPACA_OLD_SHARED_PROC); >> } >> >> +static inline bool lppaca_dedicated_proc(struct lppaca *l) >> +{ >> + return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC); >> +} >> + >> /* >> * SLB shadow buffer structure as defined in the PAPR. The save_area >> * contains adjacent ESID and VSID pairs for each shadowed SLB. The >> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h >> index 523673d..ae938ee 100644 >> --- a/arch/powerpc/include/asm/spinlock.h >> +++ b/arch/powerpc/include/asm/spinlock.h >> @@ -52,6 +52,21 @@ >> #define SYNC_IO >> #endif >> >> +/* For fixing some spinning issues in a guest. >> + * kernel would check if vcpu is preempted during a spin loop. >> + * we support that. >> + */ >> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted >> +static inline bool arch_vcpu_is_preempted(int cpu) > > This function should be guarded by #ifdef PPC_PSERIES .. #endif, right? > Because if the kernel is not compiled with guest support, > vcpu_is_preempted() should always be false, right? > oh, I forgot that. thanks for pointing it out. >> +{ >> + struct lppaca *lp = &lppaca_of(cpu); >> + >> + if (unlikely(!(lppaca_shared_proc(lp) || >> + lppaca_dedicated_proc(lp)))) > > Do you want to detect whether we are running in a guest(ie. pseries > kernel) here? Then I wonder whether "machine_is(pseries)" works here. > I tried as you said yesterday. but .h file has dependencies. As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img. > Regards, > Boqun > >> + return false; >> + return !!(be32_to_cpu(lp->yield_count) & 1); >> +} >> + >> static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock) >> { >> return lock.slock == 0; >> -- >> 2.4.11 >>
On Tue, Jun 28, 2016 at 11:39:18AM +0800, xinhui wrote: [snip] > > > +{ > > > + struct lppaca *lp = &lppaca_of(cpu); > > > + > > > + if (unlikely(!(lppaca_shared_proc(lp) || > > > + lppaca_dedicated_proc(lp)))) > > > > Do you want to detect whether we are running in a guest(ie. pseries > > kernel) here? Then I wonder whether "machine_is(pseries)" works here. > > > I tried as you said yesterday. but .h file has dependencies. > As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img. > I never said this it not a big problem ;-) The problem here is that we only need to detect the vcpu preemption in a guest, and there could be several ways we can detect whether the kernel is running in a guest. It's worthwhile to try find the best one for this. Besides, it's really better that you can make sure we are runing out of options before you introduce something like lppaca_dedicated_proc(). I have a feeling that yield_count is non-zero only if we are running in a guest, if so, we can use this and save several loads. But surely we need the confirmation from ppc maintainers. Regards, Boqun
On 2016年06月28日 13:03, Boqun Feng wrote: > On Tue, Jun 28, 2016 at 11:39:18AM +0800, xinhui wrote: > [snip] >>>> +{ >>>> + struct lppaca *lp = &lppaca_of(cpu); >>>> + >>>> + if (unlikely(!(lppaca_shared_proc(lp) || >>>> + lppaca_dedicated_proc(lp)))) >>> >>> Do you want to detect whether we are running in a guest(ie. pseries >>> kernel) here? Then I wonder whether "machine_is(pseries)" works here. >>> >> I tried as you said yesterday. but .h file has dependencies. >> As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img. >> > > I never said this it not a big problem ;-) > > The problem here is that we only need to detect the vcpu preemption in > a guest, and there could be several ways we can detect whether the > kernel is running in a guest. It's worthwhile to try find the best one > for this. Besides, it's really better that you can make sure we are > runing out of options before you introduce something like > lppaca_dedicated_proc(). > > I have a feeling that yield_count is non-zero only if we are running in > a guest, if so, we can use this and save several loads. But surely we > need the confirmation from ppc maintainers. > yes, on powernv, print the lppaca.yield_count and it is always zero. looks like only hypervisor and os can touch/modify it. > Regards, > Boqun >
diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h index d0a2a2f..0a263d3 100644 --- a/arch/powerpc/include/asm/lppaca.h +++ b/arch/powerpc/include/asm/lppaca.h @@ -111,12 +111,18 @@ extern struct lppaca lppaca[]; * we will have to transition to something better. */ #define LPPACA_OLD_SHARED_PROC 2 +#define LPPACA_OLD_DEDICATED_PROC (1 << 6) static inline bool lppaca_shared_proc(struct lppaca *l) { return !!(l->__old_status & LPPACA_OLD_SHARED_PROC); } +static inline bool lppaca_dedicated_proc(struct lppaca *l) +{ + return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC); +} + /* * SLB shadow buffer structure as defined in the PAPR. The save_area * contains adjacent ESID and VSID pairs for each shadowed SLB. The diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h index 523673d..ae938ee 100644 --- a/arch/powerpc/include/asm/spinlock.h +++ b/arch/powerpc/include/asm/spinlock.h @@ -52,6 +52,21 @@ #define SYNC_IO #endif +/* For fixing some spinning issues in a guest. + * kernel would check if vcpu is preempted during a spin loop. + * we support that. + */ +#define arch_vcpu_is_preempted arch_vcpu_is_preempted +static inline bool arch_vcpu_is_preempted(int cpu) +{ + struct lppaca *lp = &lppaca_of(cpu); + + if (unlikely(!(lppaca_shared_proc(lp) || + lppaca_dedicated_proc(lp)))) + return false; + return !!(be32_to_cpu(lp->yield_count) & 1); +} + static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock) { return lock.slock == 0;
This is to fix some holder preemption issues. Spinning at one vcpu which is preempted is meaningless. Kernel need such interfaces, So lets support it. We also should suooprt both the shared and dedicated mode. So add lppaca_dedicated_proc macro in lppaca.h Suggested-by: Boqun Feng <boqun.feng@gmail.com> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> --- arch/powerpc/include/asm/lppaca.h | 6 ++++++ arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++ 2 files changed, 21 insertions(+)