Re: [Xen-devel] [PATCH v4 11/11] tools: enable xenpm to control the intel_pstate driver

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

On 07/07/2015 20:14, Wei Liu wrote:
> On Tue, Jul 07, 2015 at 01:05:21PM +0100, Jan Beulich wrote:
> > >>> On 07.07.15 at 10:55, <wei.liu2@xxxxxxxxxx> wrote:
> > > On Thu, Jun 25, 2015 at 07:19:05PM +0800, Wei Wang wrote:
> > >> --- a/tools/libxc/include/xenctrl.h
> > >> +++ b/tools/libxc/include/xenctrl.h
> > >> @@ -2266,8 +2266,18 @@ struct xc_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;
> > >> +
> > >
> > > Don't you need struct? I don't see how union would work for you, you
> > > clearly need bot freq and pct at the same time.
> >
> > Why? The current driver uses freq; intel_pstate uses pct. What looks
> > wrong is the code below using both fields at once.
> >
> 
> I only looked at this single patch.
> 
> I got that impression from here:
> 
> +        user_para->scaling_max.freq             = sys_para->scaling_max.freq;
> +        user_para->scaling_min.freq             = sys_para->scaling_min.freq;
> +        user_para->scaling_max.pct              = sys_para->scaling_max.pct;
> +        user_para->scaling_min.pct              = sys_para->scaling_min.pct;
> 
> So using union is OK, just the code is confusing.

This actually functions well, we just have a redundant copy here. For example:
user_para->scaling_max.freq             = sys_para->scaling_max.freq;
user_para->scaling_max.pct               = sys_para->scaling_max.pct;

The two sentences are doing the same thing (the memory location is the same for 
freq and pct, it just depends on the driver to put what kind of value (freq or 
pct) into the memory).

If this causes some confusion, how about removing the 
"user_para->scaling_max.pct               = sys_para->scaling_max.pct;"  ?


Best,
Wei




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