| Message ID | 1462344751-28281-3-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 4 May 2016 16:52:14 +1000 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > When a new memory listener is registered, listener_add_address_space() > is called and which in turn calls region_add() callbacks of memory regions. > However when unregistering the memory listener, it is just removed from > the listening chain and no region_del() is called. > > This adds listener_del_address_space() and uses it in > memory_listener_unregister(). listener_add_address_space() was used as > a template with the following changes: > s/log_global_start/log_global_stop/ > s/log_start/log_stop/ > s/region_add/region_del/ > > This will allow the following patches to add/remove DMA windows > dynamically from VFIO's PCI address space's region_add()/region_del(). Following patch 1 comments, it would be a bug if the kernel actually needed this to do cleanup, we must release everything if QEMU gets shot with a SIGKILL anyway. So what does this cleanup facilitate in QEMU? Having QEMU trigger an unmap for each region_del is not going to be as efficient as just dropping the container and letting the kernel handle the cleanup all in one go. Thanks, Alex > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > memory.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/memory.c b/memory.c > index f76f85d..f762a34 100644 > --- a/memory.c > +++ b/memory.c > @@ -2185,6 +2185,49 @@ static void listener_add_address_space(MemoryListener *listener, > flatview_unref(view); > } > > +static void listener_del_address_space(MemoryListener *listener, > + AddressSpace *as) > +{ > + FlatView *view; > + FlatRange *fr; > + > + if (listener->address_space_filter > + && listener->address_space_filter != as) { > + return; > + } > + > + if (listener->begin) { > + listener->begin(listener); > + } > + if (global_dirty_log) { > + if (listener->log_global_stop) { > + listener->log_global_stop(listener); > + } > + } > + > + view = address_space_get_flatview(as); > + FOR_EACH_FLAT_RANGE(fr, view) { > + MemoryRegionSection section = { > + .mr = fr->mr, > + .address_space = as, > + .offset_within_region = fr->offset_in_region, > + .size = fr->addr.size, > + .offset_within_address_space = int128_get64(fr->addr.start), > + .readonly = fr->readonly, > + }; > + if (fr->dirty_log_mask && listener->log_stop) { > + listener->log_stop(listener, §ion, 0, fr->dirty_log_mask); > + } > + if (listener->region_del) { > + listener->region_del(listener, §ion); > + } > + } > + if (listener->commit) { > + listener->commit(listener); > + } > + flatview_unref(view); > +} > + > void memory_listener_register(MemoryListener *listener, AddressSpace *filter) > { > MemoryListener *other = NULL; > @@ -2211,6 +2254,11 @@ void memory_listener_register(MemoryListener *listener, AddressSpace *filter) > > void memory_listener_unregister(MemoryListener *listener) > { > + AddressSpace *as; > + > + QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { > + listener_del_address_space(listener, as); > + } > QTAILQ_REMOVE(&memory_listeners, listener, link); > } >
On Thu, May 05, 2016 at 04:45:04PM -0600, Alex Williamson wrote: > On Wed, 4 May 2016 16:52:14 +1000 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > When a new memory listener is registered, listener_add_address_space() > > is called and which in turn calls region_add() callbacks of memory regions. > > However when unregistering the memory listener, it is just removed from > > the listening chain and no region_del() is called. > > > > This adds listener_del_address_space() and uses it in > > memory_listener_unregister(). listener_add_address_space() was used as > > a template with the following changes: > > s/log_global_start/log_global_stop/ > > s/log_start/log_stop/ > > s/region_add/region_del/ > > > > This will allow the following patches to add/remove DMA windows > > dynamically from VFIO's PCI address space's region_add()/region_del(). > > Following patch 1 comments, it would be a bug if the kernel actually > needed this to do cleanup, we must release everything if QEMU gets shot > with a SIGKILL anyway. So what does this cleanup facilitate in QEMU? > Having QEMU trigger an unmap for each region_del is not going to be as > efficient as just dropping the container and letting the kernel handle > the cleanup all in one go. Thanks, So, what the kernel does is kind of a red herring, because that's only relevant to the specific case of the VFIO listener, whereas this is a change to the behaviour of all memory listeners. It seems plausible that some memory listeners could have a legitimate reason to want clean up region_del calls when unregistered. But, we know this could be expensive for other listeners, so I don't think we should make that behaviour standard. So I'd be thinking either a special unregister_with_delete() call, or a standalone "delete all" helper function. Assuming this is still needed at all, once the other changes to the reference counting we've discussed have been done.
diff --git a/memory.c b/memory.c index f76f85d..f762a34 100644 --- a/memory.c +++ b/memory.c @@ -2185,6 +2185,49 @@ static void listener_add_address_space(MemoryListener *listener, flatview_unref(view); } +static void listener_del_address_space(MemoryListener *listener, + AddressSpace *as) +{ + FlatView *view; + FlatRange *fr; + + if (listener->address_space_filter + && listener->address_space_filter != as) { + return; + } + + if (listener->begin) { + listener->begin(listener); + } + if (global_dirty_log) { + if (listener->log_global_stop) { + listener->log_global_stop(listener); + } + } + + view = address_space_get_flatview(as); + FOR_EACH_FLAT_RANGE(fr, view) { + MemoryRegionSection section = { + .mr = fr->mr, + .address_space = as, + .offset_within_region = fr->offset_in_region, + .size = fr->addr.size, + .offset_within_address_space = int128_get64(fr->addr.start), + .readonly = fr->readonly, + }; + if (fr->dirty_log_mask && listener->log_stop) { + listener->log_stop(listener, §ion, 0, fr->dirty_log_mask); + } + if (listener->region_del) { + listener->region_del(listener, §ion); + } + } + if (listener->commit) { + listener->commit(listener); + } + flatview_unref(view); +} + void memory_listener_register(MemoryListener *listener, AddressSpace *filter) { MemoryListener *other = NULL; @@ -2211,6 +2254,11 @@ void memory_listener_register(MemoryListener *listener, AddressSpace *filter) void memory_listener_unregister(MemoryListener *listener) { + AddressSpace *as; + + QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) { + listener_del_address_space(listener, as); + } QTAILQ_REMOVE(&memory_listeners, listener, link); }
When a new memory listener is registered, listener_add_address_space() is called and which in turn calls region_add() callbacks of memory regions. However when unregistering the memory listener, it is just removed from the listening chain and no region_del() is called. This adds listener_del_address_space() and uses it in memory_listener_unregister(). listener_add_address_space() was used as a template with the following changes: s/log_global_start/log_global_stop/ s/log_start/log_stop/ s/region_add/region_del/ This will allow the following patches to add/remove DMA windows dynamically from VFIO's PCI address space's region_add()/region_del(). Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- memory.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)