_PSD info, consisted of "shared_type" and "struct xen_psd_package", will not
only be provided from px-specific "struct xen_processor_performance", but also
in CPPC data.
In cpufreq_add/del_cpu(), a new helper get_psd_info() is introduced to
extract common _PSD info from px-specific "struct xen_processor_performance",
in the meantime, the following style corrections get applied at the same time:
- add extra space before and after bracket of if()
- remove redundant parenthesis
- no need to put brace for printk() at a seperate line
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v3 -> v4:
- new commit
---
v4 -> v5:
- let check_psd_pminfo() pass in "uint32_t shared_type"
- replace unnessary parameter "uint32_t init" with processor_pminfo[cpu]->init
- replace structure copy with const pointer delivery through
"const struct xen_psd_package **"
- blank line between non-fall-through switch-case blocks
- remove unnessary "define XEN_CPUPERF_SHARED_TYPE_xxx" movement
---
xen/drivers/cpufreq/cpufreq.c | 111 ++++++++++++++++++++++++----------
1 file changed, 79 insertions(+), 32 deletions(-)
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 08d027317c..9567221d97 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -191,9 +191,29 @@ int cpufreq_limit_change(unsigned int cpu)
return __cpufreq_set_policy(data, &policy);
}
-int cpufreq_add_cpu(unsigned int cpu)
+static int get_psd_info(unsigned int cpu, uint32_t *shared_type,
+ const struct xen_psd_package **domain_info_ptr)
{
int ret = 0;
+
+ switch ( processor_pminfo[cpu]->init )
+ {
+ case XEN_PX_INIT:
+ *shared_type = processor_pminfo[cpu]->perf.shared_type;
+ *domain_info_ptr = &processor_pminfo[cpu]->perf.domain_info;
+ break;
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+int cpufreq_add_cpu(unsigned int cpu)
+{
+ int ret;
unsigned int firstcpu;
unsigned int dom, domexist = 0;
unsigned int hw_all = 0;
@@ -201,14 +221,14 @@ int cpufreq_add_cpu(unsigned int cpu)
struct cpufreq_dom *cpufreq_dom = NULL;
struct cpufreq_policy new_policy;
struct cpufreq_policy *policy;
- struct processor_performance *perf;
+ const struct xen_psd_package *domain_info;
+ const struct xen_psd_package **domain_info_ptr = &domain_info;
+ uint32_t shared_type;
/* to protect the case when Px was not controlled by xen */
if ( !processor_pminfo[cpu] || !cpu_online(cpu) )
return -EINVAL;
- perf = &processor_pminfo[cpu]->perf;
-
if ( !(processor_pminfo[cpu]->init & XEN_PX_INIT) )
return -EINVAL;
@@ -218,10 +238,15 @@ int cpufreq_add_cpu(unsigned int cpu)
if (per_cpu(cpufreq_cpu_policy, cpu))
return 0;
- if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
+ ret = get_psd_info(cpu, &shared_type, domain_info_ptr);
+ if ( ret )
+ return ret;
+ domain_info = *domain_info_ptr;
+
+ if ( shared_type == CPUFREQ_SHARED_TYPE_HW )
hw_all = 1;
- dom = perf->domain_info.domain;
+ dom = domain_info->domain;
list_for_each(pos, &cpufreq_dom_list_head) {
cpufreq_dom = list_entry(pos, struct cpufreq_dom, node);
@@ -244,21 +269,30 @@ int cpufreq_add_cpu(unsigned int cpu)
cpufreq_dom->dom = dom;
list_add(&cpufreq_dom->node, &cpufreq_dom_list_head);
} else {
+ uint32_t firstcpu_shared_type;
+ const struct xen_psd_package *firstcpu_domain_info;
+ const struct xen_psd_package **firstcpu_domain_info_ptr =
+ &firstcpu_domain_info;
+
/* domain sanity check under whatever coordination type */
firstcpu = cpumask_first(cpufreq_dom->map);
- if ((perf->domain_info.coord_type !=
- processor_pminfo[firstcpu]->perf.domain_info.coord_type) ||
- (perf->domain_info.num_processors !=
- processor_pminfo[firstcpu]->perf.domain_info.num_processors)) {
-
+ ret = get_psd_info(firstcpu, &firstcpu_shared_type,
+ firstcpu_domain_info_ptr);
+ if ( ret )
+ return ret;
+ firstcpu_domain_info = *firstcpu_domain_info_ptr;
+
+ if ( domain_info->coord_type != firstcpu_domain_info->coord_type ||
+ domain_info->num_processors !=
+ firstcpu_domain_info->num_processors )
+ {
printk(KERN_WARNING "cpufreq fail to add CPU%d:"
"incorrect _PSD(%"PRIu64":%"PRIu64"), "
"expect(%"PRIu64"/%"PRIu64")\n",
- cpu, perf->domain_info.coord_type,
- perf->domain_info.num_processors,
- processor_pminfo[firstcpu]->perf.domain_info.coord_type,
- processor_pminfo[firstcpu]->perf.domain_info.num_processors
- );
+ cpu, domain_info->coord_type,
+ domain_info->num_processors,
+ firstcpu_domain_info->coord_type,
+ firstcpu_domain_info->num_processors);
return -EINVAL;
}
}
@@ -304,8 +338,9 @@ int cpufreq_add_cpu(unsigned int cpu)
if (ret)
goto err1;
- if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
- perf->domain_info.num_processors)) {
+ if ( hw_all || cpumask_weight(cpufreq_dom->map) ==
+ domain_info->num_processors )
+ {
memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
/*
@@ -360,29 +395,35 @@ err0:
int cpufreq_del_cpu(unsigned int cpu)
{
+ int ret;
unsigned int dom, domexist = 0;
unsigned int hw_all = 0;
struct list_head *pos;
struct cpufreq_dom *cpufreq_dom = NULL;
struct cpufreq_policy *policy;
- struct processor_performance *perf;
+ uint32_t shared_type;
+ const struct xen_psd_package *domain_info;
+ const struct xen_psd_package **domain_info_ptr = &domain_info;
/* to protect the case when Px was not controlled by xen */
if ( !processor_pminfo[cpu] || !cpu_online(cpu) )
return -EINVAL;
- perf = &processor_pminfo[cpu]->perf;
-
if ( !(processor_pminfo[cpu]->init & XEN_PX_INIT) )
return -EINVAL;
if (!per_cpu(cpufreq_cpu_policy, cpu))
return 0;
- if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
+ ret = get_psd_info(cpu, &shared_type, domain_info_ptr);
+ if ( ret )
+ return ret;
+ domain_info = *domain_info_ptr;
+
+ if ( shared_type == CPUFREQ_SHARED_TYPE_HW )
hw_all = 1;
- dom = perf->domain_info.domain;
+ dom = domain_info->domain;
policy = per_cpu(cpufreq_cpu_policy, cpu);
list_for_each(pos, &cpufreq_dom_list_head) {
@@ -398,8 +439,8 @@ int cpufreq_del_cpu(unsigned int cpu)
/* for HW_ALL, stop gov for each core of the _PSD domain */
/* for SW_ALL & SW_ANY, stop gov for the 1st core of the _PSD domain */
- if (hw_all || (cpumask_weight(cpufreq_dom->map) ==
- perf->domain_info.num_processors))
+ if ( hw_all || cpumask_weight(cpufreq_dom->map) ==
+ domain_info->num_processors )
__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
cpufreq_statistic_exit(cpu);
@@ -464,6 +505,17 @@ static void print_PPC(unsigned int platform_limit)
printk("\t_PPC: %d\n", platform_limit);
}
+static int check_psd_pminfo(uint32_t shared_type)
+{
+ /* check domain coordination */
+ if ( shared_type != CPUFREQ_SHARED_TYPE_ALL &&
+ shared_type != CPUFREQ_SHARED_TYPE_ANY &&
+ shared_type != CPUFREQ_SHARED_TYPE_HW )
+ return -EINVAL;
+
+ return 0;
+}
+
int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
{
int ret = 0, cpu;
@@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
if ( perf->flags & XEN_PX_PSD )
{
- /* check domain coordination */
- if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
- perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
- perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
- {
- ret = -EINVAL;
+ ret = check_psd_pminfo(perf->shared_type);
+ if ( ret )
goto out;
- }
pxpt->shared_type = perf->shared_type;
memcpy(&pxpt->domain_info, &perf->domain_info,
--
2.34.1On 27.05.2025 10:48, Penny Zheng wrote:
> @@ -201,14 +221,14 @@ int cpufreq_add_cpu(unsigned int cpu)
> struct cpufreq_dom *cpufreq_dom = NULL;
> struct cpufreq_policy new_policy;
> struct cpufreq_policy *policy;
> - struct processor_performance *perf;
> + const struct xen_psd_package *domain_info;
> + const struct xen_psd_package **domain_info_ptr = &domain_info;
Why's this latter variable needed? Can't you simply ...
> + uint32_t shared_type;
>
> /* to protect the case when Px was not controlled by xen */
> if ( !processor_pminfo[cpu] || !cpu_online(cpu) )
> return -EINVAL;
>
> - perf = &processor_pminfo[cpu]->perf;
> -
> if ( !(processor_pminfo[cpu]->init & XEN_PX_INIT) )
> return -EINVAL;
>
> @@ -218,10 +238,15 @@ int cpufreq_add_cpu(unsigned int cpu)
> if (per_cpu(cpufreq_cpu_policy, cpu))
> return 0;
>
> - if (perf->shared_type == CPUFREQ_SHARED_TYPE_HW)
> + ret = get_psd_info(cpu, &shared_type, domain_info_ptr);
... pass &domain_info here, ...
> + if ( ret )
> + return ret;
> + domain_info = *domain_info_ptr;
... also eliminating the need for this assignment? (Same again further down.)
> @@ -464,6 +505,17 @@ static void print_PPC(unsigned int platform_limit)
> printk("\t_PPC: %d\n", platform_limit);
> }
>
> +static int check_psd_pminfo(uint32_t shared_type)
> +{
> + /* check domain coordination */
Nit: Comment style (wants to start with a capital letter). Yes, there are many
bad examples in this file (some even visible in patch context here), but in new
code style guidelines should be followed.
> + if ( shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> + shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> + shared_type != CPUFREQ_SHARED_TYPE_HW )
> + return -EINVAL;
> +
> + return 0;
> +}
Looks as if the function would rather want to return a boolean value.
And anyway - I can't really spot the need for this helper, as I also can't
spot ...
> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
>
> if ( perf->flags & XEN_PX_PSD )
> {
> - /* check domain coordination */
> - if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> - perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> - perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
> - {
> - ret = -EINVAL;
> + ret = check_psd_pminfo(perf->shared_type);
> + if ( ret )
> goto out;
> - }
>
> pxpt->shared_type = perf->shared_type;
> memcpy(&pxpt->domain_info, &perf->domain_info,
... the need for this change. And even if there is a need, a follow-on
question would be how this relates to the subject of this patch.
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, June 11, 2025 11:34 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 27.05.2025 10:48, Penny Zheng wrote:
> > @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct
> > xen_processor_performance *perf)
> >
> > if ( perf->flags & XEN_PX_PSD )
> > {
> > - /* check domain coordination */
> > - if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> > - perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> > - perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
> > - {
> > - ret = -EINVAL;
> > + ret = check_psd_pminfo(perf->shared_type);
> > + if ( ret )
> > goto out;
> > - }
> >
> > pxpt->shared_type = perf->shared_type;
> > memcpy(&pxpt->domain_info, &perf->domain_info,
>
> ... the need for this change. And even if there is a need, a follow-on question would
> be how this relates to the subject of this patch.
>
I extracted this snippet out for sharing the same checking logic both in Px and later CPPC. They both need _PSD info
I could change title to "xen/cpufreq: make _PSD info common" and also add description in commit message for
introducing check_psd_pminfo()
> Jan
On 16.06.2025 11:43, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, June 11, 2025 11:34 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct
>> xen_processor_performance"
>>
>> On 27.05.2025 10:48, Penny Zheng wrote:
>>> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct
>>> xen_processor_performance *perf)
>>>
>>> if ( perf->flags & XEN_PX_PSD )
>>> {
>>> - /* check domain coordination */
>>> - if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
>>> - perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
>>> - perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
>>> - {
>>> - ret = -EINVAL;
>>> + ret = check_psd_pminfo(perf->shared_type);
>>> + if ( ret )
>>> goto out;
>>> - }
>>>
>>> pxpt->shared_type = perf->shared_type;
>>> memcpy(&pxpt->domain_info, &perf->domain_info,
>>
>> ... the need for this change. And even if there is a need, a follow-on question would
>> be how this relates to the subject of this patch.
>
> I extracted this snippet out for sharing the same checking logic both in Px and later CPPC. They both need _PSD info
Right, and that (iirc) becomes visible later in the series. But it needs saying
here. As it stands the description talks of only get_psd_info() right now. And
the change above is also unrelated to the "extract" mentioned in the title.
> I could change title to "xen/cpufreq: make _PSD info common" and also add description in commit message for
> introducing check_psd_pminfo()
The title was probably fine; it's the description which was lacking. In fact
I'd deem "make ... common" misleading when there's no 2nd user (yet).
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, June 16, 2025 5:51 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 16.06.2025 11:43, Penny, Zheng wrote:
> > [Public]
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, June 11, 2025 11:34 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from
> >> "struct xen_processor_performance"
> >>
> >> On 27.05.2025 10:48, Penny Zheng wrote:
> >>> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct
> >>> xen_processor_performance *perf)
> >>>
> >>> if ( perf->flags & XEN_PX_PSD )
> >>> {
> >>> - /* check domain coordination */
> >>> - if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> >>> - perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
> >>> - perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
> >>> - {
> >>> - ret = -EINVAL;
> >>> + ret = check_psd_pminfo(perf->shared_type);
> >>> + if ( ret )
> >>> goto out;
> >>> - }
> >>>
> >>> pxpt->shared_type = perf->shared_type;
> >>> memcpy(&pxpt->domain_info, &perf->domain_info,
> >>
> >> ... the need for this change. And even if there is a need, a
> >> follow-on question would be how this relates to the subject of this patch.
> >
> > I extracted this snippet out for sharing the same checking logic both
> > in Px and later CPPC. They both need _PSD info
>
> Right, and that (iirc) becomes visible later in the series. But it needs saying here. As
> it stands the description talks of only get_psd_info() right now. And the change
> above is also unrelated to the "extract" mentioned in the title.
>
> > I could change title to "xen/cpufreq: make _PSD info common" and also
> > add description in commit message for introducing check_psd_pminfo()
>
> The title was probably fine; it's the description which was lacking. In fact I'd deem
> "make ... common" misleading when there's no 2nd user (yet).
>
How about:
"
Title: xen/cpufreq: export _PSD info and checking
_PSD info, consisted of "shared_type" and "struct xen_psd_package", will not
only be provided from px-specific "struct xen_processor_performance", but also
in CPPC data.
In cpufreq_add/del_cpu(), a new helper get_psd_info() is introduced to
export _PSD info. While in set_px_pminfo(), check_psd_pminfo() is also introduced to
export _PSD value checking.
in the meantime, the following style corrections get applied at the same time: ........
```
> Jan
On 17.06.2025 06:12, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Monday, June 16, 2025 5:51 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from "struct
>> xen_processor_performance"
>>
>> On 16.06.2025 11:43, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Wednesday, June 11, 2025 11:34 PM
>>>> To: Penny, Zheng <penny.zheng@amd.com>
>>>> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
>>>> Subject: Re: [PATCH v5 03/18] xen/cpufreq: extract _PSD info from
>>>> "struct xen_processor_performance"
>>>>
>>>> On 27.05.2025 10:48, Penny Zheng wrote:
>>>>> @@ -545,14 +597,9 @@ int set_px_pminfo(uint32_t acpi_id, struct
>>>>> xen_processor_performance *perf)
>>>>>
>>>>> if ( perf->flags & XEN_PX_PSD )
>>>>> {
>>>>> - /* check domain coordination */
>>>>> - if ( perf->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
>>>>> - perf->shared_type != CPUFREQ_SHARED_TYPE_ANY &&
>>>>> - perf->shared_type != CPUFREQ_SHARED_TYPE_HW )
>>>>> - {
>>>>> - ret = -EINVAL;
>>>>> + ret = check_psd_pminfo(perf->shared_type);
>>>>> + if ( ret )
>>>>> goto out;
>>>>> - }
>>>>>
>>>>> pxpt->shared_type = perf->shared_type;
>>>>> memcpy(&pxpt->domain_info, &perf->domain_info,
>>>>
>>>> ... the need for this change. And even if there is a need, a
>>>> follow-on question would be how this relates to the subject of this patch.
>>>
>>> I extracted this snippet out for sharing the same checking logic both
>>> in Px and later CPPC. They both need _PSD info
>>
>> Right, and that (iirc) becomes visible later in the series. But it needs saying here. As
>> it stands the description talks of only get_psd_info() right now. And the change
>> above is also unrelated to the "extract" mentioned in the title.
>>
>>> I could change title to "xen/cpufreq: make _PSD info common" and also
>>> add description in commit message for introducing check_psd_pminfo()
>>
>> The title was probably fine; it's the description which was lacking. In fact I'd deem
>> "make ... common" misleading when there's no 2nd user (yet).
>>
>
> How about:
> "
> Title: xen/cpufreq: export _PSD info and checking
As said, the original title was probably fine. In the new title (and also in
the text suggested below), I wonder what "export" means.
> _PSD info, consisted of "shared_type" and "struct xen_psd_package", will not
> only be provided from px-specific "struct xen_processor_performance", but also
> in CPPC data.
>
> In cpufreq_add/del_cpu(), a new helper get_psd_info() is introduced to
> export _PSD info. While in set_px_pminfo(), check_psd_pminfo() is also introduced to
> export _PSD value checking.
How about "Two new helper functions are introduced to deal with _PSD. They
will later be re-used for handling the same data for CPPC."
Jan
> in the meantime, the following style corrections get applied at the same time: ........
> ```
>
>> Jan
© 2016 - 2025 Red Hat, Inc.