diff mbox series

[RFC,1/7] PM: Introduce em_pd_get_higher_freq()

Message ID 20190508174301.4828-2-douglas.raillard@arm.com (mailing list archive)
State RFC, archived
Headers show
Series sched/cpufreq: Make schedutil energy aware | expand

Commit Message

Douglas RAILLARD May 8, 2019, 5:42 p.m. UTC 
From: Douglas RAILLARD <douglas.raillard@arm.com>

em_pd_get_higher_freq() returns a frequency greater or equal to the
provided one while taking into account a given cost margin. It also
skips inefficient OPPs that have a higher cost than another one with a
higher frequency.

Signed-off-by: Douglas RAILLARD <douglas.raillard@arm.com>
---
 include/linux/energy_model.h | 48 ++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Patrick Bellasi May 16, 2019, 12:42 p.m. UTC | #1 
On 08-May 18:42, douglas.raillard@arm.com wrote:
> From: Douglas RAILLARD <douglas.raillard@arm.com>
> 
> em_pd_get_higher_freq() returns a frequency greater or equal to the
> provided one while taking into account a given cost margin. It also
> skips inefficient OPPs that have a higher cost than another one with a
> higher frequency.

It's worth to add a small description and definition of what we mean by
"OPP efficiency". Despite being just an RFC, it could help to better
understand what we are after.

[...]

