Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1366887
| From | Viresh Kumar <viresh.kumar@linaro.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data |
| Date | 2016-03-30 06:20 +0200 |
| Message-ID | <rifLP-8p2-5@gated-at.bofh.it> (permalink) |
| References | <rfjLX-5N2-3@gated-at.bofh.it> <rfjLX-5N2-1@gated-at.bofh.it> <rhBlo-4N8-5@gated-at.bofh.it> <ri1pv-6kK-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On 29-03-16, 14:58, Rafael J. Wysocki wrote:
> On Monday, March 28, 2016 02:33:33 PM Viresh Kumar wrote:
> > Its gonna be same for all policies sharing tunables ..
>
> The value will be the same, but the cacheline won't.
Fair enough. So this information is replicated for each policy for performance
benefits. Would it make sense to add a comment for that? Its not obvious today
why we are keeping this per-policy.
> > > +static ssize_t rate_limit_us_store(struct gov_attr_set *attr_set, const char *buf,
> > > + size_t count)
> > > +{
> > > + struct sugov_tunables *tunables = to_sugov_tunables(attr_set);
> > > + struct sugov_policy *sg_policy;
> > > + unsigned int rate_limit_us;
> > > + int ret;
> > > +
> > > + ret = sscanf(buf, "%u", &rate_limit_us);
> > > + if (ret != 1)
> > > + return -EINVAL;
> > > +
> > > + tunables->rate_limit_us = rate_limit_us;
> > > +
> > > + list_for_each_entry(sg_policy, &attr_set->policy_list, tunables_hook)
> > > + sg_policy->freq_update_delay_ns = rate_limit_us * NSEC_PER_USEC;
> > > +
> > > + return count;
> > > +}
> > > +
> > > +static struct governor_attr rate_limit_us = __ATTR_RW(rate_limit_us);
> >
> > Why not reuse gov_attr_rw() ?
>
> Would it work?
Why wouldn't it? I had a look again at that and I couldn't find a reason for it
to not work. Probably I missed something.
> > > + ret = kobject_init_and_add(&tunables->attr_set.kobj, &sugov_tunables_ktype,
> > > + get_governor_parent_kobj(policy), "%s",
> > > + schedutil_gov.name);
> > > + if (!ret)
> > > + goto out;
> > > +
> > > + /* Failure, so roll back. */
> > > + policy->governor_data = NULL;
> > > + sugov_tunables_free(tunables);
> > > +
> > > + free_sg_policy:
> > > + pr_err("cpufreq: schedutil governor initialization failed (error %d)\n", ret);
> > > + sugov_policy_free(sg_policy);
> >
> > I didn't like the way we have mixed success and failure path here, just to save
> > a single line of code (unlock).
>
> I don't follow, sorry. Yes, I can do unlock/return instead of the "goto out",
> but then the goto label is still needed.
Sorry for not being clear earlier, but this what I was suggesting it to look like:
---
static int sugov_init(struct cpufreq_policy *policy)
{
struct sugov_policy *sg_policy;
struct sugov_tunables *tunables;
unsigned int lat;
int ret = 0;
/* State should be equivalent to EXIT */
if (policy->governor_data)
return -EBUSY;
sg_policy = sugov_policy_alloc(policy);
if (!sg_policy)
return -ENOMEM;
mutex_lock(&global_tunables_lock);
if (global_tunables) {
if (WARN_ON(have_governor_per_policy())) {
ret = -EINVAL;
goto free_sg_policy;
}
policy->governor_data = sg_policy;
sg_policy->tunables = global_tunables;
gov_attr_set_get(&global_tunables->attr_set, &sg_policy->tunables_hook);
mutex_unlock(&global_tunables_lock);
return 0;
}
tunables = sugov_tunables_alloc(sg_policy);
if (!tunables) {
ret = -ENOMEM;
goto free_sg_policy;
}
tunables->rate_limit_us = LATENCY_MULTIPLIER;
lat = policy->cpuinfo.transition_latency / NSEC_PER_USEC;
if (lat)
tunables->rate_limit_us *= lat;
if (!have_governor_per_policy())
global_tunables = tunables;
policy->governor_data = sg_policy;
sg_policy->tunables = tunables;
ret = kobject_init_and_add(&tunables->attr_set.kobj, &sugov_tunables_ktype,
get_governor_parent_kobj(policy), "%s",
schedutil_gov.name);
if (!ret) {
mutex_unlock(&global_tunables_lock);
return 0;
}
/* Failure, so roll back. */
policy->governor_data = NULL;
sugov_tunables_free(tunables);
free_sg_policy:
mutex_unlock(&global_tunables_lock);
pr_err("cpufreq: schedutil governor initialization failed (error %d)\n", ret);
sugov_policy_free(sg_policy);
return ret;
---
> > Over that it does things, that aren't symmetric anymore. For example, we have
> > called sugov_policy_alloc() without locks
>
> Are you sure?
Yes.
> > and are freeing it from within locks.
>
> Both are under global_tunables_lock.
No, sugov_policy_alloc() isn't called from within locks.
> > > +static int __init sugov_module_init(void)
> > > +{
> > > + return cpufreq_register_governor(&schedutil_gov);
> > > +}
> > > +
> > > +static void __exit sugov_module_exit(void)
> > > +{
> > > + cpufreq_unregister_governor(&schedutil_gov);
> > > +}
> > > +
> > > +MODULE_AUTHOR("Rafael J. Wysocki <rafael.j.wysocki@intel.com>");
> > > +MODULE_DESCRIPTION("Utilization-based CPU frequency selection");
> > > +MODULE_LICENSE("GPL");
> >
> > Maybe a MODULE_ALIAS as well ?
>
> Sorry, I don't follow.
Oh, I was just saying that we may also want to add a MODULE_ALIAS() line here
to help auto-loading if it is built as a module?
--
viresh
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-22 03:00 +0100
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-03-26 02:20 +0100
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-26 03:10 +0100
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-27 03:40 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-03-28 20:20 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-29 14:30 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Peter Zijlstra <peterz@infradead.org> - 2016-03-31 14:30 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-31 14:40 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-04-01 20:20 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-28 11:10 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-29 15:00 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-30 03:20 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Peter Zijlstra <peterz@infradead.org> - 2016-03-31 14:30 +0200
Re: [PATCH v6 7/7][Resend] cpufreq: schedutil: New governor based on scheduler utilization data Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-30 06:20 +0200
[Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-30 04:00 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-30 07:40 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-30 13:40 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-03-30 19:10 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-30 19:30 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-03-31 03:50 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Peter Zijlstra <peterz@infradead.org> - 2016-03-31 14:20 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-03-31 14:20 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Peter Zijlstra <peterz@infradead.org> - 2016-03-31 14:50 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Peter Zijlstra <peterz@infradead.org> - 2016-03-31 14:50 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-04-01 19:50 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rafael@kernel.org> - 2016-04-01 21:20 +0200
Re: [Update][PATCH v7 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-04-01 21:30 +0200
[Update][PATCH v8 7/7] cpufreq: schedutil: New governor based on scheduler utilization data "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-02 01:10 +0200
Re: [Update][PATCH v8 7/7] cpufreq: schedutil: New governor based on scheduler utilization data Steve Muckle <steve.muckle@linaro.org> - 2016-04-02 03:10 +0200
csiph-web