| Message ID | 20231129110853.94344-6-lukasz.luba@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Introduce runtime modifiable Energy Model | expand |
On 11/29/23 11:08, Lukasz Luba wrote: > Refactor a dedicated function which will be easier to maintain and re-use > in future. The upcoming changes for the modifiable EM perf_state table > will use it (instead of duplicating the code). nit: What is being refactored? Looks like you took em_compute_cost() out of em_create_perf_table(). Cheers -- Qais Yousef > > This change is not expected to alter the general functionality. > > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > kernel/power/energy_model.c | 72 ++++++++++++++++++++++--------------- > 1 file changed, 43 insertions(+), 29 deletions(-) > > diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c > index aa7c89f9e115..3bea930410c6 100644 > --- a/kernel/power/energy_model.c > +++ b/kernel/power/energy_model.c > @@ -103,14 +103,52 @@ static void em_debug_create_pd(struct device *dev) {} > static void em_debug_remove_pd(struct device *dev) {} > #endif > > +static int em_compute_costs(struct device *dev, struct em_perf_state *table, > + struct em_data_callback *cb, int nr_states, > + unsigned long flags) > +{ > + unsigned long prev_cost = ULONG_MAX; > + u64 fmax; > + int i, ret; > + > + /* Compute the cost of each performance state. */ > + fmax = (u64) table[nr_states - 1].frequency; > + for (i = nr_states - 1; i >= 0; i--) { > + unsigned long power_res, cost; > + > + if (flags & EM_PERF_DOMAIN_ARTIFICIAL) { > + ret = cb->get_cost(dev, table[i].frequency, &cost); > + if (ret || !cost || cost > EM_MAX_POWER) { > + dev_err(dev, "EM: invalid cost %lu %d\n", > + cost, ret); > + return -EINVAL; > + } > + } else { > + power_res = table[i].power; > + cost = div64_u64(fmax * power_res, table[i].frequency); > + } > + > + table[i].cost = cost; > + > + if (table[i].cost >= prev_cost) { > + table[i].flags = EM_PERF_STATE_INEFFICIENT; > + dev_dbg(dev, "EM: OPP:%lu is inefficient\n", > + table[i].frequency); > + } else { > + prev_cost = table[i].cost; > + } > + } > + > + return 0; > +} > + > static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > int nr_states, struct em_data_callback *cb, > unsigned long flags) > { > - unsigned long power, freq, prev_freq = 0, prev_cost = ULONG_MAX; > + unsigned long power, freq, prev_freq = 0; > struct em_perf_state *table; > int i, ret; > - u64 fmax; > > table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL); > if (!table) > @@ -154,33 +192,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, > table[i].frequency = prev_freq = freq; > } > > - /* Compute the cost of each performance state. */ > - fmax = (u64) table[nr_states - 1].frequency; > - for (i = nr_states - 1; i >= 0; i--) { > - unsigned long power_res, cost; > - > - if (flags & EM_PERF_DOMAIN_ARTIFICIAL) { > - ret = cb->get_cost(dev, table[i].frequency, &cost); > - if (ret || !cost || cost > EM_MAX_POWER) { > - dev_err(dev, "EM: invalid cost %lu %d\n", > - cost, ret); > - goto free_ps_table; > - } > - } else { > - power_res = table[i].power; > - cost = div64_u64(fmax * power_res, table[i].frequency); > - } > - > - table[i].cost = cost; > - > - if (table[i].cost >= prev_cost) { > - table[i].flags = EM_PERF_STATE_INEFFICIENT; > - dev_dbg(dev, "EM: OPP:%lu is inefficient\n", > - table[i].frequency); > - } else { > - prev_cost = table[i].cost; > - } > - } > + ret = em_compute_costs(dev, table, cb, nr_states, flags); > + if (ret) > + goto free_ps_table; > > pd->table = table; > pd->nr_perf_states = nr_states; > -- > 2.25.1 >
On 12/17/23 17:58, Qais Yousef wrote: > On 11/29/23 11:08, Lukasz Luba wrote: >> Refactor a dedicated function which will be easier to maintain and re-use >> in future. The upcoming changes for the modifiable EM perf_state table >> will use it (instead of duplicating the code). > > nit: What is being refactored? Looks like you took em_compute_cost() out of > em_create_perf_table(). Yes, it's going to be re-used later for also update code path, not only register code path.
On 12/19/23 10:59, Lukasz Luba wrote: > > > On 12/17/23 17:58, Qais Yousef wrote: > > On 11/29/23 11:08, Lukasz Luba wrote: > > > Refactor a dedicated function which will be easier to maintain and re-use > > > in future. The upcoming changes for the modifiable EM perf_state table > > > will use it (instead of duplicating the code). > > > > nit: What is being refactored? Looks like you took em_compute_cost() out of > > em_create_perf_table(). > > Yes, it's going to be re-used later for also update code path, not only > register code path. Sorry I was terse. I meant the commit message could be clearer to require less effort untangling what is actually being changed.
On 12/28/23 17:14, Qais Yousef wrote: > On 12/19/23 10:59, Lukasz Luba wrote: >> >> >> On 12/17/23 17:58, Qais Yousef wrote: >>> On 11/29/23 11:08, Lukasz Luba wrote: >>>> Refactor a dedicated function which will be easier to maintain and re-use >>>> in future. The upcoming changes for the modifiable EM perf_state table >>>> will use it (instead of duplicating the code). >>> >>> nit: What is being refactored? Looks like you took em_compute_cost() out of >>> em_create_perf_table(). >> >> Yes, it's going to be re-used later for also update code path, not only >> register code path. > > Sorry I was terse. I meant the commit message could be clearer to require less > effort untangling what is actually being changed. OK, I will rephrase that description. Thanks.
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c index aa7c89f9e115..3bea930410c6 100644 --- a/kernel/power/energy_model.c +++ b/kernel/power/energy_model.c @@ -103,14 +103,52 @@ static void em_debug_create_pd(struct device *dev) {} static void em_debug_remove_pd(struct device *dev) {} #endif +static int em_compute_costs(struct device *dev, struct em_perf_state *table, + struct em_data_callback *cb, int nr_states, + unsigned long flags) +{ + unsigned long prev_cost = ULONG_MAX; + u64 fmax; + int i, ret; + + /* Compute the cost of each performance state. */ + fmax = (u64) table[nr_states - 1].frequency; + for (i = nr_states - 1; i >= 0; i--) { + unsigned long power_res, cost; + + if (flags & EM_PERF_DOMAIN_ARTIFICIAL) { + ret = cb->get_cost(dev, table[i].frequency, &cost); + if (ret || !cost || cost > EM_MAX_POWER) { + dev_err(dev, "EM: invalid cost %lu %d\n", + cost, ret); + return -EINVAL; + } + } else { + power_res = table[i].power; + cost = div64_u64(fmax * power_res, table[i].frequency); + } + + table[i].cost = cost; + + if (table[i].cost >= prev_cost) { + table[i].flags = EM_PERF_STATE_INEFFICIENT; + dev_dbg(dev, "EM: OPP:%lu is inefficient\n", + table[i].frequency); + } else { + prev_cost = table[i].cost; + } + } + + return 0; +} + static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, int nr_states, struct em_data_callback *cb, unsigned long flags) { - unsigned long power, freq, prev_freq = 0, prev_cost = ULONG_MAX; + unsigned long power, freq, prev_freq = 0; struct em_perf_state *table; int i, ret; - u64 fmax; table = kcalloc(nr_states, sizeof(*table), GFP_KERNEL); if (!table) @@ -154,33 +192,9 @@ static int em_create_perf_table(struct device *dev, struct em_perf_domain *pd, table[i].frequency = prev_freq = freq; } - /* Compute the cost of each performance state. */ - fmax = (u64) table[nr_states - 1].frequency; - for (i = nr_states - 1; i >= 0; i--) { - unsigned long power_res, cost; - - if (flags & EM_PERF_DOMAIN_ARTIFICIAL) { - ret = cb->get_cost(dev, table[i].frequency, &cost); - if (ret || !cost || cost > EM_MAX_POWER) { - dev_err(dev, "EM: invalid cost %lu %d\n", - cost, ret); - goto free_ps_table; - } - } else { - power_res = table[i].power; - cost = div64_u64(fmax * power_res, table[i].frequency); - } - - table[i].cost = cost; - - if (table[i].cost >= prev_cost) { - table[i].flags = EM_PERF_STATE_INEFFICIENT; - dev_dbg(dev, "EM: OPP:%lu is inefficient\n", - table[i].frequency); - } else { - prev_cost = table[i].cost; - } - } + ret = em_compute_costs(dev, table, cb, nr_states, flags); + if (ret) + goto free_ps_table; pd->table = table; pd->nr_perf_states = nr_states;
Refactor a dedicated function which will be easier to maintain and re-use in future. The upcoming changes for the modifiable EM perf_state table will use it (instead of duplicating the code). This change is not expected to alter the general functionality. Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> --- kernel/power/energy_model.c | 72 ++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 29 deletions(-)