Re: [PATCH] interrupt preemption support

Gregory Haskins
2007-03-19T13:54:15Z

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.