Re: [PATCH] interrupt preemption support

Gregory Haskins
2007-03-16T12:17:21Z

>>> On Fri, Mar 16, 2007 at 3:03 AM, in message <45FA414C.1040703-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
> Gregory Haskins wrote:
>> This patch adds baseline support for interrupt preemption. This lets one
> context (virtual SMP vcpu, PV driver, etc) preempt another vcpu which is
> currently in guest context when an interrupt is posted. The patch consists
> of the following:
>>
>> 1) Re- worked the vcpu mutex with a new "MPL" construct. This allows updates
> to the vcpu structure even if the vcpu is currently executing in guest mode.
> The previous code held a mutex during guest execution.
>>
>>
>
> 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. 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 ;)

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

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

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

> The paravirt network code congealing in Dor's repo has a spinlock
> protecting the interrupt bits. The main execution path takes both the
> vcpu mutex and the irq lock (when necessary), other paths take just the
> irq lock. This has the added advantage of not requiring a mutex to
> inject an interrupt, which is often necessary from (host) irq context.
>

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

>
>> 2) Exposed the previously static kvm_vcpu_ioctl_interrupt() as
> kvm_vcpu_interrupt so that other mechanisms (PV drivers) can issue interrupts
> just as the ioctl interface can
>>
>
> 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? I will revert this change to make it a static ioctl function again until I can understand how the PV drivers can update the apic (userspace or kernel, whichever it ends up being).

>
>>
>> Index: kvm- 12/kernel/kvm.h
>>
>
> Please base things off trunk. For kernel code, off the git repository,
> not the bundled kernel module (which is mangled by the release process
> in order to accommodate older kernels).

Note that it actually is based off of (approx) trunk, but my quilt series starts with the kvm-12 tarball and thus the directory name. I think my series was patched up to r4524. You comment about git vs tarball is legitimate. I will have to refactor my workflow to utilize git instead of the tarball somehow.

>
>>
>> +typedef enum
>> +{
>> + KVM_MPL_FREE,
>> + KVM_MPL_VMCS,
>> + KVM_MPL_VCPU,
>> + KVM_MPL_FULL = KVM_MPL_VCPU /* Must be equal to the last entry */
>> +}kvm_mpl_locktypes_t;
>>
>
> Either you or your mailer mangle whitespace horribly.

Its probably a combination of both. Do you guys just use the standard "linux/linus" settings for emacs (or equivalent). Sorry about that. I recently rebuilt my devel machine and don't have my .emacs brought over yet. I will fix this.

-Greg