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

Groups > linux.kernel > #1282685 > unrolled thread

[PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers

Started byViresh Kumar <viresh.kumar@linaro.org>
First post2015-12-03 05:10 +0100
Last post2015-12-11 02:50 +0100
Articles 10 on this page of 30 — 2 participants

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

  [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-03 05:10 +0100
    Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-04 01:50 +0100
      Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-04 07:20 +0100
        Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-05 02:50 +0100
          Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-05 05:20 +0100
            Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-07 02:00 +0100
              Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-07 09:00 +0100
                Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-07 23:20 +0100
                  Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-07 23:50 +0100
                    [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 01:20 +0100
                      Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable  for synchronization Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 08:00 +0100
                        Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 14:10 +0100
                          Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable  for synchronization Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 14:40 +0100
                            Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 14:50 +0100
                              Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable  for synchronization Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 15:00 +0100
                                Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 15:10 +0100
                                  Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable  for synchronization Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 16:00 +0100
                                    Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 17:20 +0100
                                      Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable  for synchronization Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 17:40 +0100
                    Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 07:50 +0100
                  Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 08:00 +0100
                    Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 13:50 +0100
                      Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 14:40 +0100
                        Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 14:40 +0100
    [PATCH V3 5/6] cpufreq: governor: replace per-cpu delayed work with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-04 07:20 +0100
      [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-09 03:10 +0100
        Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-09 22:40 +0100
          Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-10 03:40 +0100
            Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-10 22:50 +0100
              Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work  with timers Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-11 02:50 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1286286 — Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-08 08:00 +0100
SubjectRe: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qDkpI-7hQ-15@gated-at.bofh.it>
In reply to#1286010
On 07-12-15, 23:43, Rafael J. Wysocki wrote:
> On Monday, December 07, 2015 01:20:27 PM Viresh Kumar wrote:

> > At this point we might end up decrementing skip_work from
> > gov_cancel_work() and then cancel the work which we haven't queued
> > yet. And the end result will be that the work is still queued while
> > gov_cancel_work() has finished.
> 
> I'm not quite sure how that can happen.

I will describe that towards the end of this email.

> There is a bug in this code snippet, but it may cause us to fail to queue
> the work at all, so the incrementation and the check need to be done
> under the spinlock.

What bug ?

> > And we have to keep the atomic operation, as well as queue_work()
> > within the lock.
> 
> Putting queue_work() under the lock doesn't prevent any races from happening,

Then I am not able to think about it properly, but I will at least
present my case here :)

> because only one of the CPUs can execute that part of the function anyway.
> 
> > > 		queue_work(system_wq, &shared->work);
> > > 
> > > and the remaining incrementation and decrementation of skip_work are replaced
> > > with the corresponding atomic operations, it still should work, no?
> 
> Well, no, the above wouldn't work.
> 
> But what about something like this instead:
> 
> 	if (atomic_inc_return(&shared->skip_work) > 1)
> 		atomic_dec(&shared->skip_work);
> 	else
> 		queue_work(system_wq, &shared->work);
> 
> (plus the changes requisite replacements in the other places)?
> 
> Only one CPU can see the result of the atomic_inc_return() as 1 and this is the
> only one that will queue up the work item, unless I'm missing anything super
> subtle.

Looks like you are talking about the race between different timer
handlers, which race against queuing the work. Sorry if you are not.
But I am not talking about that thing..

Suppose queue_work() isn't done within the spin lock.

CPU0                                            CPU1

cpufreq_governor_stop()                         dbs_timer_handler()
-> gov_cancel_work()                            -> lock
                                                -> shared->skip_work++, as skip_work was 0. //skip_work=1
                                                -> unlock
   -> lock
   -> shared->skip_work++; //skip_work=2
   -> unlock
   -> cancel_work_sync(&shared->work);
                                                -> queue_work();
   -> gov_cancel_timers(shared->policy);
   -> shared->skip_work = 0;
                                                dbs_work_handler();



And according to how I understand it, we are screwed up at this point.
And its the same old bug which I fixed recently (which we hacked up by
using gov-lock earlier).

The work handler is still active after the policy-governor is stopped.

And your latest patch looks wrong for the same reason ...

-- 
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]

#1286436

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-12-08 13:50 +0100
Message-ID<qDpSq-2rq-19@gated-at.bofh.it>
In reply to#1286286
On Tuesday, December 08, 2015 12:26:22 PM Viresh Kumar wrote:
> On 07-12-15, 23:43, Rafael J. Wysocki wrote:
> > On Monday, December 07, 2015 01:20:27 PM Viresh Kumar wrote:
> 
> > > At this point we might end up decrementing skip_work from
> > > gov_cancel_work() and then cancel the work which we haven't queued
> > > yet. And the end result will be that the work is still queued while
> > > gov_cancel_work() has finished.
> > 
> > I'm not quite sure how that can happen.
> 
> I will describe that towards the end of this email.
> 
> > There is a bug in this code snippet, but it may cause us to fail to queue
> > the work at all, so the incrementation and the check need to be done
> > under the spinlock.
> 
> What bug ?

Well, if the timer function runs on all CPUs at the same time, they all
can see skip_work > 1 and none of them will queue the work.

> > > And we have to keep the atomic operation, as well as queue_work()
> > > within the lock.
> > 
> > Putting queue_work() under the lock doesn't prevent any races from happening,
> 
> Then I am not able to think about it properly, but I will at least
> present my case here :)
> 
> > because only one of the CPUs can execute that part of the function anyway.
> > 
> > > > 		queue_work(system_wq, &shared->work);
> > > > 
> > > > and the remaining incrementation and decrementation of skip_work are replaced
> > > > with the corresponding atomic operations, it still should work, no?
> > 
> > Well, no, the above wouldn't work.
> > 
> > But what about something like this instead:
> > 
> > 	if (atomic_inc_return(&shared->skip_work) > 1)
> > 		atomic_dec(&shared->skip_work);
> > 	else
> > 		queue_work(system_wq, &shared->work);
> > 
> > (plus the changes requisite replacements in the other places)?
> > 
> > Only one CPU can see the result of the atomic_inc_return() as 1 and this is the
> > only one that will queue up the work item, unless I'm missing anything super
> > subtle.
> 
> Looks like you are talking about the race between different timer
> handlers, which race against queuing the work. Sorry if you are not.
> But I am not talking about that thing..
> 
> Suppose queue_work() isn't done within the spin lock.
> 
> CPU0                                            CPU1
> 
> cpufreq_governor_stop()                         dbs_timer_handler()
> -> gov_cancel_work()                            -> lock
>                                                 -> shared->skip_work++, as skip_work was 0. //skip_work=1
>                                                 -> unlock
>    -> lock
>    -> shared->skip_work++; //skip_work=2
>    -> unlock
>    -> cancel_work_sync(&shared->work);
>                                                 -> queue_work();
>    -> gov_cancel_timers(shared->policy);
>    -> shared->skip_work = 0;
>                                                 dbs_work_handler();
> 
> 
> 
> And according to how I understand it, we are screwed up at this point.
> And its the same old bug which I fixed recently (which we hacked up by
> using gov-lock earlier).

You are right, I've overlooked that race (but then it is rather easy to
overlook).

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]

