Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.09.15 at 10:49, <wei.w.wang@xxxxxxxxx> wrote:
> On 09/09/2015 16:32,  Jan Beulich wrote:
>>>> On 09.09.15 at 10:11, <wei.w.wang@xxxxxxxxx> wrote:
>> On 24/07/2015 22:16,  Jan Beulich wrote:
>>>>> On 25.06.15 at 13:17, <wei.w.wang@xxxxxxxxx> wrote:
>>> --- a/xen/drivers/acpi/pmstat.c
>>> +++ b/xen/drivers/acpi/pmstat.c
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -315,8 +315,18 @@ struct xen_get_cpufreq_para {
>>>      uint32_t scaling_cur_freq;
>>>  
>>>      char scaling_governor[CPUFREQ_NAME_LEN];
>>> -    uint32_t scaling_max_freq;
>>> -    uint32_t scaling_min_freq;
>>> +
>>> +    union {
>>> +        uint32_t freq;
>>> +        uint32_t pct;
>>> +    } scaling_max;
>>> +
>>> +    union {
>>> +        uint32_t freq;
>>> +        uint32_t pct;
>>> +    } scaling_min;
>> 
>>>scaling_min and scaling_max should really be of the same type, so that
>> someone wanting to introduce helper functions
>>>or pointers to them can hand both interchangeably.
>> 
>>>Also I'm starting to get tired of repeating that it is still unclear 
>>>how a
>> consumer of the structure will know which of the
>>>two fields of the unions are applicable.
>> 
>>> Probably we don't need a union here. I plan to simply change them to 
>>> uint32_t scaling_max_perf; uint32_t scaling_max_perf;
>> 
>>> Then it's up to the driver to put what kind of value to it. It's like 
>>> we simply provide a drinking vessel, and it depends on the user to put 
>>> water or milk into it. In our case, the intel_pstate driver assigns a 
>>> percentage vale to it (in the "uint32_t" type), and the legacy driver 
>>> assigns the absolute value to it (in the "uint32_t" type, too).
> 
>>I don't see how this will solve the problem of the consumer not knowing what 
> kind of value it has to deal with.
> 
> The consumer is inside the print_cpufreq_para() function. I have put the 
> code below:

No, this is not "the" consumer but just one out of potentially many.

> +    if (!strncmp(p_cpufreq->scaling_driver,
> +                  "intel_pstate", CPUFREQ_NAME_LEN) )

And this is not really a proper way to distinguish which of an output
structure's sub-union is to be used. Just consider what happens to
the code when we end up gaining a few more drivers providing
percentage values, and perhaps another one providing a third
variant of output representation.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel