Re: [PATCH v3 14/15] xen/xenpm: Adapt cpu frequency monitor in xenpm

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-03-25 07:26, Jan Beulich wrote:
> On 06.03.2025 09:39, Penny Zheng wrote:
> > Make `xenpm get-cpureq-para/set-cpufreq-para` available in CPPC mode.
> > --- a/tools/libs/ctrl/xc_pm.c
> > +++ b/tools/libs/ctrl/xc_pm.c
> > @@ -214,13 +214,12 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> >                          user_para->gov_num * CPUFREQ_NAME_LEN * sizeof(char), 
> > XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> >   
> >       bool has_num = user_para->cpu_num &&
> > -                     user_para->freq_num &&
> >                        user_para->gov_num;
> >   
> >       if ( has_num )
> Something looks wrong here already before your patch: With how has_num is set
> and with this conditional, ...
>
> >       {
> >           if ( (!user_para->affected_cpus)                    ||
> > -             (!user_para->scaling_available_frequencies)    ||
> > +             (user_para->freq_num && 
> > !user_para->scaling_available_frequencies)    ||
> >                (user_para->gov_num && !user_para->scaling_available_governors) )
> ... this ->gov_num check, ...>>           {
> >               errno = EINVAL;
> > @@ -228,14 +227,16 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid,
> >           }
> >           if ( xc_hypercall_bounce_pre(xch, affected_cpus) )
> >               goto unlock_1;
> > -        if ( xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
> > +        if ( user_para->freq_num &&
> > +             xc_hypercall_bounce_pre(xch, scaling_available_frequencies) )
> >               goto unlock_2;
> >           if ( user_para->gov_num &&
> ... this one, and ...
>
> >                xc_hypercall_bounce_pre(xch, scaling_available_governors) )
> >               goto unlock_3;
> >   
> >           set_xen_guest_handle(sys_para->affected_cpus, affected_cpus);
> > -        set_xen_guest_handle(sys_para->scaling_available_frequencies, 
> > scaling_available_frequencies);
> > +        if ( user_para->freq_num )
> > +            set_xen_guest_handle(sys_para->scaling_available_frequencies, 
> > scaling_available_frequencies);
> (Nit: Yet another overly long line. It was too long already before, yes, but
>   that's no excuse to make it even longer.  The more that there is better
>   formatting right in context below.)
>
> >           if ( user_para->gov_num )
> ... this one are all dead code. Jason? I expect the has_num variable simply
> wants dropping altogether, thus correcting the earlier anomaly and getting
> the intended new behavior at the same time.
Hmmm.  The sysctl is executed twice - first to query the assorted *_num 
values and a second time to retrieve the results with sized arrays.
get_hwp_para() does not populate scaling_available_governors, so the 
intention was to be able to skip allocating the buffer for it.
    pmstat&xenpm: Re-arrage for cpufreq union

    Rearrange code now that xen_sysctl_pm_op's get_para fields has the
    nested union and struct.  In particular, the scaling governor
    information like scaling_available_governors is inside the union, so it
    is not always available.  Move those fields (op->u.get_para.u.s.u.*)
    together as well as the common fields (ones outside the union like
    op->u.get_para.turbo_enabled).

    With that, gov_num may be 0, so bounce buffer handling needs
    to be modified.

    scaling_governor and other fields inside op->u.get_para.u.s.u.* 
won't be
    used for hwp, so this will simplify the change when hwp support is
    introduced and re-indents these lines all together.

I noted that gov_num may be 0.  But that may have been before hwp had 
its own internal governor.  But, yes, the has_num handling looks wrong 
for gov_num == 0.  I don't have a machine with hwp to verify.

> > @@ -926,7 +926,8 @@ static int show_cpufreq_para_by_cpuid(xc_interface 
> > *xc_handle, int cpuid)
> >               ret = -ENOMEM;
> >               goto out;
> >           }
> > -        if (!(p_cpufreq->scaling_available_frequencies =
> > +        if (p_cpufreq->freq_num &&
> > +            !(p_cpufreq->scaling_available_frequencies =
> >                 malloc(p_cpufreq->freq_num * sizeof(uint32_t))))
> >           {
> >               fprintf(stderr,
> Can someone explain to me how the pre-existing logic here works? All
> three ->*_num start out as zero. Hence respective allocations (of zero
> size) may conceivably return NULL (the behavior there is implementation
> defined after all). Yet then we'd bail from the loop, and hence from the
> function. IOW adding a ->freq_num check and also a ->cpu_num one (along
> with the ->gov_num one that apparently was added during HWP development)
> would once again look like an independent (latent) bugfix to me.
I guess we rely on glibc providing non-NULL?  But also they are ignored 
for the initial query of *_num values.
Regards,
Jason