On Sun, Dec 28, 2008 at 07:24:02PM +0800, Sheng Yang wrote:
quoted text
> 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.

Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html