Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1327553
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures |
| Date | 2016-02-05 10:20 +0100 |
| Message-ID | <qYKIy-6HT-15@gated-at.bofh.it> (permalink) |
| References | <qYfbJ-uu-39@gated-at.bofh.it> <qYEjM-2cq-3@gated-at.bofh.it> <qYEjM-2cq-15@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On 05-02-16, 03:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The struct policy_dbs_info objects representing per-policy governor
> data are not accessible directly from the corresponding policy
> objects. To access them, one has to get a pointer to the
> struct cpu_dbs_info of policy->cpu and use the "shared" field of
> that which isn't really straightforward.
>
> To address that rearrange the governor data structures so the
> governor_data pointer in struct cpufreq_policy will point to
> struct policy_dbs_info and that will contain a pointer to
> struct dbs_data.
IMHO, this patch has done way too much over what's mentioned here.
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -297,16 +298,22 @@ static int alloc_policy_dbs_info(struct
> /* Allocate memory for the common information for policy->cpus */
> policy_dbs = kzalloc(sizeof(*policy_dbs), GFP_KERNEL);
> if (!policy_dbs)
> - return -ENOMEM;
> -
> - /* Set policy_dbs for all CPUs, online+offline */
> - for_each_cpu(j, policy->related_cpus)
> - gov->get_cpu_cdbs(j)->shared = policy_dbs;
> + return NULL;
>
> + policy_dbs->policy = policy;
Value of policy_dbs->policy was used to verify the state machine of
the governor and so was updated only in start/stop.
You have moved it to INIT first (which shouldn't have been part of
this patch at the least), and then there is no reasoning given on why
that isn't required as part of the state machine now, which I believe
is still required the way it was.
> mutex_init(&policy_dbs->timer_mutex);
> atomic_set(&policy_dbs->skip_work, 0);
> + init_irq_work(&policy_dbs->irq_work, dbs_irq_work);
> + init_completion(&policy_dbs->irq_work_done);
> INIT_WORK(&policy_dbs->work, dbs_work_handler);
> - return 0;
> + /* Set policy_dbs for all CPUs, online+offline */
> + for_each_cpu(j, policy->related_cpus) {
> + struct cpu_dbs_info *j_cdbs = gov->get_cpu_cdbs(j);
> +
> + j_cdbs->shared = policy_dbs;
> + j_cdbs->update_util.func = dbs_update_util_handler;
> + }
All other initializations are moved here for the good, but I think it
could have been done in a separate patch as to make review of this
trivial patch, which isn't so easy to review, easy :)
> static int cpufreq_governor_exit(struct cpufreq_policy *policy)
> {
> struct dbs_governor *gov = dbs_governor_of(policy);
> - struct dbs_data *dbs_data = policy->governor_data;
> - struct cpu_dbs_info *cdbs = gov->get_cpu_cdbs(policy->cpu);
> + struct policy_dbs_info *policy_dbs = policy->governor_data;
> + struct dbs_data *dbs_data = policy_dbs->dbs_data;
>
> /* State should be equivalent to INIT */
> - if (!cdbs->shared || cdbs->shared->policy)
For example, to make sure the current state is equivalent to INIT, we
had two checks earlier:
- First one made sure that cdbs->shared is allocated (which was done
in INIT)
- And second one makes sure that shared->policy is NULL, as that is
initialized in START.
> + if (!dbs_data)
> return -EBUSY;
But now, the current state can be INIT or START, you can't
differentiate.
> static int cpufreq_governor_start(struct cpufreq_policy *policy)
> {
> struct dbs_governor *gov = dbs_governor_of(policy);
> - struct dbs_data *dbs_data = policy->governor_data;
> + struct policy_dbs_info *policy_dbs = policy->governor_data;
> + struct dbs_data *dbs_data = policy_dbs->dbs_data;
> unsigned int sampling_rate, ignore_nice, j, cpu = policy->cpu;
> - struct cpu_dbs_info *cdbs = gov->get_cpu_cdbs(cpu);
> - struct policy_dbs_info *policy_dbs = cdbs->shared;
> int io_busy = 0;
>
> if (!policy->cur)
> return -EINVAL;
>
> /* State should be equivalent to INIT */
> - if (!policy_dbs || policy_dbs->policy)
> + if (!dbs_data)
Same here..
> static int cpufreq_governor_stop(struct cpufreq_policy *policy)
> {
> - struct dbs_governor *gov = dbs_governor_of(policy);
> - struct cpu_dbs_info *cdbs = gov->get_cpu_cdbs(policy->cpu);
> - struct policy_dbs_info *policy_dbs = cdbs->shared;
> -
> - /* State should be equivalent to START */
> - if (!policy_dbs || !policy_dbs->policy)
And here ..
> - return -EBUSY;
> -
> - gov_cancel_work(policy_dbs);
> - policy_dbs->policy = NULL;
> -
> + gov_cancel_work(policy->governor_data);
> return 0;
> }
>
> static int cpufreq_governor_limits(struct cpufreq_policy *policy)
> {
> - struct dbs_governor *gov = dbs_governor_of(policy);
> - unsigned int cpu = policy->cpu;
> - struct cpu_dbs_info *cdbs = gov->get_cpu_cdbs(cpu);
> + struct policy_dbs_info *policy_dbs = policy->governor_data;
>
> /* State should be equivalent to START */
> - if (!cdbs->shared || !cdbs->shared->policy)
> + if (!policy_dbs->dbs_data)
And here...
> return -EBUSY;
>
> - mutex_lock(&cdbs->shared->timer_mutex);
> - if (policy->max < cdbs->shared->policy->cur)
> - __cpufreq_driver_target(cdbs->shared->policy, policy->max,
> - CPUFREQ_RELATION_H);
> - else if (policy->min > cdbs->shared->policy->cur)
> - __cpufreq_driver_target(cdbs->shared->policy, policy->min,
> - CPUFREQ_RELATION_L);
> - dbs_check_cpu(policy, cpu);
> - mutex_unlock(&cdbs->shared->timer_mutex);
> + mutex_lock(&policy_dbs->timer_mutex);
> + if (policy->max < policy->cur)
> + __cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
> + else if (policy->min > policy->cur)
> + __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
> + dbs_check_cpu(policy, policy->cpu);
> + mutex_unlock(&policy_dbs->timer_mutex);
>
> return 0;
> }
--
viresh
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 0/11] cpufreq: governor: ondemand/conservative data structures rework "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-04 00:50 +0100
[PATCH 5/11] cpufreq: governor: Put governor structure into common_dbs_data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-04 00:50 +0100
Re: [PATCH 5/11] cpufreq: governor: Put governor structure into common_dbs_data Saravana Kannan <skannan@codeaurora.org> - 2016-02-04 03:00 +0100
[PATCH 11/11] cpufreq: governor: Drop cpu argument from dbs_check_cpu() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-04 00:50 +0100
Re: [PATCH 0/11] cpufreq: governor: ondemand/conservative data structures rework Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-04 06:50 +0100
Re: [PATCH 0/11] cpufreq: governor: ondemand/conservative data structures rework "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-04 18:30 +0100
[PATCH v2 7/10] cpufreq: governor: Drop the gov pointer from struct dbs_data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 7/10] cpufreq: governor: Drop the gov pointer from struct dbs_data Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 09:30 +0100
[PATCH v2 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 09:40 +0100
Re: [PATCH v2 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 23:50 +0100
[PATCH v3 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 13:50 +0100
Re: [PATCH v3 8/10] cpufreq: governor: Rename cpu_common_dbs_info to policy_dbs_info Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:40 +0100
[PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 08:00 +0100
Re: [PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 00:00 +0100
Re: [PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:40 +0100
Re: [PATCH v2 2/10] cpufreq: governor: Use common mutex for dbs_data protection "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 15:40 +0100
[PATCH v2 6/10] cpufreq: governor: Rework cpufreq_governor_dbs() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 6/10] cpufreq: governor: Rework cpufreq_governor_dbs() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 09:20 +0100
[PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 10:20 +0100
Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 23:50 +0100
Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:30 +0100
Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 15:40 +0100
[PATCH v2 1/10] cpufreq: Clean up default and fallback governor setup "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
[PATCH v2 0/10] cpufreq: governor: ondemand/conservative data structures rework "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
[PATCH v2 3/10] cpufreq: governor: Avoid passing dbs_data pointers around unnecessarily "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 3/10] cpufreq: governor: Avoid passing dbs_data pointers around unnecessarily Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 08:10 +0100
[PATCH v2 5/10] cpufreq: governor: Rename some data types and variables "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 5/10] cpufreq: governor: Rename some data types and variables Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 08:20 +0100
[PATCH v2 10/10] cpufreq: governor: Drop cpu argument from dbs_check_cpu() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 10/10] cpufreq: governor: Drop cpu argument from dbs_check_cpu() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 10:20 +0100
[PATCH v3 10/10] cpufreq: governor: Drop cpu argument from dbs_check_cpu() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 13:50 +0100
Re: [PATCH v3 10/10] cpufreq: governor: Drop cpu argument from dbs_check_cpu() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 10:40 +0100
[PATCH v2 4/10] cpufreq: governor: Put governor structure into common_dbs_data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-05 03:30 +0100
Re: [PATCH v2 4/10] cpufreq: governor: Put governor structure into common_dbs_data Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-05 08:20 +0100
Re: [PATCH v2 0/10] cpufreq: governor: ondemand/conservative data structures rework "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-06 13:50 +0100
[PATCH 1/3] cpufreq: governor: Simplify cpufreq_governor_limits() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 16:30 +0100
Re: [PATCH 1/3] cpufreq: governor: Simplify cpufreq_governor_limits() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 16:50 +0100
Re: [PATCH 1/3] cpufreq: governor: Simplify cpufreq_governor_limits() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-08 02:00 +0100
[PATCH 0/3] cpufreq: governor: Data structure rearrangement "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 16:30 +0100
[PATCH 3/3] cpufreq: governor: Symmetrize cpu_dbs_info initialization and cleanup "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 16:30 +0100
Re: [PATCH 3/3] cpufreq: governor: Symmetrize cpu_dbs_info initialization and cleanup Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 17:00 +0100
[PATCH 2/3] cpufreq: governor: Rearrange governor data structures "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-07 16:30 +0100
Re: [PATCH 2/3] cpufreq: governor: Rearrange governor data structures Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 16:50 +0100
Re: [PATCH 2/3] cpufreq: governor: Rearrange governor data structures Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 17:00 +0100
Re: [PATCH 2/3] cpufreq: governor: Rearrange governor data structures Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-07 17:00 +0100
csiph-web