Re: [PATCH v4 2/3] xen/events: don't unmask an event channel when an eoi is pending

2021-03-10 Thread Ross Lagerwall 
On 2021-03-09 08:57, Ross Lagerwall wrote:
> On 2021-03-09 05:14, Jürgen Groß wrote:
>> On 08.03.21 21:33, Boris Ostrovsky wrote:
>>>
>>> On 3/6/21 11:18 AM, Juergen Gross wrote:
 An event channel should be kept masked when an eoi is pending for it.
 When being migrated to another cpu it might be unmasked, though.

 In order to avoid this keep three different flags for each event channel
 to be able to distinguish "normal" masking/unmasking from eoi related
 masking/unmasking and temporary masking. The event channel should only
 be able to generate an interrupt if all flags are cleared.

 Cc: sta...@vger.kernel.org
 Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn 
 framework")
 Reported-by: Julien Grall 
 Signed-off-by: Juergen Gross 
 Reviewed-by: Julien Grall 
 ---
 V2:
 - introduce a lock around masking/unmasking
 - merge patch 3 into this one (Jan Beulich)
 V4:
 - don't set eoi masking flag in lateeoi_mask_ack_dynirq()
>>>
>>>
>>> Reviewed-by: Boris Ostrovsky 
>>>
>>>
>>> Ross, are you planning to test this?
>>
>> Just as another data point: With the previous version of the patches
>> a reboot loop of a guest needed max 33 reboots to loose network in
>> my tests (those were IIRC 6 test runs). With this patch version I
>> stopped the test after about 1300 reboots without having seen any
>> problems.
>>
> 
> Thanks, I'll test it today and get back to you.
> 

Tested-by: Ross Lagerwall 

The updated patch seems fine in testing.

Thanks
Ross

Re: [PATCH v4 2/3] xen/events: don't unmask an event channel when an eoi is pending

2021-03-09 Thread Ross Lagerwall 
On 2021-03-09 05:14, Jürgen Groß wrote:
> On 08.03.21 21:33, Boris Ostrovsky wrote:
>>
>> On 3/6/21 11:18 AM, Juergen Gross wrote:
>>> An event channel should be kept masked when an eoi is pending for it.
>>> When being migrated to another cpu it might be unmasked, though.
>>>
>>> In order to avoid this keep three different flags for each event channel
>>> to be able to distinguish "normal" masking/unmasking from eoi related
>>> masking/unmasking and temporary masking. The event channel should only
>>> be able to generate an interrupt if all flags are cleared.
>>>
>>> Cc: sta...@vger.kernel.org
>>> Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn 
>>> framework")
>>> Reported-by: Julien Grall 
>>> Signed-off-by: Juergen Gross 
>>> Reviewed-by: Julien Grall 
>>> ---
>>> V2:
>>> - introduce a lock around masking/unmasking
>>> - merge patch 3 into this one (Jan Beulich)
>>> V4:
>>> - don't set eoi masking flag in lateeoi_mask_ack_dynirq()
>>
>>
>> Reviewed-by: Boris Ostrovsky 
>>
>>
>> Ross, are you planning to test this?
> 
> Just as another data point: With the previous version of the patches
> a reboot loop of a guest needed max 33 reboots to loose network in
> my tests (those were IIRC 6 test runs). With this patch version I
> stopped the test after about 1300 reboots without having seen any
> problems.
> 

Thanks, I'll test it today and get back to you.

Ross

Re: [PATCH v4 2/3] xen/events: don't unmask an event channel when an eoi is pending

2021-03-08 Thread Jürgen Groß 

On 08.03.21 21:33, Boris Ostrovsky wrote:


On 3/6/21 11:18 AM, Juergen Gross wrote:

An event channel should be kept masked when an eoi is pending for it.
When being migrated to another cpu it might be unmasked, though.

In order to avoid this keep three different flags for each event channel
to be able to distinguish "normal" masking/unmasking from eoi related
masking/unmasking and temporary masking. The event channel should only
be able to generate an interrupt if all flags are cleared.

Cc: sta...@vger.kernel.org
Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn framework")
Reported-by: Julien Grall 
Signed-off-by: Juergen Gross 
Reviewed-by: Julien Grall 
---
V2:
- introduce a lock around masking/unmasking
- merge patch 3 into this one (Jan Beulich)
V4:
- don't set eoi masking flag in lateeoi_mask_ack_dynirq()



Reviewed-by: Boris Ostrovsky 


Ross, are you planning to test this?


Just as another data point: With the previous version of the patches
a reboot loop of a guest needed max 33 reboots to loose network in
my tests (those were IIRC 6 test runs). With this patch version I
stopped the test after about 1300 reboots without having seen any
problems.

Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH v4 2/3] xen/events: don't unmask an event channel when an eoi is pending

2021-03-08 Thread Boris Ostrovsky 


On 3/6/21 11:18 AM, Juergen Gross wrote:
> An event channel should be kept masked when an eoi is pending for it.
> When being migrated to another cpu it might be unmasked, though.
>
> In order to avoid this keep three different flags for each event channel
> to be able to distinguish "normal" masking/unmasking from eoi related
> masking/unmasking and temporary masking. The event channel should only
> be able to generate an interrupt if all flags are cleared.
>
> Cc: sta...@vger.kernel.org
> Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn framework")
> Reported-by: Julien Grall 
> Signed-off-by: Juergen Gross 
> Reviewed-by: Julien Grall 
> ---
> V2:
> - introduce a lock around masking/unmasking
> - merge patch 3 into this one (Jan Beulich)
> V4:
> - don't set eoi masking flag in lateeoi_mask_ack_dynirq()


Reviewed-by: Boris Ostrovsky 


Ross, are you planning to test this?


-boris

