Re: [PATCH REPOST v3 77/80] hostmem: introduce "prealloc-threads" property

[Igor Mammedov]

On Thu, 23 Jan 2020 12:38:42 +0100
Igor Mammedov  wrote:

> the property will allow user to specify number of threads to use
> in pre-allocation stage. It also will allow to reduce implicit
> hostmem dependency on current_machine.
> On object creation it will default to 1, but via machine
> compat property it will be updated to MachineState::smp::cpus
> to keep current behavior for hostmem and main RAM (which is
> now also hostmem based).
> 
> Signed-off-by: Igor Mammedov 

Jitendra,

As the one who introduced smp_cpus in backend

(1e356fc14be mem-prealloc: reduce large guest start-up and migration time.)

could you review this patch please?

> ---
> v3:
>   - use object_register_sugar_prop() instead of directly hacking
> compat_props (Paolo Bonzini )
>   - fix TODO description
> 
> CC: pbonz...@redhat.com
> CC: ehabk...@redhat.com
> ---
>  include/sysemu/hostmem.h |  2 ++
>  backends/hostmem.c   | 43 +++
>  vl.c | 14 ++
>  3 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 5db0d66..bdf8666 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -61,6 +61,7 @@ struct HostMemoryBackendClass {
>   * @parent: opaque parent object container
>   * @size: amount of memory backend provides
>   * @mr: MemoryRegion representing host memory belonging to backend
> + * @prealloc_threads: number of threads to be used for preallocatining RAM
>   */
>  struct HostMemoryBackend {
>  /* private */
> @@ -70,6 +71,7 @@ struct HostMemoryBackend {
>  uint64_t size;
>  bool merge, dump, use_canonical_path;
>  bool prealloc, force_prealloc, is_mapped, share;
> +uint32_t prealloc_threads;
>  DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>  HostMemPolicy policy;
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index e773bdf..0988986 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -223,7 +223,6 @@ static void host_memory_backend_set_prealloc(Object *obj, 
> bool value,
>  {
>  Error *local_err = NULL;
>  HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> -MachineState *ms = MACHINE(qdev_get_machine());
>  
>  if (backend->force_prealloc) {
>  if (value) {
> @@ -243,7 +242,7 @@ static void host_memory_backend_set_prealloc(Object *obj, 
> bool value,
>  void *ptr = memory_region_get_ram_ptr(>mr);
>  uint64_t sz = memory_region_size(>mr);
>  
> -os_mem_prealloc(fd, ptr, sz, ms->smp.cpus, _err);
> +os_mem_prealloc(fd, ptr, sz, backend->prealloc_threads, _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
>  return;
> @@ -252,14 +251,45 @@ static void host_memory_backend_set_prealloc(Object 
> *obj, bool value,
>  }
>  }
>  
> +static void host_memory_backend_get_prealloc_threads(Object *obj, Visitor *v,
> +const char *name, void *opaque, Error **errp)
> +{
> +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +visit_type_uint32(v, name, >prealloc_threads, errp);
> +}
> +
> +static void host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v,
> +const char *name, void *opaque, Error **errp)
> +{
> +HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> +Error *local_err = NULL;
> +uint32_t value;
> +
> +visit_type_uint32(v, name, , _err);
> +if (local_err) {
> +goto out;
> +}
> +if (value <= 0) {
> +error_setg(_err,
> +   "property '%s' of %s doesn't take value '%d'",
> +   name, object_get_typename(obj), value);
> +goto out;
> +}
> +backend->prealloc_threads = value;
> +out:
> +error_propagate(errp, local_err);
> +}
> +
>  static void host_memory_backend_init(Object *obj)
>  {
>  HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>  MachineState *machine = MACHINE(qdev_get_machine());
>  
> +/* TODO: convert access to globals to compat properties */
>  backend->merge = machine_mem_merge(machine);
>  backend->dump = machine_dump_guest_core(machine);
>  backend->prealloc = mem_prealloc;
> +backend->prealloc_threads = 1;
>  }
>  
>  static void host_memory_backend_post_init(Object *obj)
> @@ -313,7 +343,6 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>  {
>  HostMemoryBackend *backend = MEMORY_BACKEND(uc);
>  HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
> -MachineState *ms = MACHINE(qdev_get_machine());
>  Error *local_err = NULL;
>  void *ptr;
>  uint64_t sz;
> @@ -378,7 +407,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
> Error **errp)
>   */
>  if (backend->prealloc) {
>  os_mem_prealloc(memory_region_get_fd(>mr), ptr, sz,
> -ms->smp.cpus, _err);
> +backend->prealloc_threads, _err);
> 

[PATCH REPOST v3 77/80] hostmem: introduce "prealloc-threads" property

[Igor Mammedov]

the property will allow user to specify number of threads to use
in pre-allocation stage. It also will allow to reduce implicit
hostmem dependency on current_machine.
On object creation it will default to 1, but via machine
compat property it will be updated to MachineState::smp::cpus
to keep current behavior for hostmem and main RAM (which is
now also hostmem based).

Signed-off-by: Igor Mammedov 
---
v3:
  - use object_register_sugar_prop() instead of directly hacking
compat_props (Paolo Bonzini )
  - fix TODO description

CC: pbonz...@redhat.com
CC: ehabk...@redhat.com
---
 include/sysemu/hostmem.h |  2 ++
 backends/hostmem.c   | 43 +++
 vl.c | 14 ++
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 5db0d66..bdf8666 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -61,6 +61,7 @@ struct HostMemoryBackendClass {
  * @parent: opaque parent object container
  * @size: amount of memory backend provides
  * @mr: MemoryRegion representing host memory belonging to backend
+ * @prealloc_threads: number of threads to be used for preallocatining RAM
  */
 struct HostMemoryBackend {
 /* private */
@@ -70,6 +71,7 @@ struct HostMemoryBackend {
 uint64_t size;
 bool merge, dump, use_canonical_path;
 bool prealloc, force_prealloc, is_mapped, share;
+uint32_t prealloc_threads;
 DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
 HostMemPolicy policy;
 
diff --git a/backends/hostmem.c b/backends/hostmem.c
index e773bdf..0988986 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -223,7 +223,6 @@ static void host_memory_backend_set_prealloc(Object *obj, 
bool value,
 {
 Error *local_err = NULL;
 HostMemoryBackend *backend = MEMORY_BACKEND(obj);
-MachineState *ms = MACHINE(qdev_get_machine());
 
 if (backend->force_prealloc) {
 if (value) {
@@ -243,7 +242,7 @@ static void host_memory_backend_set_prealloc(Object *obj, 
bool value,
 void *ptr = memory_region_get_ram_ptr(>mr);
 uint64_t sz = memory_region_size(>mr);
 
-os_mem_prealloc(fd, ptr, sz, ms->smp.cpus, _err);
+os_mem_prealloc(fd, ptr, sz, backend->prealloc_threads, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
@@ -252,14 +251,45 @@ static void host_memory_backend_set_prealloc(Object *obj, 
bool value,
 }
 }
 
+static void host_memory_backend_get_prealloc_threads(Object *obj, Visitor *v,
+const char *name, void *opaque, Error **errp)
+{
+HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+visit_type_uint32(v, name, >prealloc_threads, errp);
+}
+
+static void host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v,
+const char *name, void *opaque, Error **errp)
+{
+HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+Error *local_err = NULL;
+uint32_t value;
+
+visit_type_uint32(v, name, , _err);
+if (local_err) {
+goto out;
+}
+if (value <= 0) {
+error_setg(_err,
+   "property '%s' of %s doesn't take value '%d'",
+   name, object_get_typename(obj), value);
+goto out;
+}
+backend->prealloc_threads = value;
+out:
+error_propagate(errp, local_err);
+}
+
 static void host_memory_backend_init(Object *obj)
 {
 HostMemoryBackend *backend = MEMORY_BACKEND(obj);
 MachineState *machine = MACHINE(qdev_get_machine());
 
+/* TODO: convert access to globals to compat properties */
 backend->merge = machine_mem_merge(machine);
 backend->dump = machine_dump_guest_core(machine);
 backend->prealloc = mem_prealloc;
+backend->prealloc_threads = 1;
 }
 
 static void host_memory_backend_post_init(Object *obj)
@@ -313,7 +343,6 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
Error **errp)
 {
 HostMemoryBackend *backend = MEMORY_BACKEND(uc);
 HostMemoryBackendClass *bc = MEMORY_BACKEND_GET_CLASS(uc);
-MachineState *ms = MACHINE(qdev_get_machine());
 Error *local_err = NULL;
 void *ptr;
 uint64_t sz;
@@ -378,7 +407,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
Error **errp)
  */
 if (backend->prealloc) {
 os_mem_prealloc(memory_region_get_fd(>mr), ptr, sz,
-ms->smp.cpus, _err);
+backend->prealloc_threads, _err);
 if (local_err) {
 goto out;
 }
@@ -456,6 +485,12 @@ host_memory_backend_class_init(ObjectClass *oc, void *data)
 host_memory_backend_set_prealloc, _abort);
 object_class_property_set_description(oc, "prealloc",
 "Preallocate memory", _abort);
+object_class_property_add(oc, "prealloc-threads", "int",
+host_memory_backend_get_prealloc_threads,
+host_memory_backend_set_prealloc_threads,
+NULL, NULL, _abort);
+