#1286484 — Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-08 14:40 +0100
SubjectRe: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qDqEO-2Xv-15@gated-at.bofh.it>
In reply to#1286436
On 08-12-15, 14:18, Rafael J. Wysocki wrote:
> Well, if the timer function runs on all CPUs at the same time, they all
> can see skip_work > 1 and none of them will queue the work.

You are talking about code after my patch, right?

Will will all of them see it > 1? At least one of them will see it 0
and queue the work, unless the governor is stopped completely.

> You are right, I've overlooked that race (but then it is rather easy to
> overlook).

Yeah, we (at least I) took a long time to understand that this was the
real problem we always had and so fixed it recently.

-- 
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]

#1286487

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-12-08 14:40 +0100
Message-ID<qDqEO-2Xv-21@gated-at.bofh.it>
In reply to#1286484
On Tuesday, December 08, 2015 07:00:36 PM Viresh Kumar wrote:
> On 08-12-15, 14:18, Rafael J. Wysocki wrote:
> > Well, if the timer function runs on all CPUs at the same time, they all
> > can see skip_work > 1 and none of them will queue the work.
> 
> You are talking about code after my patch, right?

No, I was talking about my first attempt at using the atomic variable. :-)

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]

#1283593 — [PATCH V3 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-04 07:20 +0100
Subject[PATCH V3 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qBRSO-7t4-11@gated-at.bofh.it>
In reply to#1282685
cpufreq governors evaluate load at sampling rate and based on that they
update frequency for a group of CPUs belonging to the same cpufreq
policy.

This is required to be done in a single thread for all policy->cpus, but
because we don't want to wakeup idle CPUs to do just that, we use
deferrable work for this. If we would have used a single delayed
deferrable work for the entire policy, there were chances that the CPU
required to run the handler can be in idle and we might end up not
changing the frequency for the entire group with load variations.

And so we were forced to keep per-cpu works, and only the one that
expires first need to do the real work and others are rescheduled for
next sampling time.

We have been using the more complex solution until now, where we used a
delayed deferrable work for this, which is a combination of a timer and
a work.

This could be made lightweight by keeping per-cpu deferred timers with a
single work item, which is scheduled by the first timer that expires.

This patch does just that and here are important changes:
- The timer handler will run in irq context and so we need to use a
  spin_lock instead of the timer_mutex. And so a separate timer_lock is
  created. This also makes the use of the mutex and lock quite clear, as
  we know what exactly they are protecting.
- A new field 'skip_work' is added to track when the timer handlers can
  queue a work. More comments present in code.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
V2->V3:
- Dropped unused variable policy
- Rearranged code to kill an extra label

 drivers/cpufreq/cpufreq_governor.c | 137 +++++++++++++++++++++----------------
 drivers/cpufreq/cpufreq_governor.h |  20 ++++--
 drivers/cpufreq/cpufreq_ondemand.c |   8 +--
 3 files changed, 95 insertions(+), 70 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 999e1f6addf9..c9e420bd0eec 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -158,47 +158,52 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
-static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
-		unsigned int delay)
+void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
 {
-	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
-
-	mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay);
-}
-
-void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
-		unsigned int delay, bool all_cpus)
-{
-	int i;
+	struct dbs_data *dbs_data = policy->governor_data;
+	struct cpu_dbs_info *cdbs;
+	int cpu;
 
-	if (!all_cpus) {
-		/*
-		 * Use raw_smp_processor_id() to avoid preemptible warnings.
-		 * We know that this is only called with all_cpus == false from
-		 * works that have been queued with *_work_on() functions and
-		 * those works are canceled during CPU_DOWN_PREPARE so they
-		 * can't possibly run on any other CPU.
-		 */
-		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
-	} else {
-		for_each_cpu(i, policy->cpus)
-			__gov_queue_work(i, dbs_data, delay);
+	for_each_cpu(cpu, policy->cpus) {
+		cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+		cdbs->timer.expires = jiffies + delay;
+		add_timer_on(&cdbs->timer, cpu);
 	}
 }
-EXPORT_SYMBOL_GPL(gov_queue_work);
+EXPORT_SYMBOL_GPL(gov_add_timers);
 
-static inline void gov_cancel_work(struct dbs_data *dbs_data,
-		struct cpufreq_policy *policy)
+static inline void gov_cancel_timers(struct cpufreq_policy *policy)
 {
+	struct dbs_data *dbs_data = policy->governor_data;
 	struct cpu_dbs_info *cdbs;
 	int i;
 
 	for_each_cpu(i, policy->cpus) {
 		cdbs = dbs_data->cdata->get_cpu_cdbs(i);
-		cancel_delayed_work_sync(&cdbs->dwork);
+		del_timer_sync(&cdbs->timer);
 	}
 }
 
+void gov_cancel_work(struct cpu_common_dbs_info *shared)
+{
+	unsigned long flags;
+
+	/*
+	 * No work will be queued from timer handlers after skip_work is
+	 * updated. And so we can safely cancel the work first and then the
+	 * timers.
+	 */
+	spin_lock_irqsave(&shared->timer_lock, flags);
+	shared->skip_work++;
+	spin_unlock_irqrestore(&shared->timer_lock, flags);
+
+	cancel_work_sync(&shared->work);
+
+	gov_cancel_timers(shared->policy);
+
+	shared->skip_work = 0;
+}
+
 /* Will return if we need to evaluate cpu load again or not */
 static bool need_load_eval(struct cpu_common_dbs_info *shared,
 			   unsigned int sampling_rate)
@@ -217,29 +222,21 @@ static bool need_load_eval(struct cpu_common_dbs_info *shared,
 	return true;
 }
 
-static void dbs_timer(struct work_struct *work)
+static void dbs_work_handler(struct work_struct *work)
 {
-	struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info,
-						 dwork.work);
-	struct cpu_common_dbs_info *shared = cdbs->shared;
+	struct cpu_common_dbs_info *shared = container_of(work, struct
+					cpu_common_dbs_info, work);
 	struct cpufreq_policy *policy;
 	struct dbs_data *dbs_data;
 	unsigned int sampling_rate, delay;
-	bool modify_all = true;
-
-	mutex_lock(&shared->timer_mutex);
+	bool eval_load;
 
 	policy = shared->policy;
-
-	/*
-	 * Governor might already be disabled and there is no point continuing
-	 * with the work-handler.
-	 */
-	if (!policy)
-		goto unlock;
-
 	dbs_data = policy->governor_data;
 
+	/* Kill all timers */
+	gov_cancel_timers(policy);
+
 	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
 		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 
@@ -250,14 +247,40 @@ static void dbs_timer(struct work_struct *work)
 		sampling_rate = od_tuners->sampling_rate;
 	}
 
