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

["Wang, Wei W" <wei.w.wang@xxxxxxxxx>]

On 09/09/2015 17:02,  Jan Beulich wrote:
>>> 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.

Only one driver is in use at one time. I am considering not using the union 
variable any more. We can simply use "uint32_t scaling_max_perf". No matter 
what kind of a third variant of output representation it may be, as long as it 
is in "uint32_t", it can use "uint32_t scaling_max_perf" to hold that value 
representation. The top producer of this " uint32_t scaling_max_perf " is a 
user command input via xenpm, then the related consumer has its own logic to 
deal with what's being put into " uint32_t scaling_max_perf ".

Using the drinking vessel analogy, we are not putting milk and water into the 
vessel at the same time. If the producer puts water into the vessel, then the 
consumer simply consumes water; If the producer puts milk into the vessel, then 
the consumer simply consumes milk. I think we don't need to worry about what 
type of drinking is put inside the vessel, because the vessel is just an 
intermediate place holding the liquid between the producer and consumer - the 
consumer has the privity of contract with the producer and it has the right 
logic to deal with what's inside the vessel.

Hope this is sensible.

Best,
Wei

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