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

[PATCH v3 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.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 tools/libxc/xc_pm.c                |   4 +-
 xen/drivers/acpi/pmstat.c          | 130 +++++++++++++++++++++++++++++++------
 xen/include/acpi/cpufreq/cpufreq.h |   2 +
 xen/include/public/sysctl.h        |  16 ++++-
 4 files changed, 129 insertions(+), 23 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..53d811f 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -167,7 +167,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
  * 2. Provide user PM control
  */
 static int read_scaling_available_governors(char *scaling_available_governors,
-                                            unsigned int size)
+                                            unsigned int size, bool_t internal)
 {
     unsigned int i = 0;
     struct cpufreq_governor *t;
@@ -175,12 +175,26 @@ static int read_scaling_available_governors(char *scaling_available_governors,
     if ( !scaling_available_governors )
         return -EINVAL;
 
-    list_for_each_entry(t, &cpufreq_governor_list, governor_list)
+    if (internal)
     {
+        i += scnprintf(&scaling_available_governors[0],
+				CPUFREQ_NAME_LEN, "%s ", "performance");
         i += scnprintf(&scaling_available_governors[i],
-                       CPUFREQ_NAME_LEN, "%s ", t->name);
-        if ( i > size )
-            return -EINVAL;
+				CPUFREQ_NAME_LEN, "%s ", "powersave");
+        i += scnprintf(&scaling_available_governors[i],
+				CPUFREQ_NAME_LEN, "%s ", "userspace");
+        i += scnprintf(&scaling_available_governors[i],
+				CPUFREQ_NAME_LEN, "%s ", "ondemand");
+    }
+    else
+    {
+        list_for_each_entry(t, &cpufreq_governor_list, governor_list)
+        {
+            i += scnprintf(&scaling_available_governors[i],
+                           CPUFREQ_NAME_LEN, "%s ", t->name);
+            if ( i > size )
+                return -EINVAL;
+        }
     }
     scaling_available_governors[i-1] = '\0';
 
@@ -192,6 +206,7 @@ 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;
     uint32_t gov_num = 0;
     uint32_t *affected_cpus;
     uint32_t *scaling_available_frequencies;
@@ -201,13 +216,19 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
 
     pmpt = processor_pminfo[op->cpuid];
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
+    limits = &policy->limits;
 
     if ( !pmpt || !pmpt->perf.states ||
-         !policy || !policy->governor )
+         !policy || (!policy->governor && !policy->policy) )
         return -EINVAL;
 
-    list_for_each(pos, &cpufreq_governor_list)
+    if (policy->policy)
+        gov_num = INTEL_PSTATE_INTERNAL_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)    ||
@@ -245,7 +266,7 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
            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))) )
+                gov_num * CPUFREQ_NAME_LEN * sizeof(char), !!policy->policy)) )
     {
         xfree(scaling_available_governors);
         return ret;
@@ -261,8 +282,17 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
     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 (policy->policy)
+    {
+        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 (policy->policy)
+    {
+    case CPUFREQ_POLICY_PERFORMANCE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "performance", CPUFREQ_NAME_LEN);
+        break;
+    case CPUFREQ_POLICY_POWERSAVE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "powersave", CPUFREQ_NAME_LEN);
+        break;
+    case CPUFREQ_POLICY_USERSPACE:
+        strlcpy(op->u.get_para.scaling_governor,
+            "userspace", CPUFREQ_NAME_LEN);
+        break;
+    case CPUFREQ_POLICY_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, 
@@ -307,9 +357,27 @@ static int set_cpufreq_gov(struct xen_sysctl_pm_op *op)
 
     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 (old_policy->policy)
+    {
+        if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "performance", CPUFREQ_NAME_LEN) )
+            old_policy->policy = CPUFREQ_POLICY_PERFORMANCE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "powersave", CPUFREQ_NAME_LEN) )
+            old_policy->policy = CPUFREQ_POLICY_POWERSAVE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "userspace", CPUFREQ_NAME_LEN) )
+            old_policy->policy = CPUFREQ_POLICY_USERSPACE;
+        else if ( !strnicmp(op->u.set_gov.scaling_governor,
+                       "ondemand", CPUFREQ_NAME_LEN) )
+            old_policy->policy = CPUFREQ_POLICY_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);
 }
