| Message ID | 20241107102126.2183152-7-william.roche@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hugetlbfs memory HW error fixes | expand |
On 07.11.24 11:21, “William Roche wrote: > From: David Hildenbrand <david@redhat.com> > > Let's register a RAM block notifier and react on remap notifications. > Simply re-apply the settings. Warn only when something goes wrong. > > Note: qemu_ram_remap() will not remap when RAM_PREALLOC is set. Could be > that hostmem is still missing to update that flag ... > > Signed-off-by: David Hildenbrand <david@redhat.com> > Signed-off-by: William Roche <william.roche@oracle.com> > --- > backends/hostmem.c | 29 +++++++++++++++++++++++++++++ > include/sysemu/hostmem.h | 1 + > 2 files changed, 30 insertions(+) > > diff --git a/backends/hostmem.c b/backends/hostmem.c > index bf85d716e5..fbd8708664 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -361,11 +361,32 @@ static void host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v, > backend->prealloc_threads = value; > } > > +static void host_memory_backend_ram_remapped(RAMBlockNotifier *n, void *host, > + size_t offset, size_t size) > +{ > + HostMemoryBackend *backend = container_of(n, HostMemoryBackend, > + ram_notifier); > + Error *err = NULL; > + > + if (!host_memory_backend_mr_inited(backend) || > + memory_region_get_ram_ptr(&backend->mr) != host) { > + return; > + } > + > + host_memory_backend_apply_settings(backend, host + offset, size, &err); > + if (err) { > + warn_report_err(err); I wonder if we want to fail hard instead, or have a way to tell the notifier that something wen wrong.
On 11/12/24 14:45, David Hildenbrand wrote: > On 07.11.24 11:21, “William Roche wrote: >> From: David Hildenbrand <david@redhat.com> >> >> Let's register a RAM block notifier and react on remap notifications. >> Simply re-apply the settings. Warn only when something goes wrong. >> >> Note: qemu_ram_remap() will not remap when RAM_PREALLOC is set. Could be >> that hostmem is still missing to update that flag ... >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> Signed-off-by: William Roche <william.roche@oracle.com> >> --- >> backends/hostmem.c | 29 +++++++++++++++++++++++++++++ >> include/sysemu/hostmem.h | 1 + >> 2 files changed, 30 insertions(+) >> >> diff --git a/backends/hostmem.c b/backends/hostmem.c >> index bf85d716e5..fbd8708664 100644 >> --- a/backends/hostmem.c >> +++ b/backends/hostmem.c >> @@ -361,11 +361,32 @@ static void >> host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v, >> backend->prealloc_threads = value; >> } >> +static void host_memory_backend_ram_remapped(RAMBlockNotifier *n, >> void *host, >> + size_t offset, size_t size) >> +{ >> + HostMemoryBackend *backend = container_of(n, HostMemoryBackend, >> + ram_notifier); >> + Error *err = NULL; >> + >> + if (!host_memory_backend_mr_inited(backend) || >> + memory_region_get_ram_ptr(&backend->mr) != host) { >> + return; >> + } >> + >> + host_memory_backend_apply_settings(backend, host + offset, size, >> &err); >> + if (err) { >> + warn_report_err(err); > > I wonder if we want to fail hard instead, or have a way to tell the > notifier that something wen wrong. > It depends on what the caller would do with this information. Is there a way to workaround the problem ? (I don't think so) Can the VM continue to run without doing anything about it ? (Maybe?) Currently all numa notifiers don't return errors. This function is only called from ram_block_notify_remap() in qemu_ram_remap(), I would vote for a "fail hard" in case where the settings are mandatory to continue. HTH.
On 12.11.24 19:17, William Roche wrote: > On 11/12/24 14:45, David Hildenbrand wrote: >> On 07.11.24 11:21, “William Roche wrote: >>> From: David Hildenbrand <david@redhat.com> >>> >>> Let's register a RAM block notifier and react on remap notifications. >>> Simply re-apply the settings. Warn only when something goes wrong. >>> >>> Note: qemu_ram_remap() will not remap when RAM_PREALLOC is set. Could be >>> that hostmem is still missing to update that flag ... >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> Signed-off-by: William Roche <william.roche@oracle.com> >>> --- >>> backends/hostmem.c | 29 +++++++++++++++++++++++++++++ >>> include/sysemu/hostmem.h | 1 + >>> 2 files changed, 30 insertions(+) >>> >>> diff --git a/backends/hostmem.c b/backends/hostmem.c >>> index bf85d716e5..fbd8708664 100644 >>> --- a/backends/hostmem.c >>> +++ b/backends/hostmem.c >>> @@ -361,11 +361,32 @@ static void >>> host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v, >>> backend->prealloc_threads = value; >>> } >>> +static void host_memory_backend_ram_remapped(RAMBlockNotifier *n, >>> void *host, >>> + size_t offset, size_t size) >>> +{ >>> + HostMemoryBackend *backend = container_of(n, HostMemoryBackend, >>> + ram_notifier); >>> + Error *err = NULL; >>> + >>> + if (!host_memory_backend_mr_inited(backend) || >>> + memory_region_get_ram_ptr(&backend->mr) != host) { >>> + return; >>> + } >>> + >>> + host_memory_backend_apply_settings(backend, host + offset, size, >>> &err); >>> + if (err) { >>> + warn_report_err(err); >> >> I wonder if we want to fail hard instead, or have a way to tell the >> notifier that something wen wrong. >> > > It depends on what the caller would do with this information. Is there a > way to workaround the problem ? (I don't think so) Primarily only preallocation will fail, and that ... > Can the VM continue to run without doing anything about it ? (Maybe?) > ... will make crash the QEMU at some point later (SIGBUS), which is very bad. > Currently all numa notifiers don't return errors. > > This function is only called from ram_block_notify_remap() in > qemu_ram_remap(), I would vote for a "fail hard" in case where the > settings are mandatory to continue. "fail hard" is likely the best approach for now.
diff --git a/backends/hostmem.c b/backends/hostmem.c index bf85d716e5..fbd8708664 100644 --- a/backends/hostmem.c +++ b/backends/hostmem.c @@ -361,11 +361,32 @@ static void host_memory_backend_set_prealloc_threads(Object *obj, Visitor *v, backend->prealloc_threads = value; } +static void host_memory_backend_ram_remapped(RAMBlockNotifier *n, void *host, + size_t offset, size_t size) +{ + HostMemoryBackend *backend = container_of(n, HostMemoryBackend, + ram_notifier); + Error *err = NULL; + + if (!host_memory_backend_mr_inited(backend) || + memory_region_get_ram_ptr(&backend->mr) != host) { + return; + } + + host_memory_backend_apply_settings(backend, host + offset, size, &err); + if (err) { + warn_report_err(err); + } +} + static void host_memory_backend_init(Object *obj) { HostMemoryBackend *backend = MEMORY_BACKEND(obj); MachineState *machine = MACHINE(qdev_get_machine()); + backend->ram_notifier.ram_block_remapped = host_memory_backend_ram_remapped; + ram_block_notifier_add(&backend->ram_notifier); + /* TODO: convert access to globals to compat properties */ backend->merge = machine_mem_merge(machine); backend->dump = machine_dump_guest_core(machine); @@ -379,6 +400,13 @@ static void host_memory_backend_post_init(Object *obj) object_apply_compat_props(obj); } +static void host_memory_backend_finalize(Object *obj) +{ + HostMemoryBackend *backend = MEMORY_BACKEND(obj); + + ram_block_notifier_remove(&backend->ram_notifier); +} + bool host_memory_backend_mr_inited(HostMemoryBackend *backend) { /* @@ -595,6 +623,7 @@ static const TypeInfo host_memory_backend_info = { .instance_size = sizeof(HostMemoryBackend), .instance_init = host_memory_backend_init, .instance_post_init = host_memory_backend_post_init, + .instance_finalize = host_memory_backend_finalize, .interfaces = (InterfaceInfo[]) { { TYPE_USER_CREATABLE }, { } diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h index de47ae59e4..062a68c8fc 100644 --- a/include/sysemu/hostmem.h +++ b/include/sysemu/hostmem.h @@ -81,6 +81,7 @@ struct HostMemoryBackend { HostMemPolicy policy; MemoryRegion mr; + RAMBlockNotifier ram_notifier; }; bool host_memory_backend_mr_inited(HostMemoryBackend *backend);