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

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

[Wei Wang]

Add support in the pmstat.c so that the xenpm tool can request to
access the intel_pstate driver.

v4 changes:
1) changed to use the "internal_governor struct";
2) coding style change (indentation of "gov_num++").

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 tools/libxc/xc_pm.c         |   4 +-
 xen/drivers/acpi/pmstat.c   | 148 ++++++++++++++++++++++++++++++++++++--------
 xen/include/public/sysctl.h |  16 ++++-
 3 files changed, 138 insertions(+), 30 deletions(-)

diff --git a/tools/libxc/xc_pm.c b/tools/libxc/xc_pm.c
index 5a7148e..823bab6 100644
--- a/tools/libxc/xc_pm.c
+++ b/tools/libxc/xc_pm.c
@@ -265,8 +265,8 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
         user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq;
         user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq;
         user_para->scaling_cur_freq = sys_para->scaling_cur_freq;
-        user_para->scaling_max_freq = sys_para->scaling_max_freq;
-        user_para->scaling_min_freq = sys_para->scaling_min_freq;
+        user_para->scaling_max_freq = sys_para->scaling_max.freq;
+        user_para->scaling_min_freq = sys_para->scaling_min.freq;
         user_para->turbo_enabled    = sys_para->turbo_enabled;
 
         memcpy(user_para->scaling_driver,
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index daac2da..89628aa 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -192,22 +192,33 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     uint32_t ret = 0;
     const struct processor_pminfo *pmpt;
     struct cpufreq_policy *policy;
+    struct perf_limits *limits;
+    struct internal_governor *internal_gov;
     uint32_t gov_num = 0;
     uint32_t *affected_cpus;
     uint32_t *scaling_available_frequencies;
     char     *scaling_available_governors;
     struct list_head *pos;
     uint32_t cpu, i, j = 0;
+    uint32_t cur_gov;
 
     pmpt = processor_pminfo[op->cpuid];
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
+    limits = &policy->limits;
+    internal_gov = policy->internal_gov;
+    cur_gov = internal_gov ? internal_gov->cur_gov : 0;
 
     if ( !pmpt || !pmpt->perf.states ||
-         !policy || !policy->governor )
+         !policy || (!policy->governor && !policy->internal_gov) )
         return -EINVAL;
 
