Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1257685 > unrolled thread

Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-28 07:00 +0100
    Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate  immediately Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-28 10:40 +0100
      Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-28 16:10 +0100
        Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate  immediately Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-28 16:30 +0100
          Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate  immediately Viresh Kumar <viresh.kumar@linaro.org> - 2015-10-28 16:50 +0100
          Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-10-28 17:00 +0100

#1257685 — Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately

On Tuesday, October 13, 2015 01:39:02 PM Viresh Kumar wrote:
> We are immediately updating sampling rate for already queued-works, only
> if the new expiry is lesser than the old one.
> 
> But what about the case, where the user doesn't want frequent events and
> want to increase sampling time immediately? Shouldn't we cancel the
> works (and so their interrupts) on all policy->cpus (which might occur
> very shortly).
> 
> This patch removes this special case and simplifies code by immediately
> updating the expiry.

The changelog is a complete disaster. :-/

Your argument seems to be that it should be OK to do the
cancel_delayed_work_sync()/gov_queue_work() combo in all cases, because
even if the new rate is greater than the old one, the user may actually
want it to take effect immediately and it shouldn't hurt to skip the next
sample anyway in that case.

Is this really the case, though?  What about the old rate is 1s, the new one
is 2s and the timer is just about to expire?  Won't the canceling effectively
move the next sample 3s away from the previous one which may not be desirable?

The current code just allows the timer to expire, unless that would prevent
the new rate from taking effect for too long, which seems perfectly reasonable
to me.

All that seems to be racy with respect to the delayed work execution, but that's
a different problem.

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_ondemand.c | 25 ++++---------------------
>  1 file changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 03ac6ce54042..bf0511a9735c 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -231,17 +231,8 @@ static unsigned int od_dbs_timer(struct cpu_dbs_info *cdbs,
>  static struct common_dbs_data od_dbs_cdata;
>  
>  /**
> - * update_sampling_rate - update sampling rate effective immediately if needed.
> + * update_sampling_rate - update sampling rate immediately.
>   * @new_rate: new sampling rate
> - *
> - * If new rate is smaller than the old, simply updating
> - * dbs_tuners_int.sampling_rate might not be appropriate. For example, if the
> - * original sampling_rate was 1 second and the requested new sampling rate is 10
> - * ms because the user needs immediate reaction from ondemand governor, but not
> - * sure if higher frequency will be required or not, then, the governor may
> - * change the sampling rate too late; up to 1 second later. Thus, if we are
> - * reducing the sampling rate, we need to make the new value effective
> - * immediately.
>   */
>  static void update_sampling_rate(struct dbs_data *dbs_data,
>  		unsigned int new_rate)
> @@ -255,7 +246,6 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
>  	for_each_online_cpu(cpu) {
>  		struct cpufreq_policy *policy;
>  		struct od_cpu_dbs_info_s *dbs_info;
> -		unsigned long next_sampling, appointed_at;
>  
>  		policy = cpufreq_cpu_get(cpu);
>  		if (!policy)
> @@ -270,16 +260,9 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
>  		if (!delayed_work_pending(&dbs_info->cdbs.dwork))
>  			continue;
>  
> -		next_sampling = jiffies + usecs_to_jiffies(new_rate);
> -		appointed_at = dbs_info->cdbs.dwork.timer.expires;
> -
> -		if (time_before(next_sampling, appointed_at)) {
> -			cancel_delayed_work_sync(&dbs_info->cdbs.dwork);
> -
> -			gov_queue_work(dbs_data, policy,
> -				       usecs_to_jiffies(new_rate), true);
> -
> -		}
> +		cancel_delayed_work_sync(&dbs_info->cdbs.dwork);
> +		gov_queue_work(dbs_data, policy, usecs_to_jiffies(new_rate),
> +			       true);
>  	}
>  }

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1257853 — Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately

