From: Jaroslav Jindrak <dzejrou@gmail.com>

Prior to the introduction of the prealloc-threads property, the amount
of threads used to preallocate memory was derived from the value of
smp-cpus passed to qemu, the amount of physical cpus of the host
and a hardcoded maximum value. When the prealloc-threads property
was introduced, it included a default of 1 in backends/hostmem.c and
a default of smp-cpus using the sugar API for the property itself. The
latter default is not used when the property is not specified on qemu's
command line, so guests that were not adjusted for this change suddenly
started to use the default of 1 thread to preallocate memory, which
resulted in observable slowdowns in guest boots for guests with large
memory (e.g. when using libvirt <8.2.0 or managing guests manually).

This commit restores the original behavior for these cases while not
impacting guests started with the prealloc-threads property in any way.

Fixes: 220c1fd864e9d ("hostmem: introduce "prealloc-threads" property")
Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
---
 backends/hostmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index a7bae3d713..624bb7ecd3 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -274,7 +274,7 @@ static void host_memory_backend_init(Object *obj)
     backend->merge = machine_mem_merge(machine);
     backend->dump = machine_dump_guest_core(machine);
     backend->reserve = true;
-    backend->prealloc_threads = 1;
+    backend->prealloc_threads = machine->smp.cpus;
 }
 
 static void host_memory_backend_post_init(Object *obj)
-- 
2.30.2

On 5/17/22 14:38, dzejrou@gmail.com wrote:
> From: Jaroslav Jindrak <dzejrou@gmail.com>
> 
> Prior to the introduction of the prealloc-threads property, the amount
> of threads used to preallocate memory was derived from the value of
> smp-cpus passed to qemu, the amount of physical cpus of the host
> and a hardcoded maximum value. When the prealloc-threads property
> was introduced, it included a default of 1 in backends/hostmem.c and
> a default of smp-cpus using the sugar API for the property itself. The
> latter default is not used when the property is not specified on qemu's
> command line, so guests that were not adjusted for this change suddenly
> started to use the default of 1 thread to preallocate memory, which
> resulted in observable slowdowns in guest boots for guests with large
> memory (e.g. when using libvirt <8.2.0 or managing guests manually).
> 
> This commit restores the original behavior for these cases while not
> impacting guests started with the prealloc-threads property in any way.
> 
> Fixes: 220c1fd864e9d ("hostmem: introduce "prealloc-threads" property")
> Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
> ---
>   backends/hostmem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index a7bae3d713..624bb7ecd3 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object *obj)
>       backend->merge = machine_mem_merge(machine);
>       backend->dump = machine_dump_guest_core(machine);
>       backend->reserve = true;
> -    backend->prealloc_threads = 1;
> +    backend->prealloc_threads = machine->smp.cpus;
>   }
>   
>   static void host_memory_backend_post_init(Object *obj)

Queued, thanks.

Paolo

On Tue, 17 May 2022 20:46:50 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/17/22 14:38, dzejrou@gmail.com wrote:
> > From: Jaroslav Jindrak <dzejrou@gmail.com>
> > 
> > Prior to the introduction of the prealloc-threads property, the amount
> > of threads used to preallocate memory was derived from the value of
> > smp-cpus passed to qemu, the amount of physical cpus of the host
> > and a hardcoded maximum value. When the prealloc-threads property
> > was introduced, it included a default of 1 in backends/hostmem.c and
> > a default of smp-cpus using the sugar API for the property itself. The
> > latter default is not used when the property is not specified on qemu's
> > command line, so guests that were not adjusted for this change suddenly
> > started to use the default of 1 thread to preallocate memory, which
> > resulted in observable slowdowns in guest boots for guests with large
> > memory (e.g. when using libvirt <8.2.0 or managing guests manually).
> > 
> > This commit restores the original behavior for these cases while not
> > impacting guests started with the prealloc-threads property in any way.
> > 
> > Fixes: 220c1fd864e9d ("hostmem: introduce "prealloc-threads" property")
> > Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
> > ---
> >   backends/hostmem.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index a7bae3d713..624bb7ecd3 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object *obj)
> >       backend->merge = machine_mem_merge(machine);
> >       backend->dump = machine_dump_guest_core(machine);
> >       backend->reserve = true;
> > -    backend->prealloc_threads = 1;
> > +    backend->prealloc_threads = machine->smp.cpus;
> >   }
> >   
> >   static void host_memory_backend_post_init(Object *obj)  
> 
> Queued, thanks.