@@ -321,7 +389,7 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
 
     policy = per_cpu(cpufreq_cpu_policy, op->cpuid);
 
-    if ( !policy || !policy->governor )
+    if ( !policy || (!policy->governor && !policy->policy) )
         return -EINVAL;
 
     switch(op->u.set_para.ctrl_type)
@@ -348,6 +416,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/acpi/cpufreq/cpufreq.h b/xen/include/acpi/cpufreq/cpufreq.h
index 71bb45c..e8a6bab 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/acpi/cpufreq/cpufreq.h
@@ -109,6 +109,8 @@ struct cpufreq_freqs {
  *                          CPUFREQ GOVERNORS                        *
  *********************************************************************/
 
+#define INTEL_PSTATE_INTERNAL_GOV_NUM 4
+
 /* The four internal governors used in intel_pstate */
 #define CPUFREQ_POLICY_POWERSAVE        (1)
 #define CPUFREQ_POLICY_PERFORMANCE      (2)
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 v3 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Julien Grall]

Hi,

On 11/06/2015 04:31, Wei Wang wrote:
> -    list_for_each(pos, &cpufreq_governor_list)
> +    if (policy->policy)

What if another cpufreq decides to use policy->policy?

> +        gov_num = INTEL_PSTATE_INTERNAL_GOV_NUM;

Why not using cpufreq_governor_list?

