[10/12] evtchn/fifo: use stable fields when recording "last queue" information

Commit Message

Jan Beulich Sept. 28, 2020, 11:01 a.m. UTC

Both evtchn->priority and evtchn->notify_vcpu_id could, prior to recent
locking adjustments, change behind the back of
evtchn_fifo_set_pending(). Neither the queue's priority nor the vCPU's
vcpu_id fields have similar properties, so they seem better suited for
the purpose. In particular they reflect the respective evtchn fields'
values at the time they were used to determine queue and vCPU.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Julien Grall Sept. 29, 2020, 12:29 p.m. UTC | #1

Hi Jan,

On 28/09/2020 12:01, Jan Beulich wrote:
> Both evtchn->priority and evtchn->notify_vcpu_id could, prior to recent
> locking adjustments, change behind the back of
> evtchn_fifo_set_pending(). Neither the queue's priority nor the vCPU's
> vcpu_id fields have similar properties, so they seem better suited for
> the purpose. In particular they reflect the respective evtchn fields'
> values at the time they were used to determine queue and vCPU.
> 
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Julien Grall <[e-mail]>

Cheers,

Paul Durrant Sept. 30, 2020, 7:35 a.m. UTC | #2

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> Sent: 28 September 2020 12:02
> To: [e-mail]
> Cc: Andrew Cooper <[e-mail]>; George Dunlap <[e-mail]>; Ian
> Jackson <[e-mail]>; Julien Grall <[e-mail]>; Wei Liu <[e-mail]>; Stefano Stabellini
> <[e-mail]>
> Subject: [PATCH 10/12] evtchn/fifo: use stable fields when recording "last queue" information
> 
> Both evtchn->priority and evtchn->notify_vcpu_id could, prior to recent
> locking adjustments, change behind the back of
> evtchn_fifo_set_pending(). Neither the queue's priority nor the vCPU's
> vcpu_id fields have similar properties, so they seem better suited for
> the purpose. In particular they reflect the respective evtchn fields'
> values at the time they were used to determine queue and vCPU.
> 
> Signed-off-by: Jan Beulich <[e-mail]>
> 

I think these changes make the code clearer anyway.

Reviewed-by: Paul Durrant <[e-mail]>

> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -246,8 +246,8 @@ static void evtchn_fifo_set_pending(stru
>          /* Moved to a different queue? */
>          if ( old_q != q )
>          {
> -            evtchn->last_vcpu_id = evtchn->notify_vcpu_id;
> -            evtchn->last_priority = evtchn->priority;
> +            evtchn->last_vcpu_id = v->vcpu_id;
> +            evtchn->last_priority = q->priority;
> 
>              spin_unlock_irqrestore(&old_q->lock, flags);
>              spin_lock_irqsave(&q->lock, flags);
>

Jan Beulich Sept. 30, 2020, 8:35 a.m. UTC | #3

On 30.09.2020 09:35, Paul Durrant wrote:
>> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
>> Sent: 28 September 2020 12:02
>>
>> Both evtchn->priority and evtchn->notify_vcpu_id could, prior to recent
>> locking adjustments, change behind the back of
>> evtchn_fifo_set_pending(). Neither the queue's priority nor the vCPU's
>> vcpu_id fields have similar properties, so they seem better suited for
>> the purpose. In particular they reflect the respective evtchn fields'
>> values at the time they were used to determine queue and vCPU.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> I think these changes make the code clearer anyway.
> 
> Reviewed-by: Paul Durrant <[e-mail]>

Thanks. With the change of description in the earlier patch, and with
this one possibly going in ahead of it, I'll massage the description
here somewhat, I guess.

Jan

Paul Durrant Sept. 30, 2020, 8:38 a.m. UTC | #4

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 30 September 2020 09:35
> To: [e-mail]
> Cc: [e-mail]; 'Andrew Cooper' <[e-mail]>; 'George Dunlap'
> <[e-mail]>; 'Ian Jackson' <[e-mail]>; 'Julien Grall' <[e-mail]>;
> 'Wei Liu' <[e-mail]>; 'Stefano Stabellini' <[e-mail]>
> Subject: Re: [PATCH 10/12] evtchn/fifo: use stable fields when recording "last queue" information
> 
> On 30.09.2020 09:35, Paul Durrant wrote:
> >> From: Xen-devel <[e-mail]> On Behalf Of Jan Beulich
> >> Sent: 28 September 2020 12:02
> >>
> >> Both evtchn->priority and evtchn->notify_vcpu_id could, prior to recent
> >> locking adjustments, change behind the back of
> >> evtchn_fifo_set_pending(). Neither the queue's priority nor the vCPU's
> >> vcpu_id fields have similar properties, so they seem better suited for
> >> the purpose. In particular they reflect the respective evtchn fields'
> >> values at the time they were used to determine queue and vCPU.
> >>
> >> Signed-off-by: Jan Beulich <[e-mail]>
> >
> > I think these changes make the code clearer anyway.
> >
> > Reviewed-by: Paul Durrant <[e-mail]>
> 
> Thanks. With the change of description in the earlier patch, and with
> this one possibly going in ahead of it, I'll massage the description
> here somewhat, I guess.
> 

That's fine.

  Paul

Patch

--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -246,8 +246,8 @@  static void evtchn_fifo_set_pending(stru
         /* Moved to a different queue? */
         if ( old_q != q )
         {
-            evtchn->last_vcpu_id = evtchn->notify_vcpu_id;
-            evtchn->last_priority = evtchn->priority;
+            evtchn->last_vcpu_id = v->vcpu_id;
+            evtchn->last_priority = q->priority;
 
             spin_unlock_irqrestore(&old_q->lock, flags);
             spin_lock_irqsave(&q->lock, flags);