Re: [PATCH] target/ppc/kvm: Cache timebase frequency

[Greg Kurz <groug@kaod.org>]

On Wed, 17 Mar 2021 16:39:04 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 3/17/21 4:24 PM, Greg Kurz wrote:
> > Each vCPU core exposes its timebase frequency in the DT. When running
> > under KVM, this means parsing /proc/cpuinfo in order to get the timebase
> > frequency of the host CPU.
> > 
> > The parsing appears to slow down the boot quite a bit with higher number
> > of cores:
> > 
> > # of cores     seconds spent in spapr_dt_cpus()
> >       8                  0.550122
> >      16                  1.342375
> >      32                  2.850316
> >      64                  5.922505
> >      96                  9.109224
> >     128                 12.245504
> >     256                 24.957236
> >     384                 37.389113
> > 
> > The timebase frequency of the host CPU is identical for all
> > cores and it is an invariant for the VM lifetime. Cache it
> > instead of doing the same expensive parsing again and again.
> > 
> > With this patch applied:
> > 
> >     384                 0.518382
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  target/ppc/kvm.c |   11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 298c1f882c67..9ad3dae29132 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -1819,7 +1819,13 @@ uint32_t kvmppc_get_tbfreq(void)
> >  {
> >      char line[512];
> >      char *ns;
> > -    uint32_t retval = NANOSECONDS_PER_SECOND;
> > +    static uint32_t retval = -1;
> 
> Please document why in the code ...
> 

Yeah, I've been lazy :)

> > +
> > +    if (retval != -1) {
> > +        return retval;
> > +    }
> > +
> > +    retval = NANOSECONDS_PER_SECOND;
> >  
> >      if (read_cpuinfo("timebase", line, sizeof(line))) {
> >          return retval;
> > @@ -1832,7 +1838,8 @@ uint32_t kvmppc_get_tbfreq(void)
> >  
> >      ns++;
> >  
> > -    return atoi(ns);
> > +    retval = atoi(ns);
> > +    return retval;
> >  }
> 
> ... or alternatively use self-documented code:
> 
> -- >8 --
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 298c1f882c6..2b2fe5d8148 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1815,7 +1815,7 @@ static int read_cpuinfo(const char *field, char
> *value, int len)
>      return ret;
>  }
> 
> -uint32_t kvmppc_get_tbfreq(void)
> +static uint32_t kvmppc_get_tbfreq_procfs(void)
>  {
>      char line[512];
>      char *ns;
> @@ -1835,6 +1835,17 @@ uint32_t kvmppc_get_tbfreq(void)
>      return atoi(ns);
>  }
> 
> +uint32_t kvmppc_get_tbfreq(void)
> +{
> +    static uint32_t cached_tbfreq;
> +
> +    if (!cached_tbfreq) {
> +        cached_tbfreq = kvmppc_get_tbfreq_procfs();
> +    }
> +
> +    return cached_tbfreq;
> +}
> +

This is much nicer indeed. I'll do just that in v2.

Thanks ! :)

>  bool kvmppc_get_host_serial(char **value)
>  {
>      return g_file_get_contents("/proc/device-tree/system-id", value, NULL,
> ---