-    list_for_each(pos, &cpufreq_governor_list)
-        gov_num++;
+    if (internal_gov)
+        gov_num = internal_gov->gov_num;
+    else
+    {
+        list_for_each(pos, &cpufreq_governor_list)
+            gov_num++;
+    }
 
     if ( (op->u.get_para.cpu_num  != cpumask_weight(policy->cpus)) ||
          (op->u.get_para.freq_num != pmpt->perf.state_count)    ||
@@ -241,28 +252,47 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     if ( ret )
         return ret;
 
-    if ( !(scaling_available_governors =
-           xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
-        return -ENOMEM;
-    if ( (ret = read_scaling_available_governors(scaling_available_governors,
-                gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
+    if (internal_gov)
     {
+        scaling_available_governors = internal_gov->avail_gov;
+        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
+                    scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
+        if ( ret )
+            return ret;
+    }
+    else
+    {
+        if ( !(scaling_available_governors =
+               xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
+            return -ENOMEM;
+        if ( (ret = read_scaling_available_governors(scaling_available_governors,
+                gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
+        {
+            xfree(scaling_available_governors);
+            return ret;
+        }
+        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
+                    scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
         xfree(scaling_available_governors);
-        return ret;
+        if ( ret )
+            return ret;
     }
-    ret = copy_to_guest(op->u.get_para.scaling_available_governors,
-                scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);
-    xfree(scaling_available_governors);
-    if ( ret )
-        return ret;
-
     op->u.get_para.cpuinfo_cur_freq =
         cpufreq_driver->get ? cpufreq_driver->get(op->cpuid) : policy->cur;
     op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq;
     op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq;
     op->u.get_para.scaling_cur_freq = policy->cur;
-    op->u.get_para.scaling_max_freq = policy->max;
-    op->u.get_para.scaling_min_freq = policy->min;
+    if (internal_gov)
+    {
+        op->u.get_para.scaling_max.pct = limits->max_perf_pct;
+        op->u.get_para.scaling_min.pct = limits->min_perf_pct;
+        op->u.get_para.scaling_turbo_pct = limits->turbo_pct;
+    }
+    else
+    {
+        op->u.get_para.scaling_max.freq = policy->max;
+        op->u.get_para.scaling_min.freq = policy->min;
+    }
 
     if ( cpufreq_driver->name[0] )
         strlcpy(op->u.get_para.scaling_driver, 
@@ -270,11 +300,31 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     else
         strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
 
-    if ( policy->governor->name[0] )
-        strlcpy(op->u.get_para.scaling_governor, 
-            policy->governor->name, CPUFREQ_NAME_LEN);
-    else
-        strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
+    switch (cur_gov)
+    {
+    case INTERNAL_GOV_PERFORMANCE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "performance", CPUFREQ_NAME_LEN);
+        break;
+    case INTERNAL_GOV_POWERSAVE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "powersave", CPUFREQ_NAME_LEN);
+        break;
+    case INTERNAL_GOV_USERSPACE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "userspace", CPUFREQ_NAME_LEN);
+        break;
+    case INTERNAL_GOV_ONDEMAND:
+        strlcpy(op->u.get_para.scaling_governor,
+            "ondemand", CPUFREQ_NAME_LEN);
+        break;
+    default:
+        if ( policy->governor->name[0] )
+            strlcpy(op->u.get_para.scaling_governor,
+                policy->governor->name, CPUFREQ_NAME_LEN);
+        else
+            strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
+    }
 
     /* governor specific para */
     if ( !strnicmp(op->u.get_para.scaling_governor, 
@@ -300,16 +350,36 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
 static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
 {
     struct cpufreq_policy new_policy, *old_policy;
+    struct internal_governor *internal_gov;
 
     old_policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
     if ( !old_policy )
         return -EINVAL;
+    internal_gov = old_policy->internal_gov;
 
     memcpy(&new_policy, old_policy, sizeof(struct cpufreq_policy));
 
-    new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
-    if (new_policy.governor == NULL)
-        return -EINVAL;
+    if (internal_gov && internal_gov->cur_gov)
+    {
+        if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "performance", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_PERFORMANCE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "powersave", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_POWERSAVE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "userspace", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_USERSPACE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "ondemand", CPUFREQ_NAME_LEN) )
+            internal_gov->cur_gov = INTERNAL_GOV_ONDEMAND;
+    }
+    else
+    {
+        new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
+        if (new_policy.governor == NULL)
+            return -EINVAL;
+    }
 
     return __cpufreq_set_policy(old_policy, &new_policy);
 }
@@ -318,10 +388,12 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
 {
     int ret = 0;
     struct cpufreq_policy *policy;
+    struct internal_governor *internal_gov;
 
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
+    internal_gov = policy->internal_gov;
 
-    if ( !policy || !policy->governor )
+    if ( !policy || (!policy->governor && !internal_gov) )
         return -EINVAL;
 
     switch(op->u.set_para.ctrl_type)
@@ -348,6 +420,30 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
         break;
     }
 
+    case SCALING_MAX_PCT:
+    {
+        struct cpufreq_policy new_policy;
+        struct perf_limits *limits = &new_policy.limits;
+
+        new_policy = *policy;
+        limits->max_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
+			limits->min_policy_pct, limits->max_policy_pct);
+        ret = __cpufreq_set_policy(policy, &new_policy);
+        break;
+    }
+
+    case SCALING_MIN_PCT:
+    {
+        struct cpufreq_policy new_policy;
+        struct perf_limits *limits = &new_policy.limits;
+
+        new_policy = *policy;
+        limits->min_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
+			limits->min_policy_pct, limits->max_policy_pct);
+        ret = __cpufreq_set_policy(policy, &new_policy);
+        break;
+    }
+
     case SCALING_SETSPEED:
     {
         unsigned int freq =op->u.set_para.ctrl_value;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 0cf9277..1bb730c 100644
--- 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;
+
+    uint32_t scaling_turbo_pct;
 
     /* for specific governor */
     union {
@@ -337,6 +347,8 @@ struct xen_set_cpufreq_para {
     #define SCALING_SETSPEED           3
     #define SAMPLING_RATE              4
     #define UP_THRESHOLD               5
+    #define SCALING_MAX_PCT            6
+    #define SCALING_MIN_PCT            7
 
     uint32_t ctrl_type;
     uint32_t ctrl_value;
-- 
1.9.1

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

[Jan Beulich]

>>> 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
> @@ -192,22 +192,33 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      uint32_t ret = 0;
>      const struct processor_pminfo *pmpt;
>      struct cpufreq_policy *policy;
> +    struct perf_limits *limits;
> +    struct internal_governor *internal_gov;
>      uint32_t gov_num = 0;
>      uint32_t *affected_cpus;
>      uint32_t *scaling_available_frequencies;
>      char     *scaling_available_governors;
>      struct list_head *pos;
>      uint32_t cpu, i, j = 0;
> +    uint32_t cur_gov;

Please group this on the same line as e.g. gov_num, instead of adding
yet another line with a single uint32_t variable declaration.

> @@ -241,28 +252,47 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      if ( ret )
>          return ret;
>  
> -    if ( !(scaling_available_governors =
> -           xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
> -        return -ENOMEM;
> -    if ( (ret = read_scaling_available_governors(scaling_available_governors,
> -                gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
> +    if (internal_gov)

Coding style.

>      {
> +        scaling_available_governors = internal_gov->avail_gov;
> +        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> +                    scaling_available_governors, gov_num * CPUFREQ_NAME_LEN);

Using scaling_available_governors as intermediate variable here
makes the code pretty clearly worse than just directly using
internal_gov->avail_gov. Or is there a problem doing so?

> @@ -270,11 +300,31 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>      else
>          strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
>  
> -    if ( policy->governor->name[0] )
> -        strlcpy(op->u.get_para.scaling_governor, 
> -            policy->governor->name, CPUFREQ_NAME_LEN);
> -    else
> -        strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN);
> +    switch (cur_gov)
> +    {
> +    case INTERNAL_GOV_PERFORMANCE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "performance", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_POWERSAVE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "powersave", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_USERSPACE:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "userspace", CPUFREQ_NAME_LEN);
> +        break;
> +    case INTERNAL_GOV_ONDEMAND:
> +        strlcpy(op->u.get_para.scaling_governor,
> +            "ondemand", CPUFREQ_NAME_LEN);
> +        break;
> +    default:
> +        if ( policy->governor->name[0] )
> +            strlcpy(op->u.get_para.scaling_governor,
> +                policy->governor->name, CPUFREQ_NAME_LEN);

Indentation (for all second lines of strlcpy() invocations above).

> @@ -300,16 +350,36 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
>  static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
>  {
>      struct cpufreq_policy new_policy, *old_policy;
> +    struct internal_governor *internal_gov;
>  
>      old_policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
>      if ( !old_policy )
>          return -EINVAL;
> +    internal_gov = old_policy->internal_gov;
>  
>      memcpy(&new_policy, old_policy, sizeof(struct cpufreq_policy));
>  
> -    new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
> -    if (new_policy.governor == NULL)
> -        return -EINVAL;
> +    if (internal_gov && internal_gov->cur_gov)
> +    {
> +        if ( !strnicmp(op->u.set_gov.scaling_governor,
> +                       "performance", CPUFREQ_NAME_LEN) )
> +            internal_gov->cur_gov = INTERNAL_GOV_PERFORMANCE;
> +        else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +                       "powersave", CPUFREQ_NAME_LEN) )
> +            internal_gov->cur_gov = INTERNAL_GOV_POWERSAVE;
> +        else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +                       "userspace", CPUFREQ_NAME_LEN) )
> +            internal_gov->cur_gov = INTERNAL_GOV_USERSPACE;
> +        else if ( !strnicmp(op->u.set_gov.scaling_governor,
> +                       "ondemand", CPUFREQ_NAME_LEN) )
> +            internal_gov->cur_gov = INTERNAL_GOV_ONDEMAND;
> +    }
> +    else
> +    {
> +        new_policy.governor = __find_governor(op->u.set_gov.scaling_governor);
> +        if (new_policy.governor == NULL)

Please not only fix the coding style of your own if(), but also that
of pre-existing code that you touch (like the one here).

> @@ -318,10 +388,12 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>  {
>      int ret = 0;
>      struct cpufreq_policy *policy;
> +    struct internal_governor *internal_gov;
>  
>      policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
> +    internal_gov = policy->internal_gov;
>  
> -    if ( !policy || !policy->governor )
> +    if ( !policy || (!policy->governor && !internal_gov) )
>          return -EINVAL;
>  
>      switch(op->u.set_para.ctrl_type)
> @@ -348,6 +420,30 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
>          break;
>      }
>  
> +    case SCALING_MAX_PCT:
> +    {
> +        struct cpufreq_policy new_policy;
> +        struct perf_limits *limits = &new_policy.limits;
> +
> +        new_policy = *policy;
> +        limits->max_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
> +			limits->min_policy_pct, limits->max_policy_pct);
> +        ret = __cpufreq_set_policy(policy, &new_policy);
> +        break;
> +    }
> +
> +    case SCALING_MIN_PCT:
> +    {
> +        struct cpufreq_policy new_policy;
> +        struct perf_limits *limits = &new_policy.limits;
> +
> +        new_policy = *policy;
> +        limits->min_perf_pct = clamp_t(uint32_t, op->u.set_para.ctrl_value,
> +			limits->min_policy_pct, limits->max_policy_pct);
> +        ret = __cpufreq_set_policy(policy, &new_policy);
> +        break;
> +    }

Shouldn't both of them bail when !internal_gov?

Also - do no other of the sub-ops need adjustment for the case
when an internal governor is in use?

> --- 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.

Jan

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

[Wang, Wei W]

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.

Hi Jan,

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 will finish this round of revision soon.

Best,
Wei

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

[Jan Beulich]

>>> 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.

Jan

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

[Wang, Wei W]

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

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

[Jan Beulich]

>>> On 09.09.15 at 10:49, <wei.w.wang@intel.com> wrote:
> 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:

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

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

[Wang, Wei W]

On 09/09/2015 17:02,  Jan Beulich wrote:
>>> On 09.09.15 at 10:49, <wei.w.wang@intel.com> wrote:
> 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:

>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

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

[Jan Beulich]

>>> On 09.09.15 at 11:35, <wei.w.wang@intel.com> wrote:
> 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.

This analogy breaks for a blind: How would he know whether there's
water or milk in there? He'd have to try it. Now what if we use venom
instead of milk in your analogy?

Bottom line - the consumer needs to know what kind of data it is to
expect to consume.

Jan

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

[Wang, Wei W]

On 09/09/2015 18:10,  Jan Beulich wrote:
>>> On 09.09.15 at 11:35, <wei.w.wang@intel.com> wrote:
>> 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.

> This analogy breaks for a blind: How would he know whether there's water or milk in there? He'd have to try it. 
>Now what if we use >venom instead of milk in your analogy?

>Bottom line - the consumer needs to know what kind of data it is to expect to consume.

There is a contract between the consumer and the producer. In our case, the contract is "p_cpufreq->scaling_driver". Before the right consumer consumes the value of " uint32_t scaling_max_perf ", it goes through the check:
 +    if (!strncmp(p_cpufreq->scaling_driver,
 +                  "intel_pstate", CPUFREQ_NAME_LEN) )
, where "intel_pstate" can be changed to other new driver names (contract) in the future.

Best,
Wei

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

[Jan Beulich]

>>> On 09.09.15 at 12:35, <wei.w.wang@intel.com> wrote:
> On 09/09/2015 18:10,  Jan Beulich wrote:
>>>> On 09.09.15 at 11:35, <wei.w.wang@intel.com> wrote:
>>> 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.
> 
>> This analogy breaks for a blind: How would he know whether there's water or 
> milk in there? He'd have to try it. 
>>Now what if we use >venom instead of milk in your analogy?
> 
>>Bottom line - the consumer needs to know what kind of data it is to expect to 
> consume.
> 
> There is a contract between the consumer and the producer. In our case, the 
> contract is "p_cpufreq->scaling_driver". Before the right consumer consumes the 
> value of " uint32_t scaling_max_perf ", it goes through the check:
>  +    if (!strncmp(p_cpufreq->scaling_driver,
>  +                  "intel_pstate", CPUFREQ_NAME_LEN) )
> , where "intel_pstate" can be changed to other new driver names (contract) 
> in the future.

Which is precisely what I already said doesn't scale.

Jan

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

[Wang, Wei W]

On 09/09/2015 19:57,  Jan Beulich wrote:
>>> On 09.09.15 at 12:35, <wei.w.wang@intel.com> wrote:
> On 09/09/2015 18:10,  Jan Beulich wrote:
>>>> On 09.09.15 at 11:35, <wei.w.wang@intel.com> wrote:
>>> 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.
> 
>> This analogy breaks for a blind: How would he know whether there's 
>> water or
> milk in there? He'd have to try it. 
>>Now what if we use >venom instead of milk in your analogy?
> 
>>Bottom line - the consumer needs to know what kind of data it is to 
>>expect to
> consume.
> 
> There is a contract between the consumer and the producer. In our 
> case, the contract is "p_cpufreq->scaling_driver". Before the right 
> consumer consumes the value of " uint32_t scaling_max_perf ", it goes through the check:
>  +    if (!strncmp(p_cpufreq->scaling_driver,
>  +                  "intel_pstate", CPUFREQ_NAME_LEN) )
> , where "intel_pstate" can be changed to other new driver names 
> (contract) in the future.

> Which is precisely what I already said doesn't scale.

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. 

Best,
Wei

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

[Jan Beulich]

>>> 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."

Jan

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

[Wang, Wei W]

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? 

Best,
Wei

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

[Jan Beulich]

>>> 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: ...
	}

Jan

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

[Wang, Wei W]

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

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

[Jan Beulich]

>>> On 10.09.15 at 07:35, <wei.w.wang@intel.com> wrote:
> 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?

The one I explained before: Express the data representation type
in an enum, not the driver kind. But even if we went with the above,
the strcmp() ugliness would at least be limited to the producer, not
enforced onto any number of consumers.

Jan

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

[Wang, Wei W]

On 09/09/2015 16:17,  Jan Beulich wrote:
>>> On 10.09.15 at 07:35, <wei.w.wang@intel.com> wrote:
> 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?

> The one I explained before: Express the data representation type in an enum, not the driver kind. But even if we went with the 
> above, the strcmp() ugliness would at least be limited to the producer, not enforced onto any number of consumers.

No.  I think in our previous discussion, there is no problem with "the data representation type", any future data representation, as long as it is in "uint32_t", it can use "uint32_t scaling_max_perf" to hold that value representation. Your concern was that the following doesn't scale well.

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

So we are trying to change the driver name thing to be in enum. 

Best,
Wei

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

[Jan Beulich]

>>> On 10.09.15 at 11:33, <wei.w.wang@intel.com> wrote:
> On 09/09/2015 16:17,  Jan Beulich wrote:
>>>> On 10.09.15 at 07:35, <wei.w.wang@intel.com> wrote:
>> 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?
> 
>> The one I explained before: Express the data representation type in an enum, 
> not the driver kind. But even if we went with the 
>> above, the strcmp() ugliness would at least be limited to the producer, not 
> enforced onto any number of consumers.
> 
> No.  I think in our previous discussion, there is no problem with "the data 
> representation type", any future data representation, as long as it is in 
> "uint32_t", it can use "uint32_t scaling_max_perf" to hold that value 
> representation.

Okay, "representation" was a badly chose term. "Meaning of the data"
would have been better.

> Your concern was that the following doesn't scale well.
> 
> +    if (!strncmp(p_cpufreq->scaling_driver,
> +                  "intel_pstate", CPUFREQ_NAME_LEN) )
> 
> So we are trying to change the driver name thing to be in enum. 

No, that was never what I've been asking for. I've always said
that the consumer of the data ought to have a way to know what
kind of data it is dealing with. I.e. the enum ought to represent
what meaning the data has (frequency vs percentage).

Jan

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

[Wang, Wei W]

On 10/09/2015 17:55,  Jan Beulich wrote:
>>> On 10.09.15 at 11:33, <wei.w.wang@intel.com> wrote:
> On 09/09/2015 16:17,  Jan Beulich wrote:
>>>> On 10.09.15 at 07:35, <wei.w.wang@intel.com> wrote:
>> 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?
> 
>> The one I explained before: Express the data representation type in 
>> an enum,
> not the driver kind. But even if we went with the
>> above, the strcmp() ugliness would at least be limited to the 
>> producer, not
> enforced onto any number of consumers.
> 
> No.  I think in our previous discussion, there is no problem with "the 
> data representation type", any future data representation, as long as 
> it is in "uint32_t", it can use "uint32_t scaling_max_perf" to hold 
> that value representation.

> Okay, "representation" was a badly chose term. "Meaning of the data"
> would have been better.

> Your concern was that the following doesn't scale well.
> 
> +    if (!strncmp(p_cpufreq->scaling_driver,
> +                  "intel_pstate", CPUFREQ_NAME_LEN) )
> 
> So we are trying to change the driver name thing to be in enum. 

> No, that was never what I've been asking for. I've always said that the consumer of the data ought to have a way to know what 
> kind of data it is dealing with. I.e. the enum ought to represent what meaning the data has (frequency vs percentage).

Ok. If we add this "enum meaning_of_data", the xen_get_cpufreq_para will exceed 128Byte, which cannot even pass the compilation. I am not sure how to deal with this nicely. Do you have a suggestion? 

Best,
Wei

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

[Jan Beulich]

>>> On 10.09.15 at 12:10, <wei.w.wang@intel.com> wrote:
> Ok. If we add this "enum meaning_of_data", the xen_get_cpufreq_para will 
> exceed 128Byte, which cannot even pass the compilation. I am not sure how to 
> deal with this nicely. Do you have a suggestion? 

Currently afaict the structure occupies 14 8-byte slots, with two
unused 32-bit padding fields. Are you saying that the additions
you need to make don't fit in two 8-byte plus two 4-byte slots?
And if so, the first thing to recover space from would be
"turbo_enabled", which iirc is a boolean and hence one bit would
suffice to represent it. 31 bits then would surely be enough to
store your (two value) enum into?

Jan