Re: [PATCH] kvm: remove in_range from kvm_io_device

[Gregory Haskins]

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