| Message ID | b910f89cf11f6916319f9a2fb48d9146005318b1.1624266901.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: cppc: Add support for frequency invariance | expand |
On Monday 21 Jun 2021 at 14:49:35 (+0530), Viresh Kumar wrote: > Don't pass structure instance by value, pass it by reference instead. > Might be best to justify the change a bit :) For me this is a judgement call, and I don't really see the reasons for changing it: we don't care if the structure is modified or not, as we're not reusing the data after the call to cppc_get_rate_from_fbctrs(). More so, in this scenario we might not even want for the called function to modify the counter values. Also there is no further call to a function in cppc_get_rate_from_fbctrs(), that might require references to the fb_ctrs. So what is the reason behind this change? Thanks, Ionela. > Tested-by: Vincent Guittot <vincent.guittot@linaro.org> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cppc_cpufreq.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index 35b8ae66d1fb..490175d65082 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -373,18 +373,18 @@ static inline u64 get_delta(u64 t1, u64 t0) > } > > static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data, > - struct cppc_perf_fb_ctrs fb_ctrs_t0, > - struct cppc_perf_fb_ctrs fb_ctrs_t1) > + struct cppc_perf_fb_ctrs *fb_ctrs_t0, > + struct cppc_perf_fb_ctrs *fb_ctrs_t1) > { > u64 delta_reference, delta_delivered; > u64 reference_perf, delivered_perf; > > - reference_perf = fb_ctrs_t0.reference_perf; > + reference_perf = fb_ctrs_t0->reference_perf; > > - delta_reference = get_delta(fb_ctrs_t1.reference, > - fb_ctrs_t0.reference); > - delta_delivered = get_delta(fb_ctrs_t1.delivered, > - fb_ctrs_t0.delivered); > + delta_reference = get_delta(fb_ctrs_t1->reference, > + fb_ctrs_t0->reference); > + delta_delivered = get_delta(fb_ctrs_t1->delivered, > + fb_ctrs_t0->delivered); > > /* Check to avoid divide-by zero */ > if (delta_reference || delta_delivered) > @@ -415,7 +415,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > if (ret) > return ret; > > - return cppc_get_rate_from_fbctrs(cpu_data, fb_ctrs_t0, fb_ctrs_t1); > + return cppc_get_rate_from_fbctrs(cpu_data, &fb_ctrs_t0, &fb_ctrs_t1); > } > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > -- > 2.31.1.272.g89b43f80a514 >
On 23-06-21, 14:45, Ionela Voinescu wrote: > On Monday 21 Jun 2021 at 14:49:35 (+0530), Viresh Kumar wrote: > > Don't pass structure instance by value, pass it by reference instead. > > > > Might be best to justify the change a bit :) I had it and removed later as I thought it would be obvious :) > For me this is a judgement call, and I don't really see the reasons for > changing it: we don't care if the structure is modified or not, as we're > not reusing the data after the call to cppc_get_rate_from_fbctrs(). > More so, in this scenario we might not even want for the called function > to modify the counter values. Also there is no further call to a function > in cppc_get_rate_from_fbctrs(), that might require references to the > fb_ctrs. > > So what is the reason behind this change? How about this commit log then: Theoretically speaking, call by reference is cheaper/faster than call by value for structures as the later requires the compiler to make a new copy of the whole structure (which has four u64 values here), to be used by the called function, while with call by reference we just need to pass a single pointer (u64 on 64-bit architectures) to the existing structure. Yes, on modern architectures, the compilers will likely end up using the processor registers for passing this structure as it isn't doesn't have lot of fields and it shouldn't be bad eventually, but nevertheless the code should do the right thing without assuming about the compiler's or architecture's optimizations. Don't pass structure instance by value, pass it by reference instead.
Hey, On Thursday 24 Jun 2021 at 07:52:52 (+0530), Viresh Kumar wrote: > On 23-06-21, 14:45, Ionela Voinescu wrote: > > On Monday 21 Jun 2021 at 14:49:35 (+0530), Viresh Kumar wrote: > > > Don't pass structure instance by value, pass it by reference instead. > > > > > > > Might be best to justify the change a bit :) > > I had it and removed later as I thought it would be obvious :) > > > For me this is a judgement call, and I don't really see the reasons for > > changing it: we don't care if the structure is modified or not, as we're > > not reusing the data after the call to cppc_get_rate_from_fbctrs(). > > More so, in this scenario we might not even want for the called function > > to modify the counter values. Also there is no further call to a function > > in cppc_get_rate_from_fbctrs(), that might require references to the > > fb_ctrs. > > > > So what is the reason behind this change? > > How about this commit log then: > > Theoretically speaking, call by reference is cheaper/faster than call by value > for structures as the later requires the compiler to make a new copy of the > whole structure (which has four u64 values here), to be used by the called > function, while with call by reference we just need to pass a single pointer > (u64 on 64-bit architectures) to the existing structure. > > Yes, on modern architectures, the compilers will likely end up using the > processor registers for passing this structure as it isn't doesn't have lot of > fields and it shouldn't be bad eventually, but nevertheless the code should do > the right thing without assuming about the compiler's or architecture's > optimizations. > Yes, that's why "judgement call", which I'll let you make. The code is sane and I like the longer commit message. Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com> > Don't pass structure instance by value, pass it by reference instead. > > -- > viresh
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index 35b8ae66d1fb..490175d65082 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -373,18 +373,18 @@ static inline u64 get_delta(u64 t1, u64 t0) } static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data, - struct cppc_perf_fb_ctrs fb_ctrs_t0, - struct cppc_perf_fb_ctrs fb_ctrs_t1) + struct cppc_perf_fb_ctrs *fb_ctrs_t0, + struct cppc_perf_fb_ctrs *fb_ctrs_t1) { u64 delta_reference, delta_delivered; u64 reference_perf, delivered_perf; - reference_perf = fb_ctrs_t0.reference_perf; + reference_perf = fb_ctrs_t0->reference_perf; - delta_reference = get_delta(fb_ctrs_t1.reference, - fb_ctrs_t0.reference); - delta_delivered = get_delta(fb_ctrs_t1.delivered, - fb_ctrs_t0.delivered); + delta_reference = get_delta(fb_ctrs_t1->reference, + fb_ctrs_t0->reference); + delta_delivered = get_delta(fb_ctrs_t1->delivered, + fb_ctrs_t0->delivered); /* Check to avoid divide-by zero */ if (delta_reference || delta_delivered) @@ -415,7 +415,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) if (ret) return ret; - return cppc_get_rate_from_fbctrs(cpu_data, fb_ctrs_t0, fb_ctrs_t1); + return cppc_get_rate_from_fbctrs(cpu_data, &fb_ctrs_t0, &fb_ctrs_t1); } static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)