diff mbox series

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

Message ID 53a2fc39-1bf1-38ce-bbdf-b512c5279b4f@suse.com (mailing list archive)
State New, archived
Headers show
Series evtchn: recent XSAs follow-on | expand

Commit Message

Jan Beulich Oct. 20, 2020, 2:10 p.m. UTC
There's no need to serialize all sending of vIRQ-s; all that's needed
is serialization against the closing of the respective event channels
(so far by means of a barrier). To facilitate the conversion, switch to
an ordinary write locked region in evtchn_close().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Don't introduce/use rw_barrier() here. Add comment to
    evtchn_bind_virq(). Re-base.

Comments

Julien Grall Oct. 30, 2020, 10:57 a.m. UTC | #1
Hi Jan,

On 20/10/2020 15:10, Jan Beulich wrote:
> There's no need to serialize all sending of vIRQ-s; all that's needed
> is serialization against the closing of the respective event channels
> (so far by means of a barrier). To facilitate the conversion, switch to
> an ordinary write locked region in evtchn_close().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Don't introduce/use rw_barrier() here. Add comment to
>      evtchn_bind_virq(). Re-base.
> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -160,7 +160,7 @@ struct vcpu *vcpu_create(struct domain *
>       v->vcpu_id = vcpu_id;
>       v->dirty_cpu = VCPU_CPU_CLEAN;
>   
> -    spin_lock_init(&v->virq_lock);
> +    rwlock_init(&v->virq_lock);
>   
>       tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
>   
> --- 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.

Cheers,
Jan Beulich Oct. 30, 2020, noon UTC | #2
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?

Jan
Julien Grall Oct. 30, 2020, 12:08 p.m. UTC | #3
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.

Cheers,
Jan Beulich Oct. 30, 2020, 12:25 p.m. UTC | #4
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
Julien Grall Oct. 30, 2020, 12:46 p.m. UTC | #5
On 30/10/2020 12:25, Jan Beulich wrote:
> 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. 

Yes I asked this comment because it makes easier for reader to figure 
out how your code works. But this doesn't mean I wanted to have a 
misleading comment.

If you don't plan to handle the ACCESS_ONCE(), then it is best to be 
open to say it in 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? 

I was expecting you writing that :). Well, small steps are always easier 
in order to achieve a consistent code base.

To me, this is similar to some of the reviewers pushing contributors to 
update the coding style of area touched. The difference here is without 
following a memory model, you are at the mercy of the compiler and XSAs.

> (Yes, I certainly can add
> such a patch, but no, I don't think this should be a requirement
> here.)

That's ok. But please update the comment to avoid misleading the reader.

An alternative would be to use the write_lock() here. This shouldn't 
impact that much the performance and nicely fix the issue.

Cheers,
diff mbox series

Patch

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -160,7 +160,7 @@  struct vcpu *vcpu_create(struct domain *
     v->vcpu_id = vcpu_id;
     v->dirty_cpu = VCPU_CPU_CLEAN;
 
-    spin_lock_init(&v->virq_lock);
+    rwlock_init(&v->virq_lock);
 
     tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
 
--- 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
+     * 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;
 
  out:
@@ -638,10 +645,10 @@  int evtchn_close(struct domain *d1, int
     case ECS_VIRQ:
         for_each_vcpu ( d1, v )
         {
-            if ( v->virq_to_evtchn[chn1->u.virq] != port1 )
-                continue;
-            v->virq_to_evtchn[chn1->u.virq] = 0;
-            spin_barrier(&v->virq_lock);
+            write_lock_irqsave(&v->virq_lock, flags);
+            if ( v->virq_to_evtchn[chn1->u.virq] == port1 )
+                v->virq_to_evtchn[chn1->u.virq] = 0;
+            write_unlock_irqrestore(&v->virq_lock, flags);
         }
         break;
 
@@ -797,7 +804,7 @@  void send_guest_vcpu_virq(struct vcpu *v
 
     ASSERT(!virq_is_global(virq));
 
-    spin_lock_irqsave(&v->virq_lock, flags);
+    read_lock_irqsave(&v->virq_lock, flags);
 
     port = v->virq_to_evtchn[virq];
     if ( unlikely(port == 0) )
@@ -807,7 +814,7 @@  void send_guest_vcpu_virq(struct vcpu *v
     evtchn_port_set_pending(d, v->vcpu_id, evtchn_from_port(d, port));
 
  out:
-    spin_unlock_irqrestore(&v->virq_lock, flags);
+    read_unlock_irqrestore(&v->virq_lock, flags);
 }
 
 void send_guest_global_virq(struct domain *d, uint32_t virq)
@@ -826,7 +833,7 @@  void send_guest_global_virq(struct domai
     if ( unlikely(v == NULL) )
         return;
 
-    spin_lock_irqsave(&v->virq_lock, flags);
+    read_lock_irqsave(&v->virq_lock, flags);
 
     port = v->virq_to_evtchn[virq];
     if ( unlikely(port == 0) )
@@ -836,7 +843,7 @@  void send_guest_global_virq(struct domai
     evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
 
  out:
-    spin_unlock_irqrestore(&v->virq_lock, flags);
+    read_unlock_irqrestore(&v->virq_lock, flags);
 }
 
 void send_guest_pirq(struct domain *d, const struct pirq *pirq)
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -235,7 +235,7 @@  struct vcpu
 
     /* IRQ-safe virq_lock protects against delivering VIRQ to stale evtchn. */
     evtchn_port_t    virq_to_evtchn[NR_VIRQS];
-    spinlock_t       virq_lock;
+    rwlock_t         virq_lock;
 
     /* Tasklet for continue_hypercall_on_cpu(). */
     struct tasklet   continue_hypercall_tasklet;