[PATCH v4 2/3] xen/events: don't unmask an event channel when an eoi is pending

2021-03-06 Thread Juergen Gross 
An event channel should be kept masked when an eoi is pending for it.
When being migrated to another cpu it might be unmasked, though.

In order to avoid this keep three different flags for each event channel
to be able to distinguish "normal" masking/unmasking from eoi related
masking/unmasking and temporary masking. The event channel should only
be able to generate an interrupt if all flags are cleared.

Cc: sta...@vger.kernel.org
Fixes: 54c9de89895e0a36047 ("xen/events: add a new late EOI evtchn framework")
Reported-by: Julien Grall 
Signed-off-by: Juergen Gross 
Reviewed-by: Julien Grall 
---
V2:
- introduce a lock around masking/unmasking
- merge patch 3 into this one (Jan Beulich)
V4:
- don't set eoi masking flag in lateeoi_mask_ack_dynirq()
---
 drivers/xen/events/events_2l.c   |   7 --
 drivers/xen/events/events_base.c | 101 +--
 drivers/xen/events/events_fifo.c |   7 --
 drivers/xen/events/events_internal.h |   6 --
 4 files changed, 80 insertions(+), 41 deletions(-)

diff --git a/drivers/xen/events/events_2l.c b/drivers/xen/events/events_2l.c
index a7f413c5c190..b8f2f971c2f0 100644
--- a/drivers/xen/events/events_2l.c
+++ b/drivers/xen/events/events_2l.c
@@ -77,12 +77,6 @@ static bool evtchn_2l_is_pending(evtchn_port_t port)
return sync_test_bit(port, BM(>evtchn_pending[0]));
 }
 
-static bool evtchn_2l_test_and_set_mask(evtchn_port_t port)
-{
-   struct shared_info *s = HYPERVISOR_shared_info;
-   return sync_test_and_set_bit(port, BM(>evtchn_mask[0]));
-}
-
 static void evtchn_2l_mask(evtchn_port_t port)
 {
struct shared_info *s = HYPERVISOR_shared_info;
@@ -376,7 +370,6 @@ static const struct evtchn_ops evtchn_ops_2l = {
.clear_pending = evtchn_2l_clear_pending,
.set_pending   = evtchn_2l_set_pending,
.is_pending= evtchn_2l_is_pending,
-   .test_and_set_mask = evtchn_2l_test_and_set_mask,
.mask  = evtchn_2l_mask,
.unmask= evtchn_2l_unmask,
.handle_events = evtchn_2l_handle_events,
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 7e23808892a7..b27c012c86b5 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -98,13 +98,18 @@ struct irq_info {
short refcnt;
u8 spurious_cnt;
u8 is_accounted;
-   enum xen_irq_type type; /* type */
+   short type; /* type: IRQT_* */
+   u8 mask_reason; /* Why is event channel masked */
+#define EVT_MASK_REASON_EXPLICIT   0x01
+#define EVT_MASK_REASON_TEMPORARY  0x02
+#define EVT_MASK_REASON_EOI_PENDING0x04
unsigned irq;
evtchn_port_t evtchn;   /* event channel */
unsigned short cpu; /* cpu bound */
unsigned short eoi_cpu; /* EOI must happen on this cpu-1 */
unsigned int irq_epoch; /* If eoi_cpu valid: irq_epoch of event */
u64 eoi_time;   /* Time in jiffies when to EOI. */
+   spinlock_t lock;
 
union {
unsigned short virq;
@@ -154,6 +159,7 @@ static DEFINE_RWLOCK(evtchn_rwlock);
  *   evtchn_rwlock
  * IRQ-desc lock
  *   percpu eoi_list_lock
+ * irq_info->lock
  */
 
 static LIST_HEAD(xen_irq_list_head);
@@ -304,6 +310,8 @@ static int xen_irq_info_common_setup(struct irq_info *info,
info->irq = irq;
info->evtchn = evtchn;
info->cpu = cpu;
+   info->mask_reason = EVT_MASK_REASON_EXPLICIT;
+   spin_lock_init(>lock);
 
ret = set_evtchn_to_irq(evtchn, irq);
if (ret < 0)
@@ -459,6 +467,34 @@ unsigned int cpu_from_evtchn(evtchn_port_t evtchn)
return ret;
 }
 
+static void do_mask(struct irq_info *info, u8 reason)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+
+   if (!info->mask_reason)
+   mask_evtchn(info->evtchn);
+
+   info->mask_reason |= reason;
+
+   spin_unlock_irqrestore(>lock, flags);
+}
+
+static void do_unmask(struct irq_info *info, u8 reason)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(>lock, flags);
+
+   info->mask_reason &= ~reason;
+
+   if (!info->mask_reason)
+   unmask_evtchn(info->evtchn);
+
+   spin_unlock_irqrestore(>lock, flags);
+}
+
 #ifdef CONFIG_X86
 static bool pirq_check_eoi_map(unsigned irq)
 {
@@ -605,7 +641,7 @@ static void xen_irq_lateeoi_locked(struct irq_info *info, 
bool spurious)
}
 
info->eoi_time = 0;
-   unmask_evtchn(evtchn);
+   do_unmask(info, EVT_MASK_REASON_EOI_PENDING);
 }
 
 static void xen_irq_lateeoi_worker(struct work_struct *work)
@@ -850,7 +886,8 @@ static unsigned int __startup_pirq(unsigned int irq)
goto err;
 
 out:
-   unmask_evtchn(evtchn);
+   do_unmask(info, EVT_MASK_REASON_EXPLICIT);
+
eoi_pirq(irq_get_irq_data(irq));
 
return 0;
@@ -877,7 +914,7 @@ static void shutdown_pirq(struct irq_data *data)