could you drop this patch pls (there is an more acceptable alternative,
see my other replies in this thread if we decide to put management policy
decisions in QEMU code).

(well unless layer violation is acceptable practice now
and it's really discouraging to do cleanup work if gets discarded)

PS:
There is no good default in this case (whatever number is picked
it could be good or bad depending on usecase).

> Paolo
>

On Wed, 2022-05-18 at 12:17 +0200, Igor Mammedov wrote:
> On Tue, 17 May 2022 20:46:50 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > index a7bae3d713..624bb7ecd3 100644
> > > --- a/backends/hostmem.c
> > > +++ b/backends/hostmem.c
> > > @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object
> > > *obj)
> > >       backend->merge = machine_mem_merge(machine);
> > >       backend->dump = machine_dump_guest_core(machine);
> > >       backend->reserve = true;
> > > -    backend->prealloc_threads = 1;
> > > +    backend->prealloc_threads = machine->smp.cpus;
> > >   }
> > >   
> > >   static void host_memory_backend_post_init(Object *obj)  
> > 
> > Queued, thanks.
> 
> PS:
> There is no good default in this case (whatever number is picked
> it could be good or bad depending on usecase).
> 
That is fair enough. What we observed, however, is that, with QEMU 5.2,
starting a 1024G VM takes ~34s.

Then you just update QEMU to > 5.2 (and don't do/changing anything
else) and the same VM now takes ~4m30s to start.

If users are managing QEMU via Libvirt *and* have _at_least_ Libvirt
8.2, they can indeed set, e.g., <allocation mode='immediate'
threads='NNN'/> (provided they can understand where the problem is, and
figure out that this is the solution).

If they have Libvirt < 8.2 (e.g., people/distros that have, say, QEMU
6.2 and Libvirt 8.0.0, or something like that), there's basically
nothing they can do... Except perhaps command line passthrough [1], but
that's really rather tricky!

So, I personally don't know where any default should be set and how,
but the above situation is not nice for users to have to handle.

[1] https://libvirt.org/kbase/qemu-passthrough-security.html

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

On Wed, May 18, 2022 at 03:02:48PM +0200, Dario Faggioli wrote:
> On Wed, 2022-05-18 at 12:17 +0200, Igor Mammedov wrote:
> > On Tue, 17 May 2022 20:46:50 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > > > index a7bae3d713..624bb7ecd3 100644
> > > > --- a/backends/hostmem.c
> > > > +++ b/backends/hostmem.c
> > > > @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object
> > > > *obj)
> > > >       backend->merge = machine_mem_merge(machine);
> > > >       backend->dump = machine_dump_guest_core(machine);
> > > >       backend->reserve = true;
> > > > -    backend->prealloc_threads = 1;
> > > > +    backend->prealloc_threads = machine->smp.cpus;
> > > >   }
> > > >   
> > > >   static void host_memory_backend_post_init(Object *obj)  
> > > 
> > > Queued, thanks.
> > 
> > PS:
> > There is no good default in this case (whatever number is picked
> > it could be good or bad depending on usecase).
> > 
> That is fair enough. What we observed, however, is that, with QEMU 5.2,
> starting a 1024G VM takes ~34s.
> 
> Then you just update QEMU to > 5.2 (and don't do/changing anything
> else) and the same VM now takes ~4m30s to start.
> 
> If users are managing QEMU via Libvirt *and* have _at_least_ Libvirt
> 8.2, they can indeed set, e.g., <allocation mode='immediate'
> threads='NNN'/> (provided they can understand where the problem is, and
> figure out that this is the solution).