> +/** + * em_pd_get_higher_freq() - Get the highest frequency that
> does not exceed the
> + * given cost margin compared to min_freq
> + * @pd		: performance domain for which this must be done
> + * @min_freq	: minimum frequency to return
> + * @cost_margin	: allowed margin compared to min_freq, as a per-1024 value.
                                                                    ^^^^^^^^
here...

> + *
> + * Return: the chosen frequency, guaranteed to be at least as high as min_freq.
> + */
> +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> +	unsigned long min_freq, unsigned long cost_margin)
> +{
> +	unsigned long max_cost = 0;
> +	struct em_cap_state *cs;
> +	int i;
> +
> +	if (!pd)
> +		return min_freq;
> +
> +	/* Compute the maximum allowed cost */
> +	for (i = 0; i < pd->nr_cap_states; i++) {
> +		cs = &pd->table[i];
> +		if (cs->frequency >= min_freq) {
> +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
                                                                         ^^^^
... end here we should probably better use SCHED_CAPACITY_SCALE
instead of hard-coding in values, isn't it?

> +			break;
> +		}
> +	}
> +

[...]

Best,
Patrick
Quentin Perret May 16, 2019, 1:01 p.m. UTC | #2 
On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
> > +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> > +	unsigned long min_freq, unsigned long cost_margin)
> > +{
> > +	unsigned long max_cost = 0;
> > +	struct em_cap_state *cs;
> > +	int i;
> > +
> > +	if (!pd)
> > +		return min_freq;
> > +
> > +	/* Compute the maximum allowed cost */
> > +	for (i = 0; i < pd->nr_cap_states; i++) {
> > +		cs = &pd->table[i];
> > +		if (cs->frequency >= min_freq) {
> > +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
>                                                                          ^^^^
> ... end here we should probably better use SCHED_CAPACITY_SCALE
> instead of hard-coding in values, isn't it?

I'm not sure to agree. This isn't part of the scheduler per se, and the
cost thing isn't in units of capacity, but in units of power, so I don't
think SCHED_CAPACITY_SCALE is correct here.

But I agree these hard coded values (that one, and the 512 in one of the
following patches) could use some motivation :-)

Thanks,
Quentin
Douglas RAILLARD May 16, 2019, 1:06 p.m. UTC | #3 
Hi Patrick,

On 5/16/19 1:42 PM, Patrick Bellasi wrote:
> On 08-May 18:42, douglas.raillard@arm.com wrote:
>> From: Douglas RAILLARD <douglas.raillard@arm.com>
>>
>> em_pd_get_higher_freq() returns a frequency greater or equal to the
>> provided one while taking into account a given cost margin. It also
>> skips inefficient OPPs that have a higher cost than another one with a
>> higher frequency.
> 
> It's worth to add a small description and definition of what we mean by
> "OPP efficiency". Despite being just an RFC, it could help to better
> understand what we are after.

Right, here efficiency=capacity/power.

> 
> [...]
> 
>> +/** + * em_pd_get_higher_freq() - Get the highest frequency that
>> does not exceed the
>> + * given cost margin compared to min_freq
>> + * @pd		: performance domain for which this must be done
>> + * @min_freq	: minimum frequency to return
>> + * @cost_margin	: allowed margin compared to min_freq, as a per-1024 value.
>                                                                      ^^^^^^^^
> here...
> 
>> + *
>> + * Return: the chosen frequency, guaranteed to be at least as high as min_freq.
>> + */
>> +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
>> +	unsigned long min_freq, unsigned long cost_margin)
>> +{
>> +	unsigned long max_cost = 0;
>> +	struct em_cap_state *cs;
>> +	int i;
>> +
>> +	if (!pd)
>> +		return min_freq;
>> +
>> +	/* Compute the maximum allowed cost */
>> +	for (i = 0; i < pd->nr_cap_states; i++) {
>> +		cs = &pd->table[i];
>> +		if (cs->frequency >= min_freq) {
>> +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
>                                                                           ^^^^
> ... end here we should probably better use SCHED_CAPACITY_SCALE
> instead of hard-coding in values, isn't it?

"cs->cost*cost_margin/1024" is not a capacity, it's a cost as defined here:
https://elixir.bootlin.com/linux/latest/source/include/linux/energy_model.h#L17

Actually, it's in milliwatts, but it's not better the better way to look at
it to understand it IMHO.

The margin is expressed as a "per-1024" value just like we use percents'
in everyday life, so it has no unit. If we want to avoid hard-coded values
here, I can introduce an ENERGY_COST_MARGIN_SCALE macro.

>> +			break;
>> +		}
>> +	}
>> +
> 
> [...]
> 
> Best,
> Patrick

Thanks,
Douglas
Patrick Bellasi May 16, 2019, 1:22 p.m. UTC | #4 
On 16-May 14:01, Quentin Perret wrote:
> On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
> > > +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> > > +	unsigned long min_freq, unsigned long cost_margin)
> > > +{
> > > +	unsigned long max_cost = 0;
> > > +	struct em_cap_state *cs;
> > > +	int i;
> > > +
> > > +	if (!pd)
> > > +		return min_freq;
> > > +
> > > +	/* Compute the maximum allowed cost */
> > > +	for (i = 0; i < pd->nr_cap_states; i++) {
> > > +		cs = &pd->table[i];
> > > +		if (cs->frequency >= min_freq) {
> > > +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
> >                                                                          ^^^^
> > ... end here we should probably better use SCHED_CAPACITY_SCALE
> > instead of hard-coding in values, isn't it?
> 
> I'm not sure to agree. This isn't part of the scheduler per se, and the
> cost thing isn't in units of capacity, but in units of power, so I don't
> think SCHED_CAPACITY_SCALE is correct here.

Right, I get the units do not match and it would not be elegant to use
it here...

> But I agree these hard coded values (that one, and the 512 in one of the
> following patches) could use some motivation :-)

... ultimately SCHED_CAPACITY_SCALE is just SCHED_FIXEDPOINT_SCALE,
which is adimensional. Perhaps we should use that or yet another alias
for the same.

> Thanks,
> Quentin
Douglas RAILLARD June 19, 2019, 4:08 p.m. UTC | #5 
Hi Patrick,

On 5/16/19 2:22 PM, Patrick Bellasi wrote:
> On 16-May 14:01, Quentin Perret wrote:
>> On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
>>>> +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
>>>> +	unsigned long min_freq, unsigned long cost_margin)
>>>> +{
>>>> +	unsigned long max_cost = 0;
>>>> +	struct em_cap_state *cs;
>>>> +	int i;
>>>> +
>>>> +	if (!pd)
>>>> +		return min_freq;
>>>> +
>>>> +	/* Compute the maximum allowed cost */
>>>> +	for (i = 0; i < pd->nr_cap_states; i++) {
>>>> +		cs = &pd->table[i];
>>>> +		if (cs->frequency >= min_freq) {
>>>> +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
>>>                                                                           ^^^^
>>> ... end here we should probably better use SCHED_CAPACITY_SCALE
>>> instead of hard-coding in values, isn't it?
>>
>> I'm not sure to agree. This isn't part of the scheduler per se, and the
>> cost thing isn't in units of capacity, but in units of power, so I don't
>> think SCHED_CAPACITY_SCALE is correct here.
> 
> Right, I get the units do not match and it would not be elegant to use
> it here...
> 
>> But I agree these hard coded values (that one, and the 512 in one of the
>> following patches) could use some motivation :-)
> 
> ... ultimately SCHED_CAPACITY_SCALE is just SCHED_FIXEDPOINT_SCALE,
> which is adimensional. Perhaps we should use that or yet another alias
> for the same.

Would it be a good idea to use SCHED_FIXEDPOINT_SCALE in energy.c ?
Since it's not part of the scheduler, maybe there is a scale covering a wider scope,
or we can introduce a similar ENERGY_FIXEDPOINT_SCALE in energy_model.h.


>> Thanks,
>> Quentin
> 

Thanks,
Douglas
Patrick Bellasi June 20, 2019, 1:04 p.m. UTC | #6 
On 19-Jun 17:08, Douglas Raillard wrote:
> Hi Patrick,
> 
> On 5/16/19 2:22 PM, Patrick Bellasi wrote:
> > On 16-May 14:01, Quentin Perret wrote:
> > > On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
> > > > > +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> > > > > +	unsigned long min_freq, unsigned long cost_margin)
> > > > > +{
> > > > > +	unsigned long max_cost = 0;
> > > > > +	struct em_cap_state *cs;
> > > > > +	int i;
> > > > > +
> > > > > +	if (!pd)
> > > > > +		return min_freq;
> > > > > +
> > > > > +	/* Compute the maximum allowed cost */
> > > > > +	for (i = 0; i < pd->nr_cap_states; i++) {
> > > > > +		cs = &pd->table[i];
> > > > > +		if (cs->frequency >= min_freq) {
> > > > > +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
> > > >                                                                           ^^^^
> > > > ... end here we should probably better use SCHED_CAPACITY_SCALE
> > > > instead of hard-coding in values, isn't it?
> > > 
> > > I'm not sure to agree. This isn't part of the scheduler per se, and the
> > > cost thing isn't in units of capacity, but in units of power, so I don't
> > > think SCHED_CAPACITY_SCALE is correct here.
> > 
> > Right, I get the units do not match and it would not be elegant to use
> > it here...
> > 
> > > But I agree these hard coded values (that one, and the 512 in one of the
> > > following patches) could use some motivation :-)
> > 
> > ... ultimately SCHED_CAPACITY_SCALE is just SCHED_FIXEDPOINT_SCALE,
> > which is adimensional. Perhaps we should use that or yet another alias
> > for the same.
> 
> Would it be a good idea to use SCHED_FIXEDPOINT_SCALE in energy.c ?
> Since it's not part of the scheduler, maybe there is a scale covering a wider scope,
> or we can introduce a similar ENERGY_FIXEDPOINT_SCALE in energy_model.h.

Well, in energy_model.c we have references to "capacity" and
"utilization" which are all SCHED_FIXEDPOINT_SCALE range values.
That symbol is defined in <linux/sched.h> and we already pull
in other <linux/sched/*> headers.

So, to me it seems it's not unreasonable to say that we use scheduler
related concepts and it makes more sense than introducing yet another
scaling factor.

But that's just my two cents ;)

Best,
Patrick
Quentin Perret June 21, 2019, 10:17 a.m. UTC | #7 
On Thursday 20 Jun 2019 at 14:04:39 (+0100), Patrick Bellasi wrote:
> On 19-Jun 17:08, Douglas Raillard wrote:
> > Hi Patrick,
> > 
> > On 5/16/19 2:22 PM, Patrick Bellasi wrote:
> > > On 16-May 14:01, Quentin Perret wrote:
> > > > On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
> > > > > > +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> > > > > > +	unsigned long min_freq, unsigned long cost_margin)
> > > > > > +{
> > > > > > +	unsigned long max_cost = 0;
> > > > > > +	struct em_cap_state *cs;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	if (!pd)
> > > > > > +		return min_freq;
> > > > > > +
> > > > > > +	/* Compute the maximum allowed cost */
> > > > > > +	for (i = 0; i < pd->nr_cap_states; i++) {
> > > > > > +		cs = &pd->table[i];
> > > > > > +		if (cs->frequency >= min_freq) {
> > > > > > +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
> > > > >                                                                           ^^^^
> > > > > ... end here we should probably better use SCHED_CAPACITY_SCALE
> > > > > instead of hard-coding in values, isn't it?
> > > > 
> > > > I'm not sure to agree. This isn't part of the scheduler per se, and the
> > > > cost thing isn't in units of capacity, but in units of power, so I don't
> > > > think SCHED_CAPACITY_SCALE is correct here.
> > > 
> > > Right, I get the units do not match and it would not be elegant to use
> > > it here...
> > > 
> > > > But I agree these hard coded values (that one, and the 512 in one of the
> > > > following patches) could use some motivation :-)
> > > 
> > > ... ultimately SCHED_CAPACITY_SCALE is just SCHED_FIXEDPOINT_SCALE,
> > > which is adimensional. Perhaps we should use that or yet another alias
> > > for the same.
> > 
> > Would it be a good idea to use SCHED_FIXEDPOINT_SCALE in energy.c ?
> > Since it's not part of the scheduler, maybe there is a scale covering a wider scope,
> > or we can introduce a similar ENERGY_FIXEDPOINT_SCALE in energy_model.h.
> 
> Well, in energy_model.c we have references to "capacity" and
> "utilization" which are all SCHED_FIXEDPOINT_SCALE range values.
> That symbol is defined in <linux/sched.h> and we already pull
> in other <linux/sched/*> headers.
> 
> So, to me it seems it's not unreasonable to say that we use scheduler
> related concepts and it makes more sense than introducing yet another
> scaling factor.
> 
> But that's just my two cents ;)

Perhaps use this ?

https://elixir.bootlin.com/linux/latest/source/include/linux/energy_model.h#L43

Thanks,
Quentin Perret June 21, 2019, 10:22 a.m. UTC | #8 
On Friday 21 Jun 2019 at 11:17:05 (+0100), Quentin Perret wrote:
> On Thursday 20 Jun 2019 at 14:04:39 (+0100), Patrick Bellasi wrote:
> > On 19-Jun 17:08, Douglas Raillard wrote:
> > > Hi Patrick,
> > > 
> > > On 5/16/19 2:22 PM, Patrick Bellasi wrote:
> > > > On 16-May 14:01, Quentin Perret wrote:
> > > > > On Thursday 16 May 2019 at 13:42:00 (+0100), Patrick Bellasi wrote:
> > > > > > > +static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
> > > > > > > +	unsigned long min_freq, unsigned long cost_margin)
> > > > > > > +{
> > > > > > > +	unsigned long max_cost = 0;
> > > > > > > +	struct em_cap_state *cs;
> > > > > > > +	int i;
> > > > > > > +
> > > > > > > +	if (!pd)
> > > > > > > +		return min_freq;
> > > > > > > +
> > > > > > > +	/* Compute the maximum allowed cost */
> > > > > > > +	for (i = 0; i < pd->nr_cap_states; i++) {
> > > > > > > +		cs = &pd->table[i];
> > > > > > > +		if (cs->frequency >= min_freq) {
> > > > > > > +			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
> > > > > >                                                                           ^^^^
> > > > > > ... end here we should probably better use SCHED_CAPACITY_SCALE
> > > > > > instead of hard-coding in values, isn't it?
> > > > > 
> > > > > I'm not sure to agree. This isn't part of the scheduler per se, and the
> > > > > cost thing isn't in units of capacity, but in units of power, so I don't
> > > > > think SCHED_CAPACITY_SCALE is correct here.
> > > > 
> > > > Right, I get the units do not match and it would not be elegant to use
> > > > it here...
> > > > 
> > > > > But I agree these hard coded values (that one, and the 512 in one of the
> > > > > following patches) could use some motivation :-)
> > > > 
> > > > ... ultimately SCHED_CAPACITY_SCALE is just SCHED_FIXEDPOINT_SCALE,
> > > > which is adimensional. Perhaps we should use that or yet another alias
> > > > for the same.
> > > 
> > > Would it be a good idea to use SCHED_FIXEDPOINT_SCALE in energy.c ?
> > > Since it's not part of the scheduler, maybe there is a scale covering a wider scope,
> > > or we can introduce a similar ENERGY_FIXEDPOINT_SCALE in energy_model.h.
> > 
> > Well, in energy_model.c we have references to "capacity" and
> > "utilization" which are all SCHED_FIXEDPOINT_SCALE range values.
> > That symbol is defined in <linux/sched.h> and we already pull
> > in other <linux/sched/*> headers.
> > 
> > So, to me it seems it's not unreasonable to say that we use scheduler
> > related concepts and it makes more sense than introducing yet another
> > scaling factor.
> > 
> > But that's just my two cents ;)
> 
> Perhaps use this ?
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/energy_model.h#L43
> 

Nah, bad idea actually ... Sorry for the noise
diff mbox series

Patch

diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index aa027f7bcb3e..797b4e0f758c 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -159,6 +159,48 @@  static inline int em_pd_nr_cap_states(struct em_perf_domain *pd)
 	return pd->nr_cap_states;
 }
 
+/**
+ * em_pd_get_higher_freq() - Get the highest frequency that does not exceed the
+ * given cost margin compared to min_freq
+ * @pd		: performance domain for which this must be done
+ * @min_freq	: minimum frequency to return
+ * @cost_margin	: allowed margin compared to min_freq, as a per-1024 value.
+ *
+ * Return: the chosen frequency, guaranteed to be at least as high as min_freq.
+ */
+static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
+	unsigned long min_freq, unsigned long cost_margin)
+{
+	unsigned long max_cost = 0;
+	struct em_cap_state *cs;
+	int i;
+
+	if (!pd)
+		return min_freq;
+
+	/* Compute the maximum allowed cost */
+	for (i = 0; i < pd->nr_cap_states; i++) {
+		cs = &pd->table[i];
+		if (cs->frequency >= min_freq) {
+			max_cost = cs->cost + (cs->cost * cost_margin) / 1024;
+			break;
+		}
+	}
+
+	/* Find the highest frequency that will not exceed the cost margin */
+	for (i = pd->nr_cap_states-1; i >= 0; i--) {
+		cs = &pd->table[i];
+		if (cs->cost <= max_cost)
+			return cs->frequency;
+	}
+
+	/*
+	 * We should normally never reach here, unless min_freq was higher than
+	 * the highest available frequency, which is not expected to happen.
+	 */
+	return min_freq;
+}
+
 #else
 struct em_perf_domain {};
 struct em_data_callback {};
@@ -182,6 +224,12 @@  static inline int em_pd_nr_cap_states(struct em_perf_domain *pd)
 {
 	return 0;
 }
+
+static inline unsigned long em_pd_get_higher_freq(struct em_perf_domain *pd,
+	unsigned long min_freq, unsigned long cost_margin)
+{
+	return min_freq;
+}
 #endif
 
 #endif