Re: [PATCH] interrupt preemption support

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

Hi Avi,
  You make good points.  I will convert to a nest lock design and resubmit.   Should I use two mutexes, or a mutex and spinlock?

Also, do you have any suggestions on the signum I should use to IPI the running guest?  Should I use one of the normal signals (SIGUSR) or should I start a block of defined signals in the RT range (>32)?

-Greg

>>> On Sun, Mar 18, 2007 at  1:46 AM, in message <45FCD226.2010603-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> Gregory Haskins wrote:
> 
>   
> 
>>> Whoa there.  kvm can't just add new locking constructs to the kernel.  
>>>     
>>
>> With all due respect, why not? ;)  Of course it can.  We arent really adding 
> anything exposed to the kernel outside of KVM or even adding a new one from 
> scratch.  Its simply derivative from the existing mutex/semphore that we 
> already use.   
> 
> Still it's not good practice IMO.  If the locking construct is useful, 
> the entire kernel should benefit.  If it isn't, we don't want it.  And 
> we want all the locking gurus to have a look, not just one.
> 
>> As a philosophical point, surely not ever construct we utilize in KVM has to 
> be trickled down from the upstream kernel, or KVM itself would not exist ;)
>>
>>   
> 
> We try not to introduce anything new, unless it is directly related to 
> virtualization.  We want to make our code as boring as possible (but not 
> boringer :)
> 
> That's important in not raising the barrier of entry above the minimum 
> possible.
> 
>>> It has to be added to the kernel _first_, you need to justify it with 
>>> more than just kvm as a user, show correctness, performance, and 
>>> scalability.
>>>
>>> Once it's accepted, kvm can use it.
>>>     
>>
>> I understand if what I said may not change your argument against proving the 
> construct against a larger audience, but I thought I would point it out that 
> its really just a struct+mutex in case you only read the comment and not the 
> code.  I think it just sounds worse than it is.
>>   
> 
> This will happen to anyone reading the code.  People are used to seeing 
> nested locks.  If they see something new, they have to drop whatever 
> exciting feature they were developing and go understand it.
> 
> 
>>   
>>> Formal issues aside, why is this different from taking nested locks?  
>>>     
>>
>> This essentially *is* a set of nested locks, except that it also 
> automatically protects against deadlock.  Alternatively I could have written 
> the code to simply have replaced the original vcpu- >mutex with something like 
> vcpu- >vcms_mutex + vcpu- >vcpu_mutex, and then replaced all the calls to 
> mutex_lock(&vcpu- >mutex) with a macro that grabs both (in proper order).
> 
> Won't that lock out the interrupt user?  We want only access to the vcpu 
> to be blocked during guest execution, except for the interrupt area 
> (which is just mutually excluded) and for the mmu area (likewise).
> 
>>    However, I felt as though my solution was better for two reasons:
>>
>> 1) Its impossible to bungle the lock- ordering to induce deadlock, regardless 
> of nesting depth
>> 2) Its non- contended acquisition overhead is O(1) regardless of nesting 
> depth, whereas true nested locks are O(n) (where n = depth)
>>   
> 
> First I need to understand how it fits.
> 
>> In my experience, nested locks work ok if there is only one developer on the 
> project. When you get project newbies (like me w.r.t. KVM ;) ) hacking code 
> in after the fact, bad things can happen pretty easily.  
> 
> Right, we need to document the mess.
> 
>> I thought this might perhaps be a good way to prevent that.  If it turns out 
> you guys dont like it, its no big deal to rip it out and go the more 
> traditional route.  I wanted your feedback.  Thats why I submitted this now 
> before I had all the issues worked out. ;)
>>   
> 
> Sure, early & often.
> 
>> Keep in mind that my primary intention was to fix the 
> kvm_vcpu_(ioctl)_interrupt() function to use finer grained locking than the 
> previous vcpu- >mutex that it used to use.  Because the old lock was a mutex, 
> the new nested lock was also based on a mutex.  So its true that its not 
> interrupt friendly, but it wasn't to begin with.  Whether it actually needs 
> to be is something that I do not yet know (see my comments below).
>>
>>   
> 
> It's needed all right.
> 
>>> It's not enough to issue an interrupt, there is a whole dance involved 
>>> in the guest side to ack it.  You need to go through the apic, which is 
>>> currently emulated in userspace.  We may push it to the kernel later.
>>>     
>>
>> I added this interface as a stab at accommodating your request for PV driver 
> support without fully understanding your requirements.  Based on your 
> comments, I assume that the code that invokes the INTERRUPT ioctl must have 
> already updated the APIC model? 
>>   
> 
> Yes.
> 
>> Its probably a combination of both.  Do you guys just use the standard 
> "linux/linus" settings for emacs (or equivalent).
>>   
> 
> I use the script suggested in Documentation/CodingStyle, plus the 
> show- trailing- whitespace customization variable, which saves me from 
> committing the sin of leaving trailing whitespace every now and then.
> 




-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV