[PATCH v8 0/3] xen/events: further locking adjustments

[PATCH v8 0/3] xen/events: further locking adjustments

[Juergen Gross]

This is an add-on of my event channel locking series.

Juergen Gross (3):
  xen/events: modify struct evtchn layout
  xen/events: rework fifo queue locking
  xen/events: do some cleanups in evtchn_fifo_set_pending()

 xen/common/event_fifo.c | 152 +++++++++++++++++++++-------------------
 xen/include/xen/sched.h |  34 ++++-----
 2 files changed, 98 insertions(+), 88 deletions(-)

-- 
2.26.2

[PATCH v8 1/3] xen/events: modify struct evtchn layout

[Juergen Gross]

In order to avoid latent races when updating an event channel put
xen_consumer and pending fields in different bytes. This is no problem
right now, but especially the pending indicator isn't used only when
initializing an event channel (unlike xen_consumer), so any future
addition to this byte would need to be done with a potential race kept
in mind.

At the same time move some other fields around to have less implicit
paddings and to keep related fields more closely together.

Finally switch struct evtchn to no longer use fixed sized types where
not needed.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V7:
- new patch

V8:
- retain xen_consumer to be a bitfield (Jan Beulich)
- switch to non fixed sizes types

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/event_fifo.c |  4 ++--
 xen/include/xen/sched.h | 34 ++++++++++++++++++----------------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 79090c04ca..f39e61105f 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -200,7 +200,7 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
      */
     if ( unlikely(!word) )
     {
-        evtchn->pending = 1;
+        evtchn->pending = true;
         return;
     }
 
@@ -535,7 +535,7 @@ static void setup_ports(struct domain *d, unsigned int prev_evtchns)
         evtchn = evtchn_from_port(d, port);
 
         if ( guest_test_bit(d, port, &shared_info(d, evtchn_pending)) )
-            evtchn->pending = 1;
+            evtchn->pending = true;
 
         evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
     }
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index a345cc01f8..7afbae7dd1 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -93,31 +93,33 @@ struct evtchn
 #define ECS_PIRQ         4 /* Channel is bound to a physical IRQ line.       */
 #define ECS_VIRQ         5 /* Channel is bound to a virtual IRQ line.        */
 #define ECS_IPI          6 /* Channel is bound to a virtual IPI line.        */
-    u8  state;             /* ECS_* */
-    u8  xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if nonzero */
-    u8  pending:1;
-    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
-    u32 port;
+    unsigned char state;   /* ECS_* */
+#ifndef NDEBUG
+    unsigned char old_state; /* State when taking lock in write mode. */
+#endif
+    unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */
+    unsigned int port;
     union {
         struct {
             domid_t remote_domid;
-        } unbound;     /* state == ECS_UNBOUND */
+        } unbound;          /* state == ECS_UNBOUND */
         struct {
             evtchn_port_t  remote_port;
             struct domain *remote_dom;
-        } interdomain; /* state == ECS_INTERDOMAIN */
+        } interdomain;      /* state == ECS_INTERDOMAIN */
         struct {
-            u32            irq;
+            unsigned int   irq;
             evtchn_port_t  next_port;
             evtchn_port_t  prev_port;
-        } pirq;        /* state == ECS_PIRQ */
-        u16 virq;      /* state == ECS_VIRQ */
+        } pirq;             /* state == ECS_PIRQ */
+        unsigned int virq;  /* state == ECS_VIRQ */
     } u;
-    u8 priority;
-#ifndef NDEBUG
-    u8 old_state;      /* State when taking lock in write mode. */
-#endif
-    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
+
+    bool pending;                  /* FIFO event channels only. */
+    unsigned char priority;        /* FIFO event channels only. */
+    unsigned short notify_vcpu_id; /* VCPU for local delivery notification */
+    uint32_t fifo_lastq;           /* Data for identifying last queue. */
+
 #ifdef CONFIG_XSM
     union {
 #ifdef XSM_NEED_GENERIC_EVTCHN_SSID
@@ -133,7 +135,7 @@ struct evtchn
          * allocations, and on 64-bit platforms with only FLASK enabled,
          * reduces the size of struct evtchn.
          */
-        u32 flask_sid;
+        uint32_t flask_sid;
 #endif
     } ssid;
 #endif
-- 
2.26.2

Re: [PATCH v8 1/3] xen/events: modify struct evtchn layout

[Jan Beulich]

On 25.11.2020 11:51, Juergen Gross wrote:
> In order to avoid latent races when updating an event channel put
> xen_consumer and pending fields in different bytes. This is no problem
> right now, but especially the pending indicator isn't used only when
> initializing an event channel (unlike xen_consumer), so any future
> addition to this byte would need to be done with a potential race kept
> in mind.
> 
> At the same time move some other fields around to have less implicit
> paddings and to keep related fields more closely together.
> 
> Finally switch struct evtchn to no longer use fixed sized types where
> not needed.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one more adjustment (can be done while committing, I guess):

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -93,31 +93,33 @@ struct evtchn
>  #define ECS_PIRQ         4 /* Channel is bound to a physical IRQ line.       */
>  #define ECS_VIRQ         5 /* Channel is bound to a virtual IRQ line.        */
>  #define ECS_IPI          6 /* Channel is bound to a virtual IPI line.        */
> -    u8  state;             /* ECS_* */
> -    u8  xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if nonzero */
> -    u8  pending:1;
> -    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
> -    u32 port;
> +    unsigned char state;   /* ECS_* */
> +#ifndef NDEBUG
> +    unsigned char old_state; /* State when taking lock in write mode. */
> +#endif
> +    unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */
> +    unsigned int port;

evtchn_port_t, to be in line with ...

>      union {
>          struct {
>              domid_t remote_domid;
> -        } unbound;     /* state == ECS_UNBOUND */
> +        } unbound;          /* state == ECS_UNBOUND */
>          struct {
>              evtchn_port_t  remote_port;
>              struct domain *remote_dom;
> -        } interdomain; /* state == ECS_INTERDOMAIN */
> +        } interdomain;      /* state == ECS_INTERDOMAIN */
>          struct {
> -            u32            irq;
> +            unsigned int   irq;
>              evtchn_port_t  next_port;
>              evtchn_port_t  prev_port;

... three of the fields above from here.

> -        } pirq;        /* state == ECS_PIRQ */
> -        u16 virq;      /* state == ECS_VIRQ */
> +        } pirq;             /* state == ECS_PIRQ */
> +        unsigned int virq;  /* state == ECS_VIRQ */
>      } u;
> -    u8 priority;
> -#ifndef NDEBUG
> -    u8 old_state;      /* State when taking lock in write mode. */
> -#endif
> -    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
> +
> +    bool pending;                  /* FIFO event channels only. */
> +    unsigned char priority;        /* FIFO event channels only. */
> +    unsigned short notify_vcpu_id; /* VCPU for local delivery notification */

