| Message ID | 20200413034523.110548-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Optimize kvm_arch_vcpu_ioctl_run function | expand |
Tianjia, On 2020-04-13 04:45, Tianjia Zhang wrote: > kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, > where vcpu->run is the kvm_run parameter, so it has been replaced. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- > arch/x86/kvm/x86.c | 8 ++++---- > virt/kvm/arm/arm.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bf2ecafd027..70e3f4abbd4d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu > *vcpu, struct kvm_run *kvm_run) > r = -EAGAIN; > if (signal_pending(current)) { > r = -EINTR; > - vcpu->run->exit_reason = KVM_EXIT_INTR; > + kvm_run->exit_reason = KVM_EXIT_INTR; > ++vcpu->stat.signal_exits; > } > goto out; > } > > - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > r = -EINVAL; > goto out; > } > > - if (vcpu->run->kvm_dirty_regs) { > + if (kvm_run->kvm_dirty_regs) { > r = sync_regs(vcpu); > if (r != 0) > goto out; > @@ -8767,7 +8767,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu > *vcpu, struct kvm_run *kvm_run) > > out: > kvm_put_guest_fpu(vcpu); > - if (vcpu->run->kvm_valid_regs) > + if (kvm_run->kvm_valid_regs) > store_regs(vcpu); > post_kvm_run_save(vcpu); > kvm_sigset_deactivate(vcpu); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 48d0ec44ad77..ab9d7966a4c8 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, > struct kvm_run *run) > return ret; > > if (run->exit_reason == KVM_EXIT_MMIO) { > - ret = kvm_handle_mmio_return(vcpu, vcpu->run); > + ret = kvm_handle_mmio_return(vcpu, run); > if (ret) > return ret; > } Do you have any number supporting the idea that you are optimizing anything here? Performance, code size, register pressure or any other relevant metric? Thanks, M.
On 2020/4/13 16:56, Marc Zyngier wrote: > Tianjia, > > On 2020-04-13 04:45, Tianjia Zhang wrote: >> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, >> where vcpu->run is the kvm_run parameter, so it has been replaced. >> >> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >> --- >> arch/x86/kvm/x86.c | 8 ++++---- >> virt/kvm/arm/arm.c | 2 +- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 3bf2ecafd027..70e3f4abbd4d 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >> *vcpu, struct kvm_run *kvm_run) >> r = -EAGAIN; >> if (signal_pending(current)) { >> r = -EINTR; >> - vcpu->run->exit_reason = KVM_EXIT_INTR; >> + kvm_run->exit_reason = KVM_EXIT_INTR; >> ++vcpu->stat.signal_exits; >> } >> goto out; >> } >> >> - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> r = -EINVAL; >> goto out; >> } >> >> - if (vcpu->run->kvm_dirty_regs) { >> + if (kvm_run->kvm_dirty_regs) { >> r = sync_regs(vcpu); >> if (r != 0) >> goto out; >> @@ -8767,7 +8767,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu >> *vcpu, struct kvm_run *kvm_run) >> >> out: >> kvm_put_guest_fpu(vcpu); >> - if (vcpu->run->kvm_valid_regs) >> + if (kvm_run->kvm_valid_regs) >> store_regs(vcpu); >> post_kvm_run_save(vcpu); >> kvm_sigset_deactivate(vcpu); >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 48d0ec44ad77..ab9d7966a4c8 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, >> struct kvm_run *run) >> return ret; >> >> if (run->exit_reason == KVM_EXIT_MMIO) { >> - ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> + ret = kvm_handle_mmio_return(vcpu, run); >> if (ret) >> return ret; >> } > > Do you have any number supporting the idea that you are optimizing anything > here? Performance, code size, register pressure or any other relevant > metric? > > Thanks, > > M. This is only a simplified implementation of the function, the impact on performance and register pressure can be ignored. Thanks, Tianjia
Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, > where vcpu->run is the kvm_run parameter, so it has been replaced. > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > --- > arch/x86/kvm/x86.c | 8 ++++---- > virt/kvm/arm/arm.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3bf2ecafd027..70e3f4abbd4d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > r = -EAGAIN; > if (signal_pending(current)) { > r = -EINTR; > - vcpu->run->exit_reason = KVM_EXIT_INTR; > + kvm_run->exit_reason = KVM_EXIT_INTR; I have a more generic question: why do we need to pass 'kvm_run' to kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'? The only call site looks like virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); > ++vcpu->stat.signal_exits; > } > goto out; > } > > - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { > r = -EINVAL; > goto out; > } > > - if (vcpu->run->kvm_dirty_regs) { > + if (kvm_run->kvm_dirty_regs) { > r = sync_regs(vcpu); > if (r != 0) > goto out; > @@ -8767,7 +8767,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) > > out: > kvm_put_guest_fpu(vcpu); > - if (vcpu->run->kvm_valid_regs) > + if (kvm_run->kvm_valid_regs) > store_regs(vcpu); > post_kvm_run_save(vcpu); > kvm_sigset_deactivate(vcpu); > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index 48d0ec44ad77..ab9d7966a4c8 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > return ret; > > if (run->exit_reason == KVM_EXIT_MMIO) { > - ret = kvm_handle_mmio_return(vcpu, vcpu->run); > + ret = kvm_handle_mmio_return(vcpu, run); > if (ret) > return ret; > }
On 2020/4/14 22:26, Vitaly Kuznetsov wrote: > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > >> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, >> where vcpu->run is the kvm_run parameter, so it has been replaced. >> >> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >> --- >> arch/x86/kvm/x86.c | 8 ++++---- >> virt/kvm/arm/arm.c | 2 +- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 3bf2ecafd027..70e3f4abbd4d 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >> r = -EAGAIN; >> if (signal_pending(current)) { >> r = -EINTR; >> - vcpu->run->exit_reason = KVM_EXIT_INTR; >> + kvm_run->exit_reason = KVM_EXIT_INTR; > > I have a more generic question: why do we need to pass 'kvm_run' to > kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'? > The only call site looks like > > virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); > In the earlier version, kvm_run is used to pass parameters with user mode and is not included in the vcpu structure, so it has been retained until now. Thanks, Tianjia >> ++vcpu->stat.signal_exits; >> } >> goto out; >> } >> >> - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { >> r = -EINVAL; >> goto out; >> } >> >> - if (vcpu->run->kvm_dirty_regs) { >> + if (kvm_run->kvm_dirty_regs) { >> r = sync_regs(vcpu); >> if (r != 0) >> goto out; >> @@ -8767,7 +8767,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >> >> out: >> kvm_put_guest_fpu(vcpu); >> - if (vcpu->run->kvm_valid_regs) >> + if (kvm_run->kvm_valid_regs) >> store_regs(vcpu); >> post_kvm_run_save(vcpu); >> kvm_sigset_deactivate(vcpu); >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c >> index 48d0ec44ad77..ab9d7966a4c8 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> return ret; >> >> if (run->exit_reason == KVM_EXIT_MMIO) { >> - ret = kvm_handle_mmio_return(vcpu, vcpu->run); >> + ret = kvm_handle_mmio_return(vcpu, run); >> if (ret) >> return ret; >> } >
Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: > On 2020/4/14 22:26, Vitaly Kuznetsov wrote: >> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> writes: >> >>> kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, >>> where vcpu->run is the kvm_run parameter, so it has been replaced. >>> >>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >>> --- >>> arch/x86/kvm/x86.c | 8 ++++---- >>> virt/kvm/arm/arm.c | 2 +- >>> 2 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 3bf2ecafd027..70e3f4abbd4d 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) >>> r = -EAGAIN; >>> if (signal_pending(current)) { >>> r = -EINTR; >>> - vcpu->run->exit_reason = KVM_EXIT_INTR; >>> + kvm_run->exit_reason = KVM_EXIT_INTR; >> >> I have a more generic question: why do we need to pass 'kvm_run' to >> kvm_arch_vcpu_ioctl_run() if it can be extracted from 'struct kvm_vcpu'? >> The only call site looks like >> >> virt/kvm/kvm_main.c: r = kvm_arch_vcpu_ioctl_run(vcpu, vcpu->run); >> > > In the earlier version, kvm_run is used to pass parameters with user > mode and is not included in the vcpu structure, so it has been retained > until now. > In case this is no longer needed I'd suggest we drop 'kvm_run' parameter and extract it from 'struct kvm_vcpu' when needed. This looks like a natural add-on to your cleanup patch.
On 15/04/20 11:07, Vitaly Kuznetsov wrote: > In case this is no longer needed I'd suggest we drop 'kvm_run' parameter > and extract it from 'struct kvm_vcpu' when needed. This looks like a > natural add-on to your cleanup patch. I agree, though I think it should be _instead_ of Tianjia's patch rather than on top. Paolo
On 2020/4/15 22:53, Paolo Bonzini wrote: > On 15/04/20 11:07, Vitaly Kuznetsov wrote: >> In case this is no longer needed I'd suggest we drop 'kvm_run' parameter >> and extract it from 'struct kvm_vcpu' when needed. This looks like a >> natural add-on to your cleanup patch. > > I agree, though I think it should be _instead_ of Tianjia's patch rather > than on top. > > Paolo > Thank you very much for the comments of Vitaly and Paolo, I will make a v2 patch. Thanks and best, Tianjia
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3bf2ecafd027..70e3f4abbd4d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -8726,18 +8726,18 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) r = -EAGAIN; if (signal_pending(current)) { r = -EINTR; - vcpu->run->exit_reason = KVM_EXIT_INTR; + kvm_run->exit_reason = KVM_EXIT_INTR; ++vcpu->stat.signal_exits; } goto out; } - if (vcpu->run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { + if (kvm_run->kvm_valid_regs & ~KVM_SYNC_X86_VALID_FIELDS) { r = -EINVAL; goto out; } - if (vcpu->run->kvm_dirty_regs) { + if (kvm_run->kvm_dirty_regs) { r = sync_regs(vcpu); if (r != 0) goto out; @@ -8767,7 +8767,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) out: kvm_put_guest_fpu(vcpu); - if (vcpu->run->kvm_valid_regs) + if (kvm_run->kvm_valid_regs) store_regs(vcpu); post_kvm_run_save(vcpu); kvm_sigset_deactivate(vcpu); diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index 48d0ec44ad77..ab9d7966a4c8 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -659,7 +659,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) return ret; if (run->exit_reason == KVM_EXIT_MMIO) { - ret = kvm_handle_mmio_return(vcpu, vcpu->run); + ret = kvm_handle_mmio_return(vcpu, run); if (ret) return ret; }
kvm_arch_vcpu_ioctl_run() is only called in the file kvm_main.c, where vcpu->run is the kvm_run parameter, so it has been replaced. Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> --- arch/x86/kvm/x86.c | 8 ++++---- virt/kvm/arm/arm.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)