I think you get the QEMU version numbers a bit mixed up based on
what i see in git history

Originally mem prellocation was single threaded and slow.

In v2.8.1 it switched to multi threaded with nthreads==vcpus

  commit 1e356fc14beaa3ece6c0e961bd479af58be3198b
  Author: Jitendra Kolhe <jitendra.kolhe@hpe.com>
  Date:   Fri Feb 24 09:01:43 2017 +0530

    mem-prealloc: reduce large guest start-up and migration time.

This applied to --mem-prealloc and --object memory-backend*,prealloc=on


In v5.0.0 the prealloc-threads property was introduced with

  commit ffac16fab33bb42f17e47624985220c1fd864e9d
  Author: Igor Mammedov <imammedo@redhat.com>
  Date:   Wed Feb 19 11:09:50 2020 -0500

    hostmem: introduce "prealloc-threads" property

This changed it so that --mem-prealloc stil uses nthreads=vcpus
but --object memory-backend,prealloc=on regressed to nthreads=1

When picking defaults there is never a perfect answer, it
is more a matter of the least-worst option.

It is pretty clear that nthreads=1 is terrible for any
large VMs. Defaulting it to nvcpus made conceptual sense
as the user has implicit said that they expect the VM to
be able to consume nvcpus worth of CPU time on the host,
so we might as well consume that allotted resource.

I struggle to come up with a compelling reason why it is
better to only use 1 single thread for preallocation. There
might be some niches where its useful but I can't see it
being the common case desirable behaviour.

Having different defaults based on how you configure it
is also especially unplesant experience.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 5/18/22 15:31, Daniel P. Berrangé wrote:
> When picking defaults there is never a perfect answer, it
> is more a matter of the least-worst option.
> 
> It is pretty clear that nthreads=1 is terrible for any
> large VMs. Defaulting it to nvcpus made conceptual sense
> as the user has implicit said that they expect the VM to
> be able to consume nvcpus worth of CPU time on the host,
> so we might as well consume that allotted resource.

I agree.  Yes, one could argue that the regression was on the libvirt 
side, but it's easier to fix it in QEMU.

If we later add the ability to create a memory backend before machine 
creation (for example with a QMP-only binary), then it's of course okay 
for those backends to use only one thread and require a manual choice 
for the # or preallocation threads.

Paolo

On Wed, 18 May 2022 16:06:47 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 5/18/22 15:31, Daniel P. Berrangé wrote:
> > When picking defaults there is never a perfect answer, it
> > is more a matter of the least-worst option.
> > 
> > It is pretty clear that nthreads=1 is terrible for any
> > large VMs. Defaulting it to nvcpus made conceptual sense
> > as the user has implicit said that they expect the VM to
> > be able to consume nvcpus worth of CPU time on the host,
> > so we might as well consume that allotted resource.

that assumes that allocation threads a permitted to actually
use all resources and not limited to 1 pcpu only and then it
also assumes 'more vcpus' => 'large RAM'.

> I agree.  Yes, one could argue that the regression was on the libvirt 
> side, but it's easier to fix it in QEMU.

libvirt already provides means to set threads number,
what needs fixing is setting up reasonable value in config
which depends on how VM is configured and constrains mgmt/host
put on it.

> If we later add the ability to create a memory backend before machine 
> creation (for example with a QMP-only binary), then it's of course okay 
> for those backends to use only one thread and require a manual choice 
> for the # or preallocation threads.

What I'm vehemently against is putting back direct machine
references into backend code. I'm fine with 'prealloc-threads'
property set from machine code (whether it's compat property or
some sugar_prop() crutch in vl.c to appease CLI users).

> 
> Paolo
> 

On Tue, 17 May 2022 14:38:58 +0200
dzejrou@gmail.com wrote:

