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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.10.15 at 10:48, <wei.w.wang@xxxxxxxxx> wrote:
> On 26/10/2015 17:42,  Jan Beulich wrote:
>> >>> On 26.10.15 at 08:59, <wei.w.wang@xxxxxxxxx> wrote:
>> > On 26/10/2015 15:03,  Jan Beulich wrote:
>> >> >>> "Wang, Wei W" <wei.w.wang@xxxxxxxxx> 10/26/15 7:27 AM >>>
>> >> >On 08/10/2015 12:11,  Jan Beulich wrote:
>> >> >> >>> On 14.09.15 at 04:32, <wei.w.wang@xxxxxxxxx> wrote:
>> >> >> > @@ -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?
>> >> >
>> >> >Personally, we should not combine the two.
>> >> > turbo_enabled is used by both the old pstate driver and
>> >> >intel_pstate, but  scaling_turbo_pct is only used in intel_pstate.
>> >> >If we use
>> >> "scaling_turbo_pct=0"
>> >> > to represent "turbo_enabled=0", and "scaling_turbo_pct>0" to
>> >> >represent " turbo_enabled=1", then we will need to modify the old
>> >> >driver to use scaling_turbo_pct, i.e. changing the old driver to be 
>> >> >aware 
> of
>> the "percentage"
>> >> > concept, which is proposed in intel_pstate. On the other side, I
>> >> >think keeping  turbo_enabled and scaling_turbo_pct separated makes
>> >> >the code
>> >> easier to read.
>> >>
>> >> Note that "combine" doesn't necessarily mean "eliminate the old one"
>> >> - they could as well become field of a union. The basic question you
>> >> should ask yourself in such cases is: "Do both fields have a meaning
>> >> at the same time?" If the answer is yes, then of course they should
>> >> remain separate. If the answer is no _and_ their purpose is
>> >> reasonably similar, then combining them should at least be considered.
>> >
>> > Ok. I will keep the two separated, since they do have their own
>> > meaning at the same time.
>> 
>> Being which?
> 
> Keeping both of the two there. Just as what they are now - two independent 
> fields.

That wasn't the question; I rather inquired what "meaning at the same
time" both fields have.

Jan


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