Re: [Qemu-devel] [PATCH qemu v16 02/19] memory: Call region_del() callbacks on memory listener unregistering

From: David Gibson <david@gibson.dropbear.id.au>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
	Alexander Graf <agraf@suse.de>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH qemu v16 02/19] memory: Call region_del() callbacks on memory listener unregistering
Date: Thu, 26 May 2016 11:48:44 +1000	[thread overview]
Message-ID: <20160526014844.GU17226@voom.fritz.box> (raw)
In-Reply-To: <20160505164504.0f9d3b03@t450s.home>

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]