I have to admit though that I'm not fully happy with the uses of
"unsigned char" and "unsigned short". Yes, I did ask for this
change (based on ./CODING_STYLE), but I did also hint towards the
use of bitfields. If bitfields aren't an option here to achieve
the desired dense packing, perhaps this desire should be permitted
as another reason to use fixed width types. (Question goes more
towards everyone who cares than to you specifically.)

Otoh, as long as we have the odd alignment attribute on the struct,
packing density doesn't really matter all this much. I was more
hoping for this change to be a step towards us dropping that
attribute.

Jan

Re: [PATCH v8 1/3] xen/events: modify struct evtchn layout

[Julien Grall]

Hi Jan,

On 27/11/2020 11:42, Jan Beulich wrote:
> On 25.11.2020 11:51, Juergen Gross wrote:
>> In order to avoid latent races when updating an event channel put
>> xen_consumer and pending fields in different bytes. This is no problem
>> right now, but especially the pending indicator isn't used only when
>> initializing an event channel (unlike xen_consumer), so any future
>> addition to this byte would need to be done with a potential race kept
>> in mind.
>>
>> At the same time move some other fields around to have less implicit
>> paddings and to keep related fields more closely together.
>>
>> Finally switch struct evtchn to no longer use fixed sized types where
>> not needed.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one more adjustment (can be done while committing, I guess):
> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -93,31 +93,33 @@ struct evtchn
>>   #define ECS_PIRQ         4 /* Channel is bound to a physical IRQ line.       */
>>   #define ECS_VIRQ         5 /* Channel is bound to a virtual IRQ line.        */
>>   #define ECS_IPI          6 /* Channel is bound to a virtual IPI line.        */
>> -    u8  state;             /* ECS_* */
>> -    u8  xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if nonzero */
>> -    u8  pending:1;
>> -    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
>> -    u32 port;
>> +    unsigned char state;   /* ECS_* */
>> +#ifndef NDEBUG
>> +    unsigned char old_state; /* State when taking lock in write mode. */
>> +#endif
>> +    unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */
>> +    unsigned int port;
> 
> evtchn_port_t, to be in line with ...
> 
>>       union {
>>           struct {
>>               domid_t remote_domid;
>> -        } unbound;     /* state == ECS_UNBOUND */
>> +        } unbound;          /* state == ECS_UNBOUND */
>>           struct {
>>               evtchn_port_t  remote_port;
>>               struct domain *remote_dom;
>> -        } interdomain; /* state == ECS_INTERDOMAIN */
>> +        } interdomain;      /* state == ECS_INTERDOMAIN */
>>           struct {
>> -            u32            irq;
>> +            unsigned int   irq;
>>               evtchn_port_t  next_port;
>>               evtchn_port_t  prev_port;
> 
> ... three of the fields above from here.
> 
>> -        } pirq;        /* state == ECS_PIRQ */
>> -        u16 virq;      /* state == ECS_VIRQ */
>> +        } pirq;             /* state == ECS_PIRQ */
>> +        unsigned int virq;  /* state == ECS_VIRQ */
>>       } u;
>> -    u8 priority;
>> -#ifndef NDEBUG
>> -    u8 old_state;      /* State when taking lock in write mode. */
>> -#endif
>> -    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
>> +
>> +    bool pending;                  /* FIFO event channels only. */
>> +    unsigned char priority;        /* FIFO event channels only. */
>> +    unsigned short notify_vcpu_id; /* VCPU for local delivery notification */
> 
> I have to admit though that I'm not fully happy with the uses of
> "unsigned char" and "unsigned short". Yes, I did ask for this
> change (based on ./CODING_STYLE), but I did also hint towards the
> use of bitfields. If bitfields aren't an option here to achieve
> the desired dense packing, perhaps this desire should be permitted
> as another reason to use fixed width types. (Question goes more
> towards everyone who cares than to you specifically.)

I think uint*_t would make sense here because they are storing 
information received from an hypercall (all the fields should be fixed 
size there).

But I am also fine the current patch as it is still readable.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 1/3] xen/events: modify struct evtchn layout

[Jan Beulich]

On 27.11.2020 12:57, Julien Grall wrote:
> On 27/11/2020 11:42, Jan Beulich wrote:
>> I have to admit though that I'm not fully happy with the uses of
>> "unsigned char" and "unsigned short". Yes, I did ask for this
>> change (based on ./CODING_STYLE), but I did also hint towards the
>> use of bitfields. If bitfields aren't an option here to achieve
>> the desired dense packing, perhaps this desire should be permitted
>> as another reason to use fixed width types. (Question goes more
>> towards everyone who cares than to you specifically.)
> 
> I think uint*_t would make sense here because they are storing 
> information received from an hypercall (all the fields should be fixed 
> size there).

"storing information received from a hypercall" is specifically
not a reason to use fixed width types, imo. All of uint8_t,
uint16_t, and uint32_t values coming from hypercalls are fine to
be passed around and stored as unsigned int, just as an example.
It is solely the packing aspect which might matter here.

> But I am also fine the current patch as it is still readable.

Good, thanks for checking.

Jan

[PATCH v8 2/3] xen/events: rework fifo queue locking

[Juergen Gross]

Two cpus entering evtchn_fifo_set_pending() for the same event channel
can race in case the first one gets interrupted after setting
EVTCHN_FIFO_PENDING and when the other one manages to set
EVTCHN_FIFO_LINKED before the first one is testing that bit. This can
lead to evtchn_check_pollers() being called before the event is put
properly into the queue, resulting eventually in the guest not seeing
the event pending and thus blocking forever afterwards.

Note that commit 5f2df45ead7c1195 ("xen/evtchn: rework per event channel
lock") made the race just more obvious, while the fifo event channel
implementation had this race from the beginning when an unmask operation
was running in parallel with an event channel send operation.

Using a spinlock for the per event channel lock is problematic due to
some paths needing to take the lock are called with interrupts off, so
the lock would need to disable interrupts, which in turn breaks some
use cases related to vm events.

For avoiding this race the queue locking in evtchn_fifo_set_pending()
needs to be reworked to cover the test of EVTCHN_FIFO_PENDING,
EVTCHN_FIFO_MASKED and EVTCHN_FIFO_LINKED, too. Additionally when an
event channel needs to change queues both queues need to be locked
initially.

Reported-by: Jan Beulich <jbeulich@suse.com>
Fixes: 5f2df45ead7c1195 ("xen/evtchn: rework per event channel lock")
Fixes: de6acb78bf0e137c ("evtchn: use a per-event channel lock for sending events")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V7:
- new patch

V8:
- update commit message (Jan Beulich)
- update double locking (Jan Beulich)
- add comment (Jan Beulich)
- fix handling when not getting lock (Jan Beulich)
- add const (Jan Beulich)

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/event_fifo.c | 128 ++++++++++++++++++++++------------------
 1 file changed, 70 insertions(+), 58 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index f39e61105f..443593c3b3 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -87,38 +87,6 @@ static void evtchn_fifo_init(struct domain *d, struct evtchn *evtchn)
                  d->domain_id, evtchn->port);
 }
 
