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

[Zhenyu Zhang]

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 <gshan@redhat.com> wrote:
>
> On 11/11/22 6:54 PM, Igor Mammedov wrote:
> > On Fri, 11 Nov 2022 17:34:04 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >> 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
> >
> > 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 <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
>