|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
>>> On 14.09.15 at 04:32, <wei.w.wang@xxxxxxxxx> wrote:
> --- a/tools/libxc/xc_pm.c
> +++ b/tools/libxc/xc_pm.c
> @@ -260,13 +260,14 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> }
> else
> {
> - user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
> - 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->turbo_enabled = sys_para->turbo_enabled;
> + user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq;
> + 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.pct = sys_para->scaling_max_perf;
> + user_para->scaling_min.pct = sys_para->scaling_min_perf;
> + user_para->scaling_turbo_pct = sys_para->scaling_turbo_pct;
> + user_para->turbo_enabled = sys_para->turbo_enabled;
You fail to communicate perf_alias here. How will the caller know?
It's also odd that you initialize .pct fields from _perf ones - can't the
naming and layout be arranged to match as much as possible?
> --- a/xen/drivers/acpi/pmstat.c
> +++ b/xen/drivers/acpi/pmstat.c
> @@ -191,7 +191,9 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
> uint32_t ret = 0;
> const struct processor_pminfo *pmpt;
> struct cpufreq_policy *policy;
> - uint32_t gov_num = 0;
> + struct perf_limits *limits;
> + struct internal_governor *internal_gov;
> + uint32_t cur_gov, gov_num = 0;
> uint32_t *affected_cpus;
> uint32_t *scaling_available_frequencies;
> char *scaling_available_governors;
> @@ -200,13 +202,21 @@ 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;
> + internal_gov = policy->internal_gov;
> + cur_gov = internal_gov ? internal_gov->cur_gov : 0;
Bad literal 0 (see the uses of cur_gov below).
> if ( !pmpt || !pmpt->perf.states ||
> - !policy || !policy->governor )
> + !policy || (!policy->governor && !policy->internal_gov) )
Either you think you need the local variable internal_gov - then use
it everywhere, or drop it.
> return -EINVAL;
>
> - list_for_each(pos, &cpufreq_governor_list)
> - gov_num++;
> + if ( internal_gov )
> + gov_num = internal_gov->gov_num;
Since you don't need cur_gov ahead of this, please limit the number
of conditionals by moving its initialization here.
> 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_perf = limits->max_perf_pct;
> + op->u.get_para.scaling_min_perf = limits->min_perf_pct;
> + op->u.get_para.scaling_turbo_pct = limits->turbo_pct;
> + }
> + else
> + {
> + op->u.get_para.scaling_max_perf = policy->max;
> + op->u.get_para.scaling_min_perf = policy->min;
> + }
>
> if ( cpufreq_driver->name[0] )
> + {
> strlcpy(op->u.get_para.scaling_driver,
> cpufreq_driver->name, CPUFREQ_NAME_LEN);
> + if ( !strncmp(cpufreq_driver->name,
> + "intel_pstate", CPUFREQ_NAME_LEN) )
> + op->u.get_para.perf_alias = PERCENTAGE;
> + else
> + op->u.get_para.perf_alias = FREQUENCY;
This should be done together with the other things in the if/else
right above.
> @@ -299,16 +357,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;
Wouldn't this better be done by the internal governor's code, so
it can also for itself decide which of the kinds it may not want to
support?
> @@ -340,6 +423,9 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op)
> {
> struct cpufreq_policy new_policy;
>
> + if ( !policy->governor || internal_gov )
Hard tab.
> @@ -347,10 +433,43 @@ 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;
> +
> + if ( policy->governor || !internal_gov )
> + return -EINVAL;
> +
> + 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);
Afaict all three values are of the same type - why clamp_t() then
instead of clamp()?
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -297,11 +297,28 @@ typedef struct xen_ondemand xen_ondemand_t;
> * same as sysfs file name of native linux
> */
> #define CPUFREQ_NAME_LEN 16
> +
> +enum perf_alias {
xen_ prefix missing.
> + FREQUENCY = 0,
> + PERCENTAGE = 1
> +};
> +
> struct xen_get_cpufreq_para {
> /* IN/OUT variable */
> uint32_t cpu_num;
> uint32_t freq_num;
> uint32_t gov_num;
> + int32_t turbo_enabled;
> +
> + uint32_t cpuinfo_cur_freq;
> + uint32_t cpuinfo_max_freq;
> + uint32_t cpuinfo_min_freq;
> + uint32_t scaling_cur_freq;
> +
> + uint32_t scaling_turbo_pct;
> + uint32_t scaling_max_perf;
> + uint32_t scaling_min_perf;
> + enum perf_alias perf_alias;
>
> /* for all governors */
> /* OUT variable */
> @@ -309,23 +326,13 @@ struct xen_get_cpufreq_para {
> XEN_GUEST_HANDLE_64(uint32) scaling_available_frequencies;
> XEN_GUEST_HANDLE_64(char) scaling_available_governors;
> char scaling_driver[CPUFREQ_NAME_LEN];
> -
> - uint32_t cpuinfo_cur_freq;
> - uint32_t cpuinfo_max_freq;
> - uint32_t cpuinfo_min_freq;
> - uint32_t scaling_cur_freq;
> -
> char scaling_governor[CPUFREQ_NAME_LEN];
> - uint32_t scaling_max_freq;
> - uint32_t scaling_min_freq;
>
> /* for specific governor */
> union {
> struct xen_userspace userspace;
> struct xen_ondemand ondemand;
> } u;
> -
> - int32_t turbo_enabled;
> };
Is all of this re-arrangement really needed? Also can't turbo_enabled
and scaling_turbo_pct be combined into a single field?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |