Re: [PATCH] kvm: remove in_range from kvm_io_device

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 25, 2009 at 07:27:36AM -0400, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Wed, Jun 24, 2009 at 11:49:01AM +0300, Michael S. Tsirkin wrote:
> >   
> >> On Tue, Jun 23, 2009 at 09:43:31PM -0400, Gregory Haskins wrote:
> >>     
> >>> Michael S. Tsirkin wrote:
> >>>       
> >>>> Remove in_range from kvm_io_device and ask read/write callbacks, if
> >>>> supplied, to perform range checks internally. This allows aliasing
> >>>> (mostly for in-kernel virtio), as well as better error handling by
> >>>> making it possible to pass errors up to userspace. And it's enough to
> >>>> look at the diffstat to see that it's a better API anyway.
> >>>>
> >>>> While we are at it, document locking rules for kvm_io_device.
> >>>>   
> >>>>         
> >>> Hi Michael,
> >>>
> >>> I just tried to apply this to kvm.git/master, and it blew up really
> >>> badly.  What tree should I be using?
> >>>       
> >> Ugh, this is against 2.6.30. I'll post kvm.git version soon.
> >>
> >>     
> >
> > I went ahead and tried to rebase it, to find that it conflicts with
> > recent patch 35b3038961f94e83557944ae0d30c8fa0b5012cf merged in kvm.git:
> > 'KVM: switch irq injection/acking data structures to irq_lock'
> > which now does this:
> > 	lock
> > 	find
> > 	unlock
> > 	write
> >
> > I thought for a while that it might make sense to start small and just
> > add in_range parameter for starters ...
> > However, I just realised that this only works because devices are not
> > added or removed dynamically.
> >
> > The long term fix is to switch to SRCU for bus management.  But if we
> > need to do this for iosignalfd anyway, in_range removal becomes possible
> > again.
> >
> > Short term it might be also possible to go back to keeping kvm lock
> > across both find and read - since the lock is taken, we don't
> > really win anything currently if we drop the lock earlier.
> >
> > Comments?
> >
> >   
> 
> Can we just get iosignalfd merged for now as it is, then?

The patch that adds value to in_range is still okay. We could go that
way for now.  But what I am saying is that groups are still devices, and
the patch that adds kvm_io_bus_unregister_dev seems broken with the new
locking. No?

> Its not like
> we cant patch the group/item code later to clean it up when the time is
> right.
> 
> Regards,
> -Greg
> 

What's the rush?

IMO the best plan is:
- add srcu for io bus, removing kvm lock from that space completely
- apply in_range removal patch on top of this
- iosignalfd on top

Comments?


-- 
MST