> +    else
> +    {
> +        list_for_each(pos, &cpufreq_governor_list)
>           gov_num++;

The indentation looks wrong to me.

-- 
Julien Grall

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

[Wang, Wei W]

On 11/06/2015 22:02, Julien Grall wrote:
> On 11/06/2015 04:31, Wei Wang wrote:
> > -    list_for_each(pos, &cpufreq_governor_list)
> > +    if (policy->policy)
> 
> What if another cpufreq decides to use policy->policy?

What is "another cpufreq"? The "policy" is per-CPU struct.

 > > +        gov_num = INTEL_PSTATE_INTERNAL_GOV_NUM;
> 
> Why not using cpufreq_governor_list?

That's used by the old driver. We are not going through that old governor layer.

> > +    else
> > +    {
> > +        list_for_each(pos, &cpufreq_governor_list)
> >           gov_num++;
> 
> The indentation looks wrong to me.

It has four "+$", should be correct.

Best,
Wei

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

[Jan Beulich]

>>> On 12.06.15 at 05:03, <wei.w.wang@intel.com> wrote:
> On 11/06/2015 22:02, Julien Grall wrote:
>> On 11/06/2015 04:31, Wei Wang wrote:
>> > +    else
>> > +    {
>> > +        list_for_each(pos, &cpufreq_governor_list)
>> >           gov_num++;
>> 
>> The indentation looks wrong to me.
> 
> It has four "+$", should be correct.

The gov_num++ is supposedly the body of the list_for_each(), and
hence needs to be indented by four more spaces than the loop header
(i.e. a total of 12).

Jan

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

[Julien Grall]

Hi,

On 11/06/2015 23:03, Wang, Wei W wrote:
> On 11/06/2015 22:02, Julien Grall wrote:
>> On 11/06/2015 04:31, Wei Wang wrote:
>>> -    list_for_each(pos, &cpufreq_governor_list)
>>> +    if (policy->policy)
>>
>> What if another cpufreq decides to use policy->policy?
>
> What is "another cpufreq"? The "policy" is per-CPU struct.

I mean another cpufreq driver. Correct me if I'm wrong but from the name 
policy is not intel pstate specific. That means that a new cpufreq 
driver can decide to use the field his own purpose..

>   > > +        gov_num = INTEL_PSTATE_INTERNAL_GOV_NUM;
>>
>> Why not using cpufreq_governor_list?
>
> That's used by the old driver. We are not going through that old governor layer.

This is common code, it's used by different cpufreq driver for both x86 
and ARM (not yet supported). We should not relying on any other cpufreq 
driver won't use the field policy.

Regards,

-- 
Julien Grall

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

[Wang, Wei W]

On 12/06/2015 19:14, Julien Grall wrote:
> On 11/06/2015 23:03, Wang, Wei W wrote:
> > On 11/06/2015 22:02, Julien Grall wrote:
> >> On 11/06/2015 04:31, Wei Wang wrote:
> >>> -    list_for_each(pos, &cpufreq_governor_list)
> >>> +    if (policy->policy)
> >>
> >> What if another cpufreq decides to use policy->policy?
> >
> > What is "another cpufreq"? The "policy" is per-CPU struct.
> 
> I mean another cpufreq driver. Correct me if I'm wrong but from the name
> policy is not intel pstate specific. That means that a new cpufreq driver can
> decide to use the field his own purpose..

We actually want it be intel_pstate specific. If maintainers agree, I think renaming it to intel_pstate_policy is a good option.

> >   > > +        gov_num = INTEL_PSTATE_INTERNAL_GOV_NUM;
> >>
> >> Why not using cpufreq_governor_list?
> >

This should not be a problem after renaming "->policy" to be "->intel_pstate_policy".

Best,
Wei

> > That's used by the old driver. We are not going through that old governor
> layer.
> 
> This is common code, it's used by different cpufreq driver for both x86 and
> ARM (not yet supported). We should not relying on any other cpufreq driver
> won't use the field policy.

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

[Jan Beulich]

>>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
> On 12/06/2015 19:14, Julien Grall wrote:
>> On 11/06/2015 23:03, Wang, Wei W wrote:
>> > On 11/06/2015 22:02, Julien Grall wrote:
>> >> On 11/06/2015 04:31, Wei Wang wrote:
>> >>> -    list_for_each(pos, &cpufreq_governor_list)
>> >>> +    if (policy->policy)
>> >>
>> >> What if another cpufreq decides to use policy->policy?
>> >
>> > What is "another cpufreq"? The "policy" is per-CPU struct.
>> 
>> I mean another cpufreq driver. Correct me if I'm wrong but from the name
>> policy is not intel pstate specific. That means that a new cpufreq driver 
> can
>> decide to use the field his own purpose..
> 
> We actually want it be intel_pstate specific. If maintainers agree, I think 
> renaming it to intel_pstate_policy is a good option.

No, this name is just ugly. If you need driver specific data, have
a void pointer in the generic structure; the driver can then allocate
memory to be pointed to by that, and can store there whatever
private data it needs.

Jan

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

[Wang, Wei W]

On 15/06/2015 17:15, Jan Beulich wrote:
> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
> > On 12/06/2015 19:14, Julien Grall wrote:
> >> On 11/06/2015 23:03, Wang, Wei W wrote:
> >> > On 11/06/2015 22:02, Julien Grall wrote:
> >> >> On 11/06/2015 04:31, Wei Wang wrote:
> >> >>> -    list_for_each(pos, &cpufreq_governor_list)
> >> >>> +    if (policy->policy)
> >> >>
> >> >> What if another cpufreq decides to use policy->policy?
> >> >
> >> > What is "another cpufreq"? The "policy" is per-CPU struct.
> >>
> >> I mean another cpufreq driver. Correct me if I'm wrong but from the
> >> name policy is not intel pstate specific. That means that a new
> >> cpufreq driver
> > can
> >> decide to use the field his own purpose..
> >
> > We actually want it be intel_pstate specific. If maintainers agree, I
> > think renaming it to intel_pstate_policy is a good option.
> 
> No, this name is just ugly. If you need driver specific data, have a void pointer
> in the generic structure; the driver can then allocate memory to be pointed
> to by that, and can store there whatever private data it needs.

OK. I plan to make the following changes:

1) in cpufreq_policy, add a field - void *private_data;


2) add a new structure:
 struct intel_pstate_policy {
	unsigned int policy;
}

3) in intel_pstate_cpu_setup():
         struct intel_pstate_policy *private_policy = xzalloc(struct intel_pstate_policy);
         private_policy->policy = INTEL_PSTATE_POLICY_ONDEMAND;
         policy->private_data = private_policy;

4) in intel_pstate_cpu_exit():
         xfree(policy->private_data);

5)  Change all the "if (policy->policy)" to "if (cpufreq_driver->setpolicy)"


Best,
Wei

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

[Jan Beulich]

