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

Groups > linux.kernel > #1493257 > unrolled thread

Re: [PATCH v3 0/4] implement vcpu preempted check

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/4] implement vcpu preempted check Peter Zijlstra <peterz@infradead.org> - 2016-09-29 12:20 +0200
    Re: [PATCH v3 0/4] implement vcpu preempted check Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-29 12:30 +0200
      Re: [PATCH v3 0/4] implement vcpu preempted check Peter Zijlstra <peterz@infradead.org> - 2016-09-29 12:40 +0200
        Re: [PATCH v3 0/4] implement vcpu preempted check Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2016-09-30 07:10 +0200
          Re: [PATCH v3 0/4] implement vcpu preempted check Paolo Bonzini <pbonzini@redhat.com> - 2016-09-30 09:10 +0200
            Re: [PATCH v3 0/4] implement vcpu preempted check Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2016-09-30 11:00 +0200
              Re: [PATCH v3 0/4] implement vcpu preempted check Paolo Bonzini <pbonzini@redhat.com> - 2016-09-30 11:10 +0200
                Re: [PATCH v3 0/4] implement vcpu preempted check Pan Xinhui <xinhui@linux.vnet.ibm.com> - 2016-09-30 11:40 +0200
            Re: [PATCH v3 0/4] implement vcpu preempted check Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-30 12:50 +0200
      Re: [PATCH v3 0/4] implement vcpu preempted check Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-29 12:50 +0200
        Re: [PATCH v3 0/4] implement vcpu preempted check Christian Borntraeger <borntraeger@de.ibm.com> - 2016-09-29 13:10 +0200

#1493257 — Re: [PATCH v3 0/4] implement vcpu preempted check

On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
> change from v2:
> 	no code change, fix typos, update some comments
> 
> change from v1:
> 	a simplier definition of default vcpu_is_preempted
> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner. 
> 	add more comments
> 	thanks boqun and Peter's suggestion.
> 
> This patch set aims to fix lock holder preemption issues.

So I really like the concept, but I would also really like to see
support for more hypervisors included before we can move forward with
this.

Please consider s390 and (x86/arm) KVM. Once we have a few, more can
follow later, but I think its important to not only have PPC support for
this.

[toc] | [next] | [standalone]

#1493281

On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
> On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
>> change from v2:
>> 	no code change, fix typos, update some comments
>>
>> change from v1:
>> 	a simplier definition of default vcpu_is_preempted
>> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
>> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner. 
>> 	add more comments
>> 	thanks boqun and Peter's suggestion.
>>
>> This patch set aims to fix lock holder preemption issues.
> 
> So I really like the concept, but I would also really like to see
> support for more hypervisors included before we can move forward with
> this.
> 
> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
> follow later, but I think its important to not only have PPC support for
> this.

Actually the s390 preemted check via sigp sense running  is available for
all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
longer buy s390 systems without LPAR.

As Heiko already pointed out we could simply use a small inline function
that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)

Christian

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

#1493286

On Thu, Sep 29, 2016 at 12:23:19PM +0200, Christian Borntraeger wrote:
> On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
> > On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
> >> change from v2:
> >> 	no code change, fix typos, update some comments
> >>
> >> change from v1:
> >> 	a simplier definition of default vcpu_is_preempted
> >> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
> >> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner. 
> >> 	add more comments
> >> 	thanks boqun and Peter's suggestion.
> >>
> >> This patch set aims to fix lock holder preemption issues.
> > 
> > So I really like the concept, but I would also really like to see
> > support for more hypervisors included before we can move forward with
> > this.
> > 
> > Please consider s390 and (x86/arm) KVM. Once we have a few, more can
> > follow later, but I think its important to not only have PPC support for
> > this.
> 
> Actually the s390 preemted check via sigp sense running  is available for
> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
> longer buy s390 systems without LPAR.
> 
> As Heiko already pointed out we could simply use a small inline function
> that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)

Sure, and I had vague memories of Heiko's email. This patch set however
completely fails to do that trivial hooking up.

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

#1493883

