On Monday 29 December 2008 13:42:22 Amit Shah wrote: > On Sun, Dec 28, 2008 at 07:24:02PM +0800, Sheng Yang wrote: > > On Sat, Dec 27, 2008 at 06:06:26PM -0200, Marcelo Tosatti wrote: > > > On Fri, Dec 26, 2008 at 10:30:07AM +0800, Sheng Yang wrote: > > > > Thanks to Marcelo's observation, The following code have potential > > > > issue: > > > > > > > > if (cancel_work_sync(&assigned_dev->interrupt_work)) > > > > kvm_put_kvm(kvm); > > > > > > > > In fact, cancel_work_sync() would return true either work struct is > > > > only scheduled or the callback of work struct is executed. This code > > > > only consider the former situation. > > > > > > Why not simply drop the reference inc / dec from irq handler/work > > > function? > > > > Sorry, I don't know the answer. After checking the code, I also think > > it's a little strange to increase refernce count here, and I think we > > won't suppose work_handler can release the kvm struct. > > At the time of developing that code, this was my observation: > > I see from the call chain kvm_put_kvm->...->kvm_arch_destroy_vm, no locks > are taken to actually destroy the vm. We can't be in ioctls, sure. But > shouldn't the mutex be taken to ensure there's nothing else going on while > destroying? > > At least with the workqueue model, we could be called asynchronously in > kernel context and I would have held the mutex and about to inject > interrupts while everything is being wiped off underneath. However, the > workqueue model tries its best to schedule the work on the same CPU, though > we can't use that guarantee to ensure things will be fine. > > --- > So I had to get a ref to the current vm till we had any pending work > scheduled. I think I put in comments in the code, but sadly most of my > comments we stripped out before the merge. > Not quite understand...
The free assigned device in the destroy path of VM, so as free irq. And we got cancel_work_sync() in free irq which can sync with the execution of scheduled work. And now before cancel_work_sync(), we disable the interrupt so that no more schedule work happen again. So after cancel_work_sync(), everything(I think it's irq handler and schedule work here) asynchronously should quiet down. Or I miss something? -- regards Yang, Sheng -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
