RE: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
[Public]
> -Original Message-
> From: Jan Beulich
> Sent: Monday, May 12, 2025 11:45 PM
> To: Penny, Zheng
> Cc: Huang, Ray ; Andrew Cooper
> ; Anthony PERARD ;
> Orzel, Michal ; Julien Grall ; Roger Pau
> Monné ; Stefano Stabellini ;
> xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 06.05.2025 09:22, Penny, Zheng wrote:
> >> -Original Message-
> >> From: Jan Beulich
> >> Sent: Monday, April 28, 2025 11:32 PM
> >>
> >> On 14.04.2025 09:40, Penny Zheng wrote:
> >>> --- a/xen/drivers/cpufreq/cpufreq.c
> >>> +++ b/xen/drivers/cpufreq/cpufreq.c
> >>> +{
> >>> +case XEN_PX_INIT:
> >>> +if ( shared_type )
> >>> +*shared_type = processor_pminfo[cpu]->perf.shared_type;
> >>> +if ( domain_info )
> >>> +*domain_info = processor_pminfo[cpu]->perf.domain_info;
> >>
> >> Does this need to be a structure copy? Can't you hand back just a
> >> pointer, with the function parameter being const struct xen_psd_package **?
> >>
> >
> > I've considered handing backing a pointer, then maybe we need to
> > allocate space for "struct xen_psd_package **domain_info =
> > xvzalloc(struct xen_psd_package *);", and XVFREE(xxx) it in each exit,
> especially cpufreq_add_cpu() has a lot exits, which could be a few code churn.
> > So I chose the struct copy to have the smallest change.
>
> I fear I don't see why such allocations (of space holding a single pointer)
> would be
> necessary. Afaict all you need is a local variable of the specific
> (pointer) type, the address of which you pass into here.
>
```
struct xen_psd_package *domain_info;
struct xen_psd_package **domain_info_ptr = &domain_info;
```
Oh, right, I could create a local variable to avoid nullptr initialization.
> Jan
RE: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
[Public]
Hi
> -Original Message-
> From: Jan Beulich
> Sent: Monday, May 12, 2025 11:43 PM
> To: Penny, Zheng
> Cc: Huang, Ray ; Andrew Cooper
> ; Anthony PERARD ;
> Orzel, Michal ; Julien Grall ; Roger Pau
> Monné ; Stefano Stabellini ;
> xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 06.05.2025 07:56, Penny, Zheng wrote:
> > [Public]
> >
> > Hi,
> >
> >> -Original Message-
> >> From: Jan Beulich
> >> Sent: Monday, April 28, 2025 11:32 PM
> >> To: Penny, Zheng
> >> Cc: Huang, Ray ; Andrew Cooper
> >> ; Anthony PERARD
> >> ; Orzel, Michal ;
> >> Julien Grall ; Roger Pau Monné
> >> ; Stefano Stabellini ;
> >> xen-devel@lists.xenproject.org
> >> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from
> >> "struct xen_processor_performance"
> >>
> >> On 14.04.2025 09:40, Penny Zheng wrote:
> >>> --- a/xen/include/public/platform.h
> >>> +++ b/xen/include/public/platform.h
> >>> @@ -440,6 +440,11 @@ struct xen_psd_package {
> >>> uint64_t num_processors;
> >>> };
> >>>
> >>> +/* Coordination type value */
> >>> +#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
> >> coordination */
> >>> +#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
> >> should
> >>> +set freq */ #define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be
> >> set
> >>> +from any dependent CPU */
> >>> +
> >>> struct xen_processor_performance {
> >>> uint32_t flags; /* flag for Px sub info type */
> >>> uint32_t platform_limit; /* Platform limitation on freq usage
> >>> */ @@ -449,10 +454,7 @@ struct xen_processor_performance {
> >>> XEN_GUEST_HANDLE(xen_processor_px_t) states;
> >>> struct xen_psd_package domain_info;
> >>> /* Coordination type of this processor */
> >>> -#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
> >> coordination */
> >>> -#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
> >> should
> >>> set freq */ -#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be
> >>> set
> >> from any dependent CPU */
> >>> -uint32_t shared_type;
> >>> +uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
> >>> };
> >>> typedef struct xen_processor_performance
> >>> xen_processor_performance_t;
> >>> DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
> >>
> >> What's this movement about? In the public interface nothing changes?
> >
> > As we will have shared type in "struct xen_processor_cppc" too, maybe
> > the definition would like to live at more common place, then it could be
> > shared?
> > Living inside "struct xen_processor_performance" looks like internal values
> > for
> internal field.
> > If it breaks the public interface some way, I'll change it back and
> > duplicate the definition in "struct xen_processor_cppc" too
>
> I don't think it would break anything, but I also don't see any need for the
> movement. And generally we prefer to avoid unnecessary code churn.
Understood, then I'll delete this change.
>
> Jan
Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
On 06.05.2025 09:22, Penny, Zheng wrote:
>> -Original Message-
>> From: Jan Beulich
>> Sent: Monday, April 28, 2025 11:32 PM
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> +{
>>> +case XEN_PX_INIT:
>>> +if ( shared_type )
>>> +*shared_type = processor_pminfo[cpu]->perf.shared_type;
>>> +if ( domain_info )
>>> +*domain_info = processor_pminfo[cpu]->perf.domain_info;
>>
>> Does this need to be a structure copy? Can't you hand back just a pointer,
>> with the
>> function parameter being const struct xen_psd_package **?
>>
>
> I've considered handing backing a pointer, then maybe we need to allocate
> space for
> "struct xen_psd_package **domain_info = xvzalloc(struct xen_psd_package *);",
> and XVFREE(xxx)
> it in each exit, especially cpufreq_add_cpu() has a lot exits, which could be
> a few code churn.
> So I chose the struct copy to have the smallest change.
I fear I don't see why such allocations (of space holding a single pointer)
would be necessary. Afaict all you need is a local variable of the specific
(pointer) type, the address of which you pass into here.
Jan
Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
On 06.05.2025 07:56, Penny, Zheng wrote:
> [Public]
>
> Hi,
>
>> -Original Message-
>> From: Jan Beulich
>> Sent: Monday, April 28, 2025 11:32 PM
>> To: Penny, Zheng
>> Cc: Huang, Ray ; Andrew Cooper
>> ; Anthony PERARD ;
>> Orzel, Michal ; Julien Grall ; Roger
>> Pau Monné ; Stefano Stabellini
>> ;
>> xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct
>> xen_processor_performance"
>>
>> On 14.04.2025 09:40, Penny Zheng wrote:
>>> --- a/xen/include/public/platform.h
>>> +++ b/xen/include/public/platform.h
>>> @@ -440,6 +440,11 @@ struct xen_psd_package {
>>> uint64_t num_processors;
>>> };
>>>
>>> +/* Coordination type value */
>>> +#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
>> coordination */
>>> +#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
>> should
>>> +set freq */ #define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be
>> set
>>> +from any dependent CPU */
>>> +
>>> struct xen_processor_performance {
>>> uint32_t flags; /* flag for Px sub info type */
>>> uint32_t platform_limit; /* Platform limitation on freq usage */
>>> @@ -449,10 +454,7 @@ struct xen_processor_performance {
>>> XEN_GUEST_HANDLE(xen_processor_px_t) states;
>>> struct xen_psd_package domain_info;
>>> /* Coordination type of this processor */
>>> -#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
>> coordination */
>>> -#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
>> should
>>> set freq */ -#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be set
>> from any dependent CPU */
>>> -uint32_t shared_type;
>>> +uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
>>> };
>>> typedef struct xen_processor_performance xen_processor_performance_t;
>>> DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
>>
>> What's this movement about? In the public interface nothing changes?
>
> As we will have shared type in "struct xen_processor_cppc" too, maybe the
> definition would like to live
> at more common place, then it could be shared?
> Living inside "struct xen_processor_performance" looks like internal values
> for internal field.
> If it breaks the public interface some way, I'll change it back and duplicate
> the definition in "struct xen_processor_cppc" too
I don't think it would break anything, but I also don't see any need for the
movement. And generally we prefer to avoid unnecessary code churn.
Jan
RE: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
[Public]
Hi,
> -Original Message-
> From: Jan Beulich
> Sent: Monday, April 28, 2025 11:32 PM
> To: Penny, Zheng
> Cc: Huang, Ray ; Andrew Cooper
> ; Anthony PERARD ;
> Orzel, Michal ; Julien Grall ; Roger
> Pau Monné ; Stefano Stabellini ;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > --- a/xen/drivers/cpufreq/cpufreq.c
> > +++ b/xen/drivers/cpufreq/cpufreq.c
> > +{
> > +case XEN_PX_INIT:
> > +if ( shared_type )
> > +*shared_type = processor_pminfo[cpu]->perf.shared_type;
> > +if ( domain_info )
> > +*domain_info = processor_pminfo[cpu]->perf.domain_info;
>
> Does this need to be a structure copy? Can't you hand back just a pointer,
> with the
> function parameter being const struct xen_psd_package **?
>
I've considered handing backing a pointer, then maybe we need to allocate space
for
"struct xen_psd_package **domain_info = xvzalloc(struct xen_psd_package *);",
and XVFREE(xxx)
it in each exit, especially cpufreq_add_cpu() has a lot exits, which could be a
few code churn.
So I chose the struct copy to have the smallest change.
> > +break;
> > +default:
> Jan
RE: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
[Public]
Hi,
> -Original Message-
> From: Jan Beulich
> Sent: Monday, April 28, 2025 11:32 PM
> To: Penny, Zheng
> Cc: Huang, Ray ; Andrew Cooper
> ; Anthony PERARD ;
> Orzel, Michal ; Julien Grall ; Roger
> Pau Monné ; Stefano Stabellini ;
> xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct
> xen_processor_performance"
>
> On 14.04.2025 09:40, Penny Zheng wrote:
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -440,6 +440,11 @@ struct xen_psd_package {
> > uint64_t num_processors;
> > };
> >
> > +/* Coordination type value */
> > +#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
> coordination */
> > +#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
> should
> > +set freq */ #define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be
> set
> > +from any dependent CPU */
> > +
> > struct xen_processor_performance {
> > uint32_t flags; /* flag for Px sub info type */
> > uint32_t platform_limit; /* Platform limitation on freq usage */
> > @@ -449,10 +454,7 @@ struct xen_processor_performance {
> > XEN_GUEST_HANDLE(xen_processor_px_t) states;
> > struct xen_psd_package domain_info;
> > /* Coordination type of this processor */
> > -#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed
> coordination */
> > -#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs
> should
> > set freq */ -#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be set
> from any dependent CPU */
> > -uint32_t shared_type;
> > +uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
> > };
> > typedef struct xen_processor_performance xen_processor_performance_t;
> > DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
>
> What's this movement about? In the public interface nothing changes?
>
As we will have shared type in "struct xen_processor_cppc" too, maybe the
definition would like to live
at more common place, then it could be shared?
Living inside "struct xen_processor_performance" looks like internal values for
internal field.
If it breaks the public interface some way, I'll change it back and duplicate
the definition in "struct xen_processor_cppc" too
> Jan
Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"
On 14.04.2025 09:40, Penny Zheng wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -191,9 +191,31 @@ 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(uint32_t init, unsigned int cpu,
The 1st parameter seems unnecessary: You access processor_pminfo[cpu]
below anyway, so you could as well use ...
> +uint32_t *shared_type,
> +struct xen_psd_package *domain_info)
> {
> int ret = 0;
> +
> +switch ( init )
... processor_pminfo[cpu]->init here.
> +{
> +case XEN_PX_INIT:
> +if ( shared_type )
> +*shared_type = processor_pminfo[cpu]->perf.shared_type;
> +if ( domain_info )
> +*domain_info = processor_pminfo[cpu]->perf.domain_info;
Does this need to be a structure copy? Can't you hand back just a pointer,
with the function parameter being const struct xen_psd_package **?
> +break;
> +default:
Nit: Blank line please between non-fall-through case blocks.
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -440,6 +440,11 @@ struct xen_psd_package {
> uint64_t num_processors;
> };
>
> +/* Coordination type value */
> +#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
> +#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq
> */
> +#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent
> CPU */
> +
> struct xen_processor_performance {
> uint32_t flags; /* flag for Px sub info type */
> uint32_t platform_limit; /* Platform limitation on freq usage */
> @@ -449,10 +454,7 @@ struct xen_processor_performance {
> XEN_GUEST_HANDLE(xen_processor_px_t) states;
> struct xen_psd_package domain_info;
> /* Coordination type of this processor */
> -#define XEN_CPUPERF_SHARED_TYPE_HW 1 /* HW does needed coordination */
> -#define XEN_CPUPERF_SHARED_TYPE_ALL 2 /* All dependent CPUs should set freq
> */
> -#define XEN_CPUPERF_SHARED_TYPE_ANY 3 /* Freq can be set from any dependent
> CPU */
> -uint32_t shared_type;
> +uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
> };
> typedef struct xen_processor_performance xen_processor_performance_t;
> DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);
What's this movement about? In the public interface nothing changes?
Jan