-	if (!need_load_eval(cdbs->shared, sampling_rate))
-		modify_all = false;
-
-	delay = dbs_data->cdata->gov_dbs_timer(policy, modify_all);
-	gov_queue_work(dbs_data, policy, delay, modify_all);
+	eval_load = need_load_eval(shared, sampling_rate);
 
-unlock:
+	/*
+	 * Make sure cpufreq_governor_limits() isn't evaluating load in
+	 * parallel.
+	 */
+	mutex_lock(&shared->timer_mutex);
+	delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
 	mutex_unlock(&shared->timer_mutex);
+
+	shared->skip_work--;
+	gov_add_timers(policy, delay);
+}
+
+static void dbs_timer_handler(unsigned long data)
+{
+	struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+	struct cpu_common_dbs_info *shared = cdbs->shared;
+	unsigned long flags;
+
+	spin_lock_irqsave(&shared->timer_lock, flags);
+
+	/*
+	 * Timer handler isn't allowed to queue work at the moment, because:
+	 * - Another timer handler has done that
+	 * - We are stopping the governor
+	 * - Or we are updating the sampling rate of ondemand governor
+	 */
+	if (!shared->skip_work) {
+		shared->skip_work++;
+		queue_work(system_wq, &shared->work);
+	}
+
+	spin_unlock_irqrestore(&shared->timer_lock, flags);
 }
 
 static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -288,6 +311,8 @@ static int alloc_common_dbs_info(struct cpufreq_policy *policy,
 		cdata->get_cpu_cdbs(j)->shared = shared;
 
 	mutex_init(&shared->timer_mutex);
+	spin_lock_init(&shared->timer_lock);
+	INIT_WORK(&shared->work, dbs_work_handler);
 	return 0;
 }
 
