| Message ID | 20161202084754.22860-4-rkagan@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/02/2016 09:47 AM, Roman Kagan wrote: > When async_pf is ready the guest needs to be made aware of it ASAP, > because it may be holding off a higher priority task pending the > async_pf resolution in favor of a lower priority one. > > In case async_pf's are harvested in vcpu context (x86) we have to not > only wake the vcpu up but kick it into host. > > While at this, also replace the open-coded vcpu wakeup by the existing > helper. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > --- > virt/kvm/async_pf.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > index 9cced14..5f0a66c 100644 > --- a/virt/kvm/async_pf.c > +++ b/virt/kvm/async_pf.c > @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) > * This memory barrier pairs with prepare_to_wait's set_current_state() > */ > smp_mb(); > - if (swait_active(&vcpu->wq)) > - swake_up(&vcpu->wq); > +#ifdef CONFIG_KVM_ASYNC_PF_SYNC > + kvm_vcpu_wake_up(vcpu); > +#else > + kvm_vcpu_kick(vcpu); > +#endif This will break s390, both functions are disabled for s390. On s390 do not want to kick the CPU for a completion. Instead we implement the kvm_async_page_present_sync call above and handle completion via an "pfault done" interrupt via the normal interrupt delivery. > > mmput(mm); > kvm_put_kvm(vcpu->kvm); > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
----- Original Message ----- > From: "Christian Borntraeger" <borntraeger@de.ibm.com> > To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, > kvm@vger.kernel.org > Cc: "Denis Lunev" <den@virtuozzo.com> > Sent: Friday, December 2, 2016 10:35:02 AM > Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved > > On 12/02/2016 09:47 AM, Roman Kagan wrote: > > When async_pf is ready the guest needs to be made aware of it ASAP, > > because it may be holding off a higher priority task pending the > > async_pf resolution in favor of a lower priority one. > > > > In case async_pf's are harvested in vcpu context (x86) we have to not > > only wake the vcpu up but kick it into host. > > > > While at this, also replace the open-coded vcpu wakeup by the existing > > helper. > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > --- > > virt/kvm/async_pf.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > > index 9cced14..5f0a66c 100644 > > --- a/virt/kvm/async_pf.c > > +++ b/virt/kvm/async_pf.c > > @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) > > * This memory barrier pairs with prepare_to_wait's set_current_state() > > */ > > smp_mb(); > > - if (swait_active(&vcpu->wq)) > > - swake_up(&vcpu->wq); > > +#ifdef CONFIG_KVM_ASYNC_PF_SYNC > > + kvm_vcpu_wake_up(vcpu); > > +#else > > + kvm_vcpu_kick(vcpu); > > +#endif > > This will break s390, both functions are disabled for s390. > On s390 do not want to kick the CPU for a completion. Instead we implement > the kvm_async_page_present_sync call above and handle completion via an > "pfault done" interrupt via the normal interrupt delivery. Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390? It was unused until now, but the patch makes sense as a cleanup. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/02/2016 10:40 AM, Paolo Bonzini wrote: > > > ----- Original Message ----- >> From: "Christian Borntraeger" <borntraeger@de.ibm.com> >> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, >> kvm@vger.kernel.org >> Cc: "Denis Lunev" <den@virtuozzo.com> >> Sent: Friday, December 2, 2016 10:35:02 AM >> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved >> >> On 12/02/2016 09:47 AM, Roman Kagan wrote: >>> When async_pf is ready the guest needs to be made aware of it ASAP, >>> because it may be holding off a higher priority task pending the >>> async_pf resolution in favor of a lower priority one. >>> >>> In case async_pf's are harvested in vcpu context (x86) we have to not >>> only wake the vcpu up but kick it into host. >>> >>> While at this, also replace the open-coded vcpu wakeup by the existing >>> helper. >>> >>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> >>> --- >>> virt/kvm/async_pf.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c >>> index 9cced14..5f0a66c 100644 >>> --- a/virt/kvm/async_pf.c >>> +++ b/virt/kvm/async_pf.c >>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) >>> * This memory barrier pairs with prepare_to_wait's set_current_state() >>> */ >>> smp_mb(); >>> - if (swait_active(&vcpu->wq)) >>> - swake_up(&vcpu->wq); >>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC >>> + kvm_vcpu_wake_up(vcpu); >>> +#else >>> + kvm_vcpu_kick(vcpu); >>> +#endif >> >> This will break s390, both functions are disabled for s390. >> On s390 do not want to kick the CPU for a completion. Instead we implement >> the kvm_async_page_present_sync call above and handle completion via an >> "pfault done" interrupt via the normal interrupt delivery. > > Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390? > It was unused until now, but the patch makes sense as a cleanup. We could enable that. It was some kind of a trigger, that we get a build error when someone enables that for s390 (as it might not be want you want) On the other hand, I dont think that we need a wakeup at all for the SYNC case. (as the interrupt will do that anyway) Maybe something like that - if (swait_active(&vcpu->wq)) - swake_up(&vcpu->wq); +#ifndef CONFIG_KVM_ASYNC_PF_SYNC + kvm_vcpu_kick(vcpu); +#endif would be good enough. (needs more thinking to be sure) Christian -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 02, 2016 at 10:35:02AM +0100, Christian Borntraeger wrote: > On 12/02/2016 09:47 AM, Roman Kagan wrote: > > When async_pf is ready the guest needs to be made aware of it ASAP, > > because it may be holding off a higher priority task pending the > > async_pf resolution in favor of a lower priority one. > > > > In case async_pf's are harvested in vcpu context (x86) we have to not > > only wake the vcpu up but kick it into host. > > > > While at this, also replace the open-coded vcpu wakeup by the existing > > helper. > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > --- > > virt/kvm/async_pf.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > > index 9cced14..5f0a66c 100644 > > --- a/virt/kvm/async_pf.c > > +++ b/virt/kvm/async_pf.c > > @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) > > * This memory barrier pairs with prepare_to_wait's set_current_state() > > */ > > smp_mb(); > > - if (swait_active(&vcpu->wq)) > > - swake_up(&vcpu->wq); > > +#ifdef CONFIG_KVM_ASYNC_PF_SYNC > > + kvm_vcpu_wake_up(vcpu); > > +#else > > + kvm_vcpu_kick(vcpu); > > +#endif > > This will break s390, both functions are disabled for s390. Indeed. Sorry I didn't notice that #ifndef CONFIG_S390 ... > On s390 do not want to kick the CPU for a completion. Instead we implement > the kvm_async_page_present_sync call above and handle completion via an > "pfault done" interrupt via the normal interrupt delivery. I wonder if waking up is necessary at all then? Thanks, Roman. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 02, 2016 at 11:00:00AM +0100, Christian Borntraeger wrote: > On 12/02/2016 10:40 AM, Paolo Bonzini wrote: > > ----- Original Message ----- > >> From: "Christian Borntraeger" <borntraeger@de.ibm.com> > >> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, > >> kvm@vger.kernel.org > >> Cc: "Denis Lunev" <den@virtuozzo.com> > >> Sent: Friday, December 2, 2016 10:35:02 AM > >> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved > >> > >> On 12/02/2016 09:47 AM, Roman Kagan wrote: > >>> When async_pf is ready the guest needs to be made aware of it ASAP, > >>> because it may be holding off a higher priority task pending the > >>> async_pf resolution in favor of a lower priority one. > >>> > >>> In case async_pf's are harvested in vcpu context (x86) we have to not > >>> only wake the vcpu up but kick it into host. > >>> > >>> While at this, also replace the open-coded vcpu wakeup by the existing > >>> helper. > >>> > >>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > >>> --- > >>> virt/kvm/async_pf.c | 7 +++++-- > >>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c > >>> index 9cced14..5f0a66c 100644 > >>> --- a/virt/kvm/async_pf.c > >>> +++ b/virt/kvm/async_pf.c > >>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) > >>> * This memory barrier pairs with prepare_to_wait's set_current_state() > >>> */ > >>> smp_mb(); > >>> - if (swait_active(&vcpu->wq)) > >>> - swake_up(&vcpu->wq); > >>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC > >>> + kvm_vcpu_wake_up(vcpu); > >>> +#else > >>> + kvm_vcpu_kick(vcpu); > >>> +#endif > >> > >> This will break s390, both functions are disabled for s390. > >> On s390 do not want to kick the CPU for a completion. Instead we implement > >> the kvm_async_page_present_sync call above and handle completion via an > >> "pfault done" interrupt via the normal interrupt delivery. > > > > Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390? > > It was unused until now, but the patch makes sense as a cleanup. > > > We could enable that. It was some kind of a trigger, that we get a build error > when someone enables that for s390 (as it might not be want you want) > > On the other hand, I dont think that we need a wakeup at all for the > SYNC case. (as the interrupt will do that anyway) > Maybe something like that > > - if (swait_active(&vcpu->wq)) > - swake_up(&vcpu->wq); > +#ifndef CONFIG_KVM_ASYNC_PF_SYNC > + kvm_vcpu_kick(vcpu); > +#endif > > would be good enough. (needs more thinking to be sure) As my knowlege of s390 approaches zero so that only your thinking can be applied here ;) , how about me redoing this patch leaving the open-coded wakeup as is, and then you submitting another patch on top dropping that wakeup if you find that appropriate? Thanks, Roman. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roman,
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.9-rc7 next-20161202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roman-Kagan/kvm-avoid-delaying-async_pf-ready-delivery/20161203-081205
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-default_defconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
>> ERROR: "kvm_vcpu_wake_up" [arch/s390/kvm/kvm.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 12/02/2016 12:34 PM, Roman Kagan wrote: > On Fri, Dec 02, 2016 at 11:00:00AM +0100, Christian Borntraeger wrote: >> On 12/02/2016 10:40 AM, Paolo Bonzini wrote: >>> ----- Original Message ----- >>>> From: "Christian Borntraeger" <borntraeger@de.ibm.com> >>>> To: "Roman Kagan" <rkagan@virtuozzo.com>, "Radim Krčmář" <rkrcmar@redhat.com>, "Paolo Bonzini" <pbonzini@redhat.com>, >>>> kvm@vger.kernel.org >>>> Cc: "Denis Lunev" <den@virtuozzo.com> >>>> Sent: Friday, December 2, 2016 10:35:02 AM >>>> Subject: Re: [PATCH 3/5] kvm: kick vcpu when async_pf is resolved >>>> >>>> On 12/02/2016 09:47 AM, Roman Kagan wrote: >>>>> When async_pf is ready the guest needs to be made aware of it ASAP, >>>>> because it may be holding off a higher priority task pending the >>>>> async_pf resolution in favor of a lower priority one. >>>>> >>>>> In case async_pf's are harvested in vcpu context (x86) we have to not >>>>> only wake the vcpu up but kick it into host. >>>>> >>>>> While at this, also replace the open-coded vcpu wakeup by the existing >>>>> helper. >>>>> >>>>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> >>>>> --- >>>>> virt/kvm/async_pf.c | 7 +++++-- >>>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c >>>>> index 9cced14..5f0a66c 100644 >>>>> --- a/virt/kvm/async_pf.c >>>>> +++ b/virt/kvm/async_pf.c >>>>> @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) >>>>> * This memory barrier pairs with prepare_to_wait's set_current_state() >>>>> */ >>>>> smp_mb(); >>>>> - if (swait_active(&vcpu->wq)) >>>>> - swake_up(&vcpu->wq); >>>>> +#ifdef CONFIG_KVM_ASYNC_PF_SYNC >>>>> + kvm_vcpu_wake_up(vcpu); >>>>> +#else >>>>> + kvm_vcpu_kick(vcpu); >>>>> +#endif >>>> >>>> This will break s390, both functions are disabled for s390. >>>> On s390 do not want to kick the CPU for a completion. Instead we implement >>>> the kvm_async_page_present_sync call above and handle completion via an >>>> "pfault done" interrupt via the normal interrupt delivery. >>> >>> Is there any reason (with this patch) to disable kvm_vcpu_wake_up on s390? >>> It was unused until now, but the patch makes sense as a cleanup. >> >> >> We could enable that. It was some kind of a trigger, that we get a build error >> when someone enables that for s390 (as it might not be want you want) >> >> On the other hand, I dont think that we need a wakeup at all for the >> SYNC case. (as the interrupt will do that anyway) >> Maybe something like that >> >> - if (swait_active(&vcpu->wq)) >> - swake_up(&vcpu->wq); >> +#ifndef CONFIG_KVM_ASYNC_PF_SYNC >> + kvm_vcpu_kick(vcpu); >> +#endif >> >> would be good enough. (needs more thinking to be sure) > > As my knowlege of s390 approaches zero so that only your thinking can be > applied here ;) , how about me redoing this patch leaving the open-coded > wakeup as is, and then you submitting another patch on top dropping that > wakeup if you find that appropriate? Yes, please do you patch with the swake_up and I will come up with a followup. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roman,
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.9-rc8 next-20161205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Roman-Kagan/kvm-avoid-delaying-async_pf-ready-delivery/20161203-081205
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: s390-alldefconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=s390
All errors (new ones prefixed by >>):
arch/s390/built-in.o: In function `async_pf_execute':
>> arch/s390/kvm/../../../virt/kvm/async_pf.o:(.text+0x26cee): undefined reference to `kvm_vcpu_wake_up'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c index 9cced14..5f0a66c 100644 --- a/virt/kvm/async_pf.c +++ b/virt/kvm/async_pf.c @@ -105,8 +105,11 @@ static void async_pf_execute(struct work_struct *work) * This memory barrier pairs with prepare_to_wait's set_current_state() */ smp_mb(); - if (swait_active(&vcpu->wq)) - swake_up(&vcpu->wq); +#ifdef CONFIG_KVM_ASYNC_PF_SYNC + kvm_vcpu_wake_up(vcpu); +#else + kvm_vcpu_kick(vcpu); +#endif mmput(mm); kvm_put_kvm(vcpu->kvm);
When async_pf is ready the guest needs to be made aware of it ASAP, because it may be holding off a higher priority task pending the async_pf resolution in favor of a lower priority one. In case async_pf's are harvested in vcpu context (x86) we have to not only wake the vcpu up but kick it into host. While at this, also replace the open-coded vcpu wakeup by the existing helper. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- virt/kvm/async_pf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)