[lkml]   [2009]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v2 2/4] KVM: introduce "xinterface" API for external interaction with guests
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:
> 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

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,

[unhandled content-type:application/pgp-signature]
 \ /
  Last update: 2009-10-06 16:33    [W:0.094 / U:0.356 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site