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

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

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:
> >> > -    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?
> >
> >I think it should be pmstat.c-'s job here to set "
> >internal_gov->cur_gov", which  is later passed to the internal
> >governor's implementation code, who then  decides how to deal with the
> requested governor in "internal_gov->cur_gov".
> 
> Which it then is able to communicate in which way? Without itself adjusting
> ->cur_gov again?
 
In this way: 
pmstat.c gets the string-represented governor adjusting request from the upper 
layer, parses it to the number-represented value (INTERNAL_GOV_), and feeds the 
number-represented one to the driver's internal governor implementation. If the 
internal governor implementation doesn't support the passed INTERNAL_GOV_XX, 
i.e. INTERNAL_GOV_XX goes to the "default:" section of the "switch()", which 
adjusts the internal_gov->cur_gov to the default one, e.g. 
INTERNAL_GOV_ONDEMAND.

> >> >  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?
> >
> >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. 

Best,
Wei

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