| Message ID | 20220816101250.1715523-2-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | accel/kvm: extend kvm memory listener to support | expand |
On Tue, Aug 16, 2022 at 06:12:49AM -0400, Emanuele Giuseppe Esposito wrote: > kvm listeners now need ->commit callback in order to actually send > the ioctl to the hypervisor. Therefore, add missing callers around > address_space_set_flatview(), which in turn calls > address_space_update_topology_pass() which calls ->region_* and > ->log_* callbacks. > > Using MEMORY_LISTENER_CALL_GLOBAL is a little bit an overkill, > but it is harmless, considering that other listeners that are not > invoked in address_space_update_topology_pass() won't do anything, > since they won't have anything to commit. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > softmmu/memory.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 7ba2048836..1afd3f9703 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1076,7 +1076,9 @@ static void address_space_update_topology(AddressSpace *as) > if (!g_hash_table_lookup(flat_views, physmr)) { > generate_memory_topology(physmr); > } > + MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); > address_space_set_flatview(as); > + MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); Should the pair be with MEMORY_LISTENER_CALL() rather than the global version? Since it's only updating one address space. Besides the perf implication (walking per-as list should be faster than walking global memory listener list?), I think it feels broken too since we'll call begin() then commit() (with no region_add()/region_del()/..) for all the listeners that are not registered against this AS. IIUC it will empty all regions with those listeners? Thanks,
Am 18/08/2022 um 21:34 schrieb Peter Xu: > On Tue, Aug 16, 2022 at 06:12:49AM -0400, Emanuele Giuseppe Esposito wrote: >> kvm listeners now need ->commit callback in order to actually send >> the ioctl to the hypervisor. Therefore, add missing callers around >> address_space_set_flatview(), which in turn calls >> address_space_update_topology_pass() which calls ->region_* and >> ->log_* callbacks. >> >> Using MEMORY_LISTENER_CALL_GLOBAL is a little bit an overkill, >> but it is harmless, considering that other listeners that are not >> invoked in address_space_update_topology_pass() won't do anything, >> since they won't have anything to commit. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> softmmu/memory.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 7ba2048836..1afd3f9703 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1076,7 +1076,9 @@ static void address_space_update_topology(AddressSpace *as) >> if (!g_hash_table_lookup(flat_views, physmr)) { >> generate_memory_topology(physmr); >> } >> + MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); >> address_space_set_flatview(as); >> + MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); > > Should the pair be with MEMORY_LISTENER_CALL() rather than the global > version? Since it's only updating one address space. Ideally yes, we want to call the memory listener only for this address space. Practically I don't know how to do it, as MEMORY_LISTENER_CALL 1) takes additional parameters like memory region section, and 2) calls _listener->_callback(_listener, _section, ##_args) whereas begin and commit need (_listener, ##args) only, which is what MEMORY_LISTENER_CALL_GLOBAL does. > > Besides the perf implication (walking per-as list should be faster than > walking global memory listener list?), I think it feels broken too since > we'll call begin() then commit() (with no region_add()/region_del()/..) for > all the listeners that are not registered against this AS. IIUC it will > empty all regions with those listeners? What do you mean "will empty all regions with those listeners"? But yes theoretically vhost-vdpa and physmem have commit callbacks that are independent from whether region_add or other callbacks have been called. For kvm and probably vhost it would be no problem, since there won't be any list to iterate on. I'll implement a new macro to handle this. Emanuele > > Thanks, >
On Fri, Aug 26, 2022 at 03:53:09PM +0200, Emanuele Giuseppe Esposito wrote: > What do you mean "will empty all regions with those listeners"? > But yes theoretically vhost-vdpa and physmem have commit callbacks that > are independent from whether region_add or other callbacks have been called. > For kvm and probably vhost it would be no problem, since there won't be > any list to iterate on. Right, begin()/commit() is for address space update, so it should be fine to have nothing to commit, sorry. > > I'll implement a new macro to handle this. Sounds good. Thanks,
On Fri, Aug 26, 2022 at 10:13:47AM -0400, Peter Xu wrote: > On Fri, Aug 26, 2022 at 03:53:09PM +0200, Emanuele Giuseppe Esposito wrote: > > What do you mean "will empty all regions with those listeners"? > > But yes theoretically vhost-vdpa and physmem have commit callbacks that > > are independent from whether region_add or other callbacks have been called. > > For kvm and probably vhost it would be no problem, since there won't be > > any list to iterate on. > > Right, begin()/commit() is for address space update, so it should be fine > to have nothing to commit, sorry. Hold on.. When I was replying to your patch 2 and reading the code around, I fount that this patch does affect vhost.. see region_nop() hook and also vhost's version of vhost_region_addnop(). I think vhost will sync its memory layout for each of the commit(), and any newly created AS could emptify vhost memory list even if registered on address_space_memory. The other thing is address_space_update_topology() seems to be only used by address_space_init(). It means I don't think there should have any listener registered to this AS anyway.. :) So iiuc this patch (even if converting to loop over per-as memory listeners) is not needed.
Am 27/08/2022 um 23:03 schrieb Peter Xu: > On Fri, Aug 26, 2022 at 10:13:47AM -0400, Peter Xu wrote: >> On Fri, Aug 26, 2022 at 03:53:09PM +0200, Emanuele Giuseppe Esposito wrote: >>> What do you mean "will empty all regions with those listeners"? >>> But yes theoretically vhost-vdpa and physmem have commit callbacks that >>> are independent from whether region_add or other callbacks have been called. >>> For kvm and probably vhost it would be no problem, since there won't be >>> any list to iterate on. >> >> Right, begin()/commit() is for address space update, so it should be fine >> to have nothing to commit, sorry. > > Hold on.. > > When I was replying to your patch 2 and reading the code around, I fount > that this patch does affect vhost.. see region_nop() hook and also vhost's > version of vhost_region_addnop(). I think vhost will sync its memory > layout for each of the commit(), and any newly created AS could emptify > vhost memory list even if registered on address_space_memory. > > The other thing is address_space_update_topology() seems to be only used by > address_space_init(). It means I don't think there should have any > listener registered to this AS anyway.. :) So iiuc this patch (even if > converting to loop over per-as memory listeners) is not needed. > Agree, dropping this patch :) Emanuele
diff --git a/softmmu/memory.c b/softmmu/memory.c index 7ba2048836..1afd3f9703 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1076,7 +1076,9 @@ static void address_space_update_topology(AddressSpace *as) if (!g_hash_table_lookup(flat_views, physmr)) { generate_memory_topology(physmr); } + MEMORY_LISTENER_CALL_GLOBAL(begin, Forward); address_space_set_flatview(as); + MEMORY_LISTENER_CALL_GLOBAL(commit, Forward); } void memory_region_transaction_begin(void)
kvm listeners now need ->commit callback in order to actually send the ioctl to the hypervisor. Therefore, add missing callers around address_space_set_flatview(), which in turn calls address_space_update_topology_pass() which calls ->region_* and ->log_* callbacks. Using MEMORY_LISTENER_CALL_GLOBAL is a little bit an overkill, but it is harmless, considering that other listeners that are not invoked in address_space_update_topology_pass() won't do anything, since they won't have anything to commit. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- softmmu/memory.c | 2 ++ 1 file changed, 2 insertions(+)