[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fi
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix |
Date: |
Fri, 11 Nov 2022 10:13:10 +0100 |
On Fri, 11 Nov 2022 07:47:16 +0100
Markus Armbruster <armbru@redhat.com> wrote:
> Gavin Shan <gshan@redhat.com> writes:
>
> > Hi Zhenyu,
> >
> > 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.
>
> > 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)
>