Re: [PATCH v2 2/2] IOREQ: refine when to send mapcache invalidation request

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2021 10:26, Paul Durrant wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 02 February 2021 15:15
>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger 
>> Pau Monné
>> <roger.pau@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; Julien Grall 
>> <julien@xxxxxxx>; Stefano Stabellini
>> <sstabellini@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>
>> Subject: [PATCH v2 2/2] IOREQ: refine when to send mapcache invalidation 
>> request
>>
>> XENMEM_decrease_reservation isn't the only means by which pages can get
>> removed from a guest, yet all removals ought to be signaled to qemu. Put
>> setting of the flag into the central p2m_remove_page() underlying all
>> respective hypercalls as well as a few similar places, mainly in PoD
>> code.
>>
>> Additionally there's no point sending the request for the local domain
>> when the domain acted upon is a different one. The latter domain's ioreq
>> server mapcaches need invalidating. We assume that domain to be paused
>> at the point the operation takes place, so sending the request in this
>> case happens from the hvm_do_resume() path, which as one of its first
>> steps calls handle_hvm_io_completion().
>>
>> Even without the remote operation aspect a single domain-wide flag
>> doesn't do: Guests may e.g. decrease-reservation on multiple vCPU-s in
>> parallel. Each of them needs to issue an invalidation request in due
>> course, in particular because exiting to guest context should not happen
>> before the request was actually seen by (all) the emulator(s).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Preemption related adjustment split off. Make flag per-vCPU. More
>>     places to set the flag. Also handle acting on a remote domain.
>>     Re-base.
> 
> I'm wondering if a per-vcpu flag is overkill actually. We just need
> to make sure that we don't miss sending an invalidation where
> multiple vcpus are in play. The mapcache in the emulator is global
> so issuing an invalidate for every vcpu is going to cause an
> unnecessary storm of ioreqs, isn't it?

The only time a truly unnecessary storm would occur is when for
an already running guest mapcache invalidation gets triggered
by a remote domain. This should be a pretty rare event, so I
think the storm in this case ought to be tolerable.

> Could we get away with the per-domain atomic counter?

Possible, but quite involved afaict: The potential storm above
is the price to pay for the simplicity of the model. It is
important to note that while we don't need all of the vCPU-s
to send these ioreqs, we need all of them to wait for the
request(s) to be acked. And this waiting is what we get "for
free" using the approach here, whereas we'd need to introduce
new logic for this with an atomic counter (afaict at least).

Jan