Thanks all, I will send v4 patch to fix the 80 characters limitation issue.
On Sat, Nov 12, 2022 at 6:05 AM Gavin Shan <gs...@redhat.com> wrote: > > On 11/11/22 6:54 PM, Igor Mammedov wrote: > > 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. > > > > Ok, thanks for your confirm. I think Zhenyu needs to post v4, to fix > the 80 characters limitation issue. My reviewed-by is still valid. > > >>>> > >>>>> 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 >