On Fri, 11 Nov 2022 07:47:16 +0100 Markus Armbruster <arm...@redhat.com> wrote:
> Gavin Shan <gs...@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 <zheny...@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 <gs...@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 <zheny...@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) >