On 2020/4/26 20:59, Thomas Huth wrote:
On 23/04/2020 13.00, Christian Borntraeger wrote:
On 23.04.20 12:58, Tianjia Zhang wrote:
On 2020/4/23 18:39, Cornelia Huck wrote:
On Thu, 23 Apr 2020 11:01:43 +0800
Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx> wrote:
On 2020/4/23 0:04, Cornelia Huck wrote:
On Wed, 22 Apr 2020 17:58:04 +0200
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
On 22.04.20 15:45, Cornelia Huck wrote:
On Wed, 22 Apr 2020 20:58:04 +0800
Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx> wrote:
In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu'
structure. Earlier than historical reasons, many kvm-related function
s/Earlier than/For/ ?
parameters retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time.
This patch does a unified cleanup of these remaining redundant parameters.
Signed-off-by: Tianjia Zhang <tianjia.zhang@xxxxxxxxxxxxxxxxx>
---
arch/s390/kvm/kvm-s390.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index e335a7e5ead7..d7bb2e7a07ff 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4176,8 +4176,9 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
return rc;
}
-static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
+static void sync_regs_fmt2(struct kvm_vcpu *vcpu)
{
+ struct kvm_run *kvm_run = vcpu->run;
struct runtime_instr_cb *riccb;
struct gs_cb *gscb;
@@ -4235,7 +4236,7 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
}
if (vcpu->arch.gs_enabled) {
current->thread.gs_cb = (struct gs_cb *)
- &vcpu->run->s.regs.gscb;
+ &kvm_run->s.regs.gscb;
Not sure if these changes (vcpu->run-> => kvm_run->) are really worth
it. (It seems they amount to at least as much as the changes advertised
in the patch description.)
Other opinions?
Agreed. It feels kind of random. Maybe just do the first line (move kvm_run from the
function parameter list into the variable declaration)? Not sure if this is better.
There's more in this patch that I cut... but I think just moving
kvm_run from the parameter list would be much less disruptive.
I think there are two kinds of code(`vcpu->run->` and `kvm_run->`), but
there will be more disruptive, not less.
I just fail to see the benefit; sure, kvm_run-> is convenient, but the
current code is just fine, and any rework should be balanced against
the cost (e.g. cluttering git annotate).
cluttering git annotate ? Does it mean Fix xxxx ("comment"). Is it possible to solve this problem by splitting this patch?
No its about breaking git blame (and bugfix backports) for just a cosmetic improvement.
It could be slightly more than a cosmetic improvement (depending on the
smartness of the compiler): vcpu->run-> are two dereferences, while
kvm_run-> is only one dereference. So it could be slightly more compact
and faster code.
Thomas
If the compiler is smart enough, this place can be automatically
optimized, but we can't just rely on the compiler, if not? This requires
a trade-off between code cleanliness readability and breaking git blame.
In addition, I have removed the changes here and sent a v4 patch. Please
also help review it.
Thanks and best,
Tianjia