@@ -452,7 +477,9 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		if (ignore_nice)
 			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-		INIT_DEFERRABLE_WORK(&j_cdbs->dwork, dbs_timer);
+		__setup_timer(&j_cdbs->timer, dbs_timer_handler,
+			      (unsigned long)j_cdbs,
+			      TIMER_DEFERRABLE | TIMER_IRQSAFE);
 	}
 
 	if (cdata->governor == GOV_CONSERVATIVE) {
@@ -470,8 +497,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		od_ops->powersave_bias_init_cpu(cpu);
 	}
 
-	gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate),
-		       true);
+	gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
 	return 0;
 }
 
@@ -485,16 +511,9 @@ static int cpufreq_governor_stop(struct cpufreq_policy *policy,
 	if (!shared || !shared->policy)
 		return -EBUSY;
 
-	/*
-	 * Work-handler must see this updated, as it should not proceed any
-	 * further after governor is disabled. And so timer_mutex is taken while
-	 * updating this value.
-	 */
-	mutex_lock(&shared->timer_mutex);
+	gov_cancel_work(shared);
 	shared->policy = NULL;
-	mutex_unlock(&shared->timer_mutex);
 
-	gov_cancel_work(dbs_data, policy);
 	return 0;
 }
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 0c7589016b6c..76742902491e 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -132,12 +132,20 @@ static void *get_cpu_dbs_info_s(int cpu)				\
 struct cpu_common_dbs_info {
 	struct cpufreq_policy *policy;
 	/*
-	 * percpu mutex that serializes governor limit change with dbs_timer
-	 * invocation. We do not want dbs_timer to run when user is changing
-	 * the governor or limits.
+	 * Per policy mutex that serializes load evaluation from limit-change
+	 * and work-handler.
 	 */
 	struct mutex timer_mutex;
+
+	/*
+	 * Per policy lock that serializes access to queuing work from timer
+	 * handlers.
+	 */
+	spinlock_t timer_lock;
+
 	ktime_t time_stamp;
+	unsigned int skip_work;
+	struct work_struct work;
 };
 
 /* Per cpu structures */
