Re: [patch] x86: kvm: Convert i8254/i8259 locks to raw_spinlocks

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Feb 18, 2010 at 12:18:25PM +0200, Avi Kivity wrote:
> On 02/18/2010 12:05 PM, Jan Kiszka wrote:
> >Avi Kivity wrote:
> >>On 02/18/2010 11:45 AM, Avi Kivity wrote:
> >>>On 02/18/2010 11:40 AM, Jan Kiszka wrote:
> >>>>>Meanwhile, if anyone has any idea how to kill this lock, I'd love to
> >>>>>see it.
> >>>>>
> >>>>What concurrency does it resolve in the end? On first glance, it only
> >>>>synchronize the fiddling with pre-VCPU request bits, right? What forces
> >>>>us to do this? Wouldn't it suffice to disable preemption (thus
> >>>>migration) and the let concurrent requests race for setting the bits? I
> >>>>mean if some request bit was already set on entry, we don't include the
> >>>>   related VCPU in smp_call_function_many anyway.
> >>>It's more difficult.
> >>>
> >>>vcpu 0: sets request bit on vcpu 2
> >>>           vcpu 1: test_and_set request bit on vcpu 2, returns already set
> >>>           vcpu 1: returns
> >>>vcpu 0: sends IPI
> >>>vcpu 0: returns
> >>>
> >>>so vcpu 1 returns before the IPI was performed.  If the request was a
> >>>tlb flush, for example, vcpu 1 may free a page that is still in vcpu
> >>>2's tlb.
> >>One way out would be to have a KVM_REQ_IN_PROGRESS, set it in
> >>make_request, clear it in the IPI function.
> >>
> >>If a second make_request sees it already set, it can simply busy wait
> >>until it is cleared, without sending the IPI.  Of course the busy wait
> >>means we can't enable preemption (or we may busy wait on an unscheduled
> >>task), but at least the requests can proceed in parallel instead of
> >>serializing.
>
> >...or include VCPUs with KVM_REQ_IN_PROGRESS set into the IPI set even
> >if they already have the desired request bit set.
> 
> But then we're making them take the IPI, which is pointless and
> expensive.  My approach piggy backs multiple requesters on one IPI.

I have played with this in the past (collapsing that would avoid two
simultaneous requestors from issuing two IPI's to a given vcpu, and
unification with KVM_REQ_KICK to avoid the IPI if vcpu not in guest
mode).

Its not worthwhile though, this is not a contention point with TDP
(maybe it becomes in the future with fine grained flushing, but not
ATM).

> >Then we should
> >serialize in smp_call_function_many.
> 
> Do you mean rely on s_c_f_m's internal synchronization?