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 <[EMAIL PROTECTED]>, Avi Kivity <[EMAIL PROTECTED]> 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 _______________________________________________ kvm-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/kvm-devel
