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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.07.15 at 07:15, <wei.w.wang@xxxxxxxxx> wrote:
> 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;"  ?

Better make it

        user_para->scaling_max = sys_para->scaling_max;

if at all possible. If not possible, something BUILD_BUG_ON()-like
should imo be added to make sure the fields are of equal size.

Jan


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