在 2016/9/29 18:31, Peter Zijlstra 写道:
> On Thu, Sep 29, 2016 at 12:23:19PM +0200, Christian Borntraeger wrote:
>> On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
>>> On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
>>>> change from v2:
>>>> 	no code change, fix typos, update some comments
>>>>
>>>> change from v1:
>>>> 	a simplier definition of default vcpu_is_preempted
>>>> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
>>>> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner.
>>>> 	add more comments
>>>> 	thanks boqun and Peter's suggestion.
>>>>
>>>> This patch set aims to fix lock holder preemption issues.
>>>
>>> So I really like the concept, but I would also really like to see
>>> support for more hypervisors included before we can move forward with
>>> this.
>>>
>>> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
>>> follow later, but I think its important to not only have PPC support for
>>> this.
>>
>> Actually the s390 preemted check via sigp sense running  is available for
>> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
>> longer buy s390 systems without LPAR.
>>
>> As Heiko already pointed out we could simply use a small inline function
>> that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)
>
> Sure, and I had vague memories of Heiko's email. This patch set however
> completely fails to do that trivial hooking up.
>

sorry for that.
I will try to work it out on x86.

Hi, Will
	I appreciate that if you or some other arm guys could help on it. :)

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

#1493916

> > > > Please consider s390 and (x86/arm) KVM. Once we have a few, more can
> > > > follow later, but I think its important to not only have PPC support for
> > > > this.
> > >
> > > Actually the s390 preemted check via sigp sense running  is available for
> > > all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can
> > > no longer buy s390 systems without LPAR.
> > >
> > > As Heiko already pointed out we could simply use a small inline function
> > > that calls cpu_is_preempted from arch/s390/lib/spinlock (or
> > > smp_vcpu_scheduled from smp.c)
> >
> > Sure, and I had vague memories of Heiko's email. This patch set however
> > completely fails to do that trivial hooking up.
> 
> sorry for that.
> I will try to work it out on x86.

x86 has no hypervisor support, and I'd like to understand the desired
semantics first, so I don't think it should block this series.  In
particular, there are at least the following choices:

1) exit to userspace (5-10.000 clock cycles best case) counts as
lock holder preemption

2) any time the vCPU thread not running counts as lock holder
preemption

To implement the latter you'd need a hypercall or MSR (at least as
a slow path), because the KVM preempt notifier is only active
during the KVM_RUN ioctl.

Paolo

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

#1493958

hi, Paolo
	thanks for your reply.

在 2016/9/30 14:58, Paolo Bonzini 写道:
>>>>> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
>>>>> follow later, but I think its important to not only have PPC support for
>>>>> this.
>>>>
>>>> Actually the s390 preemted check via sigp sense running  is available for
>>>> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can
>>>> no longer buy s390 systems without LPAR.
>>>>
>>>> As Heiko already pointed out we could simply use a small inline function
>>>> that calls cpu_is_preempted from arch/s390/lib/spinlock (or
>>>> smp_vcpu_scheduled from smp.c)
>>>
>>> Sure, and I had vague memories of Heiko's email. This patch set however
>>> completely fails to do that trivial hooking up.
>>
>> sorry for that.
>> I will try to work it out on x86.
>
> x86 has no hypervisor support, and I'd like to understand the desired
> semantics first, so I don't think it should block this series.  In

Once a guest do a hypercall or something similar, IOW, there is a kvm_guest_exit. we think this is a lock holder preemption.
Adn PPC implement it in this way.

