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 23:55,  Jan Beulich wrote:
>>> On 09.09.15 at 17:16, <wei.w.wang@intel.com> wrote:
> On 09/09/2015 21:12,  Jan Beulich wrote:
>>>> On 09.09.15 at 14:56, <wei.w.wang@intel.com> wrote:
>> Can you please explain more why it doesn't scale? 
>> From my point of view, any other future value representation can be 
>> passed from the producer to the related consumer through this method.
> 
>> Did you read all of my earlier replies? I already said there "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."
> 
> Yes, I have read that. I am not sure if I got your point, but my 
> meaning was when we add new drivers to the code, e.g. xx_pstate 
> driver, we can still have the name, "xx_pstate", assigned to 
> "p_cpufreq->scaling_driver" to distinguish one another. If the driver 
> uses a different variant of output representation, which cannot be 
> held by " uint32_t scaling_max_perf" (it needs "uint64_t" for example, then that driver developer needs to add a new field here like  "
> uint64_t scaling_max_perf_xx").
> What is the scaling problem? 

>	if (strcmp() == 0 ||
>	    strcmp() == 0 ||
>	    strcmp() == 0) {
>	...
>	} else if (strcmp() == 0) {
>	...
>	} else {
>	...
>	}

> is just ugly, and gets all the uglier the more strcmp()s get added.
> Have a boolean or enumeration indicating what kind of data there is, and the above changes to

>	switch (kind) {
>	case absolute: ...
>	case percentage: ...
>	}

Ok. I will replace the default "scaling_driver[CPUFREQ_NAME_LEN]" with an enum type, like this following
...
- char scaling_driver[CPUFREQ_NAME_LEN];
+ enum scaling_driver_flag scaling_driver;
...

We cannot keep both of the above two there, because there is a 128Byte size limit. Then somewhere, we need to translate the character-represented scaling_driver to our new enum-represented scaling_driver. For example, in pmstat.c, the following:

if ( cpufreq_driver->name[0] )
        strlcpy(op->u.get_para.scaling_driver,
            cpufreq_driver->name, CPUFREQ_NAME_LEN);
else
        strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);

needs to be changed to:
if ( strncmp(cpufreq_driver->name[0], "intel_pstate", CPUFREQ_NAME_LEN) == 0 )
    op->u.get_para.scaling_driver = INTEL_PSTATE;
else if ( strncmp(cpufreq_driver->name[0], "acpi_cpufreq", CPUFREQ_NAME_LEN) == 0 )
    op->u.get_para.scaling_driver = ACPI_CPUFREQ;
...

Seems we still cannot get rid of these strncmp()s. Is this acceptable, or should we change "struct cpufreq_driver" to use enum represented driver name as well, or do you have a better suggestion?

Best,
Wei