On 28-10-15, 07:28, Rafael J. Wysocki wrote:
> Your argument seems to be that it should be OK to do the
> cancel_delayed_work_sync()/gov_queue_work() combo in all cases, because
> even if the new rate is greater than the old one, the user may actually
> want it to take effect immediately and it shouldn't hurt to skip the next
> sample anyway in that case.
> 
> Is this really the case, though?  What about the old rate is 1s, the new one
> is 2s and the timer is just about to expire?  Won't the canceling effectively
> move the next sample 3s away from the previous one which may not be desirable?
> 
> The current code just allows the timer to expire, unless that would prevent
> the new rate from taking effect for too long, which seems perfectly reasonable
> to me.

Okay, what about this case: old rate is 1s, new rate it 5s and we have
just serviced the timer. With the current code we will receive
evaluate again after 1 second instead of 5. Is that desirable ?

I didn't wanted to keep special code for such corner cases. And then
how many times are we going to update sampling rates ?

But if we want to do something special, then we may schedule the work
for following delay:

delay = shared->time_stamp + new_sampling_rate.

shared->time_stamp is the last time we evaluated the load.

With this, we will be at shoot at the exact requested time, relative
to the last time we evaluated the loads.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258227

On Wednesday, October 28, 2015 03:01:09 PM Viresh Kumar wrote:
> On 28-10-15, 07:28, Rafael J. Wysocki wrote:
> > Your argument seems to be that it should be OK to do the
> > cancel_delayed_work_sync()/gov_queue_work() combo in all cases, because
> > even if the new rate is greater than the old one, the user may actually
> > want it to take effect immediately and it shouldn't hurt to skip the next
> > sample anyway in that case.
> > 
> > Is this really the case, though?  What about the old rate is 1s, the new one
> > is 2s and the timer is just about to expire?  Won't the canceling effectively
> > move the next sample 3s away from the previous one which may not be desirable?
> > 
> > The current code just allows the timer to expire, unless that would prevent
> > the new rate from taking effect for too long, which seems perfectly reasonable
> > to me.
> 
> Okay, what about this case: old rate is 1s, new rate it 5s and we have
> just serviced the timer. With the current code we will receive
> evaluate again after 1 second instead of 5. Is that desirable ?

That is OK.

The change is not guaranteed to happen instantaneously and the old rate may
stay in effect for a longer while.

The case in which that may be annoying (but arguably not incorrect) is when
the new rate is much less than the old one, but that is currently optimized
for.

> I didn't wanted to keep special code for such corner cases. And then
> how many times are we going to update sampling rates ?
> 
> But if we want to do something special, then we may schedule the work
> for following delay:
> 
> delay = shared->time_stamp + new_sampling_rate.
> 
> shared->time_stamp is the last time we evaluated the load.
> 
> With this, we will be at shoot at the exact requested time, relative
> to the last time we evaluated the loads.

Is the current code really problematic?

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258244 — Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately

On 28-10-15, 16:31, Rafael J. Wysocki wrote:
> Is the current code really problematic?

Its not problematic, but just that I didn't like special code written
here.

Also, its a blocker for the next patch which tries to schedule work on
all the policy->cpus together.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258272 — Re: [PATCH V3 2/5] cpufreq: ondemand: update sampling rate immediately

On 28-10-15, 17:13, Rafael J. Wysocki wrote:
> Well, the second statement above sort of contradicts the first one. :-)
> 
> I guess the answer is "it is problematic, because I can't do the other
> optimization then".

Hehe, right.

> To that I'd really suggest trying to rework the code to use timer
> functions directly in the first place.

I will, but this problem will be present there as well. Because at
that point of time, we will talk about per-cpu timers instead of
delayed-works.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258275

On Wednesday, October 28, 2015 08:58:11 PM Viresh Kumar wrote:
> On 28-10-15, 16:31, Rafael J. Wysocki wrote:
> > Is the current code really problematic?
> 
> Its not problematic, but just that I didn't like special code written
> here.
> 
> Also, its a blocker for the next patch which tries to schedule work on
> all the policy->cpus together.

Well, the second statement above sort of contradicts the first one. :-)

I guess the answer is "it is problematic, because I can't do the other
optimization then".

To that I'd really suggest trying to rework the code to use timer
functions directly in the first place.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web