> particular, there are at least the following choices:
>
> 1) exit to userspace (5-10.000 clock cycles best case) counts as
> lock holder preemption
>
just to avoid any misunderstanding.
You are saying that the guest does an IO operation for example and then exit to QEMU right?
Yes, in this scenario it's hard to guarntee that such IO operation or someghing like that could be finished in time.

  
> 2) any time the vCPU thread not running counts as lock holder
> preemption
>
> To implement the latter you'd need a hypercall or MSR (at least as
> a slow path), because the KVM preempt notifier is only active
> during the KVM_RUN ioctl.
>
seems a little expensive. :(
How many clock cycles it might cost.

I am still looking for one shared struct between kvm and guest kernel on x86.
and every time kvm_guest_exit/enter called, we store some info in it. So guest kernel can check one vcpu is running or not quickly.

thanks
xinhui

> Paolo
>

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

#1493974

On 30/09/2016 10:52, Pan Xinhui wrote:
>> x86 has no hypervisor support, and I'd like to understand the desired
>> semantics first, so I don't think it should block this series.  In
> 
> Once a guest do a hypercall or something similar, IOW, there is a
> kvm_guest_exit. we think this is a lock holder preemption.
> Adn PPC implement it in this way.

Ok, good.

>> particular, there are at least the following choices:
>>
>> 1) exit to userspace (5-10.000 clock cycles best case) counts as
>> lock holder preemption
>>
>> 2) any time the vCPU thread not running counts as lock holder
>> preemption
>>
>> To implement the latter you'd need a hypercall or MSR (at least as
>> a slow path), because the KVM preempt notifier is only active
>> during the KVM_RUN ioctl.
>
> seems a little expensive. :(
> How many clock cycles it might cost.

An MSR read is about 1500 clock cycles, but it need not be the fast path
(e.g. use a bit to check if the CPU is running, if not use the MSR to
check if the CPU is in userspace but the CPU thread is scheduled).  But
it's not necessary if you are just matching PPC semantics.

Then the simplest thing is to use the kvm_steal_time struct, and add a
new field to it that replaces pad[0].  You can write a 0 to the flag in
record_steal_time (not preempted) and a 1 in kvm_arch_vcpu_put
(preempted).  record_steal_time is called before the VM starts running,
immediately after KVM_RUN and also after every sched_in.

If KVM doesn't implement the flag, it won't touch that field at all.  So
the kernel can write a 0, meaning "not preempted", and not care if the
hypervisor implements the flag or not: the answer will always be safe.

The pointer to the flag can be placed in a per-cpu u32*, and again if
the u32* is NULL that means "not preempted".

Paolo


> I am still looking for one shared struct between kvm and guest kernel on
> x86.
> and every time kvm_guest_exit/enter called, we store some info in it. So
> guest kernel can check one vcpu is running or not quickly.
> 
> thanks
> xinhui
> 
>> Paolo
>>
> 

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

#1493989

在 2016/9/30 17:08, Paolo Bonzini 写道:
>
>
> On 30/09/2016 10:52, Pan Xinhui wrote:
>>> x86 has no hypervisor support, and I'd like to understand the desired
>>> semantics first, so I don't think it should block this series.  In
>>
>> Once a guest do a hypercall or something similar, IOW, there is a
>> kvm_guest_exit. we think this is a lock holder preemption.
>> Adn PPC implement it in this way.
>
> Ok, good.
>
>>> particular, there are at least the following choices:
>>>
>>> 1) exit to userspace (5-10.000 clock cycles best case) counts as
>>> lock holder preemption
>>>
>>> 2) any time the vCPU thread not running counts as lock holder
>>> preemption
>>>
>>> To implement the latter you'd need a hypercall or MSR (at least as
>>> a slow path), because the KVM preempt notifier is only active
>>> during the KVM_RUN ioctl.
>>
>> seems a little expensive. :(
>> How many clock cycles it might cost.
>
> An MSR read is about 1500 clock cycles, but it need not be the fast path
> (e.g. use a bit to check if the CPU is running, if not use the MSR to
> check if the CPU is in userspace but the CPU thread is scheduled).  But
> it's not necessary if you are just matching PPC semantics.
>
> Then the simplest thing is to use the kvm_steal_time struct, and add a
> new field to it that replaces pad[0].  You can write a 0 to the flag in
> record_steal_time (not preempted) and a 1 in kvm_arch_vcpu_put
> (preempted).  record_steal_time is called before the VM starts running,
> immediately after KVM_RUN and also after every sched_in.
>
> If KVM doesn't implement the flag, it won't touch that field at all.  So
> the kernel can write a 0, meaning "not preempted", and not care if the
> hypervisor implements the flag or not: the answer will always be safe.
>
> The pointer to the flag can be placed in a per-cpu u32*, and again if
> the u32* is NULL that means "not preempted".
>
really nice suggestion!  That's what I want :)

thanks
xinhui

> Paolo
>
>
>> I am still looking for one shared struct between kvm and guest kernel on
>> x86.
>> and every time kvm_guest_exit/enter called, we store some info in it. So
>> guest kernel can check one vcpu is running or not quickly.
>>
>> thanks
>> xinhui
>>
>>> Paolo
>>>
>>
>

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

#1494022

On 09/30/2016 08:58 AM, Paolo Bonzini wrote:
>>>>> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
>>>>> follow later, but I think its important to not only have PPC support for
>>>>> this.
>>>>
>>>> Actually the s390 preemted check via sigp sense running  is available for
>>>> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can
>>>> no longer buy s390 systems without LPAR.
>>>>
>>>> As Heiko already pointed out we could simply use a small inline function
>>>> that calls cpu_is_preempted from arch/s390/lib/spinlock (or
>>>> smp_vcpu_scheduled from smp.c)
>>>
>>> Sure, and I had vague memories of Heiko's email. This patch set however
>>> completely fails to do that trivial hooking up.
>>
>> sorry for that.
>> I will try to work it out on x86.
> 
> x86 has no hypervisor support, and I'd like to understand the desired
> semantics first, so I don't think it should block this series.  In
> particular, there are at least the following choices:

I think the semantics can be slightly different for different architectures
after all it is still a heuristics to improve performance.
> 
> 1) exit to userspace (5-10.000 clock cycles best case) counts as
> lock holder preemption
> 
> 2) any time the vCPU thread not running counts as lock holder
> preemption
> 
> To implement the latter you'd need a hypercall or MSR (at least as
> a slow path), because the KVM preempt notifier is only active
> during the KVM_RUN ioctl.

FWIW, The s390 implementation uses kvm_arch_vcpu_put/load as trigger
points for (un)setting the CPUSTAT_RUNNING. Strictly speaking an exit to
userspace is not preempted, But as KVM has no control if we are being
scheduled out when in QEMU this is the compromise that seems to work quite
well for the s390 spinlock code (which checks the running state before 
doing a yield hypercall). 
In addition an exit to QEMU is really a rare case.

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

#1493332

On 09/29/2016 12:23 PM, Christian Borntraeger wrote:
> On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
>> On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
>>> change from v2:
>>> 	no code change, fix typos, update some comments
>>>
>>> change from v1:
>>> 	a simplier definition of default vcpu_is_preempted
>>> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
>>> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner. 
>>> 	add more comments
>>> 	thanks boqun and Peter's suggestion.
>>>
>>> This patch set aims to fix lock holder preemption issues.
>>
>> So I really like the concept, but I would also really like to see
>> support for more hypervisors included before we can move forward with
>> this.
>>
>> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
>> follow later, but I think its important to not only have PPC support for
>> this.
> 
> Actually the s390 preemted check via sigp sense running  is available for
> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
> longer buy s390 systems without LPAR.
> 
> As Heiko already pointed out we could simply use a small inline function
> that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)

Maybe something like
(untested and just pasted, so white space damaged)

diff --git a/arch/s390/include/asm/spinlock.h b/arch/s390/include/asm/spinlock.h
index 63ebf37..6e82986 100644
--- a/arch/s390/include/asm/spinlock.h
+++ b/arch/s390/include/asm/spinlock.h
@@ -21,6 +21,13 @@ _raw_compare_and_swap(unsigned int *lock, unsigned int old, unsigned int new)
        return __sync_bool_compare_and_swap(lock, old, new);
 }
 
+int arch_vcpu_is_preempted(int cpu);
+#define vcpu_is_preempted cpu_is_preempted
+static inline bool cpu_is_preempted(int cpu)
+{
+       return arch_vcpu_is_preempted(cpu);
+}
+
 /*
  * Simple spin lock operations.  There are two variants, one clears IRQ's
  * on the local processor, one does not.
diff --git a/arch/s390/lib/spinlock.c b/arch/s390/lib/spinlock.c
index e5f50a7..260d179 100644
--- a/arch/s390/lib/spinlock.c
+++ b/arch/s390/lib/spinlock.c
@@ -37,7 +37,7 @@ static inline void _raw_compare_and_delay(unsigned int *lock, unsigned int old)
        asm(".insn rsy,0xeb0000000022,%0,0,%1" : : "d" (old), "Q" (*lock));
 }
 
-static inline int cpu_is_preempted(int cpu)
+int arch_vcpu_is_preempted(int cpu)
 {
        if (test_cpu_flag_of(CIF_ENABLED_WAIT, cpu))
                return 0;
@@ -45,6 +45,7 @@ static inline int cpu_is_preempted(int cpu)
                return 0;
        return 1;
 }
+EXPORT_SYMBOL(arch_vcpu_is_preempted);
 
 void arch_spin_lock_wait(arch_spinlock_t *lp)
 {



If ok I can respin into a proper patch.

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

#1493401

On 09/29/2016 12:40 PM, Christian Borntraeger wrote:
> On 09/29/2016 12:23 PM, Christian Borntraeger wrote:
>> On 09/29/2016 12:10 PM, Peter Zijlstra wrote:
>>> On Thu, Jul 21, 2016 at 07:45:10AM -0400, Pan Xinhui wrote:
>>>> change from v2:
>>>> 	no code change, fix typos, update some comments
>>>>
>>>> change from v1:
>>>> 	a simplier definition of default vcpu_is_preempted
>>>> 	skip mahcine type check on ppc, and add config. remove dedicated macro.
>>>> 	add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner. 
>>>> 	add more comments
>>>> 	thanks boqun and Peter's suggestion.
>>>>
>>>> This patch set aims to fix lock holder preemption issues.
>>>
>>> So I really like the concept, but I would also really like to see
>>> support for more hypervisors included before we can move forward with
>>> this.
>>>
>>> Please consider s390 and (x86/arm) KVM. Once we have a few, more can
>>> follow later, but I think its important to not only have PPC support for
>>> this.
>>
>> Actually the s390 preemted check via sigp sense running  is available for
>> all hypervisors (z/VM, LPAR and KVM) which implies everywhere as you can no
>> longer buy s390 systems without LPAR.
>>
>> As Heiko already pointed out we could simply use a small inline function
>> that calls cpu_is_preempted from arch/s390/lib/spinlock (or smp_vcpu_scheduled from smp.c)
> 
> Maybe something like
> (untested and just pasted, so white space damaged)

Now tested. With 8 host cpus and 16 guest cpus perf bench sched shows the
same improvements as in Pan Xinhuis cover letter.
Also the runtime shrinks a lot.

> 
> diff --git a/arch/s390/include/asm/spinlock.h b/arch/s390/include/asm/spinlock.h
> index 63ebf37..6e82986 100644
> --- a/arch/s390/include/asm/spinlock.h
> +++ b/arch/s390/include/asm/spinlock.h
> @@ -21,6 +21,13 @@ _raw_compare_and_swap(unsigned int *lock, unsigned int old, unsigned int new)
>         return __sync_bool_compare_and_swap(lock, old, new);
>  }
> 
> +int arch_vcpu_is_preempted(int cpu);
> +#define vcpu_is_preempted cpu_is_preempted
> +static inline bool cpu_is_preempted(int cpu)
> +{
> +       return arch_vcpu_is_preempted(cpu);
> +}
> +
>  /*
>   * Simple spin lock operations.  There are two variants, one clears IRQ's
>   * on the local processor, one does not.
> diff --git a/arch/s390/lib/spinlock.c b/arch/s390/lib/spinlock.c
> index e5f50a7..260d179 100644
> --- a/arch/s390/lib/spinlock.c
> +++ b/arch/s390/lib/spinlock.c
> @@ -37,7 +37,7 @@ static inline void _raw_compare_and_delay(unsigned int *lock, unsigned int old)
>         asm(".insn rsy,0xeb0000000022,%0,0,%1" : : "d" (old), "Q" (*lock));
>  }
> 
> -static inline int cpu_is_preempted(int cpu)
> +int arch_vcpu_is_preempted(int cpu)
>  {
>         if (test_cpu_flag_of(CIF_ENABLED_WAIT, cpu))
>                 return 0;
> @@ -45,6 +45,7 @@ static inline int cpu_is_preempted(int cpu)
>                 return 0;
>         return 1;
>  }
> +EXPORT_SYMBOL(arch_vcpu_is_preempted);
> 
>  void arch_spin_lock_wait(arch_spinlock_t *lp)
>  {
> 
> 
> 
> If ok I can respin into a proper patch.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web