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@xxxxxxxxxx>
>> ---
>> 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,
>
