Marcelo Tosatti wrote:
On Tue, May 26, 2009 at 10:02:59AM +0200, Christian Ehrhardt wrote:
Marcelo Tosatti wrote:
On Mon, May 25, 2009 at 01:40:49PM +0200, ehrhardt@xxxxxxxxxxxxxxxxxx wrote:
From: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>
To ensure vcpu's come out of guest context in certain cases this patch adds a
s390 specific way to kick them out of guest context. Currently it kicks them
out to rerun the vcpu_run path in the s390 code, but the mechanism itself is
expandable and with a new flag we could also add e.g. kicks to userspace etc.
Signed-off-by: Christian Ehrhardt <ehrhardt@xxxxxxxxxxxxxxxxxx>
"For now I added the optimization to skip kicking vcpus out of guest
that had the request bit already set to the s390 specific loop (sent as
v2 in a few minutes).
We might one day consider standardizing some generic kickout levels e.g.
kick to "inner loop", "arch vcpu run", "generic vcpu run", "userspace",
... whatever levels fit *all* our use cases. And then let that kicks be
implemented in an kvm_arch_* backend as it might be very different how
they behave on different architectures."
That would be ideal, yes. Two things make_all_requests handles:
1) It disables preemption with get_cpu(), so it can reliably check for
cpu id. Somehow you don't need that for s390 when kicking multiple
vcpus?
I don't even need the cpuid as make_all_requests does, I just insert a
special bit in the vcpu arch part and the vcpu will "come out to me
(host)".
Fortunateley the kick is rare and fast so I can just insert it
unconditionally (it's even ok to insert it if the vcpu is not in guest
state). That prevents us from needing vcpu lock or detailed checks which
would end up where we started (no guarantee that vcpu's come out of
guest context while trying to aquire all vcpu locks)
Let me see if I get this right: you kick the vcpus out of guest mode by
using a special bit in the vcpu arch part. OK.
What I don't understand is this:
"would end up where we started (no guarantee that vcpu's come out of
guest context while trying to aquire all vcpu locks)"
initially the mechanism looped over vcpu's and just aquired the vcpu
lock and then updated the vcpu.arch infor directly.
Avi mentioned that we have no guarantee if/when the vcpu will come out
of guest context to free a lock currently held and suggested the
mechanism x86 uses via setting vcpu->request and kicking the vcpu. Thats
the eason behind "end up where we (the discussion) started", if we would
need the vcpu lock again we would be at the beginnign of the discussion.
So you _need_ a mechanism to kick all vcpus out of guest mode?
I have a mechanism to kick a vcpu, and I use it. Due to the fact that
smp_call_* don't work as kick for us the kick is an arch specific function.
I hop ethat clarified this part :-)
2) It uses smp_call_function_many(wait=1), which guarantees that by the
time make_all_requests returns no vcpus will be using stale data (the
remote vcpus will have executed ack_flush).
yes this is really a part my s390 implementation doesn't fulfill yet.
Currently on return vcpus might still use the old memslot information.
As mentioned before letting all interrupts come "too far" out of the hot
loop would be a performance issue, therefore I think I will need some
request&confirm mechanism. I'm not sure yet but maybe it could be as
easy as this pseudo code example:
# in make_all_requests
# remember we have slots_lock write here and the reentry that updates
the vcpu specific data aquires slots_lock for read.
loop vcpus
set_bit in vcpu requests
kick vcpu #arch function
endloop
loop vcpus
until the requested bit is disappeared #as the reentry path uses
test_and_clear it will disappear
endloop
That would be a implicit synchronization and should work, as I wrote
before setting memslots while the guest is running is rare if ever
existant for s390. On x86 smp_call_many could then work without the wait
flag being set.
I see, yes.
But I assume that this synchronization approach is slower as it
serializes all vcpus on reentry (they wait for the slots_lock to get
dropped), therefore I wanted to ask how often setting memslots on
runtime will occur on x86 ? Would this approach be acceptable ?
For x86 we need slots_lock for two things:
1) to protect the memslot structures from changing (very rare), ie:
kvm_set_memory.
2) to protect updates to the dirty bitmap (operations on behalf of
guest) which take slots_lock for read versus updates to that dirty
bitmap (an ioctl that retrieves what pages have been dirtied in the
memslots, and clears the dirtyness info).
All you need for S390 is 1), AFAICS.
correct
For 1), we can drop the slots_lock usage, but instead create an
explicit synchronization point, where all vcpus are forced to (say
kvm_vcpu_block) "paused" state. qemu-kvm has such notion.
Same language?
Yes, I think i got your point :-)
But I think by keeping slots_lock we already got our synchronization
point and don't need an explicit one adding extra code and maybe locks.
As I mentioned above it should synchronize already implicit. When I
looked at it once more yesterday I realized that kvm_set_memory is not
performance critical anyway (i.e. does not have to be the fastest ioctl
on earth) so we could be one step smarter and instead of serializing all
vcpu's among each other we could set_memory just let do it one by one.
In case I lost you again due to my obviously confusing mainframe
language this week you might want to my next patch submission where I
implement that in the s390 arch code as an example. I'll put you on cc
and in that new code we might find an implicit language synchronization
for us :-)
[...]
--
Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