-static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
-                                                struct evtchn *evtchn,
-                                                unsigned long *flags)
-{
-    struct vcpu *v;
-    struct evtchn_fifo_queue *q, *old_q;
-    unsigned int try;
-    union evtchn_fifo_lastq lastq;
-
-    for ( try = 0; try < 3; try++ )
-    {
-        lastq.raw = read_atomic(&evtchn->fifo_lastq);
-        v = d->vcpu[lastq.last_vcpu_id];
-        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
-
-        spin_lock_irqsave(&old_q->lock, *flags);
-
-        v = d->vcpu[lastq.last_vcpu_id];
-        q = &v->evtchn_fifo->queue[lastq.last_priority];
-
-        if ( old_q == q )
-            return old_q;
-
-        spin_unlock_irqrestore(&old_q->lock, *flags);
-    }
-
-    gprintk(XENLOG_WARNING,
-            "dom%d port %d lost event (too many queue changes)\n",
-            d->domain_id, evtchn->port);
-    return NULL;
-}          
-
 static int try_set_link(event_word_t *word, event_word_t *w, uint32_t link)
 {
     event_word_t new, old;
@@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
     event_word_t *word;
     unsigned long flags;
     bool_t was_pending;
+    struct evtchn_fifo_queue *q, *old_q;
+    unsigned int try;
+    bool linked = true;
 
     port = evtchn->port;
     word = evtchn_fifo_word_from_port(d, port);
@@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         return;
     }
 
+    /*
+     * Lock all queues related to the event channel (in case of a queue change
+     * this might be two).
+     * It is mandatory to do that before setting and testing the PENDING bit
+     * and to hold the current queue lock until the event has put into the
+     * list of pending events in order to avoid waking up a guest without the
+     * event being visibly pending in the guest.
+     */
+    for ( try = 0; try < 4; try++ )
+    {
+        union evtchn_fifo_lastq lastq;
+        const struct vcpu *old_v;
+
+        lastq.raw = read_atomic(&evtchn->fifo_lastq);
+        old_v = d->vcpu[lastq.last_vcpu_id];
+
+        q = &v->evtchn_fifo->queue[evtchn->priority];
+        old_q = &old_v->evtchn_fifo->queue[lastq.last_priority];
+
+        if ( q == old_q )
+            spin_lock_irqsave(&q->lock, flags);
+        else if ( q < old_q )
+        {
+            spin_lock_irqsave(&q->lock, flags);
+            spin_lock(&old_q->lock);
+        }
+        else
+        {
+            spin_lock_irqsave(&old_q->lock, flags);
+            spin_lock(&q->lock);
+        }
+
+        lastq.raw = read_atomic(&evtchn->fifo_lastq);
+        old_v = d->vcpu[lastq.last_vcpu_id];
+        if ( q == &v->evtchn_fifo->queue[evtchn->priority] &&
+             old_q == &old_v->evtchn_fifo->queue[lastq.last_priority] )
+            break;
+
+        if ( q != old_q )
+            spin_unlock(&old_q->lock);
+        spin_unlock_irqrestore(&q->lock, flags);
+    }
+
     was_pending = guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
 
+    /* If we didn't get the lock bail out. */
+    if ( try == 4 )
+    {
+        gprintk(XENLOG_WARNING,
+                "dom%d port %d lost event (too many queue changes)\n",
+                d->domain_id, evtchn->port);
+        goto done;
+    }
+
     /*
      * Link the event if it unmasked and not already linked.
      */
     if ( !guest_test_bit(d, EVTCHN_FIFO_MASKED, word) &&
          !guest_test_bit(d, EVTCHN_FIFO_LINKED, word) )
     {
-        struct evtchn_fifo_queue *q, *old_q;
         event_word_t *tail_word;
-        bool_t linked = 0;
 
         /*
          * Control block not mapped.  The guest must not unmask an
@@ -225,25 +246,11 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         {
             printk(XENLOG_G_WARNING
                    "%pv has no FIFO event channel control block\n", v);
-            goto done;
+            goto unlock;
         }
 
-        /*
-         * No locking around getting the queue. This may race with
-         * changing the priority but we are allowed to signal the
-         * event once on the old priority.
-         */
-        q = &v->evtchn_fifo->queue[evtchn->priority];
-
-        old_q = lock_old_queue(d, evtchn, &flags);
-        if ( !old_q )
-            goto done;
-
         if ( guest_test_and_set_bit(d, EVTCHN_FIFO_LINKED, word) )
-        {
-            spin_unlock_irqrestore(&old_q->lock, flags);
-            goto done;
-        }
+            goto unlock;
 
         /*
          * If this event was a tail, the old queue is now empty and
@@ -262,8 +269,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
             lastq.last_priority = q->priority;
             write_atomic(&evtchn->fifo_lastq, lastq.raw);
 
-            spin_unlock_irqrestore(&old_q->lock, flags);
-            spin_lock_irqsave(&q->lock, flags);
+            spin_unlock(&old_q->lock);
+            old_q = q;
         }
 
         /*
@@ -276,6 +283,7 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
          * If the queue is empty (i.e., we haven't linked to the new
          * event), head must be updated.
          */
+        linked = false;
         if ( q->tail )
         {
             tail_word = evtchn_fifo_word_from_port(d, q->tail);
@@ -284,15 +292,19 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         if ( !linked )
             write_atomic(q->head, port);
         q->tail = port;
+    }
 
-        spin_unlock_irqrestore(&q->lock, flags);
+ unlock:
+    if ( q != old_q )
+        spin_unlock(&old_q->lock);
+    spin_unlock_irqrestore(&q->lock, flags);
 
-        if ( !linked
-             && !guest_test_and_set_bit(d, q->priority,
-                                        &v->evtchn_fifo->control_block->ready) )
-            vcpu_mark_events_pending(v);
-    }
  done:
+    if ( !linked &&
+         !guest_test_and_set_bit(d, q->priority,
+                                 &v->evtchn_fifo->control_block->ready) )
+        vcpu_mark_events_pending(v);
+
     if ( !was_pending )
         evtchn_check_pollers(d, port);
 }
-- 
2.26.2

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jan Beulich]