@@ -152,7 +160,7 @@ struct cpu_dbs_info {
 	 * wake-up from idle.
 	 */
 	unsigned int prev_load;
-	struct delayed_work dwork;
+	struct timer_list timer;
 	struct cpu_common_dbs_info *shared;
 };
 
@@ -268,11 +276,11 @@ static ssize_t show_sampling_rate_min_gov_pol				\
 
 extern struct mutex cpufreq_governor_lock;
 
+void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
+void gov_cancel_work(struct cpu_common_dbs_info *shared);
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
 int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		struct common_dbs_data *cdata, unsigned int event);
-void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
-		unsigned int delay, bool all_cpus);
 void od_register_powersave_bias_handler(unsigned int (*f)
 		(struct cpufreq_policy *, unsigned int, unsigned int),
 		unsigned int powersave_bias);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index fc0384b4d02d..f879012cf849 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -286,13 +286,11 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
 			continue;
 
 		next_sampling = jiffies + usecs_to_jiffies(new_rate);
-		appointed_at = dbs_info->cdbs.dwork.timer.expires;
+		appointed_at = dbs_info->cdbs.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);
+			gov_cancel_work(shared);
+			gov_add_timers(policy, usecs_to_jiffies(new_rate));
 
 		}
 	}
-- 
2.6.2.198.g614a2ac

--
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]

#1286980 — [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-09 03:10 +0100
Subject[PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qDCmB-22q-3@gated-at.bofh.it>
In reply to#1283593
cpufreq governors evaluate load at sampling rate and based on that they
update frequency for a group of CPUs belonging to the same cpufreq
policy.

This is required to be done in a single thread for all policy->cpus, but
because we don't want to wakeup idle CPUs to do just that, we use
deferrable work for this. If we would have used a single delayed
deferrable work for the entire policy, there were chances that the CPU
required to run the handler can be in idle and we might end up not
changing the frequency for the entire group with load variations.

And so we were forced to keep per-cpu works, and only the one that
expires first need to do the real work and others are rescheduled for
next sampling time.

We have been using the more complex solution until now, where we used a
delayed deferrable work for this, which is a combination of a timer and
a work.

This could be made lightweight by keeping per-cpu deferred timers with a
single work item, which is scheduled by the first timer that expires.

This patch does just that and here are important changes:
- The timer handler will run in irq context and so we need to use a
  spin_lock instead of the timer_mutex. And so a separate timer_lock is
  created. This also makes the use of the mutex and lock quite clear, as
  we know what exactly they are protecting.
- A new field 'skip_work' is added to track when the timer handlers can
  queue a work. More comments present in code.

Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
V4: Add the missing EXPORT_SYMBOL_GPL(gov_cancel_work).

 drivers/cpufreq/cpufreq_governor.c | 142 ++++++++++++++++++++++---------------
 drivers/cpufreq/cpufreq_governor.h |  20 ++++--
 drivers/cpufreq/cpufreq_ondemand.c |   8 +--
 3 files changed, 100 insertions(+), 70 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 999e1f6addf9..2d61eae5cc5d 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -158,47 +158,53 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu)
 }
 EXPORT_SYMBOL_GPL(dbs_check_cpu);
 
