On Fri, May 24, 2013 at 11:37:04AM -0400, Peter Feiner wrote: > On Wed, May 22, 2013 at 7:31 PM, Peter Feiner <peter@xxxxxxxxxxxxxx> wrote: > > Since some security driver operations are costly, I think it's > > worthwhile to reduce the scope of the security manager lock or > > increase the granularity by introducing more locks. After a cursory > > look, the security manager lock seems to have a much broader scope > > than necessary. The system / library calls underlying the security > > drivers are all thread safe (e.g., defining apparmor security profiles > > or chowning disk files), so a global lock isn't strictly necessary. > > Moreover, since most virSecurity calls are made whilst a virDomainObj > > lock is held and the security calls are generally domain specific, > > *most* of the security calls are probably thread safe in the absence > > of the global security manager lock. Obviously some work will have to > > be done to see where the security lock actually matters and some > > finer-grained locks will have to be introduced to handle these > > situations. > > To verify that this is worthwhile, I disabled the apparmor driver > entirely. My 20 VM creation test ran about 10s faster (down from 35s > to 25s). > > After giving this approach a little more thought, I think an > incremental series of patches is a good way to go. The responsibility > of locking could be pushed down into the security drivers. At first, > all of the drivers would lock where their managers' locked. Then each > driver could be updated to do more fine-grained locking. I'm going to > work on a patch to push the locking down into the drivers, then I'm > going to work on a patch for better locking in the apparmor driver. Yep, that sounds like a sane approach to me. Previously the security drivers had no locking at all, since they were relying on the global lock at the QEMU driver level. When I introduced the lock into the security manager module, I was pessimistic and used coarse locking. As you say, we can clearly relax this somewhat, if we have the locking in the individual security drivers. > > I also think it's worthwhile to eliminate locking from the the > > virDomainObjList lookups and traversals. Since virDomainObjLists are > > accessed in a bunch of places, I think it's a good defensive idea to > > decouple the performance of these accesses from virDomainObj locks, > > which are held during potentially long-running operations like domain > > creation. An easy way to divorce virDomainObjListSearchName from the > > virDomainObj lock would be to keep a copy of the domain names in the > > virDomainObjList and protect that list with the virDomainObjList lock. > > After removing the security driver contention, this was still a > substantial bottleneck: virConnectDefineXML could still take a few > seconds. I removed the contention by keeping a copy of the domain > definition's name in the domain object. Since the name is immutable > and the domain object is protected by the list lock, the list > traversal can read the name without taking any additional locks. This > patch reduced virConnectDefineXML to tens of milliseconds. Yep, I had a patch to add a security hash table to the domain object list, hashing based on name, but I lost the code when a disk died. I didn't find it made any difference, but agree we should just do it anyway, since it'll almost certainly be a problem in some scenarios. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list