On 25.11.2020 11:51, Juergen Gross wrote:
> Two cpus entering evtchn_fifo_set_pending() for the same event channel
> can race in case the first one gets interrupted after setting
> EVTCHN_FIFO_PENDING and when the other one manages to set
> EVTCHN_FIFO_LINKED before the first one is testing that bit. This can
> lead to evtchn_check_pollers() being called before the event is put
> properly into the queue, resulting eventually in the guest not seeing
> the event pending and thus blocking forever afterwards.
> 
> Note that commit 5f2df45ead7c1195 ("xen/evtchn: rework per event channel
> lock") made the race just more obvious, while the fifo event channel
> implementation had this race from the beginning when an unmask operation
> was running in parallel with an event channel send operation.

I notice you've altered the Fixes: tag, but you still say "from
the beginning" here.

> Using a spinlock for the per event channel lock is problematic due to
> some paths needing to take the lock are called with interrupts off, so
> the lock would need to disable interrupts, which in turn breaks some
> use cases related to vm events.

This reads as if it got put here by mistake. May I suggest to start
with "Using ... had turned out problematic ..." and then add
something like "Therefore that lock was switched to an rw one"?

> For avoiding this race the queue locking in evtchn_fifo_set_pending()
> needs to be reworked to cover the test of EVTCHN_FIFO_PENDING,
> EVTCHN_FIFO_MASKED and EVTCHN_FIFO_LINKED, too. Additionally when an
> event channel needs to change queues both queues need to be locked
> initially.

"... in order to avoid having a window with no lock held at all"?

> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>          return;
>      }
>  
> +    /*
> +     * Lock all queues related to the event channel (in case of a queue change
> +     * this might be two).
> +     * It is mandatory to do that before setting and testing the PENDING bit
> +     * and to hold the current queue lock until the event has put into the

"has been" or "was" I think?

With adjustments along these lines (which I guess could again be
done while committing) or reasons against supplied
Reviewed-by: Jan Beulich <jbeulich@suse.com>

One aspect which I wonder whether it wants adding to the description
is that this change makes a bad situation worse: Back at the time
per-channel locks were added to avoid the bottleneck of the per-
domain event lock. While a per-queue lock's scope at least isn't the
entire domain, their use on the send path still has been preventing
full parallelism here. And this patch widens the lock holding region.
If at least there was a fast path not requiring any locking ...

Jan

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3268 bytes --]

On 27.11.20 14:11, Jan Beulich wrote:
> On 25.11.2020 11:51, Juergen Gross wrote:
>> Two cpus entering evtchn_fifo_set_pending() for the same event channel
>> can race in case the first one gets interrupted after setting
>> EVTCHN_FIFO_PENDING and when the other one manages to set
>> EVTCHN_FIFO_LINKED before the first one is testing that bit. This can
>> lead to evtchn_check_pollers() being called before the event is put
>> properly into the queue, resulting eventually in the guest not seeing
>> the event pending and thus blocking forever afterwards.
>>
>> Note that commit 5f2df45ead7c1195 ("xen/evtchn: rework per event channel
>> lock") made the race just more obvious, while the fifo event channel
>> implementation had this race from the beginning when an unmask operation
>> was running in parallel with an event channel send operation.
> 
> I notice you've altered the Fixes: tag, but you still say "from
> the beginning" here.

Oh, indeed. Thanks for spotting.

> 
>> Using a spinlock for the per event channel lock is problematic due to
>> some paths needing to take the lock are called with interrupts off, so
>> the lock would need to disable interrupts, which in turn breaks some
>> use cases related to vm events.
> 
> This reads as if it got put here by mistake. May I suggest to start
> with "Using ... had turned out problematic ..." and then add
> something like "Therefore that lock was switched to an rw one"?

Fine with me.

> 
>> For avoiding this race the queue locking in evtchn_fifo_set_pending()
>> needs to be reworked to cover the test of EVTCHN_FIFO_PENDING,
>> EVTCHN_FIFO_MASKED and EVTCHN_FIFO_LINKED, too. Additionally when an
>> event channel needs to change queues both queues need to be locked
>> initially.
> 
> "... in order to avoid having a window with no lock held at all"?

Yes, this is better.

> 
>> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>>           return;
>>       }
>>   
>> +    /*
>> +     * Lock all queues related to the event channel (in case of a queue change
>> +     * this might be two).
>> +     * It is mandatory to do that before setting and testing the PENDING bit
>> +     * and to hold the current queue lock until the event has put into the
> 
> "has been" or "was" I think?

"has been".

> 
> With adjustments along these lines (which I guess could again be
> done while committing) or reasons against supplied
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> 
> One aspect which I wonder whether it wants adding to the description
> is that this change makes a bad situation worse: Back at the time
> per-channel locks were added to avoid the bottleneck of the per-
> domain event lock. While a per-queue lock's scope at least isn't the
> entire domain, their use on the send path still has been preventing
> full parallelism here. And this patch widens the lock holding region.

As the description already says that additional operations are to be
guarded by the lock I think it is rather clear that the lock holding
region is being widened.

OTOH I wouldn't reject such an addition to the commit message if you
think it is required.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Julien Grall]

Hi Juergen,

On 25/11/2020 10:51, Juergen Gross wrote:
> -static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
> -                                                struct evtchn *evtchn,
> -                                                unsigned long *flags)
> -{
> -    struct vcpu *v;
> -    struct evtchn_fifo_queue *q, *old_q;
> -    unsigned int try;
> -    union evtchn_fifo_lastq lastq;
> -
> -    for ( try = 0; try < 3; try++ )
> -    {
> -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
> -        v = d->vcpu[lastq.last_vcpu_id];
> -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
> -
> -        spin_lock_irqsave(&old_q->lock, *flags);
> -
> -        v = d->vcpu[lastq.last_vcpu_id];
> -        q = &v->evtchn_fifo->queue[lastq.last_priority];
> -
> -        if ( old_q == q )
> -            return old_q;
> -
> -        spin_unlock_irqrestore(&old_q->lock, *flags);
> -    }
> -
> -    gprintk(XENLOG_WARNING,
> -            "dom%d port %d lost event (too many queue changes)\n",
> -            d->domain_id, evtchn->port);
> -    return NULL;
> -}
> -
>   static int try_set_link(event_word_t *word, event_word_t *w, uint32_t link)
>   {
>       event_word_t new, old;
> @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>       event_word_t *word;
>       unsigned long flags;
>       bool_t was_pending;
> +    struct evtchn_fifo_queue *q, *old_q;
> +    unsigned int try;
> +    bool linked = true;
>   
>       port = evtchn->port;
>       word = evtchn_fifo_word_from_port(d, port);
> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>           return;
>       }
>   
> +    /*
> +     * Lock all queues related to the event channel (in case of a queue change
> +     * this might be two).
> +     * It is mandatory to do that before setting and testing the PENDING bit
> +     * and to hold the current queue lock until the event has put into the
> +     * list of pending events in order to avoid waking up a guest without the
> +     * event being visibly pending in the guest.
> +     */
> +    for ( try = 0; try < 4; try++ )

May I ask why the number of try is 4 rather than the original 3?

> +    {
> +        union evtchn_fifo_lastq lastq;
> +        const struct vcpu *old_v;
> +
> +        lastq.raw = read_atomic(&evtchn->fifo_lastq);
> +        old_v = d->vcpu[lastq.last_vcpu_id];
> +
> +        q = &v->evtchn_fifo->queue[evtchn->priority];
> +        old_q = &old_v->evtchn_fifo->queue[lastq.last_priority];
> +
> +        if ( q == old_q )
> +            spin_lock_irqsave(&q->lock, flags);
> +        else if ( q < old_q )
> +        {
> +            spin_lock_irqsave(&q->lock, flags);
> +            spin_lock(&old_q->lock);
> +        }
> +        else
> +        {
> +            spin_lock_irqsave(&old_q->lock, flags);
> +            spin_lock(&q->lock);
> +        }
> +
> +        lastq.raw = read_atomic(&evtchn->fifo_lastq);
> +        old_v = d->vcpu[lastq.last_vcpu_id];
> +        if ( q == &v->evtchn_fifo->queue[evtchn->priority] &&
> +             old_q == &old_v->evtchn_fifo->queue[lastq.last_priority] )
> +            break;
> +
> +        if ( q != old_q )
> +            spin_unlock(&old_q->lock);
> +        spin_unlock_irqrestore(&q->lock, flags);
> +    }
> +
>       was_pending = guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
>   
> +    /* If we didn't get the lock bail out. */
> +    if ( try == 4 )
> +    {
> +        gprintk(XENLOG_WARNING,
> +                "dom%d port %d lost event (too many queue changes)\n",
> +                d->domain_id, evtchn->port);

NIT: You can use %pd use in place of dom%d.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jan Beulich]