> From: Jaroslav Jindrak <dzejrou@gmail.com>
> 
> Prior to the introduction of the prealloc-threads property, the amount
> of threads used to preallocate memory was derived from the value of
> smp-cpus passed to qemu, the amount of physical cpus of the host
> and a hardcoded maximum value. When the prealloc-threads property
> was introduced, it included a default of 1 in backends/hostmem.c and
> a default of smp-cpus using the sugar API for the property itself. The
> latter default is not used when the property is not specified on qemu's
> command line, so guests that were not adjusted for this change suddenly
> started to use the default of 1 thread to preallocate memory, which
> resulted in observable slowdowns in guest boots for guests with large
> memory (e.g. when using libvirt <8.2.0 or managing guests manually).

current behavior in QEMU is intentionally conservative. threads
number is subject to host configuration and limitations management
layer puts on it and it's not QEMU job to conjure magic numbers that
are host/workload depended.
If user needs more prealloc threads they need to specify it explicitly
for each memory backend (i.e. convince management to do it or fix your
scripts to so).

CCing Michal, as he recently looked into similar topic.

To behave it the old way you need to use legacy -mem-prealloc option.


> This commit restores the original behavior for these cases while not
> impacting guests started with the prealloc-threads property in any way.
> 
> Fixes: 220c1fd864e9d ("hostmem: introduce "prealloc-threads" property")
> Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
> ---
>  backends/hostmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index a7bae3d713..624bb7ecd3 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object *obj)
>      backend->merge = machine_mem_merge(machine);
>      backend->dump = machine_dump_guest_core(machine);
>      backend->reserve = true;
> -    backend->prealloc_threads = 1;
> +    backend->prealloc_threads = machine->smp.cpus;
pls, do not add more dependencies to random external objects to memory backends.

If you have to do that, use machine compat properties instead, but then
the essence of the issue stays the same (user shall define optimal threads
number and provide it to qemu explicitly)

>  }
>  
>  static void host_memory_backend_post_init(Object *obj)

On Tue, May 17, 2022 at 05:12:28PM +0200, Igor Mammedov wrote:
> On Tue, 17 May 2022 14:38:58 +0200
> dzejrou@gmail.com wrote:
> 
> > From: Jaroslav Jindrak <dzejrou@gmail.com>
> > 
> > Prior to the introduction of the prealloc-threads property, the amount
> > of threads used to preallocate memory was derived from the value of
> > smp-cpus passed to qemu, the amount of physical cpus of the host
> > and a hardcoded maximum value. When the prealloc-threads property
> > was introduced, it included a default of 1 in backends/hostmem.c and
> > a default of smp-cpus using the sugar API for the property itself. The
> > latter default is not used when the property is not specified on qemu's
> > command line, so guests that were not adjusted for this change suddenly
> > started to use the default of 1 thread to preallocate memory, which
> > resulted in observable slowdowns in guest boots for guests with large
> > memory (e.g. when using libvirt <8.2.0 or managing guests manually).
> 
> current behavior in QEMU is intentionally conservative. threads
> number is subject to host configuration and limitations management
> layer puts on it and it's not QEMU job to conjure magic numbers that
> are host/workload depended.

I think that's missing the point. QEMU *did* historically set the
prealloc threads equal to num CPUs, so we have precedent here. The
referenced commit lost that behaviour because it only wired up the
defaults in one particular CLI scenario. That's a clear regression
on QEMU's side.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Tue, 17 May 2022 17:33:54 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, May 17, 2022 at 05:12:28PM +0200, Igor Mammedov wrote:
> > On Tue, 17 May 2022 14:38:58 +0200
> > dzejrou@gmail.com wrote:
> >   
> > > From: Jaroslav Jindrak <dzejrou@gmail.com>
> > > 
> > > Prior to the introduction of the prealloc-threads property, the amount
> > > of threads used to preallocate memory was derived from the value of
> > > smp-cpus passed to qemu, the amount of physical cpus of the host
> > > and a hardcoded maximum value. When the prealloc-threads property
> > > was introduced, it included a default of 1 in backends/hostmem.c and
> > > a default of smp-cpus using the sugar API for the property itself. The
> > > latter default is not used when the property is not specified on qemu's
> > > command line, so guests that were not adjusted for this change suddenly
> > > started to use the default of 1 thread to preallocate memory, which
> > > resulted in observable slowdowns in guest boots for guests with large
> > > memory (e.g. when using libvirt <8.2.0 or managing guests manually).  
> > 
> > current behavior in QEMU is intentionally conservative. threads
> > number is subject to host configuration and limitations management
> > layer puts on it and it's not QEMU job to conjure magic numbers that
> > are host/workload depended.  
> 
> I think that's missing the point. QEMU *did* historically set the
> prealloc threads equal to num CPUs, so we have precedent here. The
> referenced commit lost that behaviour because it only wired up the
> defaults in one particular CLI scenario. That's a clear regression
> on QEMU's side.

