[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: |
Markus Armbruster |
Subject: |
Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix |
Date: |
Fri, 11 Nov 2022 07:47:16 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
Gavin Shan <gshan@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 <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?
> 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)
- [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Zhenyu Zhang, 2022/11/10
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Gavin Shan, 2022/11/10
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix,
Markus Armbruster <=
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Igor Mammedov, 2022/11/11
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Gavin Shan, 2022/11/11
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Igor Mammedov, 2022/11/11
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Gavin Shan, 2022/11/11
- Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix, Zhenyu Zhang, 2022/11/14