Re: [PATCH] kvm: remove in_range from kvm_io_device

Michael S. Tsirkin wrote:
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?


The patch has been in circulation for weeks, is well tested/reviewed
(and I hope its considered well written), and I want to get on with my
life ;). Your proposal doesn't change the user->kern ABI, so any
consolidation will be just an internal change to the kernel code only.
People can start using the interface today to build things, and we can
fix up the internal code later once your proposals have had a chance to
be shaped after review, etc (which I know from experience can take a
while and change radically though the course ;).

IOW: The only thing waiting does is hide the history of the edit, since
whatever change is proposed is invariably the same amount of work for me
to convert it over. Its purely a question of whether its folded into
the history or visible as two change records. Based on that. I don't
see any problem with it just going in now. Its certainly ready from my
perspective.

So I guess the question is: What's your objection?

(BTW: I am talking about the yet unpublished "v9" which addresses all
your other comments sans the io_bus interface changes. Will push out
later today)

Regards,
-Greg


Attachment: signature.asc
Description: OpenPGP digital signature

Relevant Pages

  • Re: [PATCH] kvm: remove in_range from kvm_io_device
    ... as well as better error handling by ... Hi Michael, ... Short term it might be also possible to go back to keeping kvm lock ... srcu plan and I will just defer submitting the iosignalfd related series ...
    (Linux-Kernel)
  • Re: [PATCH] kvm: remove in_range from kvm_io_device
    ... as well as better error handling by ... Hi Michael, ... The long term fix is to switch to SRCU for bus management. ... Short term it might be also possible to go back to keeping kvm lock ...
    (Linux-Kernel)
  • Re: [PATCH] kvm: remove in_range from kvm_io_device
    ... as well as better error handling by ... Short term it might be also possible to go back to keeping kvm lock ... Can we just get iosignalfd merged for now as it is, ... The patch that adds value to in_range is still okay. ...
    (Linux-Kernel)
  • [ANNOUNCE] 2.6.31-rc4-rt1
    ... This is a major rework of the rt patch series. ... interrupt threading is now a pure extension of the mainline ... type in the lock definition. ...
    (Linux-Kernel)
  • Re: [ANNOUNCE] 2.6.31-rc4-rt1
    ... This is a major rework of the rt patch series. ... interrupt threading is now a pure extension of the mainline ... type in the lock definition. ...
    (Linux-Kernel)