>>> On 15.06.15 at 14:28, <wei.w.wang@intel.com> wrote:
> On 15/06/2015 17:15, Jan Beulich wrote:
>> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
>> > We actually want it be intel_pstate specific. If maintainers agree, I
>> > think renaming it to intel_pstate_policy is a good option.
>> 
>> No, this name is just ugly. If you need driver specific data, have a void 
> pointer
>> in the generic structure; the driver can then allocate memory to be pointed
>> to by that, and can store there whatever private data it needs.
> 
> OK. I plan to make the following changes:
> 
> 1) in cpufreq_policy, add a field - void *private_data;
> 
> 
> 2) add a new structure:
>  struct intel_pstate_policy {
> 	unsigned int policy;
> }

struct intel_pstate_private or struct intel_pstate_data please.

Jan

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

[Wang, Wei W]

On 15/06/2015 20:37, Jan Beulich wrote:
> >>> On 15.06.15 at 14:28, <wei.w.wang@intel.com> wrote:
> > On 15/06/2015 17:15, Jan Beulich wrote:
> >> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
> >> > We actually want it be intel_pstate specific. If maintainers agree,
> >> > I think renaming it to intel_pstate_policy is a good option.
> >>
> >> No, this name is just ugly. If you need driver specific data, have a
> >> void
> > pointer
> >> in the generic structure; the driver can then allocate memory to be
> >> pointed to by that, and can store there whatever private data it needs.
> >
> > OK. I plan to make the following changes:
> >
> > 1) in cpufreq_policy, add a field - void *private_data;
> >
> >
> > 2) add a new structure:
> >  struct intel_pstate_policy {
> > 	unsigned int policy;
> > }
> 
> struct intel_pstate_private or struct intel_pstate_data please.

OK, no problem.
Jan, I will wait for your other comments (if there are), and then send out the new version. Thanks.

Best,
Wei

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

[Wang, Wei W]

On 15/06/2015 20:37, Jan Beulich wrote:
> >>> On 15.06.15 at 14:28, <wei.w.wang@intel.com> wrote:
> > On 15/06/2015 17:15, Jan Beulich wrote:
> >> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
> >> > We actually want it be intel_pstate specific. If maintainers agree,
> >> > I think renaming it to intel_pstate_policy is a good option.
> >>
> >> No, this name is just ugly. If you need driver specific data, have a
> >> void
> > pointer
> >> in the generic structure; the driver can then allocate memory to be
> >> pointed to by that, and can store there whatever private data it needs.
> >
> > OK. I plan to make the following changes:
> >
> > 1) in cpufreq_policy, add a field - void *private_data;
> >
> >
> > 2) add a new structure:
> >  struct intel_pstate_policy {
> > 	unsigned int policy;
> > }
> 
> struct intel_pstate_private or struct intel_pstate_data please.
> 

"struct perf_limits" is currently used only by intel_pstate, should we also move it to the intel_pstate_private struct, instead of the cpufreq_policy?

Best,
Wei

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

[Jan Beulich]

>>> On 16.06.15 at 09:09, <wei.w.wang@intel.com> wrote:
> On 15/06/2015 20:37, Jan Beulich wrote:
>> >>> On 15.06.15 at 14:28, <wei.w.wang@intel.com> wrote:
>> > On 15/06/2015 17:15, Jan Beulich wrote:
>> >> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
>> >> > We actually want it be intel_pstate specific. If maintainers agree,
>> >> > I think renaming it to intel_pstate_policy is a good option.
>> >>
>> >> No, this name is just ugly. If you need driver specific data, have a
>> >> void
>> > pointer
>> >> in the generic structure; the driver can then allocate memory to be
>> >> pointed to by that, and can store there whatever private data it needs.
>> >
>> > OK. I plan to make the following changes:
>> >
>> > 1) in cpufreq_policy, add a field - void *private_data;
>> >
>> >
>> > 2) add a new structure:
>> >  struct intel_pstate_policy {
>> > 	unsigned int policy;
>> > }
>> 
>> struct intel_pstate_private or struct intel_pstate_data please.
>> 
> 
> "struct perf_limits" is currently used only by intel_pstate, should we also 
> move it to the intel_pstate_private struct, instead of the cpufreq_policy?

Yes of course - anything private to the driver should go there.

Jan

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

[Wang, Wei W]

