| Message ID | 20190920212509.2578-4-aarcange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM monolithic v1 | expand |
On Fri, Sep 20, 2019 at 05:24:55PM -0400, Andrea Arcangeli wrote: > request_immediate_exit is one of those few cases where the pointer to > function of the method isn't fixed at build time and it requires > special handling because hardware_setup() may override it at runtime. > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> > --- > arch/x86/kvm/vmx/vmx_ops.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/vmx_ops.c b/arch/x86/kvm/vmx/vmx_ops.c > index cdcad73935d9..25d441432901 100644 > --- a/arch/x86/kvm/vmx/vmx_ops.c > +++ b/arch/x86/kvm/vmx/vmx_ops.c > @@ -498,7 +498,10 @@ int kvm_x86_ops_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) > > void kvm_x86_ops_request_immediate_exit(struct kvm_vcpu *vcpu) > { > - vmx_request_immediate_exit(vcpu); > + if (likely(enable_preemption_timer)) > + vmx_request_immediate_exit(vcpu); > + else > + __kvm_request_immediate_exit(vcpu); Rather than wrap this in VMX code, what if we instead take advantage of a monolithic module and add an inline to query enable_preemption_timer? That'd likely save a few CALL/RET/JMP instructions and eliminate __kvm_request_immediate_exit. E.g. something like: if (req_immediate_exit) { kvm_make_request(KVM_REQ_EVENT, vcpu); if (kvm_x86_has_request_immediate_exit()) kvm_x86_request_immediate_exit(vcpu); else smp_send_reschedule(vcpu->cpu); } > } > > void kvm_x86_ops_sched_in(struct kvm_vcpu *kvm, int cpu)
On Mon, Sep 23, 2019 at 03:35:26PM -0700, Sean Christopherson wrote: > On Fri, Sep 20, 2019 at 05:24:55PM -0400, Andrea Arcangeli wrote: > > request_immediate_exit is one of those few cases where the pointer to > > function of the method isn't fixed at build time and it requires > > special handling because hardware_setup() may override it at runtime. > > > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> > > --- > > arch/x86/kvm/vmx/vmx_ops.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/vmx_ops.c b/arch/x86/kvm/vmx/vmx_ops.c > > index cdcad73935d9..25d441432901 100644 > > --- a/arch/x86/kvm/vmx/vmx_ops.c > > +++ b/arch/x86/kvm/vmx/vmx_ops.c > > @@ -498,7 +498,10 @@ int kvm_x86_ops_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) > > > > void kvm_x86_ops_request_immediate_exit(struct kvm_vcpu *vcpu) > > { > > - vmx_request_immediate_exit(vcpu); > > + if (likely(enable_preemption_timer)) > > + vmx_request_immediate_exit(vcpu); > > + else > > + __kvm_request_immediate_exit(vcpu); > > Rather than wrap this in VMX code, what if we instead take advantage of a > monolithic module and add an inline to query enable_preemption_timer? > That'd likely save a few CALL/RET/JMP instructions and eliminate > __kvm_request_immediate_exit. > > E.g. something like: > > if (req_immediate_exit) { > kvm_make_request(KVM_REQ_EVENT, vcpu); > if (kvm_x86_has_request_immediate_exit()) > kvm_x86_request_immediate_exit(vcpu); > else > smp_send_reschedule(vcpu->cpu); > } Yes, I mentioned the inlining possibilities in part of comment of 2/17: === Further incremental minor optimizations that weren't possible before are now enabled by the monolithic model. For example it would be possible later to convert some of the small external methods to inline functions from the {svm,vmx}_ops.c file to kvm_ops.h. However that will require more Makefile tweaks. === To implement your kvm_x86_has_request_immediate_exit() we need more Makefile tweaking, basically we need a -D__SVM__ -D__VMX__ kind of thing so we can put an #ifdef __SVM__ in the kvm_ops.h equivalent to put inline code there. Or some other better solution if you think of any, that was my only idea so far with regard to inlining. If you can sort out the solution to enable the inlining of svm/vmx code that would be great. However I think all optimizations allowed by the monolithic model that aren't related to the full retpoline overhead removal, should be kept incremental. I rather prefer to keep the inner working of the kvm_x86_ops to be 100% functional equivalent in the initial conversion, so things remains also more bisectable just in case and to keep the optimizations incremental. In addition to the inline optimization there is the cleanup of the kvm_x86_ops that is lots of work left to do. To do that it requires a lot of changes to every place that checks the kvm_x86_ops->something pointer or that isn't initialized statically. I didn't even try to implement that part because I wanted to get to a point that was fully equivalent for easier review and fewer risk of breakage. So I sent the patchset at the point when there were zero reptolines left running in the KVM code, but more work is required to fully leverage the monolithic approach and eliminate the now mostly dead code of kvm_x86_ops structure. I would rather prioritize on the changes required to the full removal of kvm_x86_ops before further inline optimizations, but it would be fine to do inline optimizations first as long as they're not mixed up with an initial simpler conversion to monolithic model. Thanks, Andrea
On Mon, Sep 23, 2019 at 07:06:26PM -0400, Andrea Arcangeli wrote: > On Mon, Sep 23, 2019 at 03:35:26PM -0700, Sean Christopherson wrote: > > On Fri, Sep 20, 2019 at 05:24:55PM -0400, Andrea Arcangeli wrote: > > > request_immediate_exit is one of those few cases where the pointer to > > > function of the method isn't fixed at build time and it requires > > > special handling because hardware_setup() may override it at runtime. > > > > > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> > > > --- > > > arch/x86/kvm/vmx/vmx_ops.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kvm/vmx/vmx_ops.c b/arch/x86/kvm/vmx/vmx_ops.c > > > index cdcad73935d9..25d441432901 100644 > > > --- a/arch/x86/kvm/vmx/vmx_ops.c > > > +++ b/arch/x86/kvm/vmx/vmx_ops.c > > > @@ -498,7 +498,10 @@ int kvm_x86_ops_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) > > > > > > void kvm_x86_ops_request_immediate_exit(struct kvm_vcpu *vcpu) > > > { > > > - vmx_request_immediate_exit(vcpu); > > > + if (likely(enable_preemption_timer)) > > > + vmx_request_immediate_exit(vcpu); > > > + else > > > + __kvm_request_immediate_exit(vcpu); > > > > Rather than wrap this in VMX code, what if we instead take advantage of a > > monolithic module and add an inline to query enable_preemption_timer? > > That'd likely save a few CALL/RET/JMP instructions and eliminate > > __kvm_request_immediate_exit. > > > > E.g. something like: > > > > if (req_immediate_exit) { > > kvm_make_request(KVM_REQ_EVENT, vcpu); > > if (kvm_x86_has_request_immediate_exit()) > > kvm_x86_request_immediate_exit(vcpu); > > else > > smp_send_reschedule(vcpu->cpu); > > } > > Yes, I mentioned the inlining possibilities in part of comment of > 2/17: > > === > Further incremental minor optimizations that weren't possible before > are now enabled by the monolithic model. For example it would be > possible later to convert some of the small external methods to inline > functions from the {svm,vmx}_ops.c file to kvm_ops.h. However that > will require more Makefile tweaks. > === With a straight rename to kvm_x86_<function>() instead of wrappers, we shouldn't need kvm_ops.c. kvm_ops.h might be helpful, but it'd be just as easy to keep them in kvm_host.h and would likely yield a more insightful diff[*]. > To implement your kvm_x86_has_request_immediate_exit() we need more > Makefile tweaking, basically we need a -D__SVM__ -D__VMX__ kind of > thing so we can put an #ifdef __SVM__ in the kvm_ops.h equivalent to > put inline code there. Or some other better solution if you think of > any, that was my only idea so far with regard to inlining. Hmm, I was thinking more along the lines of extending the kvm_host.h pattern down into vendor specific code, e.g. arch/x86/kvm/vmx/kvm_host.h. Probably with a different name though, two of those is confusing enough. It'd still need Makefile changes, but we wouldn't litter the code with #ifdefs. Future enhancments can also take advantage of the per-vendor header to inline other things. Such a header would also make it possible to fully remove kvm_x86_ops in this series (I think). [*] Tying into the thought above, if we go for a straight rename and eliminate the conditionally-implemented kvm_x86_ops ahead of time, e.g. with inlines that return -EINVAL or something, then the conversion to direct calls can be a straight replacement of "kvm_x86_ops->" with "kvm_x86_" at the same time the declarations are changed from members of kvm_x86_ops to externs. Actually, typing out the above made me realize the immediate exit code can be: if (req_immediate_exit) { kvm_make_request(KVM_REQ_EVENT, vcpu); if (kvm_x86_request_immediate_exit(vcpu)) smp_send_reschedule(vcpu->cpu); } Where kvm_x86_request_immediate_exit() returns 0 on success, e.g. the SVM implementation can be "return -EINVAL" or whatever is appropriate, which I assume the compiler can optimize out. Or maybe a boolean return is better in this case?
On Mon, Sep 23, 2019 at 04:45:00PM -0700, Sean Christopherson wrote: > With a straight rename to kvm_x86_<function>() instead of wrappers, we > shouldn't need kvm_ops.c. kvm_ops.h might be helpful, but it'd be just > as easy to keep them in kvm_host.h and would likely yield a more > insightful diff[*]. Yes, I already commented about this change Paolo asked. > Hmm, I was thinking more along the lines of extending the kvm_host.h > pattern down into vendor specific code, e.g. arch/x86/kvm/vmx/kvm_host.h. > Probably with a different name though, two of those is confusing enough. > > It'd still need Makefile changes, but we wouldn't litter the code with > #ifdefs. Future enhancments can also take advantage of the per-vendor > header to inline other things. Such a header would also make it possible > to fully remove kvm_x86_ops in this series (I think). It's common in include/linux/* to include the same .h file that just implements the inlines depending on some #ifdef, but in this case it's simpler to have different .h files to keep the two versions more separated as they may be maintained by different groups, so including different .h file sounds better than -D__VMX__ -D__VMX__ agreed. > [*] Tying into the thought above, if we go for a straight rename and > eliminate the conditionally-implemented kvm_x86_ops ahead of time, > e.g. with inlines that return -EINVAL or something, then the > conversion to direct calls can be a straight replacement of > "kvm_x86_ops->" with "kvm_x86_" at the same time the declarations > are changed from members of kvm_x86_ops to externs. > > Actually, typing out the above made me realize the immediate exit code > can be: > > if (req_immediate_exit) { > kvm_make_request(KVM_REQ_EVENT, vcpu); > if (kvm_x86_request_immediate_exit(vcpu)) > smp_send_reschedule(vcpu->cpu); > } > > Where kvm_x86_request_immediate_exit() returns 0 on success, e.g. the SVM > implementation can be "return -EINVAL" or whatever is appropriate, which > I assume the compiler can optimize out. Or maybe a boolean return is > better in this case? While the final cleanup of kvm_x86_ops doesn't strictly require the inlining Makefile tricks to be functional just yet, it would be beneficial to remove some branch at runtime from non frequently invoked methods and to get the final optimal implementation sorted out during the initial cleanup of the structure, this should reduce the number of patches to get to the most optimal possible end result. So I think making the inline work in the Makefile as a dependency to remove kvm_x86_ops is a fine plan. Thanks, Andrea
diff --git a/arch/x86/kvm/vmx/vmx_ops.c b/arch/x86/kvm/vmx/vmx_ops.c index cdcad73935d9..25d441432901 100644 --- a/arch/x86/kvm/vmx/vmx_ops.c +++ b/arch/x86/kvm/vmx/vmx_ops.c @@ -498,7 +498,10 @@ int kvm_x86_ops_check_nested_events(struct kvm_vcpu *vcpu, bool external_intr) void kvm_x86_ops_request_immediate_exit(struct kvm_vcpu *vcpu) { - vmx_request_immediate_exit(vcpu); + if (likely(enable_preemption_timer)) + vmx_request_immediate_exit(vcpu); + else + __kvm_request_immediate_exit(vcpu); } void kvm_x86_ops_sched_in(struct kvm_vcpu *kvm, int cpu)
request_immediate_exit is one of those few cases where the pointer to function of the method isn't fixed at build time and it requires special handling because hardware_setup() may override it at runtime. Signed-off-by: Andrea Arcangeli <aarcange@redhat.com> --- arch/x86/kvm/vmx/vmx_ops.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)