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

[Gavin Shan <gshan@redhat.com>]

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