| Message ID | 20230614180253.89958-2-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 14.06.2023 20:02, Jason Andryuk wrote: > @@ -121,6 +122,12 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) > if (!governor) > return -EINVAL; > > + if (cpufreq_governor_internal && !governor->internal) > + return -EINVAL; > + > + if (!cpufreq_governor_internal && governor->internal) > + return -EINVAL; First just a nit: Why not simply if (cpufreq_governor_internal != governor->internal) return -EINVAL; ? Yet then I find this approach a little odd anyway. I think the registration attempts would better be suppressed, thus also not resulting in (apparently) failed init-calls. Especially for the userspace governor this would then also mean / allow to avoid registering of the CPU notifier. Jan
On Thu, Jun 15, 2023 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 14.06.2023 20:02, Jason Andryuk wrote: > > @@ -121,6 +122,12 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) > > if (!governor) > > return -EINVAL; > > > > + if (cpufreq_governor_internal && !governor->internal) > > + return -EINVAL; > > + > > + if (!cpufreq_governor_internal && governor->internal) > > + return -EINVAL; > > First just a nit: Why not simply > > if (cpufreq_governor_internal != governor->internal) > return -EINVAL; > > ? Yes, that would work. > Yet then I find this approach a little odd anyway. I think the > registration attempts would better be suppressed, thus also not > resulting in (apparently) failed init-calls. Especially for the > userspace governor this would then also mean / allow to avoid > registering of the CPU notifier. So are you suggesting that each caller check cpufreq_governor_internal and potentially skip calling cpufreq_register_governor()? e.g. the start of cpufreq_gov_userspace_init() would gain: if (cpufreq_governor_internal) return 0 ? I put the check in cpufreq_register_governor() since then it is handled in a single place instead of being spread around. To leave the check in cpufreq_register_governor(), cpufreq_gov_userspace_init() could be rearranged to call cpufreq_register_governor() first and only call register_cpu_notifier() on success? Or do you want the whole governor registration to be re-worked to be more explicit? Remove initcalls and have governors registered according to the cpufreq driver? Regards, Jason
On 15.06.2023 16:04, Jason Andryuk wrote: > On Thu, Jun 15, 2023 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote: >> Yet then I find this approach a little odd anyway. I think the >> registration attempts would better be suppressed, thus also not >> resulting in (apparently) failed init-calls. Especially for the >> userspace governor this would then also mean / allow to avoid >> registering of the CPU notifier. > > So are you suggesting that each caller check cpufreq_governor_internal > and potentially skip calling cpufreq_register_governor()? e.g. the > start of cpufreq_gov_userspace_init() would gain: > > if (cpufreq_governor_internal) > return 0 > > ? Right. > I put the check in cpufreq_register_governor() since then it is > handled in a single place instead of being spread around. I understand this was the goal, but I'm still wondering why we would try registration we know will fail. Plus, as said and even if benign right now, I'm not happy to see initcall failures being introduced. > To leave the check in cpufreq_register_governor(), > cpufreq_gov_userspace_init() could be rearranged to call > cpufreq_register_governor() first and only call > register_cpu_notifier() on success? Might be an option, but would address only part of my concerns. > Or do you want the whole governor registration to be re-worked to be > more explicit? Remove initcalls and have governors registered > according to the cpufreq driver? No, I'm not after any extensive re-work. Jan
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c index 2321c7dd07..cccf9a64c8 100644 --- a/xen/drivers/cpufreq/cpufreq.c +++ b/xen/drivers/cpufreq/cpufreq.c @@ -56,6 +56,7 @@ struct cpufreq_dom { }; static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head); +bool __initdata cpufreq_governor_internal; struct cpufreq_governor *__read_mostly cpufreq_opt_governor; LIST_HEAD_READ_MOSTLY(cpufreq_governor_list); @@ -121,6 +122,12 @@ int __init cpufreq_register_governor(struct cpufreq_governor *governor) if (!governor) return -EINVAL; + if (cpufreq_governor_internal && !governor->internal) + return -EINVAL; + + if (!cpufreq_governor_internal && governor->internal) + 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 35dcf21e8f..1c0872506a 100644 --- a/xen/include/acpi/cpufreq/cpufreq.h +++ b/xen/include/acpi/cpufreq/cpufreq.h @@ -106,6 +106,7 @@ struct cpufreq_governor { unsigned int event); bool_t (*handle_option)(const char *name, const char *value); struct list_head governor_list; + bool internal; }; extern struct cpufreq_governor *cpufreq_opt_governor; @@ -114,6 +115,8 @@ 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; extern int cpufreq_register_governor(struct cpufreq_governor *governor);
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, will be used. This is set during presmp_initcall, so that it can suppress governor registration during initcall. Add an internal flag to struct cpufreq_governor to indicate such governors. This way, the unusable governors are not registered, so the 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> --- v4: Rework to use an internal flag Removed Jan's Ack since the approach is different. v3: Switch to initdata Add Jan Acked-by Commit message s/they/the/ typo Don't register hwp-internal when running non-hwp - Marek v2: Switch to "-internal" Add blank line in header --- xen/drivers/cpufreq/cpufreq.c | 7 +++++++ xen/include/acpi/cpufreq/cpufreq.h | 3 +++ 2 files changed, 10 insertions(+)