Re: [Xen-devel] [PATCH v2 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c

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

On 26/05/2015 22:16, Jan Beulich wrote 
> >>> On 13.05.16 at 09:51, <wei.w.wang@xxxxxxxxx> wrote:
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -261,29 +272,47 @@ 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.max_perf_pct = policy->max_perf_pct;
> > +        op->u.get_para.scaling_min.min_perf_pct = policy->min_perf_pct;
> > +        op->u.get_para.scaling_turbo_pct = policy->turbo_pct;
> > +    } else {
> > +        op->u.get_para.scaling_max.max_freq = policy->max;
> > +        op->u.get_para.scaling_min.min_freq = policy->min;
> > +    }
> 
> How does the caller then know which of the union member meanings
> apply?

The end caller is xenpm. It's aware of the running pstate driver, so it knows 
the difference between freq and pct.

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -314,8 +314,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 max_freq;
> > +        int32_t  max_perf_pct;
> > +    } scaling_max;
> > +
> > +    union {
> > +        uint32_t min_freq;
> > +        int32_t  min_perf_pct;
> > +    } scaling_min;
> > +
> > +    int32_t scaling_turbo_pct;
> 
> Again all of these should presumably be uint32_t. Plus the max/min
> duplication should be avoided (i.e. drop the prefixes from the union
> fields). Nor do I think perf_pct is really all that useful - pct or
> percentage would do afaict.

Will change it to:
union {
        uint32_t freq;
        uint32_t  pct;
} scaling_max;

union {
        uint32_t freq;
        uint32_t  pct;
} scaling_min;


Best,
Wei


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel