diff mbox series

[RFC,1/2] softmmu/memory: add missing begin/commit callback calls

Message ID 20220816101250.1715523-2-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series accel/kvm: extend kvm memory listener to support | expand

Commit Message

Emanuele Giuseppe Esposito Aug. 16, 2022, 10:12 a.m. UTC
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(+)

Comments

Peter Xu Aug. 18, 2022, 7:34 p.m. UTC | #1
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,
Emanuele Giuseppe Esposito Aug. 26, 2022, 1:53 p.m. UTC | #2
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,
>
Peter Xu Aug. 26, 2022, 2:13 p.m. UTC | #3
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,
Peter Xu Aug. 27, 2022, 9:03 p.m. UTC | #4
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.
Emanuele Giuseppe Esposito Sept. 9, 2022, 8:02 a.m. UTC | #5
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 mbox series

Patch

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)