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

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

Posted by Zhenyu Zhang 1 year, 5 months ago

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(-)

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)
-- 
2.31.1

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

Posted by Gavin Shan 1 year, 5 months ago

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.
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")

> 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)

Thanks,
Gavin

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

Posted by Markus Armbruster 1 year, 5 months ago

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)

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

Posted by Igor Mammedov 1 year, 5 months ago

On Fri, 11 Nov 2022 07:47:16 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> 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?

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.

> 
> > 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)
>

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

Posted by Gavin Shan 1 year, 5 months ago

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

>>
>>> 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

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

Posted by Igor Mammedov 1 year, 5 months ago

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
> 
>

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

Posted by Gavin Shan 1 year, 5 months ago

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

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

Posted by Zhenyu Zhang 1 year, 5 months ago

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
>