[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 6/8] evtchn: convert vIRQ lock to an r/w one



On 30.10.2020 13:08, Julien Grall wrote:
> On 30/10/2020 12:00, Jan Beulich wrote:
>> On 30.10.2020 11:57, Julien Grall wrote:
>>> On 20/10/2020 15:10, Jan Beulich wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -449,6 +449,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t
>>>>    
>>>>        spin_unlock_irqrestore(&chn->lock, flags);
>>>>    
>>>> +    /*
>>>> +     * If by any, the update of virq_to_evtchn[] would need guarding by
>>>> +     * virq_lock, but since this is the last action here, there's no 
>>>> strict
>>>> +     * need to acquire the lock. Hnece holding event_lock isn't helpful
>>>
>>> s/Hnece/Hence/
>>>
>>>> +     * anymore at this point, but utilize that its unlocking acts as the
>>>> +     * otherwise necessary smp_wmb() here.
>>>> +     */
>>>>        v->virq_to_evtchn[virq] = bind->port = port;
>>>
>>> I think all access to v->virq_to_evtchn[virq] should use ACCESS_ONCE()
>>> or {read, write}_atomic() to avoid any store/load tearing.
>>
>> IOW you're suggesting this to be the subject of a separate patch?
>> I don't think such a conversion belongs here (nor even in this
>> series, seeing the much wider applicability of such a change
>> throughout the code base).
>> Or are you seeing anything here which
>> would require such a conversion to be done as a prereq?
> 
> Yes, your comment implies that it is fine to write to virq_to_evtchn[] 
> without the lock taken. However, this is *only* valid if the compiler 
> doesn't tear down load/store.
> 
> So this is a pre-req to get your comment valid.

That's an interesting way to view it. I'm only adding the comment,
without changing the code here. Iirc it was you who asked me to
add the comment. And now this is leading to you wanting me to do
entirely unrelated changes, when the code base is full of similar
assumptions towards no torn accesses? (Yes, I certainly can add
such a patch, but no, I don't think this should be a requirement
here.)

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.