* [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix @ 2022-11-11 3:05 Zhenyu Zhang 2022-11-11 4:58 ` Gavin Shan 0 siblings, 1 reply; 8+ messages in thread From: Zhenyu Zhang @ 2022-11-11 3:05 UTC (permalink / raw) To: qemu-devel Cc: armbru, philmd, shan.gavin, eric.auger, imammedo, zhenyzha, lijin 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 3:05 [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix Zhenyu Zhang @ 2022-11-11 4:58 ` Gavin Shan 2022-11-11 6:47 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Gavin Shan @ 2022-11-11 4:58 UTC (permalink / raw) To: Zhenyu Zhang, qemu-devel Cc: armbru, philmd, shan.gavin, eric.auger, imammedo, lijin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 4:58 ` Gavin Shan @ 2022-11-11 6:47 ` Markus Armbruster 2022-11-11 9:13 ` Igor Mammedov 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2022-11-11 6:47 UTC (permalink / raw) To: Gavin Shan Cc: Zhenyu Zhang, qemu-devel, philmd, shan.gavin, eric.auger, imammedo, lijin 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 6:47 ` Markus Armbruster @ 2022-11-11 9:13 ` Igor Mammedov 2022-11-11 9:34 ` Gavin Shan 0 siblings, 1 reply; 8+ messages in thread From: Igor Mammedov @ 2022-11-11 9:13 UTC (permalink / raw) To: Markus Armbruster Cc: Gavin Shan, Zhenyu Zhang, qemu-devel, philmd, shan.gavin, eric.auger, lijin 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) > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 9:13 ` Igor Mammedov @ 2022-11-11 9:34 ` Gavin Shan 2022-11-11 10:54 ` Igor Mammedov 0 siblings, 1 reply; 8+ messages in thread From: Gavin Shan @ 2022-11-11 9:34 UTC (permalink / raw) To: Igor Mammedov, Markus Armbruster Cc: Zhenyu Zhang, qemu-devel, philmd, shan.gavin, eric.auger, lijin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 9:34 ` Gavin Shan @ 2022-11-11 10:54 ` Igor Mammedov 2022-11-11 22:05 ` Gavin Shan 0 siblings, 1 reply; 8+ messages in thread From: Igor Mammedov @ 2022-11-11 10:54 UTC (permalink / raw) To: Gavin Shan Cc: Markus Armbruster, Zhenyu Zhang, qemu-devel, philmd, shan.gavin, eric.auger, lijin 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 10:54 ` Igor Mammedov @ 2022-11-11 22:05 ` Gavin Shan 2022-11-14 1:27 ` Zhenyu Zhang 0 siblings, 1 reply; 8+ messages in thread From: Gavin Shan @ 2022-11-11 22:05 UTC (permalink / raw) To: Igor Mammedov Cc: Markus Armbruster, Zhenyu Zhang, qemu-devel, philmd, shan.gavin, eric.auger, lijin 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix 2022-11-11 22:05 ` Gavin Shan @ 2022-11-14 1:27 ` Zhenyu Zhang 0 siblings, 0 replies; 8+ messages in thread From: Zhenyu Zhang @ 2022-11-14 1:27 UTC (permalink / raw) To: Gavin Shan Cc: Igor Mammedov, Markus Armbruster, qemu-devel, philmd, shan.gavin, eric.auger, lijin 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-14 23:35 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-11 3:05 [PATCH v3] qapi/qom: Memory backend property prealloc-threads doc fix Zhenyu Zhang 2022-11-11 4:58 ` Gavin Shan 2022-11-11 6:47 ` Markus Armbruster 2022-11-11 9:13 ` Igor Mammedov 2022-11-11 9:34 ` Gavin Shan 2022-11-11 10:54 ` Igor Mammedov 2022-11-11 22:05 ` Gavin Shan 2022-11-14 1:27 ` Zhenyu Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).