On 27.11.2020 14:58, Julien Grall wrote:
> On 25/11/2020 10:51, Juergen Gross wrote:
>> +    /* If we didn't get the lock bail out. */
>> +    if ( try == 4 )
>> +    {
>> +        gprintk(XENLOG_WARNING,
>> +                "dom%d port %d lost event (too many queue changes)\n",
>> +                d->domain_id, evtchn->port);
> 
> NIT: You can use %pd use in place of dom%d.

Oh, indeed - not just can, but imo really should. I'll record this
for on-commit adjustment, unless a v9 becomes necessary anyway.

Jan

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4695 bytes --]

On 27.11.20 14:58, Julien Grall wrote:
> Hi Juergen,
> 
> On 25/11/2020 10:51, Juergen Gross wrote:
>> -static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
>> -                                                struct evtchn *evtchn,
>> -                                                unsigned long *flags)
>> -{
>> -    struct vcpu *v;
>> -    struct evtchn_fifo_queue *q, *old_q;
>> -    unsigned int try;
>> -    union evtchn_fifo_lastq lastq;
>> -
>> -    for ( try = 0; try < 3; try++ )
>> -    {
>> -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>> -        v = d->vcpu[lastq.last_vcpu_id];
>> -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
>> -
>> -        spin_lock_irqsave(&old_q->lock, *flags);
>> -
>> -        v = d->vcpu[lastq.last_vcpu_id];
>> -        q = &v->evtchn_fifo->queue[lastq.last_priority];
>> -
>> -        if ( old_q == q )
>> -            return old_q;
>> -
>> -        spin_unlock_irqrestore(&old_q->lock, *flags);
>> -    }
>> -
>> -    gprintk(XENLOG_WARNING,
>> -            "dom%d port %d lost event (too many queue changes)\n",
>> -            d->domain_id, evtchn->port);
>> -    return NULL;
>> -}
>> -
>>   static int try_set_link(event_word_t *word, event_word_t *w, 
>> uint32_t link)
>>   {
>>       event_word_t new, old;
>> @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu 
>> *v, struct evtchn *evtchn)
>>       event_word_t *word;
>>       unsigned long flags;
>>       bool_t was_pending;
>> +    struct evtchn_fifo_queue *q, *old_q;
>> +    unsigned int try;
>> +    bool linked = true;
>>       port = evtchn->port;
>>       word = evtchn_fifo_word_from_port(d, port);
>> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu 
>> *v, struct evtchn *evtchn)
>>           return;
>>       }
>> +    /*
>> +     * Lock all queues related to the event channel (in case of a 
>> queue change
>> +     * this might be two).
>> +     * It is mandatory to do that before setting and testing the 
>> PENDING bit
>> +     * and to hold the current queue lock until the event has put 
>> into the
>> +     * list of pending events in order to avoid waking up a guest 
>> without the
>> +     * event being visibly pending in the guest.
>> +     */
>> +    for ( try = 0; try < 4; try++ )
> 
> May I ask why the number of try is 4 rather than the original 3?

Oh, I think this is just a typo. OTOH it doesn't really matter.

> 
>> +    {
>> +        union evtchn_fifo_lastq lastq;
>> +        const struct vcpu *old_v;
>> +
>> +        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>> +        old_v = d->vcpu[lastq.last_vcpu_id];
>> +
>> +        q = &v->evtchn_fifo->queue[evtchn->priority];
>> +        old_q = &old_v->evtchn_fifo->queue[lastq.last_priority];
>> +
>> +        if ( q == old_q )
>> +            spin_lock_irqsave(&q->lock, flags);
>> +        else if ( q < old_q )
>> +        {
>> +            spin_lock_irqsave(&q->lock, flags);
>> +            spin_lock(&old_q->lock);
>> +        }
>> +        else
>> +        {
>> +            spin_lock_irqsave(&old_q->lock, flags);
>> +            spin_lock(&q->lock);
>> +        }
>> +
>> +        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>> +        old_v = d->vcpu[lastq.last_vcpu_id];
>> +        if ( q == &v->evtchn_fifo->queue[evtchn->priority] &&
>> +             old_q == &old_v->evtchn_fifo->queue[lastq.last_priority] )
>> +            break;
>> +
>> +        if ( q != old_q )
>> +            spin_unlock(&old_q->lock);
>> +        spin_unlock_irqrestore(&q->lock, flags);
>> +    }
>> +
>>       was_pending = guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
>> +    /* If we didn't get the lock bail out. */
>> +    if ( try == 4 )
>> +    {
>> +        gprintk(XENLOG_WARNING,
>> +                "dom%d port %d lost event (too many queue changes)\n",
>> +                d->domain_id, evtchn->port);
> 
> NIT: You can use %pd use in place of dom%d.

Yes, indeed. This was just moved around. :-)


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Julien Grall]

Hi Juergen,

On 27/11/2020 14:05, Jürgen Groß wrote:
> On 27.11.20 14:58, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 25/11/2020 10:51, Juergen Gross wrote:
>>> -static struct evtchn_fifo_queue *lock_old_queue(const struct domain *d,
>>> -                                                struct evtchn *evtchn,
>>> -                                                unsigned long *flags)
>>> -{
>>> -    struct vcpu *v;
>>> -    struct evtchn_fifo_queue *q, *old_q;
>>> -    unsigned int try;
>>> -    union evtchn_fifo_lastq lastq;
>>> -
>>> -    for ( try = 0; try < 3; try++ )
>>> -    {
>>> -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>> -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
>>> -
>>> -        spin_lock_irqsave(&old_q->lock, *flags);
>>> -
>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>> -        q = &v->evtchn_fifo->queue[lastq.last_priority];
>>> -
>>> -        if ( old_q == q )
>>> -            return old_q;
>>> -
>>> -        spin_unlock_irqrestore(&old_q->lock, *flags);
>>> -    }
>>> -
>>> -    gprintk(XENLOG_WARNING,
>>> -            "dom%d port %d lost event (too many queue changes)\n",
>>> -            d->domain_id, evtchn->port);
>>> -    return NULL;
>>> -}
>>> -
>>>   static int try_set_link(event_word_t *word, event_word_t *w, 
>>> uint32_t link)
>>>   {
>>>       event_word_t new, old;
>>> @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>> *v, struct evtchn *evtchn)
>>>       event_word_t *word;
>>>       unsigned long flags;
>>>       bool_t was_pending;
>>> +    struct evtchn_fifo_queue *q, *old_q;
>>> +    unsigned int try;
>>> +    bool linked = true;
>>>       port = evtchn->port;
>>>       word = evtchn_fifo_word_from_port(d, port);
>>> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>> *v, struct evtchn *evtchn)
>>>           return;
>>>       }
>>> +    /*
>>> +     * Lock all queues related to the event channel (in case of a 
>>> queue change
>>> +     * this might be two).
>>> +     * It is mandatory to do that before setting and testing the 
>>> PENDING bit
>>> +     * and to hold the current queue lock until the event has put 
>>> into the
>>> +     * list of pending events in order to avoid waking up a guest 
>>> without the
>>> +     * event being visibly pending in the guest.
>>> +     */
>>> +    for ( try = 0; try < 4; try++ )
>>
>> May I ask why the number of try is 4 rather than the original 3?
> 
> Oh, I think this is just a typo. OTOH it doesn't really matter.

