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

Well, okay, I'm tired of discussing issues like this.

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

Jan

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