Re: [PATCH 3/4] KVM: Adds ability to preempt an executing VCPU

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Tue, May 8, 2007 at  4:13 AM, in message <46403117.2070000-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
>>
>> I am perhaps being a bit overzealous here.  What I found in practice is that 
> the LVTT can screw things up on shutdown, so I was being pretty conservative 
> on the synchronization here.  
>>
>>   
> 
> That may point out to a different sync problem.  All pending timers 
> ought to have been canceled before we reach here.  Please check to make 
> sure this isn't papering over another problem.
> 

You are definitely right there.  I had added this logic in the early stage of debugging.  It turned out that I was missing an apic_dropref, which effectively meant the hrtimer_cancel() was never being issued.  That was the root-cause of my "LVTT expiration after guest shutdown" bug.  I left the sync code in as a conservative measure, but I will clean this up.

>>
>>   
> 
> I approach it from the other direction: to me, a locked assignment says 
> that something is fundamentally wrong.  Usually anything under a lock is 
> a read- modify- write operation, otherwise the writes just stomp on each 
> other.
> 

Interesting. That makes sense.  So if I replace the assignment cases with wmb, do I need to sprinkle rmbs anywhere or is that take care of naturally by the places where we take the lock for a compound operation?

>
> 
> [preemption is disabled here anyway]
>

Ack.  I will remove the calls
 
>>>> +		smp_call_function_single(direct_ipi,
>>>> +					 kvm_vcpu_guest_intr,
>>>> +					 vcpu, 0, 0);
>>>> +		preempt_enable();
>>>> +	}
>>>>   
>>>>       
>>> I see why you must issue the IPI outside the spin_lock_irqsave(), but 
>>> aren't you now opening a race?  vcpu enters guest mode, irq on other 
>>> cpu, irq sets direct_ipi to wakeup guest, releases lock, vcpu exits to 
>>> userspace (or migrates to another cpu), ipi is issued but nobody cares.
>>>     
>>
>> Its subtle, but I think its ok.  The race is actually against the setting of 
> the irq.pending.  This *must* happen inside the lock or the guest could exit 
> and miss the interrupt.  Once the pending bit is set, however, the guest can 
> be woken up in any old fashion and the behavior should be correct.  If the 
> guest has already exited before the IPI is issued, its effectively a no- op 
> (well, really its just a wasted IPI/reschedule event,  but no harm is done).  
> Does this make sense?  Did I miss something else?
>>   
> 
> No, you are correct wrt the vcpu migrating to another cpu.
> 
> What about vs. exit to userspace where we may sleep?

My logic being correct is predicated on the assumption that you and I made a week or two ago:  That the user-space will not sleep for anything but HLT.  If userspace *can* sleep on other things besides HLT, I agree that there is a race here.  If it is limited to HLT, we will be taken care of by the virtue of the fact that irq.pending be set before the handle_halt() logic is checked.  I admit that I was coding against an assumption that I do not yet know for a fact to be true.  I will update the comments to note this assumption so its clearer, and we can address it in the future if its ever revealed to be false.





-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/