diff mbox series

[09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq

Message ID 1615294733-22761-10-git-send-email-aisheng.dong@nxp.com (mailing list archive)
State New, archived
Headers show
Series PM / devfreq: a few small fixes and improvements | expand

Commit Message

Dong Aisheng March 9, 2021, 12:58 p.m. UTC 
devfreq_simple_ondemand_data only needs to be initialized once when
calling devm_devfreq_add_device. It's unnecessary to put the data
check logic in the hot path (.get_target_freq()) where it will be
called all the time during polling. Instead, we only check and initialize
it one time during DEVFREQ_GOV_START.

This also helps check data validability in advance during DEVFREQ_GOV_START
rather than checking it later when running .get_target_freq().

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/devfreq/governor_simpleondemand.c | 50 +++++++++++++++--------
 1 file changed, 34 insertions(+), 16 deletions(-)

Comments

Chanwoo Choi March 9, 2021, 4:09 p.m. UTC | #1 
On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> devfreq_simple_ondemand_data only needs to be initialized once when
> calling devm_devfreq_add_device. It's unnecessary to put the data
> check logic in the hot path (.get_target_freq()) where it will be
> called all the time during polling. Instead, we only check and initialize
> it one time during DEVFREQ_GOV_START.
> 
> This also helps check data validability in advance during DEVFREQ_GOV_START
> rather than checking it later when running .get_target_freq().
> 
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   drivers/devfreq/governor_simpleondemand.c | 50 +++++++++++++++--------
>   1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index ea287b57cbf3..341eb7e9dc04 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -15,15 +15,19 @@
>   /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
>   #define DFSO_UPTHRESHOLD	(90)
>   #define DFSO_DOWNDIFFERENCTIAL	(5)
> +
> +static struct devfreq_simple_ondemand_data od_default = {
> +	.upthreshold = DFSO_UPTHRESHOLD,
> +	.downdifferential = DFSO_DOWNDIFFERENCTIAL,
> +};
> +
>   static int devfreq_simple_ondemand_func(struct devfreq *df,
>   					unsigned long *freq)
>   {
>   	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;
> +	struct devfreq_simple_ondemand_data *od = df->data;
>   
>   	err = devfreq_update_stats(df);
>   	if (err)
> @@ -31,16 +35,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>   
>   	stat = &df->last_status;
>   
> -	if (data) {
> -		if (data->upthreshold)
> -			dfso_upthreshold = data->upthreshold;
> -		if (data->downdifferential)
> -			dfso_downdifferential = data->downdifferential;
> -	}
> -	if (dfso_upthreshold > 100 ||
> -	    dfso_upthreshold < dfso_downdifferential)
> -		return -EINVAL;
> -
>   	/* Assume MAX if it is going to be divided by zero */
>   	if (stat->total_time == 0) {
>   		*freq = DEVFREQ_MAX_FREQ;
> @@ -55,7 +49,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>   
>   	/* Set MAX if it's busy enough */
>   	if (stat->busy_time * 100 >
> -	    stat->total_time * dfso_upthreshold) {
> +	    stat->total_time * od->upthreshold) {
>   		*freq = DEVFREQ_MAX_FREQ;
>   		return 0;
>   	}
> @@ -68,7 +62,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>   
>   	/* Keep the current frequency */
>   	if (stat->busy_time * 100 >
> -	    stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
> +	    stat->total_time * (od->upthreshold - od->downdifferential)) {
>   		*freq = stat->current_frequency;
>   		return 0;
>   	}
> @@ -78,17 +72,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>   	a *= stat->current_frequency;
>   	b = div_u64(a, stat->total_time);
>   	b *= 100;
> -	b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> +	b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
>   	*freq = (unsigned long) b;
>   
>   	return 0;
>   }
>   
> +static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
> +{
> +	struct devfreq_simple_ondemand_data *od = devfreq->data;
> +
> +	if (od) {
> +		if (!od->upthreshold)
> +			od->upthreshold = DFSO_UPTHRESHOLD;
> +
> +		if (!od->downdifferential)
> +			od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
> +
> +		if (od->upthreshold > 100 ||
> +		    od->upthreshold < od->downdifferential)
> +			return -EINVAL;
> +	} else {
> +		od = &od_default;
> +	}
> +
> +	return 0;
> +}
> +
>   static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
>   				unsigned int event, void *data)
>   {
>   	switch (event) {
>   	case DEVFREQ_GOV_START:
> +		if (devfreq_simple_ondemand_check_od(devfreq))
> +			return -EINVAL;
> +
>   		return devfreq_monitor_start(devfreq);
>   
>   	case DEVFREQ_GOV_STOP:
> 

I'm editing the upthreshold and downdifferential for exposing them
via sysfs. So that after my work to expose them via sysfs,
send the patches if you think that need to do more about them.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=dc9e557845c17cee173a6adcc3ae14940da03f44
Dong Aisheng March 10, 2021, 3:02 a.m. UTC | #2 
On Wed, Mar 10, 2021 at 12:09 AM Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > devfreq_simple_ondemand_data only needs to be initialized once when
> > calling devm_devfreq_add_device. It's unnecessary to put the data
> > check logic in the hot path (.get_target_freq()) where it will be
> > called all the time during polling. Instead, we only check and initialize
> > it one time during DEVFREQ_GOV_START.
> >
> > This also helps check data validability in advance during DEVFREQ_GOV_START
> > rather than checking it later when running .get_target_freq().
> >
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >   drivers/devfreq/governor_simpleondemand.c | 50 +++++++++++++++--------
> >   1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> > index ea287b57cbf3..341eb7e9dc04 100644
> > --- a/drivers/devfreq/governor_simpleondemand.c
> > +++ b/drivers/devfreq/governor_simpleondemand.c
> > @@ -15,15 +15,19 @@
> >   /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
> >   #define DFSO_UPTHRESHOLD    (90)
> >   #define DFSO_DOWNDIFFERENCTIAL      (5)
> > +
> > +static struct devfreq_simple_ondemand_data od_default = {
> > +     .upthreshold = DFSO_UPTHRESHOLD,
> > +     .downdifferential = DFSO_DOWNDIFFERENCTIAL,
> > +};
> > +
> >   static int devfreq_simple_ondemand_func(struct devfreq *df,
> >                                       unsigned long *freq)
> >   {
> >       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;
> > +     struct devfreq_simple_ondemand_data *od = df->data;
> >
> >       err = devfreq_update_stats(df);
> >       if (err)
> > @@ -31,16 +35,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> >
> >       stat = &df->last_status;
> >
> > -     if (data) {
> > -             if (data->upthreshold)
> > -                     dfso_upthreshold = data->upthreshold;
> > -             if (data->downdifferential)
> > -                     dfso_downdifferential = data->downdifferential;
> > -     }
> > -     if (dfso_upthreshold > 100 ||
> > -         dfso_upthreshold < dfso_downdifferential)
> > -             return -EINVAL;
> > -
> >       /* Assume MAX if it is going to be divided by zero */
> >       if (stat->total_time == 0) {
> >               *freq = DEVFREQ_MAX_FREQ;
> > @@ -55,7 +49,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> >
> >       /* Set MAX if it's busy enough */
> >       if (stat->busy_time * 100 >
> > -         stat->total_time * dfso_upthreshold) {
> > +         stat->total_time * od->upthreshold) {
> >               *freq = DEVFREQ_MAX_FREQ;
> >               return 0;
> >       }
> > @@ -68,7 +62,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> >
> >       /* Keep the current frequency */
> >       if (stat->busy_time * 100 >
> > -         stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
> > +         stat->total_time * (od->upthreshold - od->downdifferential)) {
> >               *freq = stat->current_frequency;
> >               return 0;
> >       }
> > @@ -78,17 +72,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> >       a *= stat->current_frequency;
> >       b = div_u64(a, stat->total_time);
> >       b *= 100;
> > -     b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> > +     b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
> >       *freq = (unsigned long) b;
> >
> >       return 0;
> >   }
> >
> > +static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
> > +{
> > +     struct devfreq_simple_ondemand_data *od = devfreq->data;
> > +
> > +     if (od) {
> > +             if (!od->upthreshold)
> > +                     od->upthreshold = DFSO_UPTHRESHOLD;
> > +
> > +             if (!od->downdifferential)
> > +                     od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
> > +
> > +             if (od->upthreshold > 100 ||
> > +                 od->upthreshold < od->downdifferential)
> > +                     return -EINVAL;
> > +     } else {
> > +             od = &od_default;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
> >                               unsigned int event, void *data)
> >   {
> >       switch (event) {
> >       case DEVFREQ_GOV_START:
> > +             if (devfreq_simple_ondemand_check_od(devfreq))
> > +                     return -EINVAL;
> > +
> >               return devfreq_monitor_start(devfreq);
> >
> >       case DEVFREQ_GOV_STOP:
> >
>
> I'm editing the upthreshold and downdifferential for exposing them
> via sysfs. So that after my work to expose them via sysfs,
> send the patches if you think that need to do more about them.

Thanks for letting me know..
I can rework after your patch merged.

Regards
Aisheng

>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=dc9e557845c17cee173a6adcc3ae14940da03f44
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi
diff mbox series

Patch

diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index ea287b57cbf3..341eb7e9dc04 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -15,15 +15,19 @@ 
 /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
 #define DFSO_UPTHRESHOLD	(90)
 #define DFSO_DOWNDIFFERENCTIAL	(5)
+
+static struct devfreq_simple_ondemand_data od_default = {
+	.upthreshold = DFSO_UPTHRESHOLD,
+	.downdifferential = DFSO_DOWNDIFFERENCTIAL,
+};
+
 static int devfreq_simple_ondemand_func(struct devfreq *df,
 					unsigned long *freq)
 {
 	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;
+	struct devfreq_simple_ondemand_data *od = df->data;
 
 	err = devfreq_update_stats(df);
 	if (err)
@@ -31,16 +35,6 @@  static int devfreq_simple_ondemand_func(struct devfreq *df,
 
 	stat = &df->last_status;
 
-	if (data) {
-		if (data->upthreshold)
-			dfso_upthreshold = data->upthreshold;
-		if (data->downdifferential)
-			dfso_downdifferential = data->downdifferential;
-	}
-	if (dfso_upthreshold > 100 ||
-	    dfso_upthreshold < dfso_downdifferential)
-		return -EINVAL;
-
 	/* Assume MAX if it is going to be divided by zero */
 	if (stat->total_time == 0) {
 		*freq = DEVFREQ_MAX_FREQ;
@@ -55,7 +49,7 @@  static int devfreq_simple_ondemand_func(struct devfreq *df,
 
 	/* Set MAX if it's busy enough */
 	if (stat->busy_time * 100 >
-	    stat->total_time * dfso_upthreshold) {
+	    stat->total_time * od->upthreshold) {
 		*freq = DEVFREQ_MAX_FREQ;
 		return 0;
 	}
@@ -68,7 +62,7 @@  static int devfreq_simple_ondemand_func(struct devfreq *df,
 
 	/* Keep the current frequency */
 	if (stat->busy_time * 100 >
-	    stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
+	    stat->total_time * (od->upthreshold - od->downdifferential)) {
 		*freq = stat->current_frequency;
 		return 0;
 	}
@@ -78,17 +72,41 @@  static int devfreq_simple_ondemand_func(struct devfreq *df,
 	a *= stat->current_frequency;
 	b = div_u64(a, stat->total_time);
 	b *= 100;
-	b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
+	b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
 	*freq = (unsigned long) b;
 
 	return 0;
 }
 
+static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
+{
+	struct devfreq_simple_ondemand_data *od = devfreq->data;
+
+	if (od) {
+		if (!od->upthreshold)
+			od->upthreshold = DFSO_UPTHRESHOLD;
+
+		if (!od->downdifferential)
+			od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
+
+		if (od->upthreshold > 100 ||
+		    od->upthreshold < od->downdifferential)
+			return -EINVAL;
+	} else {
+		od = &od_default;
+	}
+
+	return 0;
+}
+
 static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
 				unsigned int event, void *data)
 {
 	switch (event) {
 	case DEVFREQ_GOV_START:
+		if (devfreq_simple_ondemand_check_od(devfreq))
+			return -EINVAL;
+
 		return devfreq_monitor_start(devfreq);
 
 	case DEVFREQ_GOV_STOP: