| Message ID | 20200422125810.34847-2-tianjia.zhang@linux.alibaba.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clean up redundant 'kvm_run' parameters | expand |
On Wed, 22 Apr 2020 20:58:04 +0800 Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> > --- > 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? > restore_gs_cb(current->thread.gs_cb); > } > preempt_enable();
On 22.04.20 15:45, Cornelia Huck wrote: > On Wed, 22 Apr 2020 20:58:04 +0800 > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >> --- >> 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.
On Wed, 22 Apr 2020 17:58:04 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 22.04.20 15:45, Cornelia Huck wrote: > > On Wed, 22 Apr 2020 20:58:04 +0800 > > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> > >> --- > >> 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.
On 2020/4/22 21:45, Cornelia Huck wrote: > On Wed, 22 Apr 2020 20:58:04 +0800 > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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/ ? > Yes, it should be replaced like this. >> 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@linux.alibaba.com> >> --- >> 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? > Why not replace `vcpu->run->` to `kvm_run->` ? If not, there will be both styles of code, which is confusing. I will be confused and think that this is something different. Thanks, Tianjia >> restore_gs_cb(current->thread.gs_cb); >> } >> preempt_enable();
On 2020/4/23 0:04, Cornelia Huck wrote: > On Wed, 22 Apr 2020 17:58:04 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 22.04.20 15:45, Cornelia Huck wrote: >>> On Wed, 22 Apr 2020 20:58:04 +0800 >>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>> --- >>>> 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. Thanks, Tianjia
On 2020/4/22 23:58, Christian Borntraeger wrote: > > > On 22.04.20 15:45, Cornelia Huck wrote: >> On Wed, 22 Apr 2020 20:58:04 +0800 >> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>> --- >>> 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. > Why not, `kvm_run` is equivalent to `vcpu->run`, which is also part of the cleanup, or do you mean to put this change in another patch? Thanks, Tianjia
On Thu, 23 Apr 2020 11:01:43 +0800 Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote: > On 2020/4/23 0:04, Cornelia Huck wrote: > > On Wed, 22 Apr 2020 17:58:04 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 22.04.20 15:45, Cornelia Huck wrote: > >>> On Wed, 22 Apr 2020 20:58:04 +0800 > >>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> > >>>> --- > >>>> 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).
On 2020/4/23 18:39, Cornelia Huck wrote: > On Thu, 23 Apr 2020 11:01:43 +0800 > Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote: > >> On 2020/4/23 0:04, Cornelia Huck wrote: >>> On Wed, 22 Apr 2020 17:58:04 +0200 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> On 22.04.20 15:45, Cornelia Huck wrote: >>>>> On Wed, 22 Apr 2020 20:58:04 +0800 >>>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>>>> --- >>>>>> 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?
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@linux.alibaba.com> wrote: >> >>> On 2020/4/23 0:04, Cornelia Huck wrote: >>>> On Wed, 22 Apr 2020 17:58:04 +0200 >>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>> >>>>> On 22.04.20 15:45, Cornelia Huck wrote: >>>>>> On Wed, 22 Apr 2020 20:58:04 +0800 >>>>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>>>>> --- >>>>>>> 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. And I agree with Conny: the cost is higher than the benefit.
On 2020/4/23 19: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@linux.alibaba.com> wrote: >>> >>>> On 2020/4/23 0:04, Cornelia Huck wrote: >>>>> On Wed, 22 Apr 2020 17:58:04 +0200 >>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>>> >>>>>> On 22.04.20 15:45, Cornelia Huck wrote: >>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800 >>>>>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>>>>>> --- >>>>>>>> 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. > And I agree with Conny: the cost is higher than the benefit. > I will make a fix in the v3 version. Help to see if there are problems with the next few patches. Thanks, Tianjia
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@linux.alibaba.com> wrote: >>> >>>> On 2020/4/23 0:04, Cornelia Huck wrote: >>>>> On Wed, 22 Apr 2020 17:58:04 +0200 >>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>>> >>>>>> On 22.04.20 15:45, Cornelia Huck wrote: >>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800 >>>>>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>>>>>> --- >>>>>>>> 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
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@linux.alibaba.com> wrote: >>>> >>>>> On 2020/4/23 0:04, Cornelia Huck wrote: >>>>>> On Wed, 22 Apr 2020 17:58:04 +0200 >>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>>>>> >>>>>>> On 22.04.20 15:45, Cornelia Huck wrote: >>>>>>>> On Wed, 22 Apr 2020 20:58:04 +0800 >>>>>>>> Tianjia Zhang <tianjia.zhang@linux.alibaba.com> 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@linux.alibaba.com> >>>>>>>>> --- >>>>>>>>> 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
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; restore_gs_cb(current->thread.gs_cb); } preempt_enable(); @@ -4243,8 +4244,10 @@ static void sync_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) /* SIE will load etoken directly from SDNX and therefore kvm_run */ } -static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void sync_regs(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + if (kvm_run->kvm_dirty_regs & KVM_SYNC_PREFIX) kvm_s390_set_prefix(vcpu, kvm_run->s.regs.prefix); if (kvm_run->kvm_dirty_regs & KVM_SYNC_CRS) { @@ -4257,23 +4260,23 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) vcpu->arch.sie_block->ckc = kvm_run->s.regs.ckc; } save_access_regs(vcpu->arch.host_acrs); - restore_access_regs(vcpu->run->s.regs.acrs); + restore_access_regs(kvm_run->s.regs.acrs); /* save host (userspace) fprs/vrs */ save_fpu_regs(); vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc; vcpu->arch.host_fpregs.regs = current->thread.fpu.regs; if (MACHINE_HAS_VX) - current->thread.fpu.regs = vcpu->run->s.regs.vrs; + current->thread.fpu.regs = kvm_run->s.regs.vrs; else - current->thread.fpu.regs = vcpu->run->s.regs.fprs; - current->thread.fpu.fpc = vcpu->run->s.regs.fpc; + current->thread.fpu.regs = kvm_run->s.regs.fprs; + current->thread.fpu.fpc = kvm_run->s.regs.fpc; if (test_fp_ctl(current->thread.fpu.fpc)) /* User space provided an invalid FPC, let's clear it */ current->thread.fpu.fpc = 0; /* Sync fmt2 only data */ if (likely(!kvm_s390_pv_cpu_is_protected(vcpu))) { - sync_regs_fmt2(vcpu, kvm_run); + sync_regs_fmt2(vcpu); } else { /* * In several places we have to modify our internal view to @@ -4292,8 +4295,10 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) kvm_run->kvm_dirty_regs = 0; } -static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void store_regs_fmt2(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + kvm_run->s.regs.todpr = vcpu->arch.sie_block->todpr; kvm_run->s.regs.pp = vcpu->arch.sie_block->pp; kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; @@ -4313,8 +4318,10 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) /* SIE will save etoken directly into SDNX and therefore kvm_run */ } -static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) +static void store_regs(struct kvm_vcpu *vcpu) { + struct kvm_run *kvm_run = vcpu->run; + kvm_run->psw_mask = vcpu->arch.sie_block->gpsw.mask; kvm_run->psw_addr = vcpu->arch.sie_block->gpsw.addr; kvm_run->s.regs.prefix = kvm_s390_get_prefix(vcpu); @@ -4324,16 +4331,16 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) kvm_run->s.regs.pft = vcpu->arch.pfault_token; kvm_run->s.regs.pfs = vcpu->arch.pfault_select; kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; - save_access_regs(vcpu->run->s.regs.acrs); + save_access_regs(kvm_run->s.regs.acrs); restore_access_regs(vcpu->arch.host_acrs); /* Save guest register state */ save_fpu_regs(); - vcpu->run->s.regs.fpc = current->thread.fpu.fpc; + kvm_run->s.regs.fpc = current->thread.fpu.fpc; /* Restore will be done lazily at return */ current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; current->thread.fpu.regs = vcpu->arch.host_fpregs.regs; if (likely(!kvm_s390_pv_cpu_is_protected(vcpu))) - store_regs_fmt2(vcpu, kvm_run); + store_regs_fmt2(vcpu); } int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) @@ -4371,7 +4378,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) goto out; } - sync_regs(vcpu, kvm_run); + sync_regs(vcpu); enable_cpu_timer_accounting(vcpu); might_fault(); @@ -4393,7 +4400,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu) } disable_cpu_timer_accounting(vcpu); - store_regs(vcpu, kvm_run); + store_regs(vcpu); kvm_sigset_deactivate(vcpu);
In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu' structure. Earlier than historical reasons, many kvm-related function 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@linux.alibaba.com> --- arch/s390/kvm/kvm-s390.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)