| Message ID | 20210503192810.36084-2-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 03.05.2021 21:27, Jason Andryuk wrote: > For hwp, the standard governors are not usable, and only the internal > one is applicable. So you say "one" here but use plural in the subject. Which one is it (going to be)? > Add the cpufreq_governor_internal boolean to > indicate when an internal governor, like hwp-internal, will be used. > This is set during presmp_initcall, so that it can suppress governor DYM s/is/will be/? Afaict this is going to happen later in the series. Which is a good indication that such "hanging in the air" changes aren't necessarily the best way of splitting a set of changes, ... > --- a/xen/drivers/cpufreq/cpufreq.c > +++ b/xen/drivers/cpufreq/cpufreq.c > @@ -57,6 +57,7 @@ struct cpufreq_dom { > }; > static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head); > > +bool __read_mostly cpufreq_governor_internal; ... also supported by you introducing a non-static variable without any consumer outside of this file (and without any producer at all). > @@ -122,6 +123,9 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) > if (!governor) > return -EINVAL; > > + if (cpufreq_governor_internal && strstr(governor->name, "internal") == NULL) I wonder whether designating any governors ending in "-internal" wouldn't be less prone for possible future ambiguities. > --- a/xen/include/acpi/cpufreq/cpufreq.h > +++ b/xen/include/acpi/cpufreq/cpufreq.h > @@ -115,6 +115,7 @@ extern struct cpufreq_governor cpufreq_gov_dbs; > extern struct cpufreq_governor cpufreq_gov_userspace; > extern struct cpufreq_governor cpufreq_gov_performance; > extern struct cpufreq_governor cpufreq_gov_powersave; > +extern bool cpufreq_governor_internal; Please separate from the governor declarations by a blank line. Sorry, all quite nit-like remarks, but still ... Jan
On Wed, May 26, 2021 at 9:18 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 03.05.2021 21:27, Jason Andryuk wrote: > > For hwp, the standard governors are not usable, and only the internal > > one is applicable. > > So you say "one" here but use plural in the subject. Which one is > it (going to be)? hwp only uses a single governor, but this is common code. AMD or ARM could require their own internal governor which is why the subject say plural. > > Add the cpufreq_governor_internal boolean to > > indicate when an internal governor, like hwp-internal, will be used. > > This is set during presmp_initcall, so that it can suppress governor > > DYM s/is/will be/? Afaict this is going to happen later in the series. > Which is a good indication that such "hanging in the air" changes > aren't necessarily the best way of splitting a set of changes, ... In terms of the patch series, yes, "will be". The use of "is" is directing how to use the feature. Yes, it is "hanging in the air", but I was trying to explain the "why" and "how" of using it. I was trying to split this preparatory change from the actual hwp introduction. I suppose it could be ordered after hwp, and the extra, unusable governors would be advertised until then. > > --- a/xen/drivers/cpufreq/cpufreq.c > > +++ b/xen/drivers/cpufreq/cpufreq.c > > @@ -57,6 +57,7 @@ struct cpufreq_dom { > > }; > > static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head); > > > > +bool __read_mostly cpufreq_governor_internal; > > ... also supported by you introducing a non-static variable without > any consumer outside of this file (and without any producer at all). > > > @@ -122,6 +123,9 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) > > if (!governor) > > return -EINVAL; > > > > + if (cpufreq_governor_internal && strstr(governor->name, "internal") == NULL) > > I wonder whether designating any governors ending in "-internal" > wouldn't be less prone for possible future ambiguities. Yes, that would be good. > > --- a/xen/include/acpi/cpufreq/cpufreq.h > > +++ b/xen/include/acpi/cpufreq/cpufreq.h > > @@ -115,6 +115,7 @@ extern struct cpufreq_governor cpufreq_gov_dbs; > > extern struct cpufreq_governor cpufreq_gov_userspace; > > extern struct cpufreq_governor cpufreq_gov_performance; > > extern struct cpufreq_governor cpufreq_gov_powersave; > > +extern bool cpufreq_governor_internal; > > Please separate from the governor declarations by a blank line. Sure. > Sorry, all quite nit-like remarks, but still ... It's fine. Would a design session be useful to discuss hwp? Regards, Jason
On 26.05.2021 16:12, Jason Andryuk wrote: > On Wed, May 26, 2021 at 9:18 AM Jan Beulich <jbeulich@suse.com> wrote: >> Sorry, all quite nit-like remarks, but still ... > > It's fine. Would a design session be useful to discuss hwp? Is there anything beyond patch review that's necessary there? I'm also not really set up to usefully join design sessions, I'm afraid. Jan
On Wed, May 26, 2021 at 11:09 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 26.05.2021 16:12, Jason Andryuk wrote: > > On Wed, May 26, 2021 at 9:18 AM Jan Beulich <jbeulich@suse.com> wrote: > >> Sorry, all quite nit-like remarks, but still ... > > > > It's fine. Would a design session be useful to discuss hwp? > > Is there anything beyond patch review that's necessary there? I'm > also not really set up to usefully join design sessions, I'm afraid. It's not necessary, but I figured I'd offer since Xen Summit is going on. The part where it might be useful is the hypercall interface. I basically exposed all the MSR configuration. I couldn't think of a useful abstraction, and this way the full range of configuration is available. Additionally, there are some convenience presets to make it a little more user friendly. Thank you for reviewing. Regards, Jason
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index 419aae83ee..6cbf150538 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -57,6 +57,7 @@ struct cpufreq_dom { }; static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head); +bool __read_mostly cpufreq_governor_internal; struct cpufreq_governor *__read_mostly cpufreq_opt_governor; LIST_HEAD_READ_MOSTLY(cpufreq_governor_list); @@ -122,6 +123,9 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) if (!governor) return -EINVAL; + if (cpufreq_governor_internal && strstr(governor->name, "internal") == NULL) + return -EINVAL; + if (__find_governor(governor->name) != NULL) return -EEXIST; diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h index e88b20bfed..56df5eebed 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -115,6 +115,7 @@ extern struct cpufreq_governor cpufreq_gov_dbs; extern struct cpufreq_governor cpufreq_gov_userspace; extern struct cpufreq_governor cpufreq_gov_performance; extern struct cpufreq_governor cpufreq_gov_powersave; +extern bool cpufreq_governor_internal; extern struct list_head cpufreq_governor_list;
For hwp, the standard governors are not usable, and only the internal one is applicable. Add the cpufreq_governor_internal boolean to indicate when an internal governor, like hwp-internal, will be used. This is set during presmp_initcall, so that it can suppress governor registration during initcall. Only a governor with a name containing "internal" will be allowed in that case. This way, the unuseable governors are not registered, so they internal one is the only one returned to userspace. This means incompatible governors won't be advertised to userspace. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- xen/drivers/cpufreq/cpufreq.c | 4 ++++ xen/include/acpi/cpufreq/cpufreq.h | 1 + 2 files changed, 5 insertions(+)