Re: [PATCH 1/5] Protect irq_sources_bitmap by kvm->lock instead of kvm->irq_lock

[Gregory Haskins <gregory.haskins@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

Gregory Haskins wrote:
> Gleb Natapov wrote:
>   
>> On Mon, Jul 13, 2009 at 10:29:02AM -0400, Gregory Haskins wrote:
>>   
>>     
>>> Gleb Natapov wrote:
>>>     
>>>       
>>>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>>>> index 6c57e46..ce8fcd3 100644
>>>> --- a/virt/kvm/irq_comm.c
>>>> +++ b/virt/kvm/irq_comm.c
>>>> @@ -210,7 +210,8 @@ int kvm_request_irq_source_id(struct kvm *kvm)
>>>>  	unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
>>>>  	int irq_source_id;
>>>>  
>>>> -	mutex_lock(&kvm->irq_lock);
>>>> +	WARN_ON(!mutex_is_locked(&kvm->lock));
>>>>   
>>>>       
>>>>         
>>> Shouldn't this be fatal?  (e.g. BUG_ON).  I know the usage between
>>> BUG/WARN is controversial, but it seems to me that something is
>>> completely broken if you expect it to be locked and its not.  Might as
>>> well fail the system, IMO.
>>>
>>>     
>>>       
>> Well I don't really care but we have WARN_ON() in the code currently.
>>   
>>     
>
> Well, that is perhaps unfortunate, but not relevant.  I am not reviewing
> those patches ;)
>
>   
>> Besides the chances are good that even without locking around this
>> function nothing will break, so why kill host kernel?
>>   
>>     
>
> The question to ask is: Is it legal to continue to run if the mutex is
> found unlocked?  If not, the offending caller should be found/fixed as
> early as possible IMO, and an oops should be sufficient to do so.  I
> think WARN_ON tends to gets overused/abused, so lets not perpetuate it
> simply because of precedence.
>   

Err..precedent, I mean.  Heh.

/me needs more coffee.

-Greg

> Kind Regards,
> -Greg
>
>
>   



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]