[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 11:54:43 +0100 |
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.
> >>
> >>> 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
>
>