-static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data,
-		unsigned int delay)
+void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay)
 {
-	struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
-
-	mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay);
-}
-
-void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
-		unsigned int delay, bool all_cpus)
-{
-	int i;
+	struct dbs_data *dbs_data = policy->governor_data;
+	struct cpu_dbs_info *cdbs;
+	int cpu;
 
-	if (!all_cpus) {
-		/*
-		 * Use raw_smp_processor_id() to avoid preemptible warnings.
-		 * We know that this is only called with all_cpus == false from
-		 * works that have been queued with *_work_on() functions and
-		 * those works are canceled during CPU_DOWN_PREPARE so they
-		 * can't possibly run on any other CPU.
-		 */
-		__gov_queue_work(raw_smp_processor_id(), dbs_data, delay);
-	} else {
-		for_each_cpu(i, policy->cpus)
-			__gov_queue_work(i, dbs_data, delay);
+	for_each_cpu(cpu, policy->cpus) {
+		cdbs = dbs_data->cdata->get_cpu_cdbs(cpu);
+		cdbs->timer.expires = jiffies + delay;
+		add_timer_on(&cdbs->timer, cpu);
 	}
 }
-EXPORT_SYMBOL_GPL(gov_queue_work);
+EXPORT_SYMBOL_GPL(gov_add_timers);
 
-static inline void gov_cancel_work(struct dbs_data *dbs_data,
-		struct cpufreq_policy *policy)
+static inline void gov_cancel_timers(struct cpufreq_policy *policy)
 {
+	struct dbs_data *dbs_data = policy->governor_data;
 	struct cpu_dbs_info *cdbs;
 	int i;
 
 	for_each_cpu(i, policy->cpus) {
 		cdbs = dbs_data->cdata->get_cpu_cdbs(i);
-		cancel_delayed_work_sync(&cdbs->dwork);
+		del_timer_sync(&cdbs->timer);
 	}
 }
 
+void gov_cancel_work(struct cpu_common_dbs_info *shared)
+{
+	unsigned long flags;
+
+	/*
+	 * No work will be queued from timer handlers after skip_work is
+	 * updated. And so we can safely cancel the work first and then the
+	 * timers.
+	 */
+	spin_lock_irqsave(&shared->timer_lock, flags);
+	shared->skip_work++;
+	spin_unlock_irqrestore(&shared->timer_lock, flags);
+
+	cancel_work_sync(&shared->work);
+
+	gov_cancel_timers(shared->policy);
+
+	shared->skip_work = 0;
+}
+EXPORT_SYMBOL_GPL(gov_cancel_work);
+
 /* Will return if we need to evaluate cpu load again or not */
 static bool need_load_eval(struct cpu_common_dbs_info *shared,
 			   unsigned int sampling_rate)
@@ -217,29 +223,22 @@ static bool need_load_eval(struct cpu_common_dbs_info *shared,
 	return true;
 }
 
-static void dbs_timer(struct work_struct *work)
+static void dbs_work_handler(struct work_struct *work)
 {
-	struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info,
-						 dwork.work);
-	struct cpu_common_dbs_info *shared = cdbs->shared;
+	struct cpu_common_dbs_info *shared = container_of(work, struct
+					cpu_common_dbs_info, work);
 	struct cpufreq_policy *policy;
 	struct dbs_data *dbs_data;
 	unsigned int sampling_rate, delay;
-	bool modify_all = true;
-
-	mutex_lock(&shared->timer_mutex);
+	unsigned long flags;
+	bool eval_load;
 
 	policy = shared->policy;
-
-	/*
-	 * Governor might already be disabled and there is no point continuing
-	 * with the work-handler.
-	 */
-	if (!policy)
-		goto unlock;
-
 	dbs_data = policy->governor_data;
 
+	/* Kill all timers */
+	gov_cancel_timers(policy);
+
 	if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
 		struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
 
@@ -250,14 +249,43 @@ static void dbs_timer(struct work_struct *work)
 		sampling_rate = od_tuners->sampling_rate;
 	}
 
-	if (!need_load_eval(cdbs->shared, sampling_rate))
-		modify_all = false;
+	eval_load = need_load_eval(shared, sampling_rate);
 
-	delay = dbs_data->cdata->gov_dbs_timer(policy, modify_all);
-	gov_queue_work(dbs_data, policy, delay, modify_all);
-
-unlock:
+	/*
+	 * Make sure cpufreq_governor_limits() isn't evaluating load in
+	 * parallel.
+	 */
+	mutex_lock(&shared->timer_mutex);
+	delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load);
 	mutex_unlock(&shared->timer_mutex);
