On Fri, 11 Nov 2022 17:34:04 +0800 Gavin Shan <gs...@redhat.com> wrote:
> On 11/11/22 5:13 PM, Igor Mammedov wrote: > > On Fri, 11 Nov 2022 07:47:16 +0100 > > Markus Armbruster <arm...@redhat.com> wrote: > >> Gavin Shan <gs...@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 <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. > > > > 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 default was not supposed to be consistent with legacy mem-prealloc and sugar property takes care of mem-prealloc=on case. so commit message in its current form looks fine to me. > >> > >>> 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) > >> > > > > Markus's suggestion works :) > > Thanks, > Gavin > >