| Message ID | 1435928310-15938-3-git-send-email-javi.merino@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Jul 3, 2015 at 9:58 PM, Javi Merino <javi.merino@arm.com> wrote: > The return value of get_dev_status() can be reused. Cache it so that > other parts of the kernel can reuse it instead of having to call the > same function again. > > Cc: MyungJoo Ham <myungjoo.ham@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Signed-off-by: Javi Merino <javi.merino@arm.com> > --- > drivers/devfreq/devfreq.c | 5 +++++ > drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++-------------- > include/linux/devfreq.h | 7 +++++++ > 3 files changed, 30 insertions(+), 15 deletions(-) > What if there are two entities using the stat at the same time? If you are going to have the status at a shared object (struct devfreq), you need to consider racing conditions as well: two threads accessing last_status at the same time, resulting in calling devfreq_update_stats() while the other has called it beforehand and using the variables in the stat struct. By doing it locally (having stat as struct devfreq_dev_status, not a pointer), we can avoid such problems. The object (struct devfreq_dev_status) it not guaranteed to be accessed by the governor only. Or do you intend to share the stat value with another entity, with the assumption that only one will be calling devfreq_update_stats() ? In that case, what if the one who's supposed to call devfreq_update_stats() is paused? For example, what if the user has switched the governor from simpleondemand to userspace or performance? Cheers, MyungJoo. > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 6390d5db6816..d253a73f2b86 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -810,6 +810,11 @@ unlock: > } > EXPORT_SYMBOL(devfreq_qos_set_min); > > +int devfreq_update_stats(struct devfreq *df) > +{ > + return df->profile->get_dev_status(df->dev.parent, &df->last_status); > +} > + > static ssize_t governor_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c > index 0720ba84ca92..ae72ba5e78df 100644 > --- a/drivers/devfreq/governor_simpleondemand.c > +++ b/drivers/devfreq/governor_simpleondemand.c > @@ -21,17 +21,20 @@ > static int devfreq_simple_ondemand_func(struct devfreq *df, > unsigned long *freq) > { > - struct devfreq_dev_status stat; > - int err = df->profile->get_dev_status(df->dev.parent, &stat); > + int err; > + struct devfreq_dev_status *stat; > unsigned long long a, b; > unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; > unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; > struct devfreq_simple_ondemand_data *data = df->data; > unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; > > + err = devfreq_update_stats(df); > if (err) > return err; > > + stat = &df->last_status; > + > if (data) { > if (data->upthreshold) > dfso_upthreshold = data->upthreshold; > @@ -43,41 +46,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, > return -EINVAL; > > /* Assume MAX if it is going to be divided by zero */ > - if (stat.total_time == 0) { > + if (stat->total_time == 0) { > *freq = max; > return 0; > } > > /* Prevent overflow */ > - if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) { > - stat.busy_time >>= 7; > - stat.total_time >>= 7; > + if (stat->busy_time >= (1 << 24) || stat->total_time >= (1 << 24)) { > + stat->busy_time >>= 7; > + stat->total_time >>= 7; > } > > /* Set MAX if it's busy enough */ > - if (stat.busy_time * 100 > > - stat.total_time * dfso_upthreshold) { > + if (stat->busy_time * 100 > > + stat->total_time * dfso_upthreshold) { > *freq = max; > return 0; > } > > /* Set MAX if we do not know the initial frequency */ > - if (stat.current_frequency == 0) { > + if (stat->current_frequency == 0) { > *freq = max; > return 0; > } > > /* Keep the current frequency */ > - if (stat.busy_time * 100 > > - stat.total_time * (dfso_upthreshold - dfso_downdifferential)) { > - *freq = stat.current_frequency; > + if (stat->busy_time * 100 > > + stat->total_time * (dfso_upthreshold - dfso_downdifferential)) { > + *freq = stat->current_frequency; > return 0; > } > > /* Set the desired frequency based on the load */ > - a = stat.busy_time; > - a *= stat.current_frequency; > - b = div_u64(a, stat.total_time); > + a = stat->busy_time; > + a *= stat->current_frequency; > + b = div_u64(a, stat->total_time); > b *= 100; > b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); > *freq = (unsigned long) b; > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index ffbe1f62ec2c..be351cfacec8 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -161,6 +161,7 @@ struct devfreq { > struct delayed_work work; > > unsigned long previous_freq; > + struct devfreq_dev_status last_status; > > void *data; /* private data for governors */ > > @@ -206,6 +207,7 @@ extern void devm_devfreq_unregister_opp_notifier(struct device *dev, > > int devfreq_qos_set_min(struct devfreq *df, unsigned long value); > int devfreq_qos_set_max(struct devfreq *df, unsigned long value); > +int devfreq_update_stats(struct devfreq *df); > > #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) > /** > @@ -302,6 +304,11 @@ static inline int devfreq_qos_set_max(struct devfreq *df, unsigned long value) > { > return -EINVAL; > } > + > +static inline int devfreq_update_stats(struct devfreq *df) > +{ > + return -EINVAL; > +} > #endif /* CONFIG_PM_DEVFREQ */ > > #endif /* __LINUX_DEVFREQ_H__ */ > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 04, 2015 at 04:15:51AM +0100, MyungJoo Ham wrote: > On Fri, Jul 3, 2015 at 9:58 PM, Javi Merino <javi.merino@arm.com> wrote: > > The return value of get_dev_status() can be reused. Cache it so that > > other parts of the kernel can reuse it instead of having to call the > > same function again. > > > > Cc: MyungJoo Ham <myungjoo.ham@samsung.com> > > Cc: Kyungmin Park <kyungmin.park@samsung.com> > > Signed-off-by: Javi Merino <javi.merino@arm.com> > > --- > > drivers/devfreq/devfreq.c | 5 +++++ > > drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++-------------- > > include/linux/devfreq.h | 7 +++++++ > > 3 files changed, 30 insertions(+), 15 deletions(-) > > > > What if there are two entities using the stat at the same time? If you > are going to have the status at a shared object (struct devfreq), you > need to consider racing conditions as well: two threads accessing > last_status at the same time, resulting in calling > devfreq_update_stats() while the other has called it beforehand and > using the variables in the stat struct. By doing it locally (having > stat as struct devfreq_dev_status, not a pointer), we can avoid such > problems. The object (struct devfreq_dev_status) it not guaranteed to > be accessed by the governor only. The main problem with get_dev_status() is that, as I understand it, its results are described to be an update "since the last call", without taking into account who was the last caller. Therefore, in practice it can only be called from one entity. For example imagine a devfreq governor and a devfreq cooling device want to have access to the stats and they both call get_dev_stats() on a 100ms cadence, you may end up with the following situation: Time (ms) devfreq governor devfreq cooling device 000.000 get_dev_status() 001.000 get_dev_status() . . 100.000 get_dev_status() 101.000 get_dev_status() . . 200.000 get_dev_status() 201.000 get_dev_status() The devfreq cooling device calls get_dev_status() on a 100ms cadence, the status is getting is only for the last 1ms (the time since the last call to get_dev_status()). > Or do you intend to share the stat value with another entity, with the > assumption that only one will be calling devfreq_update_stats() ? > In that case, what if the one who's supposed to call > devfreq_update_stats() is paused? For example, what if the user has > switched the governor from simpleondemand to userspace or performance? Ideally, the way to solve this would be to change the definition of get_dev_status() to make it absolute: total_time and busy_time refer to time since the system was booted. That way, different components can call get_dev_status() whenever they feel like and calculate the values for whichever interval they want to, independently. With the current definition of get_dev_status(), the only solution that I can think of is to have one entity to call get_dev_status() which shares the result by putting it on the devfreq status. The solution in this patch puts it in the governor because I preferred to have a simple solution to kick off this discussion. To summarize, the solutions that I can think of to solve multiple entities having access to the status information are: 1) Change get_dev_status() to return absolute values for busy_time and total_time 2) Make core devfreq call get_dev_status() periodically (for example, before calling the governor) and all the entities that want access can do so via a pointer in devfreq 3) Make the simple ondemand governor call get_dev_status() periodically and caching it, forcing all the other entities to rely on that governor being active What do you prefer? Is there a better one that I haven't thought about? Thanks, Javi -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 6390d5db6816..d253a73f2b86 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -810,6 +810,11 @@ unlock: } EXPORT_SYMBOL(devfreq_qos_set_min); +int devfreq_update_stats(struct devfreq *df) +{ + return df->profile->get_dev_status(df->dev.parent, &df->last_status); +} + static ssize_t governor_show(struct device *dev, struct device_attribute *attr, char *buf) { diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c index 0720ba84ca92..ae72ba5e78df 100644 --- a/drivers/devfreq/governor_simpleondemand.c +++ b/drivers/devfreq/governor_simpleondemand.c @@ -21,17 +21,20 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, unsigned long *freq) { - struct devfreq_dev_status stat; - int err = df->profile->get_dev_status(df->dev.parent, &stat); + int err; + struct devfreq_dev_status *stat; unsigned long long a, b; unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD; unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL; struct devfreq_simple_ondemand_data *data = df->data; unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX; + err = devfreq_update_stats(df); if (err) return err; + stat = &df->last_status; + if (data) { if (data->upthreshold) dfso_upthreshold = data->upthreshold; @@ -43,41 +46,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df, return -EINVAL; /* Assume MAX if it is going to be divided by zero */ - if (stat.total_time == 0) { + if (stat->total_time == 0) { *freq = max; return 0; } /* Prevent overflow */ - if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) { - stat.busy_time >>= 7; - stat.total_time >>= 7; + if (stat->busy_time >= (1 << 24) || stat->total_time >= (1 << 24)) { + stat->busy_time >>= 7; + stat->total_time >>= 7; } /* Set MAX if it's busy enough */ - if (stat.busy_time * 100 > - stat.total_time * dfso_upthreshold) { + if (stat->busy_time * 100 > + stat->total_time * dfso_upthreshold) { *freq = max; return 0; } /* Set MAX if we do not know the initial frequency */ - if (stat.current_frequency == 0) { + if (stat->current_frequency == 0) { *freq = max; return 0; } /* Keep the current frequency */ - if (stat.busy_time * 100 > - stat.total_time * (dfso_upthreshold - dfso_downdifferential)) { - *freq = stat.current_frequency; + if (stat->busy_time * 100 > + stat->total_time * (dfso_upthreshold - dfso_downdifferential)) { + *freq = stat->current_frequency; return 0; } /* Set the desired frequency based on the load */ - a = stat.busy_time; - a *= stat.current_frequency; - b = div_u64(a, stat.total_time); + a = stat->busy_time; + a *= stat->current_frequency; + b = div_u64(a, stat->total_time); b *= 100; b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2)); *freq = (unsigned long) b; diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index ffbe1f62ec2c..be351cfacec8 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -161,6 +161,7 @@ struct devfreq { struct delayed_work work; unsigned long previous_freq; + struct devfreq_dev_status last_status; void *data; /* private data for governors */ @@ -206,6 +207,7 @@ extern void devm_devfreq_unregister_opp_notifier(struct device *dev, int devfreq_qos_set_min(struct devfreq *df, unsigned long value); int devfreq_qos_set_max(struct devfreq *df, unsigned long value); +int devfreq_update_stats(struct devfreq *df); #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) /** @@ -302,6 +304,11 @@ static inline int devfreq_qos_set_max(struct devfreq *df, unsigned long value) { return -EINVAL; } + +static inline int devfreq_update_stats(struct devfreq *df) +{ + return -EINVAL; +} #endif /* CONFIG_PM_DEVFREQ */ #endif /* __LINUX_DEVFREQ_H__ */
The return value of get_dev_status() can be reused. Cache it so that other parts of the kernel can reuse it instead of having to call the same function again. Cc: MyungJoo Ham <myungjoo.ham@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> Signed-off-by: Javi Merino <javi.merino@arm.com> --- drivers/devfreq/devfreq.c | 5 +++++ drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++-------------- include/linux/devfreq.h | 7 +++++++ 3 files changed, 30 insertions(+), 15 deletions(-)