Re: [PATCH] interrupt preemption support

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

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.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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