Re: [PATCH v2 2/4] KVM: introduce "xinterface" API for external interaction with guests

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

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

Gregory Haskins wrote:
> Avi Kivity wrote:
>> On 10/06/2009 01:57 AM, Gregory Haskins wrote:
>>> Avi Kivity wrote:
>>>   
>>>> On 10/02/2009 10:19 PM, Gregory Haskins wrote:

>>>>> +
>>>>> +static inline void
>>>>> +_kvm_xinterface_release(struct kref *kref)
>>>>> +{
>>>>> +    struct kvm_xinterface *intf;
>>>>> +    struct module *owner;
>>>>> +
>>>>> +    intf = container_of(kref, struct kvm_xinterface, kref);
>>>>> +
>>>>> +    owner = intf->owner;
>>>>> +    rmb();
>>>>>
>>>>>        
>>>> Why rmb?
>>>>      
>>> the intf->ops->release() line may invalidate the intf pointer, so we
>>> want to ensure that the read completes before the release() is called.
>>>
>>> TBH: I'm not 100% its needed, but I was being conservative.
>>>    
>> rmb()s are only needed if an external agent can issue writes, otherwise
>> you'd need one after every statement.
> 
> I was following lessons learned here:
> 
> http://lkml.org/lkml/2009/7/7/175
> 
> Perhaps mb() or barrier() are more appropriate than rmb()?  I'm CC'ing
> David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we
worry about the case where one cpu reorders the read to be after the
->release(), and another cpu might grab the memory that was kfree()'d
within the ->release() and scribble something else on it before the read
completes.

I know rmb() typically needs to be paired with wmb() to be correct, so
you are probably right to say that the rmb() itself is not appropriate.
 This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required.  As David mentions, perhaps
"smp_mb()" is more appropriate for this application.  I also speculate
barrier() may be all that we need.

Kind Regards,
-Greg


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