+
+	spin_lock_irqsave(&shared->timer_lock, flags);
+	shared->skip_work--;
+	spin_unlock_irqrestore(&shared->timer_lock, flags);
+
+	gov_add_timers(policy, delay);
+}
+
+static void dbs_timer_handler(unsigned long data)
+{
+	struct cpu_dbs_info *cdbs = (struct cpu_dbs_info *)data;
+	struct cpu_common_dbs_info *shared = cdbs->shared;
+	unsigned long flags;
+
+	spin_lock_irqsave(&shared->timer_lock, flags);
+
+	/*
+	 * Timer handler isn't allowed to queue work at the moment, because:
+	 * - Another timer handler has done that
+	 * - We are stopping the governor
+	 * - Or we are updating the sampling rate of ondemand governor
+	 */
+	if (!shared->skip_work) {
+		shared->skip_work++;
+		queue_work(system_wq, &shared->work);
+	}
+
+	spin_unlock_irqrestore(&shared->timer_lock, flags);
 }
 
 static void set_sampling_rate(struct dbs_data *dbs_data,
@@ -288,6 +316,8 @@ static int alloc_common_dbs_info(struct cpufreq_policy *policy,
 		cdata->get_cpu_cdbs(j)->shared = shared;
 
 	mutex_init(&shared->timer_mutex);
+	spin_lock_init(&shared->timer_lock);
+	INIT_WORK(&shared->work, dbs_work_handler);
 	return 0;
 }
 
@@ -452,7 +482,9 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		if (ignore_nice)
 			j_cdbs->prev_cpu_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-		INIT_DEFERRABLE_WORK(&j_cdbs->dwork, dbs_timer);
+		__setup_timer(&j_cdbs->timer, dbs_timer_handler,
+			      (unsigned long)j_cdbs,
+			      TIMER_DEFERRABLE | TIMER_IRQSAFE);
 	}
 
 	if (cdata->governor == GOV_CONSERVATIVE) {
@@ -470,8 +502,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy,
 		od_ops->powersave_bias_init_cpu(cpu);
 	}
 
-	gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate),
-		       true);
+	gov_add_timers(policy, delay_for_sampling_rate(sampling_rate));
 	return 0;
 }
 
@@ -485,16 +516,9 @@ static int cpufreq_governor_stop(struct cpufreq_policy *policy,
 	if (!shared || !shared->policy)
 		return -EBUSY;
 
-	/*
-	 * Work-handler must see this updated, as it should not proceed any
-	 * further after governor is disabled. And so timer_mutex is taken while
-	 * updating this value.
-	 */
-	mutex_lock(&shared->timer_mutex);
+	gov_cancel_work(shared);
 	shared->policy = NULL;
-	mutex_unlock(&shared->timer_mutex);
 
-	gov_cancel_work(dbs_data, policy);
 	return 0;
 }
 
diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 0c7589016b6c..76742902491e 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -132,12 +132,20 @@ static void *get_cpu_dbs_info_s(int cpu)				\
 struct cpu_common_dbs_info {
 	struct cpufreq_policy *policy;
 	/*
-	 * percpu mutex that serializes governor limit change with dbs_timer
-	 * invocation. We do not want dbs_timer to run when user is changing
-	 * the governor or limits.
+	 * Per policy mutex that serializes load evaluation from limit-change
+	 * and work-handler.
 	 */
 	struct mutex timer_mutex;
+
+	/*
+	 * Per policy lock that serializes access to queuing work from timer
+	 * handlers.
+	 */
+	spinlock_t timer_lock;
+
 	ktime_t time_stamp;
+	unsigned int skip_work;
+	struct work_struct work;
 };
 
 /* Per cpu structures */
@@ -152,7 +160,7 @@ struct cpu_dbs_info {
 	 * wake-up from idle.
 	 */
 	unsigned int prev_load;
-	struct delayed_work dwork;
+	struct timer_list timer;
 	struct cpu_common_dbs_info *shared;
 };
 
@@ -268,11 +276,11 @@ static ssize_t show_sampling_rate_min_gov_pol				\
 
 extern struct mutex cpufreq_governor_lock;
 
+void gov_add_timers(struct cpufreq_policy *policy, unsigned int delay);
+void gov_cancel_work(struct cpu_common_dbs_info *shared);
 void dbs_check_cpu(struct dbs_data *dbs_data, int cpu);
 int cpufreq_governor_dbs(struct cpufreq_policy *policy,
 		struct common_dbs_data *cdata, unsigned int event);
