On Mon, May 8, 2023 at 2:33 AM Jan Beulich <jbeul...@suse.com> wrote:
>
> On 05.05.2023 17:35, Jason Andryuk wrote:
> > On Fri, May 5, 2023 at 3:01 AM Jan Beulich <jbeul...@suse.com> wrote:
> >>
> >> On 04.05.2023 18:56, Jason Andryuk wrote:
> >>> On Thu, May 4, 2023 at 9:11 AM Jan Beulich <jbeul...@suse.com> wrote:
> >>>> On 01.05.2023 21:30, Jason Andryuk wrote:
> >>>>> --- a/docs/misc/xen-command-line.pandoc
> >>>>> +++ b/docs/misc/xen-command-line.pandoc
> >>>>> @@ -499,7 +499,7 @@ If set, force use of the performance counters for
> >>>>> oprofile, rather than detectin
> >>>>> available support.
> >>>>>
> >>>>> ### cpufreq
> >>>>> -> `= none | {{ <boolean> | xen }
> >>>>> [:[powersave|performance|ondemand|userspace][,<maxfreq>][,[<minfreq>][,[verbose]]]]}
> >>>>> | dom0-kernel`
> >>>>> +> `= none | {{ <boolean> | xen }
> >>>>> [:[powersave|performance|ondemand|userspace][,<hdc>][,[<hwp>]][,[<maxfreq>]][,[<minfreq>]][,[verbose]]]}
> >>>>> | dom0-kernel`
> >>>>
> >>>> Considering you use a special internal governor, the 4 governor
> >>>> alternatives are
> >>>> meaningless for hwp. Hence at the command line level recognizing "hwp"
> >>>> as if it
> >>>> was another governor name would seem better to me. This would then also
> >>>> get rid
> >>>> of one of the two special "no-" prefix parsing cases (which I'm not
> >>>> overly
> >>>> happy about).
> >>>>
> >>>> Even if not done that way I'm puzzled by the way you spell out the
> >>>> interaction
> >>>> of "hwp" and "hdc": As you say in the description, "hdc" is meaningful
> >>>> only when
> >>>> "hwp" was specified, so even if not merged with the governors group
> >>>> "hwp" should
> >>>> come first, and "hdc" ought to be rejected if "hwp" wasn't first
> >>>> specified. (The
> >>>> way you've spelled it out it actually looks to be kind of the other way
> >>>> around.)
> >>>
> >>> I placed them in alphabetical order, but, yes, it doesn't make sense.
> >>>
> >>>> Strictly speaking "maxfreq" and "minfreq" also should be objected to
> >>>> when "hwp"
> >>>> was specified.
> >>>>
> >>>> Overall I'm getting the impression that beyond your "verbose" related
> >>>> adjustment
> >>>> more is needed, if you're meaning to get things closer to how we parse
> >>>> the
> >>>> option (splitting across multiple lines to help see what I mean):
> >>>>
> >>>> `= none
> >>>> | {{ <boolean> | xen } [:{powersave|performance|ondemand|userspace}
> >>>>
> >>>> [{,hwp[,hdc]|[,maxfreq=<maxfreq>[,minfreq=<minfreq>]}]
> >>>> [,verbose]]}
> >>>> | dom0-kernel`
> >>>>
> >>>> (We're still parsing in a more relaxed way, e.g. minfreq may come ahead
> >>>> of
> >>>> maxfreq, but better be more tight in the doc than too relaxed.)
> >>>>
> >>>> Furthermore while max/min freq don't apply directly, there are still two
> >>>> MSRs
> >>>> controlling bounds at the package and logical processor levels.
> >>>
> >>> Well, we only program the logical processor level MSRs because we
> >>> don't have a good idea of the packages to know when we can skip
> >>> writing an MSR.
> >>>
> >>> How about this:
> >>> `= none
> >>> | {{ <boolean> | xen } {
> >>> [:{powersave|performance|ondemand|userspace}[,maxfreq=<maxfreq>[,minfreq=<minfreq>]]
> >>> | [:hwp[,hdc]] }
> >>> [,verbose]]}
> >>> | dom0-kernel`
> >>
> >> Looks right, yes.
> >
> > There is a wrinkle to using the hwp governor. The hwp governor was
> > named "hwp-internal", so it needs to be renamed to "hwp" for use with
> > command line parsing. That means the checking for "-internal" needs
> > to change to just "hwp" which removes the generality of the original
> > implementation.
>
> I'm afraid I don't see why this would strictly be necessary or a
> consequence.
Maybe I took your comment too far when you mentioned using hwp as a
fake governor. I used the actual HWP struct cpufreq_governor to hook
into cpufreq_cmdline_parse(). cpufreq_cmdline_parse() uses the that
name for comparison. But the naming stops being an issue if struct
cpufreq_governor gains a bool .internal flag. That flag also makes
the registration checks clearer.
> > The other issue is that if you select "hwp" as the governor, but HWP
> > hardware support is not available, then hwp_available() needs to reset
> > the governor back to the default. This feels like a layering
> > violation.
>
> Layering violation - yes. But why would the governor need resetting in
> this case? If HWP was asked for but isn't available, I don't think any
> other cpufreq handling (and hence governor) should be put in place.
> And turning off cpufreq altogether (if necessary in the first place)
> wouldn't, to me, feel as much like a layering violation.
My goal was for Xen to use HWP if available and fallback to the acpi
cpufreq driver if not. That to me seems more user-friendly than
disabling cpufreq.
if ( hwp_available() )
ret = hwp_register_driver();
else
ret = cpufreq_register_driver(&acpi_cpufreq_driver);
If we are setting cpufreq_opt_governor to enter hwp_available(), but
then HWP isn't available, it seems to me that we need to reset
cpufreq_opt_governor when exiting hwp_available() false.
Regards,
Jason
