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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Gregory Haskins wrote:
>>>> 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.
>
>   

Okay.  An alternative to removing it is replacing it with a BUG_ON() so 
make sure the constraint is checked.

>>>   
>>>       
>> 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?
>   

I was going to say yes, but I'm not so sure now.  In any case I'm still 
uneasy about the lack of rmw in there.

See Documentation/memory-barriers.txt for an interesting, if difficult, 
discussion of the subject.

>>>   
>>>       
>> 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.
>   

Yeah, I keep forgetting this.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
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/