-void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy,
-		unsigned int delay, bool all_cpus);
 void od_register_powersave_bias_handler(unsigned int (*f)
 		(struct cpufreq_policy *, unsigned int, unsigned int),
 		unsigned int powersave_bias);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index fc0384b4d02d..f879012cf849 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -286,13 +286,11 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
 			continue;
 
 		next_sampling = jiffies + usecs_to_jiffies(new_rate);
-		appointed_at = dbs_info->cdbs.dwork.timer.expires;
+		appointed_at = dbs_info->cdbs.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);
+			gov_cancel_work(shared);
+			gov_add_timers(policy, usecs_to_jiffies(new_rate));
 
 		}
 	}
-- 
2.6.2.198.g614a2ac

--
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]

#1287882 — Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-12-09 22:40 +0100
SubjectRe: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qDUCT-5lP-29@gated-at.bofh.it>
In reply to#1286980
On Wednesday, December 09, 2015 07:34:42 AM Viresh Kumar wrote:
> cpufreq governors evaluate load at sampling rate and based on that they
> update frequency for a group of CPUs belonging to the same cpufreq
> policy.
> 
> This is required to be done in a single thread for all policy->cpus, but
> because we don't want to wakeup idle CPUs to do just that, we use
> deferrable work for this. If we would have used a single delayed
> deferrable work for the entire policy, there were chances that the CPU
> required to run the handler can be in idle and we might end up not
> changing the frequency for the entire group with load variations.
> 
> And so we were forced to keep per-cpu works, and only the one that
> expires first need to do the real work and others are rescheduled for
> next sampling time.
> 
> We have been using the more complex solution until now, where we used a
> delayed deferrable work for this, which is a combination of a timer and
> a work.
> 
> This could be made lightweight by keeping per-cpu deferred timers with a
> single work item, which is scheduled by the first timer that expires.
> 
> This patch does just that and here are important changes:
> - The timer handler will run in irq context and so we need to use a
>   spin_lock instead of the timer_mutex. And so a separate timer_lock is
>   created. This also makes the use of the mutex and lock quite clear, as
>   we know what exactly they are protecting.
> - A new field 'skip_work' is added to track when the timer handlers can
>   queue a work. More comments present in code.
> 
> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Reviewed-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>

OK, replaced the one in my tree with this one, thanks!

BTW, can you please add an extra From: line to the bodies of your patch
messages?

For some unknown reason Patchwork or your mailer or the combination of the
two mangles your name for me and I have to fix it up manually in every patch
from you which is a !@#$%^&*() pain.

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]

#1288147 — Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-10 03:40 +0100
SubjectRe: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qDZjb-8n3-9@gated-at.bofh.it>
In reply to#1287882
On 09-12-15, 23:06, Rafael J. Wysocki wrote:
> BTW, can you please add an extra From: line to the bodies of your patch
> messages?
> 
> For some unknown reason Patchwork or your mailer or the combination of the
> two mangles your name for me and I have to fix it up manually in every patch
> from you which is a !@#$%^&*() pain.

I have raised an RT request for this, lets see what's wrong.

-- 
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]

#1288949 — Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers

From"Rafael J. Wysocki" <rjw@rjwysocki.net>
Date2015-12-10 22:50 +0100
SubjectRe: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qEhg6-3j4-9@gated-at.bofh.it>
In reply to#1288147
On Thursday, December 10, 2015 08:06:26 AM Viresh Kumar wrote:
> On 09-12-15, 23:06, Rafael J. Wysocki wrote:
> > BTW, can you please add an extra From: line to the bodies of your patch
> > messages?
> > 
> > For some unknown reason Patchwork or your mailer or the combination of the
> > two mangles your name for me and I have to fix it up manually in every patch
> > from you which is a !@#$%^&*() pain.
> 
> I have raised an RT request for this, lets see what's wrong.

Well, if you did what I asked for, it would work around the problem whatever
that is.

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]

#1289053 — Re: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers

FromViresh Kumar <viresh.kumar@linaro.org>
Date2015-12-11 02:50 +0100
SubjectRe: [PATCH V4 5/6] cpufreq: governor: replace per-cpu delayed work with timers
Message-ID<qEl0l-5Qp-9@gated-at.bofh.it>
In reply to#1288949
On 10-12-15, 23:17, Rafael J. Wysocki wrote:
> Well, if you did what I asked for, it would work around the problem whatever
> that is.

Yeah, I will do that until it is resolved from backend.

-- 
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] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web