commit preserved behavior with legacy options to reduce disturbance.
Considering that users will have update QEMU which most likely includes
newer libvirt as well which lets configure number of allocation threads [1].
Users should fix configuration rather then pulling back layer violation hacks
(precedent is not a good excuse wrt reluctance to adapt to new behavior)

1) https://libvirt.org/formatdomain.html#elementsMemoryAllocation

> 
> With regards,
> Daniel

On Tue, 17 May 2022 at 17:12, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Tue, 17 May 2022 14:38:58 +0200
> dzejrou@gmail.com wrote:
>
> > From: Jaroslav Jindrak <dzejrou@gmail.com>
> >
> > Prior to the introduction of the prealloc-threads property, the amount
> > of threads used to preallocate memory was derived from the value of
> > smp-cpus passed to qemu, the amount of physical cpus of the host
> > and a hardcoded maximum value. When the prealloc-threads property
> > was introduced, it included a default of 1 in backends/hostmem.c and
> > a default of smp-cpus using the sugar API for the property itself. The
> > latter default is not used when the property is not specified on qemu's
> > command line, so guests that were not adjusted for this change suddenly
> > started to use the default of 1 thread to preallocate memory, which
> > resulted in observable slowdowns in guest boots for guests with large
> > memory (e.g. when using libvirt <8.2.0 or managing guests manually).
>
> current behavior in QEMU is intentionally conservative. threads
> number is subject to host configuration and limitations management
> layer puts on it and it's not QEMU job to conjure magic numbers that
> are host/workload depended.
> If user needs more prealloc threads they need to specify it explicitly
> for each memory backend (i.e. convince management to do it or fix your
> scripts to so).

I understand that, but I'd say that a sudden change of the default to
1, which can
lead to guest boot slowdowns of about 2-8 times (and boots taking several
minutes) because of a change that was not very well documented (to the point
that it took libvirt about 2 years [0] to add support for this
property) does qualify
as a regression. After all, this patch does not conjure any new
numbers, but rather
restores the original behavior of qemu if the user does _not_ use the new
prealloc-threads property.

[0] https://github.com/libvirt/libvirt/commit/ba7f98126fa84d354ce72929b77cc111a9a557a9

> CCing Michal, as he recently looked into similar topic.
>
> To behave it the old way you need to use legacy -mem-prealloc option.
>
>
> > This commit restores the original behavior for these cases while not
> > impacting guests started with the prealloc-threads property in any way.
> >
> > Fixes: 220c1fd864e9d ("hostmem: introduce "prealloc-threads" property")
> > Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
> > ---
> >  backends/hostmem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index a7bae3d713..624bb7ecd3 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -274,7 +274,7 @@ static void host_memory_backend_init(Object *obj)
> >      backend->merge = machine_mem_merge(machine);
> >      backend->dump = machine_dump_guest_core(machine);
> >      backend->reserve = true;
> > -    backend->prealloc_threads = 1;
> > +    backend->prealloc_threads = machine->smp.cpus;
> pls, do not add more dependencies to random external objects to memory backends.
>
> If you have to do that, use machine compat properties instead, but then
> the essence of the issue stays the same (user shall define optimal threads
> number and provide it to qemu explicitly)
>
> >  }
> >
> >  static void host_memory_backend_post_init(Object *obj)
>