Re: [PATCH] kvm: remove in_range from kvm_io_device

[Gregory Haskins]

Michael S. Tsirkin wrote:
> On Thu, Jun 25, 2009 at 08:08:04AM -0400, Gregory Haskins wrote:
>   
>> 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 ;).
>>     
>
> Hey, I feel your pain, I've been reviewing these ..
>
>   
>> 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?
>>     
>
> No objections, only comments ;)
>   

:)
>   
>> (BTW: I am talking about the yet unpublished "v9" which addresses all
>> your other comments sans the io_bus interface changes.
>>     
>
> I thought we agreed on the io_bus approach. What changed?
>   

Oh yeah, nothing changed.  I still totally agree with what you want to
do.  I am just thinking that you are proposing some relatively
non-trivial changes that will take another few weeks to get reviewed and
accepted, whereas iosignalfd is more or less ready to go aside from
integrating with this change.  Its an additional maintenance burden on
my part to live out of tree so I aim to minimize that since I can't see
much benefit in waiting.  However, it's not a huge deal either way, really.

>   
>>  Will push out
>> later today)
>>     
>
> BTW, is the group removal race handled there somehow?
> Here's what I have in mind:
> kvm does
> 	lock
> 	dev = find
> 	unlock
>
> 	<---------- at this point group device is removed
>
> 	write access to device that has been removed
>
>
>   

Hmm...you are right.  This looks like it was introduced with that recent
locking patch you cited.  Well, I can still fix it now easily by putting
an rcu-read-lock around the access.  Longer term we should move to
srcu.  Thoughts?

-Greg

Attachment: signature.asc
Description: OpenPGP digital signature