Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1431127 > unrolled thread
| Started by | Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> |
|---|---|
| First post | 2016-06-25 15:50 +0200 |
| Last post | 2016-06-27 12:30 +0200 |
| Articles | 11 on this page of 31 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] locking/osq: Drop the overload of osq lock Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-25 15:50 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 16:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-25 17:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 18:10 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 18:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-25 19:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 21:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 07:00 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-27 10:00 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 12:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-25 18:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 20:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 18:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-25 18:50 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-25 19:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 21:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-25 21:40 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-26 04:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 07:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-26 08:10 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 09:00 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-26 16:10 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 18:00 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-26 09:00 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 09:10 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-26 16:30 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock panxinhui <xinhui@linux.vnet.ibm.com> - 2016-06-26 17:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-27 08:50 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 09:40 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Peter Zijlstra <peterz@infradead.org> - 2016-06-27 10:20 +0200
Re: [PATCH] locking/osq: Drop the overload of osq lock Boqun Feng <boqun.feng@gmail.com> - 2016-06-27 12:30 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | panxinhui <xinhui@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-26 09:00 +0200 |
| Message-ID | <rOccV-7Pw-3@gated-at.bofh.it> |
| In reply to | #1431345 |
> 在 2016年6月26日，14:10，Boqun Feng <boqun.feng@gmail.com> 写道：
>
> On Sun, Jun 26, 2016 at 01:21:04PM +0800, panxinhui wrote:
>>
>>> 在 2016年6月26日，03:20，Peter Zijlstra <peterz@infradead.org> 写道：
>>>
>>> On Sun, Jun 26, 2016 at 01:27:56AM +0800, panxinhui wrote:
>>>>>> Would that not have issues where the owner cpu is kept running but the
>>>>>> spinner (ie. _this_ vcpu) gets preempted? I would think that in that
>>>>>> case we too want to stop spinning.
>>>>>>
>>>>>
>>>> do you mean that the spinner detect itself had yield out during the
>>>> big spin loop?
>>>>
>>>> It is very possible to happen. BUT if spinner(on this vcpu) yield
>>>> out, the next spinner would break the spin loop. AND if spinner
>>>> detect itself yield out once, it’s very possible to get the osq lock
>>>> soon as long as the ower vcpu is running.
>>>>
>>>> SO I think we need just check the owner vcpu’s yield_count.
>>>
>>> I had a quick look at KVM and it looks like it only has
>>> kvm_cpu::preempted, which would suggest the interface boqun proposed.
>>>
>>> We'll have to look at many of the other virt platforms as well to see
>>> what they can do.
>>>
>>> We could also provide _both_ interfaces and a platform can implement
>>> whichever variant (or both) it can.
>>>
>> the kvm code on ppc has implemented yield_count.
>> It let me feel a little relaxed. :)
>>
>> looks like we could introduce the interface like below.
>>
>> bool vcpu_is_preempted(int cpu)
>> {
>> return arch_vcpu_is_preempted(cpu);
>> }
>>
>> #ifdef arch_vcpu_has_yield_count
>> bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
>> {
>> return arch_get_vcpu_yield_count() != yield_count;
>> }
>>
>> #else
>> bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
>> {
>> /*just let called know it is preepmpted*/
>> return vcpu_is_preempted(cpu);
>> }
>> #endif
>>
>
> Unfortunately, on PPC, we compile pseries code along with powernv code
> in a kernel binary, therefore we need to wire the proper primitives at
> runtime.
>
no, we can use same vcpu preempted check ops actually.
just like how arch_spin_lock() does.
if (SHARED_PROCESSOR)
__spin_yield(lock);
so we can
u32 arch_get_vcpu_yield_count(int cpu)
{
if (SHARED_PROCESSOR)
return be32_to_cpu(lppaca_of(cpu).yield_count;
return 0;
}
and all these things can be don inline then.
> I've cooked the following patch, please note I haven't test this, this
> is just a POC.
>
> Regards,
> Boqun
>
> ---------------->8
> virt: Introduce vcpu preemption detection functions
>
> Lock holder preemption is an serious problem in virtualized environment
> for locking. Different archs and hypervisors employ different ways to
> try to solve the problem in different locking mechanisms. And recently
> Pan Xinhui found a significant performance drop in osq_lock(), which
> could be solved by providing a mechanism to detect the preemption of
> vcpu.
>
> Therefore this patch introduces several vcpu preemption detection
> primitives, which allows locking primtives to save the time of
> unnecesarry spinning. These primitives act as an abstract layer between
> locking functions and arch or hypervisor related code.
>
> There are two sets of primitives:
>
> 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
> pairwisely in a same preempt disable critical section. And they
> could detect whether a vcpu preemption happens between them.
>
> 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
> preempted.
>
> This patch also implements those primitives on pseries and wire them up.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> arch/powerpc/platforms/pseries/Kconfig | 1 +
> arch/powerpc/platforms/pseries/setup.c | 28 +++++++++++
> include/linux/vcpu_preempt.h | 90 ++++++++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 include/linux/vcpu_preempt.h
>
> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
> index bec90fb30425..7d24c3e48878 100644
> --- a/arch/powerpc/platforms/pseries/Kconfig
> +++ b/arch/powerpc/platforms/pseries/Kconfig
> @@ -21,6 +21,7 @@ config PPC_PSERIES
> select HOTPLUG_CPU if SMP
> select ARCH_RANDOM
> select PPC_DOORBELL
> + select HAS_VCPU_PREEMPTION_DETECTION
> default y
>
it is already too many config there…
as I comment above, we could make these things inline, so looks like we needn't to add such config.
> config PPC_SPLPAR
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 9883bc7ea007..5d4aed54e039 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -42,6 +42,7 @@
> #include <linux/of.h>
> #include <linux/of_pci.h>
> #include <linux/kexec.h>
> +#include <linux/vcpu_preempt.h>
>
> #include <asm/mmu.h>
> #include <asm/processor.h>
> @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
> of_pci_check_probe_only();
> }
>
> +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
> +EXPORT_SYMBOL(vcpu_preempt_ops);
> +
> +static long pseries_vcpu_preempt_count(void)
> +{
> + return be32_to_cpu(get_lppaca()->yield_count);
> +}
> +
> +static bool pseries_vcpu_is_preempted(int cpu)
> +{
> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
> +}
> +
> +static bool pseries_vcpu_has_preempted(long vpc)
> +{
> + return pseries_vcpu_preempt_count() != vpc;
> +}
> +
> +static void __init pseries_setup_vcpu_preempt_ops(void)
> +{
> + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
> + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
> + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
> +}
> +
> static void __init pSeries_setup_arch(void)
> {
> set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
> @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
> "%ld\n", rc);
> }
> }
> +
> + pseries_setup_vcpu_preempt_ops();
> }
>
> static int __init pSeries_init_panel(void)
> diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
> new file mode 100644
> index 000000000000..4a88414bb83f
> --- /dev/null
> +++ b/include/linux/vcpu_preempt.h
How about add them in sched.h
I just think they are similar. correct me if I got a mistake :)
thanks
xinhui
> @@ -0,0 +1,90 @@
> +/*
> + * Primitives for checking the vcpu preemption from the guest.
> + */
> +
> +static long __vcpu_preempt_count(void)
> +{
> + return 0;
> +}
> +
> +static bool __vcpu_has_preempted(long vpc)
> +{
> + return false;
> +}
> +
> +static bool __vcpu_is_preempted(int cpu)
> +{
> + return false;
> +}
> +
> +struct vcpu_preempt_ops {
> + /*
> + * Get the current vcpu's "preempt count", which is going to use for
> + * checking whether the current vcpu has ever been preempted
> + */
> + long (*preempt_count)(void);
> +
> + /*
> + * Return whether a vcpu is preempted
> + */
> + bool (*is_preempted)(int cpu);
> +
> + /*
> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
> + * happened after the .preempt_count() was called.
> + */
> + bool (*has_preempted)(long vpc);
> +};
> +
> +struct vcpu_preempt_ops vcpu_preempt_ops;
> +
> +/* Default boilerplate */
> +#define DEFAULT_VCPU_PREEMPT_OPS \
> + { \
> + .preempt_count = __vcpu_preempt_count, \
> + .is_preempted = __vcpu_is_preempted, \
> + .has_preempted = __vcpu_has_preempted \
> + }
> +
> +#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
> +/*
> + * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
> + *
> + * The vpc is used for checking whether the current vcpu has ever been
> + * preempted via vcpu_has_preempted().
> + *
> + * This function and vcpu_has_preepmted() should be called in the same
> + * preemption disabled critical section.
> + */
> +static long vcpu_preempt_count(void)
> +{
> + return vcpu_preempt_ops.preempt_count();
> +}
> +
> +/*
> + * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
> + */
> +static bool vcpu_is_preempted(int cpu)
> +{
> + return vcpu_preempt_ops.is_preempted(cpu);
> +}
> +
> +/*
> + * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
> + * preempted.
> + *
> + * The checked duration is between the vcpu_preempt_count() which returns @vpc
> + * is called and this function called.
> + *
> + * This function and corresponding vcpu_preempt_count() should be in the same
> + * preemption disabled cirtial section.
> + */
> +static bool vcpu_has_preempted(long vpc)
> +{
> + return vcpu_preempt_ops.has_preempt(vpc);
> +}
> +#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
> +#define vcpu_preempt_count() __vcpu_preempt_count()
> +#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
> +#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
> +#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
> --
> 2.9.0
>
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2016-06-26 16:10 +0200 |
| Message-ID | <rOiV3-3WD-3@gated-at.bofh.it> |
| In reply to | #1431351 |
[Multipart message — attachments visible in raw view] — view raw
On Sun, Jun 26, 2016 at 02:58:00PM +0800, panxinhui wrote:
[snip]
> >>
> >
> > Unfortunately, on PPC, we compile pseries code along with powernv code
> > in a kernel binary, therefore we need to wire the proper primitives at
> > runtime.
> >
> no, we can use same vcpu preempted check ops actually.
> just like how arch_spin_lock() does.
>
> if (SHARED_PROCESSOR)
> __spin_yield(lock);
>
> so we can
> u32 arch_get_vcpu_yield_count(int cpu)
> {
> if (SHARED_PROCESSOR)
SHARED_PROCESSOR is true only if a PPC_PSERIES kernel runs in shared
processor mode. Using this here will rule out the situation where a
PPC_PSERIES kernel(ppc guest) runs in dedicated processor mode. I don't
think we can rule out the dedicated processor mode, because even in the
dedicated mode, the lock holder may still be preempted. So why? Why do
we only want the yield_count in the shared processor mode?
> return be32_to_cpu(lppaca_of(cpu).yield_count;
> return 0;
> }
>
> and all these things can be don inline then.
>
> > I've cooked the following patch, please note I haven't test this, this
> > is just a POC.
> >
> > Regards,
> > Boqun
> >
> > ---------------->8
> > virt: Introduce vcpu preemption detection functions
> >
> > Lock holder preemption is an serious problem in virtualized environment
> > for locking. Different archs and hypervisors employ different ways to
> > try to solve the problem in different locking mechanisms. And recently
> > Pan Xinhui found a significant performance drop in osq_lock(), which
> > could be solved by providing a mechanism to detect the preemption of
> > vcpu.
> >
> > Therefore this patch introduces several vcpu preemption detection
> > primitives, which allows locking primtives to save the time of
> > unnecesarry spinning. These primitives act as an abstract layer between
> > locking functions and arch or hypervisor related code.
> >
> > There are two sets of primitives:
> >
> > 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
> > pairwisely in a same preempt disable critical section. And they
> > could detect whether a vcpu preemption happens between them.
> >
> > 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
> > preempted.
> >
> > This patch also implements those primitives on pseries and wire them up.
> >
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> > arch/powerpc/platforms/pseries/Kconfig | 1 +
> > arch/powerpc/platforms/pseries/setup.c | 28 +++++++++++
> > include/linux/vcpu_preempt.h | 90 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 119 insertions(+)
> > create mode 100644 include/linux/vcpu_preempt.h
> >
> > diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
> > index bec90fb30425..7d24c3e48878 100644
> > --- a/arch/powerpc/platforms/pseries/Kconfig
> > +++ b/arch/powerpc/platforms/pseries/Kconfig
> > @@ -21,6 +21,7 @@ config PPC_PSERIES
> > select HOTPLUG_CPU if SMP
> > select ARCH_RANDOM
> > select PPC_DOORBELL
> > + select HAS_VCPU_PREEMPTION_DETECTION
> > default y
> >
> it is already too many config there…
Why is this a problem? It's a Kconfig file, there are not too many
readability issues here. It simply serve as a database for the
relationships between different config options. menuconfig and its
friends can do their job well. BTW, have you read config X86? ;-)
> as I comment above, we could make these things inline, so looks like we needn't to add such config.
>
Another reason, I didn't make those primitives arch-specific inline
functions, is that they are actually not only arch-specific but also
(virtual-)environment-specific, or hypervisor-specific, that is their
implementations differ when the kernel runs on different hypervisors(KVM
and Xen, for example).
If you make the implementation part inline functions, you end up
handling different virtual environment every time the functions are
called, which is a waste of time. We are lucky because phyp and kvm
share the same interface of yield count, but this might not be true for
other archs and other hypervisors.
> > config PPC_SPLPAR
> > diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> > index 9883bc7ea007..5d4aed54e039 100644
> > --- a/arch/powerpc/platforms/pseries/setup.c
> > +++ b/arch/powerpc/platforms/pseries/setup.c
> > @@ -42,6 +42,7 @@
> > #include <linux/of.h>
> > #include <linux/of_pci.h>
> > #include <linux/kexec.h>
> > +#include <linux/vcpu_preempt.h>
> >
> > #include <asm/mmu.h>
> > #include <asm/processor.h>
> > @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
> > of_pci_check_probe_only();
> > }
> >
> > +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
> > +EXPORT_SYMBOL(vcpu_preempt_ops);
> > +
> > +static long pseries_vcpu_preempt_count(void)
> > +{
> > + return be32_to_cpu(get_lppaca()->yield_count);
> > +}
> > +
> > +static bool pseries_vcpu_is_preempted(int cpu)
> > +{
> > + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
> > +}
> > +
> > +static bool pseries_vcpu_has_preempted(long vpc)
> > +{
> > + return pseries_vcpu_preempt_count() != vpc;
> > +}
> > +
> > +static void __init pseries_setup_vcpu_preempt_ops(void)
> > +{
> > + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
> > + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
> > + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
> > +}
> > +
> > static void __init pSeries_setup_arch(void)
> > {
> > set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
> > @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
> > "%ld\n", rc);
> > }
> > }
> > +
> > + pseries_setup_vcpu_preempt_ops();
> > }
> >
> > static int __init pSeries_init_panel(void)
> > diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
> > new file mode 100644
> > index 000000000000..4a88414bb83f
> > --- /dev/null
> > +++ b/include/linux/vcpu_preempt.h
>
> How about add them in sched.h
I don't have a strong opinion on which header file to use. TBH, I was
trying to use some header files for parvirt stuff, but failed to find
one, so I add a new one because I don't want people confused between
preemption of vcpus and preemption of tasks. But once again, I don't
have a strong opinion ;-)
> I just think they are similar. correct me if I got a mistake :)
>
> thanks
> xinhui
>
> > @@ -0,0 +1,90 @@
> > +/*
> > + * Primitives for checking the vcpu preemption from the guest.
> > + */
> > +
> > +static long __vcpu_preempt_count(void)
> > +{
> > + return 0;
> > +}
> > +
> > +static bool __vcpu_has_preempted(long vpc)
> > +{
> > + return false;
> > +}
> > +
> > +static bool __vcpu_is_preempted(int cpu)
> > +{
> > + return false;
> > +}
> > +
> > +struct vcpu_preempt_ops {
> > + /*
> > + * Get the current vcpu's "preempt count", which is going to use for
> > + * checking whether the current vcpu has ever been preempted
> > + */
> > + long (*preempt_count)(void);
> > +
> > + /*
> > + * Return whether a vcpu is preempted
> > + */
> > + bool (*is_preempted)(int cpu);
> > +
> > + /*
> > + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
> > + * happened after the .preempt_count() was called.
> > + */
> > + bool (*has_preempted)(long vpc);
> > +};
> > +
> > +struct vcpu_preempt_ops vcpu_preempt_ops;
> > +
> > +/* Default boilerplate */
> > +#define DEFAULT_VCPU_PREEMPT_OPS \
> > + { \
> > + .preempt_count = __vcpu_preempt_count, \
> > + .is_preempted = __vcpu_is_preempted, \
> > + .has_preempted = __vcpu_has_preempted \
> > + }
> > +
> > +#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
> > +/*
> > + * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
> > + *
> > + * The vpc is used for checking whether the current vcpu has ever been
> > + * preempted via vcpu_has_preempted().
> > + *
> > + * This function and vcpu_has_preepmted() should be called in the same
> > + * preemption disabled critical section.
> > + */
> > +static long vcpu_preempt_count(void)
> > +{
> > + return vcpu_preempt_ops.preempt_count();
> > +}
> > +
> > +/*
> > + * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
> > + */
> > +static bool vcpu_is_preempted(int cpu)
> > +{
> > + return vcpu_preempt_ops.is_preempted(cpu);
> > +}
> > +
> > +/*
> > + * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
> > + * preempted.
> > + *
> > + * The checked duration is between the vcpu_preempt_count() which returns @vpc
> > + * is called and this function called.
> > + *
> > + * This function and corresponding vcpu_preempt_count() should be in the same
> > + * preemption disabled cirtial section.
> > + */
> > +static bool vcpu_has_preempted(long vpc)
> > +{
> > + return vcpu_preempt_ops.has_preempt(vpc);
> > +}
> > +#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
> > +#define vcpu_preempt_count() __vcpu_preempt_count()
> > +#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
> > +#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
> > +#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
> > --
> > 2.9.0
> >
>
[toc] | [prev] | [next] | [standalone]
| From | panxinhui <xinhui@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-26 18:00 +0200 |
| Message-ID | <rOkDv-4M6-1@gated-at.bofh.it> |
| In reply to | #1431511 |
> 在 2016年6月26日，22:11，Boqun Feng <boqun.feng@gmail.com> 写道：
>
> On Sun, Jun 26, 2016 at 02:58:00PM +0800, panxinhui wrote:
> [snip]
>>>>
>>>
>>> Unfortunately, on PPC, we compile pseries code along with powernv code
>>> in a kernel binary, therefore we need to wire the proper primitives at
>>> runtime.
>>>
>> no, we can use same vcpu preempted check ops actually.
>> just like how arch_spin_lock() does.
>>
>> if (SHARED_PROCESSOR)
>> __spin_yield(lock);
>>
>> so we can
>> u32 arch_get_vcpu_yield_count(int cpu)
>> {
>> if (SHARED_PROCESSOR)
>
> SHARED_PROCESSOR is true only if a PPC_PSERIES kernel runs in shared
> processor mode. Using this here will rule out the situation where a
> PPC_PSERIES kernel(ppc guest) runs indedicated processor mode. I don’t
> think we can rule out the dedicated processor mode, because even in the
well, copied from the codes
/*
* We are using a non architected field to determine if a partition is
* shared or dedicated. This currently works on both KVM and PHYP, but
* we will have to transition to something better.
*/
#define LPPACA_OLD_SHARED_PROC 2
static inline bool lppaca_shared_proc(struct lppaca *l)
{
return !!(l->__old_status & LPPACA_OLD_SHARED_PROC);
}
OKay, I have no idea about the difference between dedicated processor mode and the so-called shared..
Let me talk to you f2f monday….
> dedicated mode, the lock holder may still be preempted. So why? Why do
> we only want the yield_count in the shared processor mode?
>
>> return be32_to_cpu(lppaca_of(cpu).yield_count;
>> return 0;
>> }
>>
>> and all these things can be don inline then.
>>
>>> I've cooked the following patch, please note I haven't test this, this
>>> is just a POC.
>>>
>>> Regards,
>>> Boqun
>>>
>>> ---------------->8
>>> virt: Introduce vcpu preemption detection functions
>>>
>>> Lock holder preemption is an serious problem in virtualized environment
>>> for locking. Different archs and hypervisors employ different ways to
>>> try to solve the problem in different locking mechanisms. And recently
>>> Pan Xinhui found a significant performance drop in osq_lock(), which
>>> could be solved by providing a mechanism to detect the preemption of
>>> vcpu.
>>>
>>> Therefore this patch introduces several vcpu preemption detection
>>> primitives, which allows locking primtives to save the time of
>>> unnecesarry spinning. These primitives act as an abstract layer between
>>> locking functions and arch or hypervisor related code.
>>>
>>> There are two sets of primitives:
>>>
>>> 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
>>> pairwisely in a same preempt disable critical section. And they
>>> could detect whether a vcpu preemption happens between them.
>>>
>>> 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
>>> preempted.
>>>
>>> This patch also implements those primitives on pseries and wire them up.
>>>
>>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>>> ---
>>> arch/powerpc/platforms/pseries/Kconfig | 1 +
>>> arch/powerpc/platforms/pseries/setup.c | 28 +++++++++++
>>> include/linux/vcpu_preempt.h | 90 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 119 insertions(+)
>>> create mode 100644 include/linux/vcpu_preempt.h
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
>>> index bec90fb30425..7d24c3e48878 100644
>>> --- a/arch/powerpc/platforms/pseries/Kconfig
>>> +++ b/arch/powerpc/platforms/pseries/Kconfig
>>> @@ -21,6 +21,7 @@ config PPC_PSERIES
>>> select HOTPLUG_CPU if SMP
>>> select ARCH_RANDOM
>>> select PPC_DOORBELL
>>> + select HAS_VCPU_PREEMPTION_DETECTION
>>> default y
>>>
>> it is already too many config there…
>
> Why is this a problem? It's a Kconfig file, there are not too many
> readability issues here. It simply serve as a database for the
> relationships between different config options. menuconfig and its
just a personal favor…
I also a little hate the callback ops if it is not necessary to use, as it breaks my reading of the codes :-
AND same reason, I don’t want to vim .config then /any config_
anyway, this is not a big problem…
> friends can do their job well. BTW, have you read config X86? ;-)
>
>> as I comment above, we could make these things inline, so looks like we needn't to add such config.
>>
>
> Another reason, I didn't make those primitives arch-specific inline
> functions, is that they are actually not only arch-specific but also
> (virtual-)environment-specific, or hypervisor-specific, that is their
> implementations differ when the kernel runs on different hypervisors(KVM
> and Xen, for example).
>
> If you make the implementation part inline functions, you end up
> handling different virtual environment every time the functions are
> called, which is a waste of time. We are lucky because phyp and kvm
well, the call is also expensive.
> share the same interface of yield count, but this might not be true for
> other archs and other hypervisors.
>
fair enough.
>>> config PPC_SPLPAR
>>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>>> index 9883bc7ea007..5d4aed54e039 100644
>>> --- a/arch/powerpc/platforms/pseries/setup.c
>>> +++ b/arch/powerpc/platforms/pseries/setup.c
>>> @@ -42,6 +42,7 @@
>>> #include <linux/of.h>
>>> #include <linux/of_pci.h>
>>> #include <linux/kexec.h>
>>> +#include <linux/vcpu_preempt.h>
>>>
>>> #include <asm/mmu.h>
>>> #include <asm/processor.h>
>>> @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
>>> of_pci_check_probe_only();
>>> }
>>>
>>> +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
>>> +EXPORT_SYMBOL(vcpu_preempt_ops);
>>> +
>>> +static long pseries_vcpu_preempt_count(void)
>>> +{
>>> + return be32_to_cpu(get_lppaca()->yield_count);
>>> +}
>>> +
>>> +static bool pseries_vcpu_is_preempted(int cpu)
>>> +{
>>> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
>>> +}
>>> +
>>> +static bool pseries_vcpu_has_preempted(long vpc)
>>> +{
>>> + return pseries_vcpu_preempt_count() != vpc;
>>> +}
>>> +
>>> +static void __init pseries_setup_vcpu_preempt_ops(void)
>>> +{
>>> + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
>>> + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
>>> + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
>>> +}
>>> +
>>> static void __init pSeries_setup_arch(void)
>>> {
>>> set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
>>> @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
>>> "%ld\n", rc);
>>> }
>>> }
>>> +
>>> + pseries_setup_vcpu_preempt_ops();
>>> }
>>>
>>> static int __init pSeries_init_panel(void)
>>> diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
>>> new file mode 100644
>>> index 000000000000..4a88414bb83f
>>> --- /dev/null
>>> +++ b/include/linux/vcpu_preempt.h
>>
>> How about add them in sched.h
>
> I don't have a strong opinion on which header file to use. TBH, I was
> trying to use some header files for parvirt stuff, but failed to find
> one, so I add a new one because I don't want people confused between
> preemption of vcpus and preemption of tasks. But once again, I don't
> have a strong opinion ;-)
>
>> I just think they are similar. correct me if I got a mistake :)
>>
>> thanks
>> xinhui
>>
>>> @@ -0,0 +1,90 @@
>>> +/*
>>> + * Primitives for checking the vcpu preemption from the guest.
>>> + */
>>> +
>>> +static long __vcpu_preempt_count(void)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +static bool __vcpu_has_preempted(long vpc)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +static bool __vcpu_is_preempted(int cpu)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +struct vcpu_preempt_ops {
>>> + /*
>>> + * Get the current vcpu's "preempt count", which is going to use for
>>> + * checking whether the current vcpu has ever been preempted
>>> + */
>>> + long (*preempt_count)(void);
>>> +
>>>
>>> + /*
>>> + * Return whether a vcpu is preempted
>>> + */
>>> + bool (*is_preempted)(int cpu);
>>> +
>>> + /*
>>> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
>>> + * happened after the .preempt_count() was called.
>>> + */
>>> + bool (*has_preempted)(long vpc);
>>> +};
>>> +
>>> +struct vcpu_preempt_ops vcpu_preempt_ops;
>>> +
>>> +/* Default boilerplate */
>>> +#define DEFAULT_VCPU_PREEMPT_OPS \
>>> + { \
>>> + .preempt_count = __vcpu_preempt_count, \
>>> + .is_preempted = __vcpu_is_preempted, \
>>> + .has_preempted = __vcpu_has_preempted \
>>> + }
>>> +
>>> +#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
>>> +/*
>>> + * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
>>> + *
>>> + * The vpc is used for checking whether the current vcpu has ever been
>>> + * preempted via vcpu_has_preempted().
>>> + *
>>> + * This function and vcpu_has_preepmted() should be called in the same
>>> + * preemption disabled critical section.
>>> + */
>>> +static long vcpu_preempt_count(void)
>>> +{
>>> + return vcpu_preempt_ops.preempt_count();
>>> +}
>>> +
>>> +/*
>>> + * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
>>> + */
>>> +static bool vcpu_is_preempted(int cpu)
>>> +{
>>> + return vcpu_preempt_ops.is_preempted(cpu);
>>> +}
>>> +
>>> +/*
>>> + * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
>>> + * preempted.
>>> + *
>>> + * The checked duration is between the vcpu_preempt_count() which returns @vpc
>>> + * is called and this function called.
>>> + *
>>> + * This function and corresponding vcpu_preempt_count() should be in the same
>>> + * preemption disabled cirtial section.
>>> + */
>>> +static bool vcpu_has_preempted(long vpc)
>>> +{
>>> + return vcpu_preempt_ops.has_preempt(vpc);
>>> +}
>>> +#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
>>> +#define vcpu_preempt_count() __vcpu_preempt_count()
>>> +#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
>>> +#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
>>> +#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
>>> --
>>> 2.9.0
>>>
>>
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2016-06-26 09:00 +0200 |
| Message-ID | <rOccV-7Pw-1@gated-at.bofh.it> |
| In reply to | #1431345 |
On Sun, Jun 26, 2016 at 02:10:57PM +0800, Boqun Feng wrote:
> On Sun, Jun 26, 2016 at 01:21:04PM +0800, panxinhui wrote:
> >
> > > 在 2016年6月26日，03:20，Peter Zijlstra <peterz@infradead.org> 写道：
> > >
> > > On Sun, Jun 26, 2016 at 01:27:56AM +0800, panxinhui wrote:
> > >>>> Would that not have issues where the owner cpu is kept running but the
> > >>>> spinner (ie. _this_ vcpu) gets preempted? I would think that in that
> > >>>> case we too want to stop spinning.
> > >>>>
> > >>>
> > >> do you mean that the spinner detect itself had yield out during the
> > >> big spin loop?
> > >>
> > >> It is very possible to happen. BUT if spinner(on this vcpu) yield
> > >> out, the next spinner would break the spin loop. AND if spinner
> > >> detect itself yield out once, it’s very possible to get the osq lock
> > >> soon as long as the ower vcpu is running.
> > >>
> > >> SO I think we need just check the owner vcpu’s yield_count.
> > >
> > > I had a quick look at KVM and it looks like it only has
> > > kvm_cpu::preempted, which would suggest the interface boqun proposed.
> > >
> > > We'll have to look at many of the other virt platforms as well to see
> > > what they can do.
> > >
> > > We could also provide _both_ interfaces and a platform can implement
> > > whichever variant (or both) it can.
> > >
> > the kvm code on ppc has implemented yield_count.
> > It let me feel a little relaxed. :)
> >
> > looks like we could introduce the interface like below.
> >
> > bool vcpu_is_preempted(int cpu)
> > {
> > return arch_vcpu_is_preempted(cpu);
> > }
> >
> > #ifdef arch_vcpu_has_yield_count
> > bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
> > {
> > return arch_get_vcpu_yield_count() != yield_count;
> > }
> >
> > #else
> > bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
> > {
> > /*just let called know it is preepmpted*/
> > return vcpu_is_preempted(cpu);
> > }
> > #endif
> >
>
> Unfortunately, on PPC, we compile pseries code along with powernv code
> in a kernel binary, therefore we need to wire the proper primitives at
> runtime.
>
> I've cooked the following patch, please note I haven't test this, this
> is just a POC.
>
> Regards,
> Boqun
>
> ---------------->8
> virt: Introduce vcpu preemption detection functions
>
> Lock holder preemption is an serious problem in virtualized environment
> for locking. Different archs and hypervisors employ different ways to
> try to solve the problem in different locking mechanisms. And recently
> Pan Xinhui found a significant performance drop in osq_lock(), which
> could be solved by providing a mechanism to detect the preemption of
> vcpu.
>
> Therefore this patch introduces several vcpu preemption detection
> primitives, which allows locking primtives to save the time of
> unnecesarry spinning. These primitives act as an abstract layer between
> locking functions and arch or hypervisor related code.
>
> There are two sets of primitives:
>
> 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
> pairwisely in a same preempt disable critical section. And they
> could detect whether a vcpu preemption happens between them.
>
> 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
> preempted.
>
> This patch also implements those primitives on pseries and wire them up.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> arch/powerpc/platforms/pseries/Kconfig | 1 +
> arch/powerpc/platforms/pseries/setup.c | 28 +++++++++++
> include/linux/vcpu_preempt.h | 90 ++++++++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 include/linux/vcpu_preempt.h
>
> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
> index bec90fb30425..7d24c3e48878 100644
> --- a/arch/powerpc/platforms/pseries/Kconfig
> +++ b/arch/powerpc/platforms/pseries/Kconfig
> @@ -21,6 +21,7 @@ config PPC_PSERIES
> select HOTPLUG_CPU if SMP
> select ARCH_RANDOM
> select PPC_DOORBELL
> + select HAS_VCPU_PREEMPTION_DETECTION
> default y
>
> config PPC_SPLPAR
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 9883bc7ea007..5d4aed54e039 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -42,6 +42,7 @@
> #include <linux/of.h>
> #include <linux/of_pci.h>
> #include <linux/kexec.h>
> +#include <linux/vcpu_preempt.h>
>
> #include <asm/mmu.h>
> #include <asm/processor.h>
> @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
> of_pci_check_probe_only();
> }
>
> +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
> +EXPORT_SYMBOL(vcpu_preempt_ops);
> +
> +static long pseries_vcpu_preempt_count(void)
> +{
> + return be32_to_cpu(get_lppaca()->yield_count);
> +}
> +
> +static bool pseries_vcpu_is_preempted(int cpu)
> +{
> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
> +}
> +
> +static bool pseries_vcpu_has_preempted(long vpc)
> +{
> + return pseries_vcpu_preempt_count() != vpc;
> +}
> +
> +static void __init pseries_setup_vcpu_preempt_ops(void)
> +{
> + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
> + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
> + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
> +}
> +
> static void __init pSeries_setup_arch(void)
> {
> set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
> @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
> "%ld\n", rc);
> }
> }
> +
> + pseries_setup_vcpu_preempt_ops();
> }
>
> static int __init pSeries_init_panel(void)
> diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
> new file mode 100644
> index 000000000000..4a88414bb83f
> --- /dev/null
> +++ b/include/linux/vcpu_preempt.h
> @@ -0,0 +1,90 @@
> +/*
> + * Primitives for checking the vcpu preemption from the guest.
> + */
> +
> +static long __vcpu_preempt_count(void)
> +{
> + return 0;
> +}
> +
> +static bool __vcpu_has_preempted(long vpc)
> +{
> + return false;
> +}
> +
> +static bool __vcpu_is_preempted(int cpu)
> +{
> + return false;
> +}
> +
> +struct vcpu_preempt_ops {
> + /*
> + * Get the current vcpu's "preempt count", which is going to use for
> + * checking whether the current vcpu has ever been preempted
> + */
> + long (*preempt_count)(void);
> +
> + /*
> + * Return whether a vcpu is preempted
> + */
> + bool (*is_preempted)(int cpu);
> +
> + /*
> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
> + * happened after the .preempt_count() was called.
> + */
> + bool (*has_preempted)(long vpc);
> +};
> +
> +struct vcpu_preempt_ops vcpu_preempt_ops;
> +
This should be:
extern struct vcpu_preempt_ops vcpu_preempt_ops;
And I tested this one along with modified version of Xinhui's patch.
The test showed that even in a not over-committed system, the overhead
of mutex_lock() could go down from 2.58%(w/o patches) to 0.87%(w/
patches).
Xinhui, I did also modifiy the title and commit log of your patch, feel
free to take anything into your patch.
Regards,
Boqun
----------------->8
locking/osq: Stop spinning in osq_lock if related vcpus get preempted
In a virtualized environment, a cpu may hold the osq_lock while its vcpu
gets preempted, which makes the spinning of other cpus in the osq
pointless, because it may be quite a while for the holder to get back to
run and notify its successor to go.
To fix this, we use the vcpu preemption detection mechanism to check
whether the holder is preempted out, if so we just stop spinning in the
osq_lock(). Morever, if the spinner has ever been preempted, it also
indicates that the spining in osq_lock() is pointless, so we detect this
and stop spinning too.
Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
---
kernel/locking/osq_lock.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
index 05a37857ab55..bed5a56a6aed 100644
--- a/kernel/locking/osq_lock.c
+++ b/kernel/locking/osq_lock.c
@@ -1,6 +1,7 @@
#include <linux/percpu.h>
#include <linux/sched.h>
#include <linux/osq_lock.h>
+#include <linux/vcpu_preempt.h>
/*
* An MCS like lock especially tailored for optimistic spinning for sleeping
@@ -87,6 +88,8 @@ bool osq_lock(struct optimistic_spin_queue *lock)
struct optimistic_spin_node *prev, *next;
int curr = encode_cpu(smp_processor_id());
int old;
+ int loops;
+ long vpc;
node->locked = 0;
node->next = NULL;
@@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
node->prev = prev;
WRITE_ONCE(prev->next, node);
+ old = old - 1;
+ vpc = vcpu_preempt_count();
+
/*
* Normally @prev is untouchable after the above store; because at that
* moment unlock can proceed and wipe the node element from stack.
@@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
while (!READ_ONCE(node->locked)) {
/*
* If we need to reschedule bail... so we can block.
+ * An over-committed guest with more vCPUs than pCPUs
+ * might fall in this loop and cause a huge overload.
+ * This is because vCPU A(prev) hold the osq lock and yield out,
+ * vCPU B(node) wait ->locked to be set, IOW, wait till
+ * vCPU A run and unlock the osq lock.
+ * NOTE that vCPU A and vCPU B might run on same physical cpu.
*/
- if (need_resched())
+ if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
goto unqueue;
cpu_relax_lowlatency();
[toc] | [prev] | [next] | [standalone]
| From | panxinhui <xinhui@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-26 09:10 +0200 |
| Message-ID | <rOcmB-87X-3@gated-at.bofh.it> |
| In reply to | #1431352 |
> 在 2016年6月26日，14:59，Boqun Feng <boqun.feng@gmail.com> 写道：
>
> On Sun, Jun 26, 2016 at 02:10:57PM +0800, Boqun Feng wrote:
>> On Sun, Jun 26, 2016 at 01:21:04PM +0800, panxinhui wrote:
>>>
>>>> 在 2016年6月26日，03:20，Peter Zijlstra <peterz@infradead.org> 写道：
>>>>
>>>> On Sun, Jun 26, 2016 at 01:27:56AM +0800, panxinhui wrote:
>>>>>>> Would that not have issues where the owner cpu is kept running but the
>>>>>>> spinner (ie. _this_ vcpu) gets preempted? I would think that in that
>>>>>>> case we too want to stop spinning.
>>>>>>>
>>>>>>
>>>>> do you mean that the spinner detect itself had yield out during the
>>>>> big spin loop?
>>>>>
>>>>> It is very possible to happen. BUT if spinner(on this vcpu) yield
>>>>> out, the next spinner would break the spin loop. AND if spinner
>>>>> detect itself yield out once, it’s very possible to get the osq lock
>>>>> soon as long as the ower vcpu is running.
>>>>>
>>>>> SO I think we need just check the owner vcpu’s yield_count.
>>>>
>>>> I had a quick look at KVM and it looks like it only has
>>>> kvm_cpu::preempted, which would suggest the interface boqun proposed.
>>>>
>>>> We'll have to look at many of the other virt platforms as well to see
>>>> what they can do.
>>>>
>>>> We could also provide _both_ interfaces and a platform can implement
>>>> whichever variant (or both) it can.
>>>>
>>> the kvm code on ppc has implemented yield_count.
>>> It let me feel a little relaxed. :)
>>>
>>> looks like we could introduce the interface like below.
>>>
>>> bool vcpu_is_preempted(int cpu)
>>> {
>>> return arch_vcpu_is_preempted(cpu);
>>> }
>>>
>>> #ifdef arch_vcpu_has_yield_count
>>> bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
>>> {
>>> return arch_get_vcpu_yield_count() != yield_count;
>>> }
>>>
>>> #else
>>> bool vcpu_has_preemtped_once(int cpu, unsigned int yield_count)
>>> {
>>> /*just let called know it is preepmpted*/
>>> return vcpu_is_preempted(cpu);
>>> }
>>> #endif
>>>
>>
>> Unfortunately, on PPC, we compile pseries code along with powernv code
>> in a kernel binary, therefore we need to wire the proper primitives at
>> runtime.
>>
>> I've cooked the following patch, please note I haven't test this, this
>> is just a POC.
>>
>> Regards,
>> Boqun
>>
>> ---------------->8
>> virt: Introduce vcpu preemption detection functions
>>
>> Lock holder preemption is an serious problem in virtualized environment
>> for locking. Different archs and hypervisors employ different ways to
>> try to solve the problem in different locking mechanisms. And recently
>> Pan Xinhui found a significant performance drop in osq_lock(), which
>> could be solved by providing a mechanism to detect the preemption of
>> vcpu.
>>
>> Therefore this patch introduces several vcpu preemption detection
>> primitives, which allows locking primtives to save the time of
>> unnecesarry spinning. These primitives act as an abstract layer between
>> locking functions and arch or hypervisor related code.
>>
>> There are two sets of primitives:
>>
>> 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
>> pairwisely in a same preempt disable critical section. And they
>> could detect whether a vcpu preemption happens between them.
>>
>> 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
>> preempted.
>>
>> This patch also implements those primitives on pseries and wire them up.
>>
>> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
>> ---
>> arch/powerpc/platforms/pseries/Kconfig | 1 +
>> arch/powerpc/platforms/pseries/setup.c | 28 +++++++++++
>> include/linux/vcpu_preempt.h | 90 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 119 insertions(+)
>> create mode 100644 include/linux/vcpu_preempt.h
>>
>> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
>> index bec90fb30425..7d24c3e48878 100644
>> --- a/arch/powerpc/platforms/pseries/Kconfig
>> +++ b/arch/powerpc/platforms/pseries/Kconfig
>> @@ -21,6 +21,7 @@ config PPC_PSERIES
>> select HOTPLUG_CPU if SMP
>> select ARCH_RANDOM
>> select PPC_DOORBELL
>> + select HAS_VCPU_PREEMPTION_DETECTION
>> default y
>>
>> config PPC_SPLPAR
>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>> index 9883bc7ea007..5d4aed54e039 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -42,6 +42,7 @@
>> #include <linux/of.h>
>> #include <linux/of_pci.h>
>> #include <linux/kexec.h>
>> +#include <linux/vcpu_preempt.h>
>>
>> #include <asm/mmu.h>
>> #include <asm/processor.h>
>> @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
>> of_pci_check_probe_only();
>> }
>>
>> +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
>> +EXPORT_SYMBOL(vcpu_preempt_ops);
>> +
>> +static long pseries_vcpu_preempt_count(void)
>> +{
>> + return be32_to_cpu(get_lppaca()->yield_count);
>> +}
>> +
>> +static bool pseries_vcpu_is_preempted(int cpu)
>> +{
>> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
>> +}
>> +
>> +static bool pseries_vcpu_has_preempted(long vpc)
>> +{
>> + return pseries_vcpu_preempt_count() != vpc;
>> +}
>> +
>> +static void __init pseries_setup_vcpu_preempt_ops(void)
>> +{
>> + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
>> + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
>> + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
>> +}
>> +
>> static void __init pSeries_setup_arch(void)
>> {
>> set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
>> @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
>> "%ld\n", rc);
>> }
>> }
>> +
>> + pseries_setup_vcpu_preempt_ops();
>> }
>>
>> static int __init pSeries_init_panel(void)
>> diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
>> new file mode 100644
>> index 000000000000..4a88414bb83f
>> --- /dev/null
>> +++ b/include/linux/vcpu_preempt.h
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Primitives for checking the vcpu preemption from the guest.
>> + */
>> +
>> +static long __vcpu_preempt_count(void)
>> +{
>> + return 0;
>> +}
>> +
>> +static bool __vcpu_has_preempted(long vpc)
>> +{
>> + return false;
>> +}
>> +
>> +static bool __vcpu_is_preempted(int cpu)
>> +{
>> + return false;
>> +}
>> +
>> +struct vcpu_preempt_ops {
>> + /*
>> + * Get the current vcpu's "preempt count", which is going to use for
>> + * checking whether the current vcpu has ever been preempted
>> + */
>> + long (*preempt_count)(void);
>> +
>> + /*
>> + * Return whether a vcpu is preempted
>> + */
>> + bool (*is_preempted)(int cpu);
>> +
>> + /*
>> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
>> + * happened after the .preempt_count() was called.
>> + */
>> + bool (*has_preempted)(long vpc);
>> +};
>> +
>> +struct vcpu_preempt_ops vcpu_preempt_ops;
>> +
>
> This should be:
>
> extern struct vcpu_preempt_ops vcpu_preempt_ops;
>
> And I tested this one along with modified version of Xinhui's patch.
>
> The test showed that even in a not over-committed system, the overhead
> of mutex_lock() could go down from 2.58%(w/o patches) to 0.87%(w/
> patches).
>
> Xinhui, I did also modifiy the title and commit log of your patch, feel
> free to take anything into your patch.
>
thanks boqun, I will review your patch.
> Regards,
> Boqun
>
> ----------------->8
> locking/osq: Stop spinning in osq_lock if related vcpus get preempted
>
> In a virtualized environment, a cpu may hold the osq_lock while its vcpu
> gets preempted, which makes the spinning of other cpus in the osq
> pointless, because it may be quite a while for the holder to get back to
> run and notify its successor to go.
>
> To fix this, we use the vcpu preemption detection mechanism to check
> whether the holder is preempted out, if so we just stop spinning in the
> osq_lock(). Morever, if the spinner has ever been preempted, it also
> indicates that the spining in osq_lock() is pointless, so we detect this
> and stop spinning too.
>
> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
> ---
> kernel/locking/osq_lock.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/locking/osq_lock.c b/kernel/locking/osq_lock.c
> index 05a37857ab55..bed5a56a6aed 100644
> --- a/kernel/locking/osq_lock.c
> +++ b/kernel/locking/osq_lock.c
> @@ -1,6 +1,7 @@
> #include <linux/percpu.h>
> #include <linux/sched.h>
> #include <linux/osq_lock.h>
> +#include <linux/vcpu_preempt.h>
>
> /*
> * An MCS like lock especially tailored for optimistic spinning for sleeping
> @@ -87,6 +88,8 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> struct optimistic_spin_node *prev, *next;
> int curr = encode_cpu(smp_processor_id());
> int old;
> + int loops;
> + long vpc;
>
> node->locked = 0;
> node->next = NULL;
> @@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> node->prev = prev;
> WRITE_ONCE(prev->next, node);
>
> + old = old - 1;
> + vpc = vcpu_preempt_count();
> +
> /*
> * Normally @prev is untouchable after the above store; because at that
> * moment unlock can proceed and wipe the node element from stack.
> @@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> while (!READ_ONCE(node->locked)) {
> /*
> * If we need to reschedule bail... so we can block.
> + * An over-committed guest with more vCPUs than pCPUs
> + * might fall in this loop and cause a huge overload.
> + * This is because vCPU A(prev) hold the osq lock and yield out,
> + * vCPU B(node) wait ->locked to be set, IOW, wait till
> + * vCPU A run and unlock the osq lock.
> + * NOTE that vCPU A and vCPU B might run on same physical cpu.
> */
> - if (need_resched())
> + if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
> goto unqueue;
>
the prev might change, so we need read node->prev every loop, then check vcpu preempted.
> cpu_relax_lowlatency();
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2016-06-26 16:30 +0200 |
| Message-ID | <rOjeq-42X-1@gated-at.bofh.it> |
| In reply to | #1431353 |
[Multipart message — attachments visible in raw view] — view raw
On Sun, Jun 26, 2016 at 03:08:20PM +0800, panxinhui wrote:
[snip]
> > @@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > node->prev = prev;
> > WRITE_ONCE(prev->next, node);
> >
> > + old = old - 1;
> > + vpc = vcpu_preempt_count();
> > +
> > /*
> > * Normally @prev is untouchable after the above store; because at that
> > * moment unlock can proceed and wipe the node element from stack.
> > @@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > while (!READ_ONCE(node->locked)) {
> > /*
> > * If we need to reschedule bail... so we can block.
> > + * An over-committed guest with more vCPUs than pCPUs
> > + * might fall in this loop and cause a huge overload.
> > + * This is because vCPU A(prev) hold the osq lock and yield out,
> > + * vCPU B(node) wait ->locked to be set, IOW, wait till
> > + * vCPU A run and unlock the osq lock.
> > + * NOTE that vCPU A and vCPU B might run on same physical cpu.
> > */
> > - if (need_resched())
> > + if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
> > goto unqueue;
> >
>
> the prev might change, so we need read node->prev every loop, then check vcpu preempted.
>
Right you are on the possibility of the prev's change, however, even if
we reread node->prev, the prev is still not stable after we read, that
is the prev can change after we read in the loop and before we check the
vcpu preemption in the next loop, therefore whether the reread is
worthwhile, depends on some real tests I think.
Regards,
Boqun
> > cpu_relax_lowlatency();
>
[toc] | [prev] | [next] | [standalone]
| From | panxinhui <xinhui@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-26 17:20 +0200 |
| Message-ID | <rOk0O-4z9-5@gated-at.bofh.it> |
| In reply to | #1431515 |
> 在 2016年6月26日，22:29，Boqun Feng <boqun.feng@gmail.com> 写道：
>
> On Sun, Jun 26, 2016 at 03:08:20PM +0800, panxinhui wrote:
> [snip]
>>> @@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>>> node->prev = prev;
>>> WRITE_ONCE(prev->next, node);
>>>
>>> + old = old - 1;
>>> + vpc = vcpu_preempt_count();
>>> +
>>> /*
>>> * Normally @prev is untouchable after the above store; because at that
>>> * moment unlock can proceed and wipe the node element from stack.
>>> @@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
>>> while (!READ_ONCE(node->locked)) {
>>> /*
>>> * If we need to reschedule bail... so we can block.
>>> + * An over-committed guest with more vCPUs than pCPUs
>>> + * might fall in this loop and cause a huge overload.
>>> + * This is because vCPU A(prev) hold the osq lock and yield out,
>>> + * vCPU B(node) wait ->locked to be set, IOW, wait till
>>> + * vCPU A run and unlock the osq lock.
>>> + * NOTE that vCPU A and vCPU B might run on same physical cpu.
>>> */
>>> - if (need_resched())
>>> + if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
>>> goto unqueue;
>>>
>>
>> the prev might change, so we need read node->prev every loop, then check vcpu preempted.
>>
>
> Right you are on the possibility of the prev's change, however, even if
> we reread node->prev, the prev is still not stable after we read, that
> is the prev can change after we read in the loop and before we check the
but the next loops we will detect the owner is preempted, right?
> vcpu preemption in the next loop, therefore whether the reread is
well,think the case below.
3 vcpu try to get the osq lock. IF you don’t re-read the prev.
A B C
preempted
detect A preempted, then break loops
and do unqueue
spin loops, because B (the prev ) is running,
preempted
detect B preempted then break loops and do queue.// looks like it’ too late to break the loops
running
SO we can get the rules,
IF the queue nodes looks like
vcpu A,B,C,D,E,F,G,….. more spinners
say, if vcpu B,F is preempted, then, we could know vcpu C,D,E should stop the spinning. right? and all other vcpu after F, say G,H,…should stop spinning too.
thanks
xinhui
> Regards,
> Boqun
>
>>> cpu_relax_lowlatency();
>>
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2016-06-27 08:50 +0200 |
| Message-ID | <rOywN-56U-7@gated-at.bofh.it> |
| In reply to | #1431352 |
On Sun, Jun 26, 2016 at 02:59:26PM +0800, Boqun Feng wrote:
[snip]
>
> This should be:
>
> extern struct vcpu_preempt_ops vcpu_preempt_ops;
>
> And I tested this one along with modified version of Xinhui's patch.
>
> The test showed that even in a not over-committed system, the overhead
> of mutex_lock() could go down from 2.58%(w/o patches) to 0.87%(w/
> patches).
>
Clearly, I made several more mistakes in the patch, and the vcpu
preemption could never been activated, so please ignore this bench
result.
Here is a new version of vcpu preempt. Thoughts?
Regards,
Boqun
---------------->8
virt: Introduce vcpu preemption detection functions
Lock holder preemption is an serious problem in virtualized environment
for locking. Different archs and hypervisors employ different ways to
try to solve the problem in different locking mechanisms. And recently
Pan Xinhui found a significant performance drop in osq_lock(), which
could be solved by providing a mechanism to detect the preemption of
vcpu.
Therefore this patch introduces several vcpu preemption detection
primitives, which allows locking primtives to save the time of
unnecesarry spinning. These primitives act as an abstract layer between
locking functions and arch or hypervisor related code.
There are two sets of primitives:
1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
pairwisely in a same preempt disable critical section. And they
could detect whether a vcpu preemption happens between them.
2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
preempted.
This patch also implements those primitives on pseries and wire them up.
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
arch/powerpc/platforms/pseries/Kconfig | 1 +
arch/powerpc/platforms/pseries/setup.c | 28 ++++++++++++
include/linux/vcpu_preempt.h | 82 ++++++++++++++++++++++++++++++++++
kernel/Kconfig.preempt | 5 ++-
4 files changed, 115 insertions(+), 1 deletion(-)
create mode 100644 include/linux/vcpu_preempt.h
diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
index bec90fb30425..7d24c3e48878 100644
--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig
@@ -21,6 +21,7 @@ config PPC_PSERIES
select HOTPLUG_CPU if SMP
select ARCH_RANDOM
select PPC_DOORBELL
+ select HAS_VCPU_PREEMPTION_DETECTION
default y
config PPC_SPLPAR
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 9883bc7ea007..5d4aed54e039 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -42,6 +42,7 @@
#include <linux/of.h>
#include <linux/of_pci.h>
#include <linux/kexec.h>
+#include <linux/vcpu_preempt.h>
#include <asm/mmu.h>
#include <asm/processor.h>
@@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
of_pci_check_probe_only();
}
+struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
+EXPORT_SYMBOL(vcpu_preempt_ops);
+
+static long pseries_vcpu_preempt_count(void)
+{
+ return be32_to_cpu(get_lppaca()->yield_count);
+}
+
+static bool pseries_vcpu_is_preempted(int cpu)
+{
+ return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
+}
+
+static bool pseries_vcpu_has_preempted(long vpc)
+{
+ return pseries_vcpu_preempt_count() != vpc;
+}
+
+static void __init pseries_setup_vcpu_preempt_ops(void)
+{
+ vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
+ vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
+ vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
+}
+
static void __init pSeries_setup_arch(void)
{
set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
@@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
"%ld\n", rc);
}
}
+
+ pseries_setup_vcpu_preempt_ops();
}
static int __init pSeries_init_panel(void)
diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
new file mode 100644
index 000000000000..e76e054a7917
--- /dev/null
+++ b/include/linux/vcpu_preempt.h
@@ -0,0 +1,82 @@
+/*
+ * Primitives for checking the vcpu preemption from the guest.
+ */
+
+static long __vcpu_preempt_count(void)
+{
+ return 0;
+}
+
+static bool __vcpu_has_preempted(long vpc)
+{
+ return false;
+}
+
+static bool __vcpu_is_preempted(int cpu)
+{
+ return false;
+}
+
+struct vcpu_preempt_ops {
+ /*
+ * Get the current vcpu's "preempt count", which is going to use for
+ * checking whether the current vcpu has ever been preempted
+ */
+ long (*preempt_count)(void);
+
+ /*
+ * Return whether a vcpu is preempted
+ */
+ bool (*is_preempted)(int cpu);
+
+ /*
+ * Given a "vcpu preempt count", Return whether a vcpu preemption ever
+ * happened after the .preempt_count() was called.
+ */
+ bool (*has_preempted)(long vpc);
+};
+
+extern struct vcpu_preempt_ops vcpu_preempt_ops;
+
+/* Default boilerplate */
+#define DEFAULT_VCPU_PREEMPT_OPS \
+ { \
+ .preempt_count = __vcpu_preempt_count, \
+ .is_preempted = __vcpu_is_preempted, \
+ .has_preempted = __vcpu_has_preempted \
+ }
+
+#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
+/*
+ * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
+ *
+ * The vpc is used for checking whether the current vcpu has ever been
+ * preempted via vcpu_has_preempted().
+ *
+ * This function and vcpu_has_preepmted() should be called in the same
+ * preemption disabled critical section.
+ */
+#define vcpu_preempt_count() vcpu_preempt_ops.preempt_count()
+
+/*
+ * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
+ */
+#define vcpu_is_preempted(cpu) vcpu_preempt_ops.is_preempted(cpu)
+
+/*
+ * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
+ * preempted.
+ *
+ * The checked duration is between the vcpu_preempt_count() which returns @vpc
+ * is called and this function called.
+ *
+ * This function and corresponding vcpu_preempt_count() should be in the same
+ * preemption disabled cirtial section.
+ */
+#define vcpu_has_preempted(vpc) vcpu_preempt_ops.has_preempted(vpc)
+
+#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
+#define vcpu_preempt_count() __vcpu_preempt_count()
+#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
+#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
+#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
index 3f9c97419f02..cb88bc3a2fc6 100644
--- a/kernel/Kconfig.preempt
+++ b/kernel/Kconfig.preempt
@@ -55,4 +55,7 @@ config PREEMPT
endchoice
config PREEMPT_COUNT
- bool
\ No newline at end of file
+ bool
+
+config HAS_VCPU_PREEMPTION_DETECTION
+ bool
--
2.9.0
[toc] | [prev] | [next] | [standalone]
| From | xinhui <xinhui.pan@linux.vnet.ibm.com> |
|---|---|
| Date | 2016-06-27 09:40 +0200 |
| Message-ID | <rOzjb-5CO-1@gated-at.bofh.it> |
| In reply to | #1431757 |
On 2016年06月27日 14:45, Boqun Feng wrote:
> On Sun, Jun 26, 2016 at 02:59:26PM +0800, Boqun Feng wrote:
> [snip]
>>
>> This should be:
>>
>> extern struct vcpu_preempt_ops vcpu_preempt_ops;
>>
>> And I tested this one along with modified version of Xinhui's patch.
>>
>> The test showed that even in a not over-committed system, the overhead
>> of mutex_lock() could go down from 2.58%(w/o patches) to 0.87%(w/
>> patches).
>>
>
> Clearly, I made several more mistakes in the patch, and the vcpu
> preemption could never been activated, so please ignore this bench
> result.
>
> Here is a new version of vcpu preempt. Thoughts?
>
> Regards,
> Boqun
>
> ---------------->8
> virt: Introduce vcpu preemption detection functions
>
> Lock holder preemption is an serious problem in virtualized environment
> for locking. Different archs and hypervisors employ different ways to
> try to solve the problem in different locking mechanisms. And recently
> Pan Xinhui found a significant performance drop in osq_lock(), which
> could be solved by providing a mechanism to detect the preemption of
> vcpu.
>
> Therefore this patch introduces several vcpu preemption detection
> primitives, which allows locking primtives to save the time of
> unnecesarry spinning. These primitives act as an abstract layer between
> locking functions and arch or hypervisor related code.
>
> There are two sets of primitives:
>
> 1. vcpu_preempt_count() and vcpu_has_preempted(), they must be used
> pairwisely in a same preempt disable critical section. And they
> could detect whether a vcpu preemption happens between them.
>
> 2. vcpu_is_preempted(), used to check whether other cpu's vcpu is
> preempted.
>
> This patch also implements those primitives on pseries and wire them up.
>
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
> arch/powerpc/platforms/pseries/Kconfig | 1 +
> arch/powerpc/platforms/pseries/setup.c | 28 ++++++++++++
> include/linux/vcpu_preempt.h | 82 ++++++++++++++++++++++++++++++++++
> kernel/Kconfig.preempt | 5 ++-
> 4 files changed, 115 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/vcpu_preempt.h
>
> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
> index bec90fb30425..7d24c3e48878 100644
> --- a/arch/powerpc/platforms/pseries/Kconfig
> +++ b/arch/powerpc/platforms/pseries/Kconfig
> @@ -21,6 +21,7 @@ config PPC_PSERIES
> select HOTPLUG_CPU if SMP
> select ARCH_RANDOM
> select PPC_DOORBELL
> + select HAS_VCPU_PREEMPTION_DETECTION
> default y
>
> config PPC_SPLPAR
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index 9883bc7ea007..5d4aed54e039 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -42,6 +42,7 @@
> #include <linux/of.h>
> #include <linux/of_pci.h>
> #include <linux/kexec.h>
> +#include <linux/vcpu_preempt.h>
>
> #include <asm/mmu.h>
> #include <asm/processor.h>
> @@ -501,6 +502,31 @@ static void __init find_and_init_phbs(void)
> of_pci_check_probe_only();
> }
>
> +struct vcpu_preempt_ops vcpu_preempt_ops = DEFAULT_VCPU_PREEMPT_OPS;
> +EXPORT_SYMBOL(vcpu_preempt_ops);
> +
> +static long pseries_vcpu_preempt_count(void)
> +{
> + return be32_to_cpu(get_lppaca()->yield_count);
> +}
> +
> +static bool pseries_vcpu_is_preempted(int cpu)
> +{
> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
> +}
> +
> +static bool pseries_vcpu_has_preempted(long vpc)
> +{
> + return pseries_vcpu_preempt_count() != vpc;
> +}
> +
> +static void __init pseries_setup_vcpu_preempt_ops(void)
> +{
> + vcpu_preempt_ops.preempt_count = pseries_vcpu_preempt_count;
> + vcpu_preempt_ops.is_preempted = pseries_vcpu_is_preempted;
> + vcpu_preempt_ops.has_preempted = pseries_vcpu_has_preempted;
> +}
> +
> static void __init pSeries_setup_arch(void)
> {
> set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
> @@ -549,6 +575,8 @@ static void __init pSeries_setup_arch(void)
> "%ld\n", rc);
> }
> }
> +
> + pseries_setup_vcpu_preempt_ops();
> }
>
> static int __init pSeries_init_panel(void)
> diff --git a/include/linux/vcpu_preempt.h b/include/linux/vcpu_preempt.h
> new file mode 100644
> index 000000000000..e76e054a7917
> --- /dev/null
> +++ b/include/linux/vcpu_preempt.h
> @@ -0,0 +1,82 @@
> +/*
> + * Primitives for checking the vcpu preemption from the guest.
> + */
> +
> +static long __vcpu_preempt_count(void)
> +{
> + return 0;
> +}
> +
> +static bool __vcpu_has_preempted(long vpc)
> +{
> + return false;
> +}
> +
> +static bool __vcpu_is_preempted(int cpu)
> +{
> + return false;
> +}
> +
> +struct vcpu_preempt_ops {
> + /*
> + * Get the current vcpu's "preempt count", which is going to use for
> + * checking whether the current vcpu has ever been preempted
> + */
> + long (*preempt_count)(void);
> +
> + /*
> + * Return whether a vcpu is preempted
> + */
> + bool (*is_preempted)(int cpu);
> +
> + /*
> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
> + * happened after the .preempt_count() was called.
> + */
> + bool (*has_preempted)(long vpc);
> +};
> +
> +extern struct vcpu_preempt_ops vcpu_preempt_ops;
> +
> +/* Default boilerplate */
> +#define DEFAULT_VCPU_PREEMPT_OPS \
> + { \
> + .preempt_count = __vcpu_preempt_count, \
> + .is_preempted = __vcpu_is_preempted, \
> + .has_preempted = __vcpu_has_preempted \
> + }
> +
> +#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
> +/*
> + * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
> + *
> + * The vpc is used for checking whether the current vcpu has ever been
> + * preempted via vcpu_has_preempted().
> + *
> + * This function and vcpu_has_preepmted() should be called in the same
> + * preemption disabled critical section.
> + */
> +#define vcpu_preempt_count() vcpu_preempt_ops.preempt_count()
> +
> +/*
> + * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
> + */
> +#define vcpu_is_preempted(cpu) vcpu_preempt_ops.is_preempted(cpu)
> +
> +/*
> + * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
> + * preempted.
> + *
> + * The checked duration is between the vcpu_preempt_count() which returns @vpc
> + * is called and this function called.
> + *
> + * This function and corresponding vcpu_preempt_count() should be in the same
> + * preemption disabled cirtial section.
> + */
> +#define vcpu_has_preempted(vpc) vcpu_preempt_ops.has_preempted(vpc)
> +
> +#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
> +#define vcpu_preempt_count() __vcpu_preempt_count()
> +#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
> +#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
> +#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
AND if possible, We can hide the detail about vcpu preempted check. IOW, caller better do not know vcpu_preempt_count() .
currently, users have to code like below.
long vpc = vcpu_preempt_count();
while()
if (vcpu_has_preempted(vpc))
break;
the vpc is long, this the implementation detail.
SO we can introduce :
vcpu_preempt_critical_start(void);
{
/* record the yield count*/
__this_cpu_write(vpc, vcpu_preempt_count());
}
vcpu_preempt_critical_end(void)
{
// do nothing.
}
vcpu_has_preempted(void)
{
vcpu_preempt_ops.has_preempted(__this_cpu_read(vpc));
}
then we can code like below
vcpu_preempt_critical_start()
while()
if (vcpu_has_preempted())
break;
vcpu_preempt_critical_end()
looks a little better?
thanks
xinhui
> diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
> index 3f9c97419f02..cb88bc3a2fc6 100644
> --- a/kernel/Kconfig.preempt
> +++ b/kernel/Kconfig.preempt
> @@ -55,4 +55,7 @@ config PREEMPT
> endchoice
>
> config PREEMPT_COUNT
> - bool
> \ No newline at end of file
> + bool
> +
> +config HAS_VCPU_PREEMPTION_DETECTION
> + bool
>
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2016-06-27 10:20 +0200 |
| Message-ID | <rOzVT-66k-13@gated-at.bofh.it> |
| In reply to | #1431757 |
On Mon, Jun 27, 2016 at 02:45:06PM +0800, Boqun Feng wrote:
> +++ b/include/linux/vcpu_preempt.h
> @@ -0,0 +1,82 @@
> +/*
> + * Primitives for checking the vcpu preemption from the guest.
> + */
> +
> +static long __vcpu_preempt_count(void)
> +{
> + return 0;
> +}
> +
> +static bool __vcpu_has_preempted(long vpc)
> +{
> + return false;
> +}
> +
> +static bool __vcpu_is_preempted(int cpu)
> +{
> + return false;
> +}
> +
> +struct vcpu_preempt_ops {
> + /*
> + * Get the current vcpu's "preempt count", which is going to use for
> + * checking whether the current vcpu has ever been preempted
> + */
> + long (*preempt_count)(void);
> +
> + /*
> + * Return whether a vcpu is preempted
> + */
> + bool (*is_preempted)(int cpu);
> +
> + /*
> + * Given a "vcpu preempt count", Return whether a vcpu preemption ever
> + * happened after the .preempt_count() was called.
> + */
> + bool (*has_preempted)(long vpc);
> +};
> +
> +extern struct vcpu_preempt_ops vcpu_preempt_ops;
> +
> +/* Default boilerplate */
> +#define DEFAULT_VCPU_PREEMPT_OPS \
> + { \
> + .preempt_count = __vcpu_preempt_count, \
> + .is_preempted = __vcpu_is_preempted, \
> + .has_preempted = __vcpu_has_preempted \
> + }
> +
> +#ifdef CONFIG_HAS_VCPU_PREEMPTION_DETECTION
> +/*
> + * vcpu_preempt_count: Get the current cpu's "vcpu preempt count"(vpc).
> + *
> + * The vpc is used for checking whether the current vcpu has ever been
> + * preempted via vcpu_has_preempted().
> + *
> + * This function and vcpu_has_preepmted() should be called in the same
> + * preemption disabled critical section.
> + */
> +#define vcpu_preempt_count() vcpu_preempt_ops.preempt_count()
> +
> +/*
> + * vcpu_is_preempted: Check whether @cpu's vcpu is preempted.
> + */
> +#define vcpu_is_preempted(cpu) vcpu_preempt_ops.is_preempted(cpu)
> +
> +/*
> + * vcpu_has_preepmted: Check whether the current cpu's vcpu has ever been
> + * preempted.
> + *
> + * The checked duration is between the vcpu_preempt_count() which returns @vpc
> + * is called and this function called.
> + *
> + * This function and corresponding vcpu_preempt_count() should be in the same
> + * preemption disabled cirtial section.
> + */
> +#define vcpu_has_preempted(vpc) vcpu_preempt_ops.has_preempted(vpc)
> +
> +#else /* CONFIG_HAS_VCPU_PREEMPTION_DETECTION */
> +#define vcpu_preempt_count() __vcpu_preempt_count()
> +#define vcpu_is_preempted(cpu) __vcpu_is_preempted(cpu)
> +#define vcpu_has_preempted(vpc) __vcpu_has_preempted(vpc)
> +#endif /* CONFIG_HAS_VCPU_PREEPMTION_DETECTION */
No, this is entirely insane, also broken.
No vectors, no actual function calls, nothing like that. You want the
below to completely compile away and generate the exact 100% same code
it does today.
> +++ b/kernel/locking/osq_lock.c
> @@ -1,6 +1,7 @@
> #include <linux/percpu.h>
> #include <linux/sched.h>
> #include <linux/osq_lock.h>
> +#include <linux/vcpu_preempt.h>
>
> /*
> * An MCS like lock especially tailored for optimistic spinning for sleeping
> @@ -87,6 +88,8 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> struct optimistic_spin_node *prev, *next;
> int curr = encode_cpu(smp_processor_id());
> int old;
> + int loops;
> + long vpc;
>
> node->locked = 0;
> node->next = NULL;
> @@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> node->prev = prev;
> WRITE_ONCE(prev->next, node);
>
> + old = old - 1;
That's just nasty, and could result in an unconditional decrement being
issues, even though its never used.
> + vpc = vcpu_preempt_count();
> +
> /*
> * Normally @prev is untouchable after the above store; because at that
> * moment unlock can proceed and wipe the node element from stack.
> @@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> while (!READ_ONCE(node->locked)) {
> /*
> * If we need to reschedule bail... so we can block.
> + * An over-committed guest with more vCPUs than pCPUs
> + * might fall in this loop and cause a huge overload.
> + * This is because vCPU A(prev) hold the osq lock and yield out,
> + * vCPU B(node) wait ->locked to be set, IOW, wait till
> + * vCPU A run and unlock the osq lock.
> + * NOTE that vCPU A and vCPU B might run on same physical cpu.
> */
> - if (need_resched())
> + if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
> goto unqueue;
>
> cpu_relax_lowlatency();
>
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2016-06-27 12:30 +0200 |
| Message-ID | <rOBXI-7pb-29@gated-at.bofh.it> |
| In reply to | #1431802 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, Jun 27, 2016 at 10:09:59AM +0200, Peter Zijlstra wrote:
[snip]
>
> No, this is entirely insane, also broken.
>
> No vectors, no actual function calls, nothing like that. You want the
> below to completely compile away and generate the exact 100% same code
> it does today.
>
Point taken.
As Xinhui also posted something similar, which worked better on not
effecting the generated code if not enabled. I think I'd better to drop
this workload to him ;-)
> > +++ b/kernel/locking/osq_lock.c
> > @@ -1,6 +1,7 @@
> > #include <linux/percpu.h>
> > #include <linux/sched.h>
> > #include <linux/osq_lock.h>
> > +#include <linux/vcpu_preempt.h>
> >
> > /*
> > * An MCS like lock especially tailored for optimistic spinning for sleeping
> > @@ -87,6 +88,8 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > struct optimistic_spin_node *prev, *next;
> > int curr = encode_cpu(smp_processor_id());
> > int old;
> > + int loops;
> > + long vpc;
> >
> > node->locked = 0;
> > node->next = NULL;
> > @@ -106,6 +109,9 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > node->prev = prev;
> > WRITE_ONCE(prev->next, node);
> >
> > + old = old - 1;
>
> That's just nasty, and could result in an unconditional decrement being
> issues, even though its never used.
>
Right, better to calculate this decrement at the argument of
vcpu_is_preempted() callsite, and define the primitive of host code as
#define vcpu_is_preempted(cpu) false
, which could probably be optimized out.
Regards,
Boqun
> > + vpc = vcpu_preempt_count();
> > +
> > /*
> > * Normally @prev is untouchable after the above store; because at that
> > * moment unlock can proceed and wipe the node element from stack.
> > @@ -118,8 +124,14 @@ bool osq_lock(struct optimistic_spin_queue *lock)
> > while (!READ_ONCE(node->locked)) {
> > /*
> > * If we need to reschedule bail... so we can block.
> > + * An over-committed guest with more vCPUs than pCPUs
> > + * might fall in this loop and cause a huge overload.
> > + * This is because vCPU A(prev) hold the osq lock and yield out,
> > + * vCPU B(node) wait ->locked to be set, IOW, wait till
> > + * vCPU A run and unlock the osq lock.
> > + * NOTE that vCPU A and vCPU B might run on same physical cpu.
> > */
> > - if (need_resched())
> > + if (need_resched() || vcpu_is_preempted(old) || vcpu_has_preempted(vpc))
> > goto unqueue;
> >
> > cpu_relax_lowlatency();
> >
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web