I agree that the number of try was likely random and therefore using a 
different number should not matter.

However, this is making more difficult to review the patch because this 
is an unexplained change.

I would prefer if this is dropped. But if you want to keep this change, 
then it should be explained in the commit message.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3596 bytes --]

On 27.11.20 15:11, Julien Grall wrote:
> Hi Juergen,
> 
> On 27/11/2020 14:05, Jürgen Groß wrote:
>> On 27.11.20 14:58, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 25/11/2020 10:51, Juergen Gross wrote:
>>>> -static struct evtchn_fifo_queue *lock_old_queue(const struct domain 
>>>> *d,
>>>> -                                                struct evtchn *evtchn,
>>>> -                                                unsigned long *flags)
>>>> -{
>>>> -    struct vcpu *v;
>>>> -    struct evtchn_fifo_queue *q, *old_q;
>>>> -    unsigned int try;
>>>> -    union evtchn_fifo_lastq lastq;
>>>> -
>>>> -    for ( try = 0; try < 3; try++ )
>>>> -    {
>>>> -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>>> -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
>>>> -
>>>> -        spin_lock_irqsave(&old_q->lock, *flags);
>>>> -
>>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>>> -        q = &v->evtchn_fifo->queue[lastq.last_priority];
>>>> -
>>>> -        if ( old_q == q )
>>>> -            return old_q;
>>>> -
>>>> -        spin_unlock_irqrestore(&old_q->lock, *flags);
>>>> -    }
>>>> -
>>>> -    gprintk(XENLOG_WARNING,
>>>> -            "dom%d port %d lost event (too many queue changes)\n",
>>>> -            d->domain_id, evtchn->port);
>>>> -    return NULL;
>>>> -}
>>>> -
>>>>   static int try_set_link(event_word_t *word, event_word_t *w, 
>>>> uint32_t link)
>>>>   {
>>>>       event_word_t new, old;
>>>> @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>>> *v, struct evtchn *evtchn)
>>>>       event_word_t *word;
>>>>       unsigned long flags;
>>>>       bool_t was_pending;
>>>> +    struct evtchn_fifo_queue *q, *old_q;
>>>> +    unsigned int try;
>>>> +    bool linked = true;
>>>>       port = evtchn->port;
>>>>       word = evtchn_fifo_word_from_port(d, port);
>>>> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct 
>>>> vcpu *v, struct evtchn *evtchn)
>>>>           return;
>>>>       }
>>>> +    /*
>>>> +     * Lock all queues related to the event channel (in case of a 
>>>> queue change
>>>> +     * this might be two).
>>>> +     * It is mandatory to do that before setting and testing the 
>>>> PENDING bit
>>>> +     * and to hold the current queue lock until the event has put 
>>>> into the
>>>> +     * list of pending events in order to avoid waking up a guest 
>>>> without the
>>>> +     * event being visibly pending in the guest.
>>>> +     */
>>>> +    for ( try = 0; try < 4; try++ )
>>>
>>> May I ask why the number of try is 4 rather than the original 3?
>>
>> Oh, I think this is just a typo. OTOH it doesn't really matter.
> 
> I agree that the number of try was likely random and therefore using a 
> different number should not matter.
> 
> However, this is making more difficult to review the patch because this 
> is an unexplained change.
> 
> I would prefer if this is dropped. But if you want to keep this change, 
> then it should be explained in the commit message.

Well, I could argue that there is potentially one lock more to take, so
the retry number is increased by one, too. ;-)

I think we can just switch back to 3.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH v8 2/3] xen/events: rework fifo queue locking

[Julien Grall]

On 27/11/2020 14:14, Jürgen Groß wrote:
> On 27.11.20 15:11, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 27/11/2020 14:05, Jürgen Groß wrote:
>>> On 27.11.20 14:58, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 25/11/2020 10:51, Juergen Gross wrote:
>>>>> -static struct evtchn_fifo_queue *lock_old_queue(const struct 
>>>>> domain *d,
>>>>> -                                                struct evtchn 
>>>>> *evtchn,
>>>>> -                                                unsigned long *flags)
>>>>> -{
>>>>> -    struct vcpu *v;
>>>>> -    struct evtchn_fifo_queue *q, *old_q;
>>>>> -    unsigned int try;
>>>>> -    union evtchn_fifo_lastq lastq;
>>>>> -
>>>>> -    for ( try = 0; try < 3; try++ )
>>>>> -    {
>>>>> -        lastq.raw = read_atomic(&evtchn->fifo_lastq);
>>>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>>>> -        old_q = &v->evtchn_fifo->queue[lastq.last_priority];
>>>>> -
>>>>> -        spin_lock_irqsave(&old_q->lock, *flags);
>>>>> -
>>>>> -        v = d->vcpu[lastq.last_vcpu_id];
>>>>> -        q = &v->evtchn_fifo->queue[lastq.last_priority];
>>>>> -
>>>>> -        if ( old_q == q )
>>>>> -            return old_q;
>>>>> -
>>>>> -        spin_unlock_irqrestore(&old_q->lock, *flags);
>>>>> -    }
>>>>> -
>>>>> -    gprintk(XENLOG_WARNING,
>>>>> -            "dom%d port %d lost event (too many queue changes)\n",
>>>>> -            d->domain_id, evtchn->port);
>>>>> -    return NULL;
>>>>> -}
>>>>> -
>>>>>   static int try_set_link(event_word_t *word, event_word_t *w, 
>>>>> uint32_t link)
>>>>>   {
>>>>>       event_word_t new, old;
>>>>> @@ -190,6 +158,9 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>>>> *v, struct evtchn *evtchn)
>>>>>       event_word_t *word;
>>>>>       unsigned long flags;
>>>>>       bool_t was_pending;
>>>>> +    struct evtchn_fifo_queue *q, *old_q;
>>>>> +    unsigned int try;
>>>>> +    bool linked = true;
>>>>>       port = evtchn->port;
>>>>>       word = evtchn_fifo_word_from_port(d, port);
>>>>> @@ -204,17 +175,67 @@ static void evtchn_fifo_set_pending(struct 
>>>>> vcpu *v, struct evtchn *evtchn)
>>>>>           return;
>>>>>       }
>>>>> +    /*
>>>>> +     * Lock all queues related to the event channel (in case of a 
>>>>> queue change
>>>>> +     * this might be two).
>>>>> +     * It is mandatory to do that before setting and testing the 
>>>>> PENDING bit
>>>>> +     * and to hold the current queue lock until the event has put 
>>>>> into the
>>>>> +     * list of pending events in order to avoid waking up a guest 
>>>>> without the
>>>>> +     * event being visibly pending in the guest.
>>>>> +     */
>>>>> +    for ( try = 0; try < 4; try++ )
>>>>
>>>> May I ask why the number of try is 4 rather than the original 3?
>>>
>>> Oh, I think this is just a typo. OTOH it doesn't really matter.
>>
>> I agree that the number of try was likely random and therefore using a 
>> different number should not matter.
>>
>> However, this is making more difficult to review the patch because 
>> this is an unexplained change.
>>
>> I would prefer if this is dropped. But if you want to keep this 
>> change, then it should be explained in the commit message.
> 
> Well, I could argue that there is potentially one lock more to take, so
> the retry number is increased by one, too. ;-)