On 17/06/2015 15:54, Jan Beulich wrote:
> >>> On 16.06.15 at 09:09, <wei.w.wang@intel.com> wrote:
> > On 15/06/2015 20:37, Jan Beulich wrote:
> >> >>> On 15.06.15 at 14:28, <wei.w.wang@intel.com> wrote:
> >> > On 15/06/2015 17:15, Jan Beulich wrote:
> >> >> >>> On 15.06.15 at 02:30, <wei.w.wang@intel.com> wrote:
> >> >> > We actually want it be intel_pstate specific. If maintainers
> >> >> > agree, I think renaming it to intel_pstate_policy is a good option.
> >> >>
> >> >> No, this name is just ugly. If you need driver specific data, have
> >> >> a void
> >> > pointer
> >> >> in the generic structure; the driver can then allocate memory to
> >> >> be pointed to by that, and can store there whatever private data it
> needs.
> >> >
> >> > OK. I plan to make the following changes:
> >> >
> >> > 1) in cpufreq_policy, add a field - void *private_data;
> >> >
> >> >
> >> > 2) add a new structure:
> >> >  struct intel_pstate_policy {
> >> > 	unsigned int policy;
> >> > }
> >>
> >> struct intel_pstate_private or struct intel_pstate_data please.
> >>
> >
> > "struct perf_limits" is currently used only by intel_pstate, should we
> > also move it to the intel_pstate_private struct, instead of the
> cpufreq_policy?
> 
> Yes of course - anything private to the driver should go there.

Just realized that " struct perf_limits " will also need to be used in the common code (e.g. set max_perf_pct in pmstat.c), so I plan to keep it in the cpufreq_policy struct. Please let me know if you have a different opinion. 

Best,
Wei 

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

[Wang, Wei W]

Hi Jan,
On 11/06/2015 16:31, Wei Wang wrote:
> Add support in the pmstat.c so that the xenpm tool can request to access the
> intel_pstate driver.

I want to propose some other changes here to commonize the intel_pstate implementation in this common code (pmstat.c).
 
1) introduce a new struct:
    struct internal_governor {
        char *avail_gov;
        uint32_t cur_gov;         
        uint32_t gov_num;
    }

2) a new callback in cpufreq_driver, "cpufreq_driver->get_internal_gov(internal_gov)",
     This allows other possible cpufreq drivers, which implement their own internal governors, to return their own interenal governor info (e.g. we will not need to use the INTEL_PSTATE_GOV_NUM, which is defined to be 4, in this patch).

Do you think this would be better?

Best,
Wei

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

[Wang, Wei W]

On 23/06/2015 09:41, Wei Wang wrote:
> On 11/06/2015 16:31, Wei Wang wrote:
> > Add support in the pmstat.c so that the xenpm tool can request to
> > access the intel_pstate driver.
> 
> I want to propose some other changes here to commonize the intel_pstate
> implementation in this common code (pmstat.c).
> 
> 1) introduce a new struct:
>     struct internal_governor {
>         char *avail_gov;
>         uint32_t cur_gov;
>         uint32_t gov_num;
>     }
> 
> 2) a new callback in cpufreq_driver, "cpufreq_driver-
> >get_internal_gov(internal_gov)",
>      This allows other possible cpufreq drivers, which implement their own
> internal governors, to return their own interenal governor info (e.g. we will
> not need to use the INTEL_PSTATE_GOV_NUM, which is defined to be 4, in
> this patch).


Jan, do you have any comments on this change?

Best,
Wei

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

[Jan Beulich]

>>> On 23.06.15 at 03:40, <wei.w.wang@intel.com> wrote:
> Hi Jan,
> On 11/06/2015 16:31, Wei Wang wrote:
>> Add support in the pmstat.c so that the xenpm tool can request to access the
>> intel_pstate driver.
> 
> I want to propose some other changes here to commonize the intel_pstate 
> implementation in this common code (pmstat.c).
>  
> 1) introduce a new struct:
>     struct internal_governor {
>         char *avail_gov;
>         uint32_t cur_gov;         
>         uint32_t gov_num;
>     }
> 
> 2) a new callback in cpufreq_driver, 
> "cpufreq_driver->get_internal_gov(internal_gov)",
>      This allows other possible cpufreq drivers, which implement their own 
> internal governors, to return their own interenal governor info (e.g. we will 
> not need to use the INTEL_PSTATE_GOV_NUM, which is defined to be 4, in this 
> patch).
> 
> Do you think this would be better?

I really need to see this in context.

Jan