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

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

On 09/09/2015 16:32,  Jan Beulich wrote:
>>> On 09.09.15 at 10:11, <wei.w.wang@intel.com> wrote:
> On 24/07/2015 22:16,  Jan Beulich wrote:
>>>> On 25.06.15 at 13:17, <wei.w.wang@intel.com> 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:

+    if (!strncmp(p_cpufreq->scaling_driver,
+                  "intel_pstate", CPUFREQ_NAME_LEN) )
+    {
+        printf("max_perf_pct         : %d\n", p_cpufreq->scaling_max.pct);
+        printf("min_perf_pct         : %d\n", p_cpufreq->scaling_min.pct);
+        printf("turbo_pct            : %d\n", p_cpufreq->scaling_turbo_pct);
+    }
+    else
+    {
+        printf("scaling_avail_freq   :");
+        for ( i = 0; i < p_cpufreq->freq_num; i++ )
+            if ( p_cpufreq->scaling_available_frequencies[i] ==
+                 p_cpufreq->scaling_cur_freq )
+                printf(" *%d", p_cpufreq->scaling_available_frequencies[i]);
+            else
+                printf(" %d", p_cpufreq->scaling_available_frequencies[i]);
+        printf("\n");
+        printf("scaling frequency    : max [%u] min [%u] cur [%u]\n",
+               p_cpufreq->scaling_max.freq,
+               p_cpufreq->scaling_min.freq,
+               p_cpufreq->scaling_cur_freq);

"p_cpufreq->scaling_driver" is the flag which distinguishes the usage of this "scaling_max_perf" field.


Best,
Wei