I will not argue against switching 4 :). I care more about explaining 
what we do because this is really frustrating to read some of the older 
commit where there are not much rationale (I probably wrote some).

Cheers,

-- 
Julien Grall

[PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Juergen Gross]

evtchn_fifo_set_pending() can be simplified a little bit.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
---
 xen/common/event_fifo.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index 443593c3b3..77609539b1 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         return;
     }
 
+    /*
+     * Control block not mapped.  The guest must not unmask an
+     * event until the control block is initialized, so we can
+     * just drop the event.
+     */
+    if ( unlikely(!v->evtchn_fifo->control_block) )
+    {
+        printk(XENLOG_G_WARNING
+               "%pv has no FIFO event channel control block\n", v);
+        return;
+    }
+
     /*
      * Lock all queues related to the event channel (in case of a queue change
      * this might be two).
@@ -233,25 +245,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
      * Link the event if it unmasked and not already linked.
      */
     if ( !guest_test_bit(d, EVTCHN_FIFO_MASKED, word) &&
-         !guest_test_bit(d, EVTCHN_FIFO_LINKED, word) )
+         !guest_test_and_set_bit(d, EVTCHN_FIFO_LINKED, word) )
     {
-        event_word_t *tail_word;
-
-        /*
-         * Control block not mapped.  The guest must not unmask an
-         * event until the control block is initialized, so we can
-         * just drop the event.
-         */
-        if ( unlikely(!v->evtchn_fifo->control_block) )
-        {
-            printk(XENLOG_G_WARNING
-                   "%pv has no FIFO event channel control block\n", v);
-            goto unlock;
-        }
-
-        if ( guest_test_and_set_bit(d, EVTCHN_FIFO_LINKED, word) )
-            goto unlock;
-
         /*
          * If this event was a tail, the old queue is now empty and
          * its tail must be invalidated to prevent adding an event to
@@ -286,6 +281,8 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         linked = false;
         if ( q->tail )
         {
+            event_word_t *tail_word;
+
             tail_word = evtchn_fifo_word_from_port(d, q->tail);
             linked = evtchn_fifo_set_link(d, tail_word, port);
         }
@@ -294,7 +291,6 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
         q->tail = port;
     }
 
- unlock:
     if ( q != old_q )
         spin_unlock(&old_q->lock);
     spin_unlock_irqrestore(&q->lock, flags);
-- 
2.26.2

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jan Beulich]

On 25.11.2020 11:51, Juergen Gross wrote:
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>          return;
>      }
>  
> +    /*
> +     * Control block not mapped.  The guest must not unmask an
> +     * event until the control block is initialized, so we can
> +     * just drop the event.
> +     */
> +    if ( unlikely(!v->evtchn_fifo->control_block) )
> +    {
> +        printk(XENLOG_G_WARNING
> +               "%pv has no FIFO event channel control block\n", v);
> +        return;
> +    }

This results in bypassing the setting of PENDING and the possible
call to evtchn_check_pollers(). It may in particular be the case
that a very special purpose guest uses event channels just for
waking up pollers, which - afaict - then doesn't require setting
up a control block. To give an example, I could easily see an XTF
test avoid that step if indeed it's unnecessary.

Jan

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1203 bytes --]

On 27.11.20 14:27, Jan Beulich wrote:
> On 25.11.2020 11:51, Juergen Gross wrote:
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>>           return;
>>       }
>>   
>> +    /*
>> +     * Control block not mapped.  The guest must not unmask an
>> +     * event until the control block is initialized, so we can
>> +     * just drop the event.
>> +     */
>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>> +    {
>> +        printk(XENLOG_G_WARNING
>> +               "%pv has no FIFO event channel control block\n", v);
>> +        return;
>> +    }
> 
> This results in bypassing the setting of PENDING and the possible
> call to evtchn_check_pollers(). It may in particular be the case
> that a very special purpose guest uses event channels just for
> waking up pollers, which - afaict - then doesn't require setting
> up a control block. To give an example, I could easily see an XTF
> test avoid that step if indeed it's unnecessary.

Okay, I can move the test after setting PENDING and do a "goto unlock"
instead of returning.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Julien Grall]

On 25/11/2020 10:51, Juergen Gross wrote:
> evtchn_fifo_set_pending() can be simplified a little bit.

The commit message is quite light... For posterity, it would be good to 
explain why the simplication can be done. In particular, there is a 
chance in behavior after this patch.

> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V8:
> - new patch
> ---
>   xen/common/event_fifo.c | 34 +++++++++++++++-------------------
>   1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index 443593c3b3..77609539b1 100644
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct evtchn *evtchn)
>           return;
>       }
>   
> +    /*
> +     * Control block not mapped.  The guest must not unmask an
> +     * event until the control block is initialized, so we can
> +     * just drop the event.
> +     */
> +    if ( unlikely(!v->evtchn_fifo->control_block) )

Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set 
concurrently to this access.

Thankfully, once the control block is mapped, it can't be unmapped. 
However, there is still a possibility that you may see half of the update.

Shouldn't the field access with ACCESS_ONCE()?

Cheers,

-- 
Julien Grall

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1649 bytes --]

On 27.11.20 15:23, Julien Grall wrote:
> 
> 
> On 25/11/2020 10:51, Juergen Gross wrote:
>> evtchn_fifo_set_pending() can be simplified a little bit.
> 
> The commit message is quite light... For posterity, it would be good to 
> explain why the simplication can be done. In particular, there is a 
> chance in behavior after this patch.
> 
>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V8:
>> - new patch
>> ---
>>   xen/common/event_fifo.c | 34 +++++++++++++++-------------------
>>   1 file changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index 443593c3b3..77609539b1 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu 
>> *v, struct evtchn *evtchn)
>>           return;
>>       }
>> +    /*
>> +     * Control block not mapped.  The guest must not unmask an
>> +     * event until the control block is initialized, so we can
>> +     * just drop the event.
>> +     */
>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
> 
> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set 
> concurrently to this access.
> 
> Thankfully, once the control block is mapped, it can't be unmapped. 
> However, there is still a possibility that you may see half of the update.
> 
> Shouldn't the field access with ACCESS_ONCE()?

