Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix

[Gavin Shan]

On 11/11/22 5:13 PM, Igor Mammedov wrote:
> On Fri, 11 Nov 2022 07:47:16 +0100
> Markus Armbruster <armbru@redhat.com> wrote: 
> > Gavin Shan <gshan@redhat.com> writes:
> > > On 11/11/22 11:05 AM, Zhenyu Zhang wrote:
> > > > Commit ffac16fab3 "hostmem: introduce "prealloc-threads" property"
> > > > (v5.0.0) changed the default number of threads from number of CPUs
> > > > to 1.  This was deemed a regression, and fixed in commit f8d426a685
> > > > "hostmem: default the amount of prealloc-threads to smp-cpus".
> > > > Except the documentation remained unchanged.  Update it now.
> > > > Signed-off-by: Zhenyu Zhang <zhenyzha@redhat.com>
> > > > ---
> > > > v3: Covers historical descriptions                  (Markus)
> > > > v2: The property is changed to smp-cpus since 5.0   (Phild)
> > > > ---
> > > >    qapi/qom.json | 2 +-
> > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > >    
> > >
> > > With the following comments addressed:
> > >
> > > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > >
> > > ---
> > >
> > > Please consider amending the commit log to something like below.
> > >
> > > The default "prealloc-threads" value is set to 1 when the property is
> > > added by commit ffac16fab33b ("hostmem: introduce "prealloc-threads"
> > > property") in v5.0.0. The default value is conflicting with the sugar
> > > property as the value provided by the sugar property is number of CPUs.
> >
> > What is the sugar property?  Can you explain the conflict in a bit more
> > detail?
>
> my guess is that Gavin means mem_prealloc compat glue in 
> qemu_process_sugar_options()
>
> property value should be set according to following order
>       default -> compat -> explicit value
> so I don't see any conflict here.
>
> PS:
> if it we up to me, default would have stayed 1,
> and prealloc-threads fixup to vCPUs number would happen in vl.c
> similar to what is done in qemu_process_sugar_options(),
> keeping backend clean of external dependencies.

Yes, it's the sugar property I was talking about. I'm not sure if
we have a more popular name for this property: compat property or
sugar property.

When 'mem-prealloc=on' and 'prealloc-threads=xxx' aren't provided,
the value is 1 before commit f8d426a6852c is applied. It's not
inconsistent with 'mem-prealloc=on'. It's the conflict I was talking
about and it's fixed by commit f8d426a6852c

> > > The conflict has been fixed by commit f8d426a6852c ("hostmem: default
> > > the amount of prealloc-threads to smp-cpus"). However, 'qapi/qom.json'
> > > was missed to be updated accordingly in the commit.
> > >
> > > Update 'qapi/qom.json' to reflect the change in commit f8d426a6852c.
> > >
> > > Signed-off-by: Zhenyu Zhang <zhenyzha@redhat.com>
> > >
> > > When a specific commit is mentioned in the commit log, we usually have
> > > fixed format like below.
> > >
> > > commit ffac16fab33b ("hostmem: introduce "prealloc-threads" property")
> > > commit f8d426a6852c ("hostmem: default the amount of prealloc-threads to 
> > > smp-cpus")
> >
> > This is certainly a common format, but the other one is also in use.
> >
> > > > diff --git a/qapi/qom.json b/qapi/qom.json
> > > > index 30e76653ad..dfd89bc6d4 100644
> > > > --- a/qapi/qom.json
> > > > +++ b/qapi/qom.json
> > > > @@ -576,7 +576,7 @@
> > > >    #
> > > >    # @prealloc: if true, preallocate memory (default: false)
> > > >    #
> > > > -# @prealloc-threads: number of CPU threads to use for prealloc (default: 1)
> > > > +# @prealloc-threads: number of CPU threads to use for prealloc (default: 
> > > > number of CPUs) (since 5.0)
> > > >    #
> > > >    # @prealloc-context: thread context to use for creation of preallocation 
> > > > threads
> > > >    #                    (default: none) (since 7.2)
> > > >    
> > >
> > > The line seems exceeding 80 characters. It'd better to limit each line in 75 
> > > characters.
> > > So you probably need:
> > >
> > >      # @prealloc-threads: number of CPU threads to use for prealloc (default: 
> > > number of CPUs)
> > >      #                    (since 5.0)
> >
> > Still exceeds :)
> >
> > I suggested
> >
> >        # @prealloc-threads: number of CPU threads to use for prealloc
> >        #                    (default: number of CPUs) (since 5.0)

Markus's suggestion works :)

Thanks,
Gavin