[V3,2/4] cpufreq: cppc: Pass structure instance by reference

Don't pass structure instance by value, pass it by reference instead.

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(-)

-- 
2.31.1.272.g89b43f80a514

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.

-- 
viresh

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