Shouldn't this be another patch? Especially as the writing side needs
the same treatment.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

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

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jan Beulich]

On 27.11.2020 15:39, Jürgen Groß wrote:
> On 27.11.20 15:23, Julien Grall wrote:
>> On 25/11/2020 10:51, Juergen Gross wrote:
>>> --- a/xen/common/event_fifo.c
>>> +++ b/xen/common/event_fifo.c
>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>> *v, struct evtchn *evtchn)
>>>           return;
>>>       }
>>> +    /*
>>> +     * Control block not mapped.  The guest must not unmask an
>>> +     * event until the control block is initialized, so we can
>>> +     * just drop the event.
>>> +     */
>>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>>
>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set 
>> concurrently to this access.
>>
>> Thankfully, once the control block is mapped, it can't be unmapped. 
>> However, there is still a possibility that you may see half of the update.
>>
>> Shouldn't the field access with ACCESS_ONCE()?
> 
> Shouldn't this be another patch? Especially as the writing side needs
> the same treatment.

Indeed. As said on several different occasions - our code base is
full of places where we chance torn accesses, if there really was
a compiler to let us down on this. This recurring pattern
shouldn't lead to unrelated patches getting bloated, unless _all_
affected sites get touched anyway.

Jan

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Julien Grall]

On 27/11/2020 14:48, Jan Beulich wrote:
> On 27.11.2020 15:39, Jürgen Groß wrote:
>> On 27.11.20 15:23, Julien Grall wrote:
>>> On 25/11/2020 10:51, Juergen Gross wrote:
>>>> --- a/xen/common/event_fifo.c
>>>> +++ b/xen/common/event_fifo.c
>>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu
>>>> *v, struct evtchn *evtchn)
>>>>            return;
>>>>        }
>>>> +    /*
>>>> +     * Control block not mapped.  The guest must not unmask an
>>>> +     * event until the control block is initialized, so we can
>>>> +     * just drop the event.
>>>> +     */
>>>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>>>
>>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set
>>> concurrently to this access.
>>>
>>> Thankfully, once the control block is mapped, it can't be unmapped.
>>> However, there is still a possibility that you may see half of the update.
>>>
>>> Shouldn't the field access with ACCESS_ONCE()?
>>
>> Shouldn't this be another patch? Especially as the writing side needs
>> the same treatment.
> 
> Indeed. As said on several different occasions - our code base is
> full of places where we chance torn accesses, if there really was
> a compiler to let us down on this.

I am quite amazed you that you managed to test all the version of 
GCC/Clang that were built and confirm this is unlikely to happen :).

> This recurring pattern
> shouldn't lead to unrelated patches getting bloated, unless _all_
> affected sites get touched anyway.

You probably missed the point where I say "sort of unrelated". This 
wasn't not a suggestion to fix it here (I should have been clearer 
though) but instead point out issue as I see them.

Cheers,

-- 
Julien Grall

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Jan Beulich]

On 27.11.2020 16:17, Julien Grall wrote:
> 
> 
> On 27/11/2020 14:48, Jan Beulich wrote:
>> On 27.11.2020 15:39, Jürgen Groß wrote:
>>> On 27.11.20 15:23, Julien Grall wrote:
>>>> On 25/11/2020 10:51, Juergen Gross wrote:
>>>>> --- a/xen/common/event_fifo.c
>>>>> +++ b/xen/common/event_fifo.c
>>>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu
>>>>> *v, struct evtchn *evtchn)
>>>>>            return;
>>>>>        }
>>>>> +    /*
>>>>> +     * Control block not mapped.  The guest must not unmask an
>>>>> +     * event until the control block is initialized, so we can
>>>>> +     * just drop the event.
>>>>> +     */
>>>>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>>>>
>>>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set
>>>> concurrently to this access.
>>>>
>>>> Thankfully, once the control block is mapped, it can't be unmapped.
>>>> However, there is still a possibility that you may see half of the update.
>>>>
>>>> Shouldn't the field access with ACCESS_ONCE()?
>>>
>>> Shouldn't this be another patch? Especially as the writing side needs
>>> the same treatment.
>>
>> Indeed. As said on several different occasions - our code base is
>> full of places where we chance torn accesses, if there really was
>> a compiler to let us down on this.
> 
> I am quite amazed you that you managed to test all the version of 
> GCC/Clang that were built and confirm this is unlikely to happen :).

It's (obviously) not that I tested all of them, but that I know
at least a little bit of how gcc generates code, that I'm unaware
of reports to the contrary, and that it would seem odd for a
compiler to split accesses when they can be done by one insn. Of
course one could build a compiler doing only byte accesses ...

>> This recurring pattern
>> shouldn't lead to unrelated patches getting bloated, unless _all_
>> affected sites get touched anyway.
> 
> You probably missed the point where I say "sort of unrelated". This 
> wasn't not a suggestion to fix it here (I should have been clearer 
> though) but instead point out issue as I see them.

Point taken.

Jan

Re: [PATCH v8 3/3] xen/events: do some cleanups in evtchn_fifo_set_pending()

[Julien Grall]

On 27/11/2020 14:39, Jürgen Groß wrote:
> On 27.11.20 15:23, Julien Grall wrote:
>>
>>
>> On 25/11/2020 10:51, Juergen Gross wrote:
>>> evtchn_fifo_set_pending() can be simplified a little bit.
>>
>> The commit message is quite light... For posterity, it would be good 
>> to explain why the simplication can be done. In particular, there is a 
>> chance in behavior after this patch.
>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V8:
>>> - new patch
>>> ---
>>>   xen/common/event_fifo.c | 34 +++++++++++++++-------------------
>>>   1 file changed, 15 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>>> index 443593c3b3..77609539b1 100644
>>> --- a/xen/common/event_fifo.c
>>> +++ b/xen/common/event_fifo.c
>>> @@ -175,6 +175,18 @@ static void evtchn_fifo_set_pending(struct vcpu 
>>> *v, struct evtchn *evtchn)
>>>           return;
>>>       }
>>> +    /*
>>> +     * Control block not mapped.  The guest must not unmask an
>>> +     * event until the control block is initialized, so we can
>>> +     * just drop the event.
>>> +     */
>>> +    if ( unlikely(!v->evtchn_fifo->control_block) )
>>
>> Sort of unrelated, AFAICT, v->evtchn_fifo->control_block can be set 
>> concurrently to this access.
>>
>> Thankfully, once the control block is mapped, it can't be unmapped. 
>> However, there is still a possibility that you may see half of the 
>> update.
>>
>> Shouldn't the field access with ACCESS_ONCE()?
> 
> Shouldn't this be another patch? Especially as the writing side needs
> the same treatment.

Yes it should. Sorry I should have been clearer.

I am happy to also wrote the patch if you feel you had enough with event 
channel :).

Cheers,

-- 
Julien Grall