unfair servicing of DomU vbd requests

unfair servicing of DomU vbd requests

[James Harper]

A user of GPLPV (see thread "blue screen in windows balloon driver") is
getting a bug check in Windows under extremely high memory usage and
swapfile thrashing tests across multiple DomU's. Responses to my query
on the ntdev mailing list say that this would happen if an IO request is
not completed after 70 seconds during high memory/pagefile pressure,
which is what is happening.

It appears that Dom0 is not servicing vbd requests from DomU's fairly so
one or two end up getting stalled while the others are mostly okay. How
are vbd requests supposed to be serviced? Is there potential for one to
be overlooked for a long period of time? Is there some settings that
could be changed to avoid this happening?

Thanks

James

Re: unfair servicing of DomU vbd requests

[Keir Fraser]

On 03/03/2011 02:25, "James Harper" <james.harper@bendigoit.com.au> wrote:

> A user of GPLPV (see thread "blue screen in windows balloon driver") is
> getting a bug check in Windows under extremely high memory usage and
> swapfile thrashing tests across multiple DomU's. Responses to my query
> on the ntdev mailing list say that this would happen if an IO request is
> not completed after 70 seconds during high memory/pagefile pressure,
> which is what is happening.
> 
> It appears that Dom0 is not servicing vbd requests from DomU's fairly so
> one or two end up getting stalled while the others are mostly okay. How
> are vbd requests supposed to be serviced? Is there potential for one to
> be overlooked for a long period of time? Is there some settings that
> could be changed to avoid this happening?

Dom0 does round-robin scanning of pending event channels these days, which
helps fairness a fair bit. When a pending event is found, the corresponding
blkfront has a batch of requests pulled down and submitted into Linux's
block subsystem at which point we have no more control over scheduling (it
could generally be configured though -- Linux has an admin mechanism for
that).

 -- Keir

> Thanks
> 
> James
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: unfair servicing of DomU vbd requests

[Ian Campbell]

On Thu, 2011-03-03 at 07:29 +0000, Keir Fraser wrote:
> On 03/03/2011 02:25, "James Harper" <james.harper@bendigoit.com.au> wrote:
> 
> > A user of GPLPV (see thread "blue screen in windows balloon driver") is
> > getting a bug check in Windows under extremely high memory usage and
> > swapfile thrashing tests across multiple DomU's. Responses to my query
> > on the ntdev mailing list say that this would happen if an IO request is
> > not completed after 70 seconds during high memory/pagefile pressure,
> > which is what is happening.
> > 
> > It appears that Dom0 is not servicing vbd requests from DomU's fairly so
> > one or two end up getting stalled while the others are mostly okay. How
> > are vbd requests supposed to be serviced? Is there potential for one to
> > be overlooked for a long period of time? Is there some settings that
> > could be changed to avoid this happening?
> 
> Dom0 does round-robin scanning of pending event channels these days, which
> helps fairness a fair bit.

I have a feeling this isn't true of pvops kernels...

looks like we need to pull 324:7fe1c6d02a2b (and subsequent fixes) out
of 2.6.18-xen.hg into the pvops world.

I'll take a look shortly if no one beats me to it.

>  When a pending event is found, the corresponding
> blkfront has a batch of requests pulled down and submitted into Linux's
> block subsystem at which point we have no more control over scheduling (it
> could generally be configured though -- Linux has an admin mechanism for
> that).
> 
>  -- Keir
> 
> > Thanks
> > 
> > James
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: unfair servicing of DomU vbd requests

[James Harper]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

> > > It appears that Dom0 is not servicing vbd requests from DomU's fairly so
> > > one or two end up getting stalled while the others are mostly okay. How
> > > are vbd requests supposed to be serviced? Is there potential for one to
> > > be overlooked for a long period of time? Is there some settings that
> > > could be changed to avoid this happening?
> >
> > Dom0 does round-robin scanning of pending event channels these days, which
> > helps fairness a fair bit.
> 
> I have a feeling this isn't true of pvops kernels...
> 
> looks like we need to pull 324:7fe1c6d02a2b (and subsequent fixes) out
> of 2.6.18-xen.hg into the pvops world.
> 

I hope that's true and easy to fix. It would certainly explain why one DomU can starve enough to the point where it's IO doesn't get serviced for >70 seconds.

Thanks for looking at this.

James

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: unfair servicing of DomU vbd requests

[Keir Fraser]

On 03/03/2011 08:28, "James Harper" <james.harper@bendigoit.com.au> wrote:

>>>> It appears that Dom0 is not servicing vbd requests from DomU's fairly so
>>>> one or two end up getting stalled while the others are mostly okay. How
>>>> are vbd requests supposed to be serviced? Is there potential for one to
>>>> be overlooked for a long period of time? Is there some settings that
>>>> could be changed to avoid this happening?
>>> 
>>> Dom0 does round-robin scanning of pending event channels these days, which
>>> helps fairness a fair bit.
>> 
>> I have a feeling this isn't true of pvops kernels...
>> 
>> looks like we need to pull 324:7fe1c6d02a2b (and subsequent fixes) out
>> of 2.6.18-xen.hg into the pvops world.
>> 
> 
> I hope that's true and easy to fix. It would certainly explain why one DomU
> can starve enough to the point where it's IO doesn't get serviced for >70
> seconds.

Without the round-robin servicing, unfairness to the point of starvation is
a distinct possibility.

 -- Keir

> Thanks for looking at this.
> 
> James

[GIT/PATCH 0/5] Re: unfair servicing of DomU vbd requests

[Ian Campbell]

On Thu, 2011-03-03 at 08:30 +0000, Keir Fraser wrote: 
> Without the round-robin servicing, unfairness to the point of starvation is
> a distinct possibility.

Indeed. The following is a forward port of these patches from
2.6.18-xen.hg: 
      * 324:7fe1c6d02a2b 
      * 325:b2768401db94 
      * 988:c88a02a22a05 
      * 990:427276ac595d 
      * 991:9ba6d9f3fbc0

Hopefully I didn't miss any.

The switch in pvops from "l1"/"l2" naming convention to "word"/"bit"
made the conflict resolution a bit prone to thinkos but hopefully I
didn't b0rk it too badly. Review with that in mind greatly appreciated.

boots a dom0 + a pv guest.

Pull request on top of konrad/stable/irq.cleanup:

The following changes since commit c5ae07bb307b658c8458f29ca77d237aec0f9327:
  Ian Campbell (1):
        xen: events: remove dom0 specific xen_create_msi_irq

are available in the git repository at:

  git://xenbits.xen.org/people/ianc/linux-2.6.git irq-fairness

Ian Campbell (1):
      xen: events: Make last processed event channel a per-cpu variable.

Keir Fraser (3):
      xen: events: Clean up round-robin evtchn scan.
      xen: events: Make round-robin scan fairer by snapshotting each l2 word
      xen: events: Remove redundant clear of l2i at end of round-robin loop

Scott Rixner (1):
      xen: events: Process event channels notifications in round-robin order.

 drivers/xen/events.c |   80 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 72 insertions(+), 8 deletions(-)



Ian.

[PATCH 1/5] xen: events: Process event channels notifications in round-robin order.

[Ian Campbell]

From: Scott Rixner <rixner@rice.edu>

Avoids fairness issue resulting from domain 0 processing lowest
numbered event channel first.

Bugzilla #1115 "Event channel port scanning unfair".

Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
[ijc: forward ported from linux-2.6.18-xen.hg 324:7fe1c6d02a2b
 various variables have different names in this tree:
  l1  -> pending_words
  l2  -> pending_bits
  l1i -> word_idx
  l2i -> bit_idx
]
---
 drivers/xen/events.c |   72 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 6befe62..75cc6f5 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1028,6 +1028,11 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 static DEFINE_PER_CPU(unsigned, xed_nesting_count);
 
 /*
+ * Mask out the i least significant bits of w
+ */
+#define MASK_LSBS(w, i) (w & ((~0UL) << i))
+
+/*
  * Search the CPUs pending events bitmasks.  For each one found, map
  * the event number to an irq, and feed it into do_IRQ() for
  * handling.
@@ -1038,6 +1043,9 @@ static DEFINE_PER_CPU(unsigned, xed_nesting_count);
  */
 static void __xen_evtchn_do_upcall(void)
 {
+	static unsigned int last_word_idx = BITS_PER_LONG - 1;
+	static unsigned int last_bit_idx = BITS_PER_LONG - 1;
+	int word_idx, bit_idx;
 	int cpu = get_cpu();
 	struct shared_info *s = HYPERVISOR_shared_info;
 	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
@@ -1056,17 +1064,50 @@ static void __xen_evtchn_do_upcall(void)
 		wmb();
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
+
+		word_idx = last_word_idx;
+		bit_idx = last_bit_idx;
+
 		while (pending_words != 0) {
 			unsigned long pending_bits;
-			int word_idx = __ffs(pending_words);
-			pending_words &= ~(1UL << word_idx);
+			unsigned long words;
+
+			word_idx = (word_idx + 1) % BITS_PER_LONG;
+			words = MASK_LSBS(pending_words, word_idx);
+
+			/*
+			 * If we masked out all events, wrap around to the
+			 * beginning.
+			 */
+			if (words == 0) {
+				word_idx = BITS_PER_LONG - 1;
+				bit_idx = BITS_PER_LONG - 1;
+				continue;
+			}
+			word_idx = __ffs(words);
 
-			while ((pending_bits = active_evtchns(cpu, s, word_idx)) != 0) {
-				int bit_idx = __ffs(pending_bits);
-				int port = (word_idx * BITS_PER_LONG) + bit_idx;
-				int irq = evtchn_to_irq[port];
+			do {
+				unsigned long bits;
+				int port, irq;
 				struct irq_desc *desc;
 
+				pending_bits = active_evtchns(cpu, s, word_idx);
+
+				bit_idx = (bit_idx + 1) % BITS_PER_LONG;
+				bits = MASK_LSBS(pending_bits, bit_idx);
+
+				/* If we masked out all events, move on. */
+				if (bits == 0) {
+					bit_idx = BITS_PER_LONG - 1;
+					break;
+				}
+
+				bit_idx = __ffs(bits);
+
+				/* Process port. */
+				port = (word_idx * BITS_PER_LONG) + bit_idx;
+				irq = evtchn_to_irq[port];
+
 				mask_evtchn(port);
 				clear_evtchn(port);
 
@@ -1075,7 +1116,24 @@ static void __xen_evtchn_do_upcall(void)
 					if (desc)
 						generic_handle_irq_desc(irq, desc);
 				}
-			}
+
+				/*
+				 * If this is the final port processed, we'll
+				 * pick up here+1 next time.
+				 */
+				last_word_idx = word_idx;
+				last_bit_idx = bit_idx;
+
+			} while (bit_idx != BITS_PER_LONG - 1);
+
+			pending_bits = active_evtchns(cpu, s, word_idx);
+
+			/*
+			 * We handled all ports, so we can clear the
+			 * selector bit.
+			 */
+			if (pending_bits == 0)
+				pending_words &= ~(1UL << word_idx);
 		}
 
 		BUG_ON(!irqs_disabled());
-- 
1.5.6.5

[PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
[ijc: forward ported from linux-2.6.18-xen.hg 325:b2768401db94]
---
 drivers/xen/events.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 75cc6f5..1fc3192 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1026,6 +1026,8 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 }
 
 static DEFINE_PER_CPU(unsigned, xed_nesting_count);
+static DEFINE_PER_CPU(unsigned int, last_word_idx) = { BITS_PER_LONG - 1 };
+static DEFINE_PER_CPU(unsigned int, last_bit_idx) = { BITS_PER_LONG - 1 };
 
 /*
  * Mask out the i least significant bits of w
@@ -1043,8 +1045,6 @@ static DEFINE_PER_CPU(unsigned, xed_nesting_count);
  */
 static void __xen_evtchn_do_upcall(void)
 {
-	static unsigned int last_word_idx = BITS_PER_LONG - 1;
-	static unsigned int last_bit_idx = BITS_PER_LONG - 1;
 	int word_idx, bit_idx;
 	int cpu = get_cpu();
 	struct shared_info *s = HYPERVISOR_shared_info;
@@ -1065,8 +1065,8 @@ static void __xen_evtchn_do_upcall(void)
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 
-		word_idx = last_word_idx;
-		bit_idx = last_bit_idx;
+		word_idx = __this_cpu_read(last_word_idx);
+		bit_idx = __this_cpu_read(last_bit_idx);
 
 		while (pending_words != 0) {
 			unsigned long pending_bits;
@@ -1121,9 +1121,8 @@ static void __xen_evtchn_do_upcall(void)
 				 * If this is the final port processed, we'll
 				 * pick up here+1 next time.
 				 */
-				last_word_idx = word_idx;
-				last_bit_idx = bit_idx;
-
+				__this_cpu_write(last_word_idx, word_idx);
+				__this_cpu_write(last_bit_idx, bit_idx);
 			} while (bit_idx != BITS_PER_LONG - 1);
 
 			pending_bits = active_evtchns(cpu, s, word_idx);
-- 
1.5.6.5

[PATCH 3/5] xen: events: Clean up round-robin evtchn scan.

[Ian Campbell]

From: Keir Fraser <keir.fraser@citrix.com>

Also fixes a couple of boundary cases.

Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
[ijc: forward ported from linux-2.6.18-xen.hg 988:c88a02a22a05]
---
 drivers/xen/events.c |   44 ++++++++++++++++++++------------------------
 1 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 1fc3192..c49cb6d 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1026,8 +1026,8 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
 }
 
 static DEFINE_PER_CPU(unsigned, xed_nesting_count);
-static DEFINE_PER_CPU(unsigned int, last_word_idx) = { BITS_PER_LONG - 1 };
-static DEFINE_PER_CPU(unsigned int, last_bit_idx) = { BITS_PER_LONG - 1 };
+static DEFINE_PER_CPU(unsigned int, current_word_idx);
+static DEFINE_PER_CPU(unsigned int, current_bit_idx);
 
 /*
  * Mask out the i least significant bits of w
@@ -1065,23 +1065,21 @@ static void __xen_evtchn_do_upcall(void)
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 
-		word_idx = __this_cpu_read(last_word_idx);
-		bit_idx = __this_cpu_read(last_bit_idx);
+		word_idx = __this_cpu_read(current_word_idx);
+		bit_idx = __this_cpu_read(current_bit_idx);
 
 		while (pending_words != 0) {
 			unsigned long pending_bits;
 			unsigned long words;
 
-			word_idx = (word_idx + 1) % BITS_PER_LONG;
 			words = MASK_LSBS(pending_words, word_idx);
 
 			/*
-			 * If we masked out all events, wrap around to the
-			 * beginning.
+			 * If we masked out all events, wrap to beginning.
 			 */
 			if (words == 0) {
-				word_idx = BITS_PER_LONG - 1;
-				bit_idx = BITS_PER_LONG - 1;
+				word_idx = 0;
+				bit_idx = 0;
 				continue;
 			}
 			word_idx = __ffs(words);
@@ -1093,14 +1091,11 @@ static void __xen_evtchn_do_upcall(void)
 
 				pending_bits = active_evtchns(cpu, s, word_idx);
 
-				bit_idx = (bit_idx + 1) % BITS_PER_LONG;
 				bits = MASK_LSBS(pending_bits, bit_idx);
 
 				/* If we masked out all events, move on. */
-				if (bits == 0) {
-					bit_idx = BITS_PER_LONG - 1;
+				if (bits == 0)
 					break;
-				}
 
 				bit_idx = __ffs(bits);
 
@@ -1117,22 +1112,23 @@ static void __xen_evtchn_do_upcall(void)
 						generic_handle_irq_desc(irq, desc);
 				}
 
-				/*
-				 * If this is the final port processed, we'll
-				 * pick up here+1 next time.
-				 */
-				__this_cpu_write(last_word_idx, word_idx);
-				__this_cpu_write(last_bit_idx, bit_idx);
-			} while (bit_idx != BITS_PER_LONG - 1);
+				bit_idx = (bit_idx + 1) % BITS_PER_LONG;
+
+				/* Next caller starts at last processed + 1 */
+				__this_cpu_write(current_word_idx,
+						 bit_idx ? word_idx :
+						 (word_idx+1) % BITS_PER_LONG);
+				__this_cpu_write(current_bit_idx, bit_idx);
+			} while (bit_idx != 0);
 
 			pending_bits = active_evtchns(cpu, s, word_idx);
 
-			/*
-			 * We handled all ports, so we can clear the
-			 * selector bit.
-			 */
+			/* If we handled all ports, clear the selector bit. */
 			if (pending_bits == 0)
 				pending_words &= ~(1UL << word_idx);
+
+			word_idx = (word_idx + 1) % BITS_PER_LONG;
+			bit_idx = 0;
 		}
 
 		BUG_ON(!irqs_disabled());
-- 
1.5.6.5

[PATCH 4/5] xen: events: Make round-robin scan fairer by snapshotting each l2 word

[Ian Campbell]

From: Keir Fraser <keir.fraser@citrix.com>

Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
[ijc: forward ported from linux-2.6.18-xen.hg 990:427276ac595d]
---
 drivers/xen/events.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index c49cb6d..0c3f17b 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1045,7 +1045,9 @@ static DEFINE_PER_CPU(unsigned int, current_bit_idx);
  */
 static void __xen_evtchn_do_upcall(void)
 {
+	int start_word_idx, start_bit_idx;
 	int word_idx, bit_idx;
+	int i;
 	int cpu = get_cpu();
 	struct shared_info *s = HYPERVISOR_shared_info;
 	struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
@@ -1065,10 +1067,12 @@ static void __xen_evtchn_do_upcall(void)
 #endif
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 
-		word_idx = __this_cpu_read(current_word_idx);
-		bit_idx = __this_cpu_read(current_bit_idx);
+		start_word_idx = __this_cpu_read(current_word_idx);
+		start_bit_idx = __this_cpu_read(current_bit_idx);
+
+		word_idx = start_word_idx;
 
-		while (pending_words != 0) {
+		for (i = 0; pending_words != 0; i++) {
 			unsigned long pending_bits;
 			unsigned long words;
 
@@ -1084,13 +1088,23 @@ static void __xen_evtchn_do_upcall(void)
 			}
 			word_idx = __ffs(words);
 
+			pending_bits = active_evtchns(cpu, s, word_idx);
+			bit_idx = 0; /* usually scan entire word from start */
+			if (word_idx == start_word_idx) {
+				/* We scan the starting word in two parts */
+				if (i == 0)
+					/* 1st time: start in the middle */
+					bit_idx = start_bit_idx;
+				else
+					/* 2nd time: mask bits done already */
+					bit_idx &= (1UL << start_bit_idx) - 1;
+			}
+
 			do {
 				unsigned long bits;
 				int port, irq;
 				struct irq_desc *desc;
 
-				pending_bits = active_evtchns(cpu, s, word_idx);
-
 				bits = MASK_LSBS(pending_bits, bit_idx);
 
 				/* If we masked out all events, move on. */
@@ -1121,10 +1135,8 @@ static void __xen_evtchn_do_upcall(void)
 				__this_cpu_write(current_bit_idx, bit_idx);
 			} while (bit_idx != 0);
 
-			pending_bits = active_evtchns(cpu, s, word_idx);
-
-			/* If we handled all ports, clear the selector bit. */
-			if (pending_bits == 0)
+			/* Scan start_l1i twice; all others once. */
+			if ((word_idx != start_word_idx) || (i != 0))
 				pending_words &= ~(1UL << word_idx);
 
 			word_idx = (word_idx + 1) % BITS_PER_LONG;
-- 
1.5.6.5

[PATCH 5/5] xen: events: Remove redundant clear of l2i at end of round-robin loop

[Ian Campbell]

From: Keir Fraser <keir.fraser@citrix.com>

Signed-off-by: Keir Fraser <keir.fraser@citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
[ijc: forward ported from linux-2.6.18-xen.hg 991:9ba6d9f3fbc0]
---
 drivers/xen/events.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 0c3f17b..8786e06 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1140,7 +1140,6 @@ static void __xen_evtchn_do_upcall(void)
 				pending_words &= ~(1UL << word_idx);
 
 			word_idx = (word_idx + 1) % BITS_PER_LONG;
-			bit_idx = 0;
 		}
 
 		BUG_ON(!irqs_disabled());
-- 
1.5.6.5

Re: [GIT/PATCH 0/5] Re: unfair servicing of DomU vbd requests

[John Weekes]

On 3/3/2011 9:09 AM, Ian Campbell wrote:
> On Thu, 2011-03-03 at 08:30 +0000, Keir Fraser wrote:
>> >  Without the round-robin servicing, unfairness to the point of starvation is
>> >  a distinct possibility.
> Indeed. The following is a forward port of these patches from
> 2.6.18-xen.hg:

Thanks for working on this, Ian.

I think that this has affected me several times, as I've noticed these 
symptoms before (heavy disk access by a few domains, followed by several 
Windows domUs mysteriously rebooting) -- as recently as Wednesday, in 
fact. The I/O from rebooting domUs, unfortunately, makes everything even 
worse, often leading to further Windows domUs crashing.

I look forward to this being merged into stable-2.6.32. In the meantime, 
I'll monkey around with git as you described and try to get a local 
version working. I'm itching to test it out, since this also might 
explain some of the occasional the longish disk delays that I see on 
machines with a medium I/O load (in which the dom0 still responds 
normally, but one of the domUs frequently stalls waiting for I/O).

-John

Re: [GIT/PATCH 0/5] Re: unfair servicing of DomU vbd requests

[Ian Campbell]

On Fri, 2011-03-04 at 08:40 +0000, John Weekes wrote:
> On 3/3/2011 9:09 AM, Ian Campbell wrote:
> > On Thu, 2011-03-03 at 08:30 +0000, Keir Fraser wrote:
> >> >  Without the round-robin servicing, unfairness to the point of starvation is
> >> >  a distinct possibility.
> > Indeed. The following is a forward port of these patches from
> > 2.6.18-xen.hg:
> 
> Thanks for working on this, Ian.
> 
> I think that this has affected me several times, as I've noticed these 
> symptoms before (heavy disk access by a few domains, followed by several 
> Windows domUs mysteriously rebooting) -- as recently as Wednesday, in 
> fact. The I/O from rebooting domUs, unfortunately, makes everything even 
> worse, often leading to further Windows domUs crashing.
> 
> I look forward to this being merged into stable-2.6.32. In the meantime, 
> I'll monkey around with git as you described and try to get a local 
> version working. I'm itching to test it out, since this also might 
> explain some of the occasional the longish disk delays that I see on 
> machines with a medium I/O load (in which the dom0 still responds 
> normally, but one of the domUs frequently stalls waiting for I/O).

This series was for more recent kernels. It seems to backport with
minimum fuss (switch from __this_cpu_(read|write) to __get_cpu_var).

I only compile tested this one file and haven't even booted it but the
result is available on top of xen.git xen/next-2.6.32 at:

The following changes since commit 892d2f052e979cf1916647c752b94cf62ec1c6dc:
  Jeremy Fitzhardinge (1):
        Merge commit 'v2.6.32.28' into xen/next-2.6.32

are available in the git repository at:

  git://xenbits.xen.org/people/ianc/linux-2.6.git irq-fairness-2.6.32

Ian Campbell (1):
      xen: events: Make last processed event channel a per-cpu variable.

Keir Fraser (3):
      xen: events: Clean up round-robin evtchn scan.
      xen: events: Make round-robin scan fairer by snapshotting each l2 word
      xen: events: Remove redundant clear of l2i at end of round-robin loop

Scott Rixner (1):
      xen: events: Process event channels notifications in round-robin order.

 drivers/xen/events.c |   80 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 72 insertions(+), 8 deletions(-)

Ian.

Re: [GIT/PATCH 0/5] Re: unfair servicing of DomU vbd requests

[John Weekes]

On 3/4/2011 1:15 AM, Ian Campbell wrote:
> This series was for more recent kernels. It seems to backport with
> minimum fuss (switch from __this_cpu_(read|write) to __get_cpu_var).
>
> I only compile tested this one file and haven't even booted it but the
> result is available on top of xen.git xen/next-2.6.32 at:
> ...
>    git://xenbits.xen.org/people/ianc/linux-2.6.git irq-fairness-2.6.32

Thanks for the rebase and for the earlier instructions on how to merge 
your git topic branch, which were clear and easy to follow.

I've have this code running on a lightly-used production machine right 
now with no problems observed so far. I will continue to monitor and 
roll it out to a higher-usage machine that has had problems without the 
RR bits within the next few days, if all goes well.

-John

Re: [PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Konrad Rzeszutek Wilk]

On Thu, Mar 03, 2011 at 05:10:12PM +0000, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

You cloned yourself?

> [ijc: forward ported from linux-2.6.18-xen.hg 325:b2768401db94]
> ---
>  drivers/xen/events.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 75cc6f5..1fc3192 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -1026,6 +1026,8 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
>  }
>  
>  static DEFINE_PER_CPU(unsigned, xed_nesting_count);
> +static DEFINE_PER_CPU(unsigned int, last_word_idx) = { BITS_PER_LONG - 1 };
> +static DEFINE_PER_CPU(unsigned int, last_bit_idx) = { BITS_PER_LONG - 1 };
>  
>  /*
>   * Mask out the i least significant bits of w
> @@ -1043,8 +1045,6 @@ static DEFINE_PER_CPU(unsigned, xed_nesting_count);
>   */
>  static void __xen_evtchn_do_upcall(void)
>  {
> -	static unsigned int last_word_idx = BITS_PER_LONG - 1;
> -	static unsigned int last_bit_idx = BITS_PER_LONG - 1;
>  	int word_idx, bit_idx;
>  	int cpu = get_cpu();
>  	struct shared_info *s = HYPERVISOR_shared_info;
> @@ -1065,8 +1065,8 @@ static void __xen_evtchn_do_upcall(void)
>  #endif
>  		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
>  
> -		word_idx = last_word_idx;
> -		bit_idx = last_bit_idx;
> +		word_idx = __this_cpu_read(last_word_idx);
> +		bit_idx = __this_cpu_read(last_bit_idx);
>  
>  		while (pending_words != 0) {
>  			unsigned long pending_bits;
> @@ -1121,9 +1121,8 @@ static void __xen_evtchn_do_upcall(void)
>  				 * If this is the final port processed, we'll
>  				 * pick up here+1 next time.
>  				 */
> -				last_word_idx = word_idx;
> -				last_bit_idx = bit_idx;
> -
> +				__this_cpu_write(last_word_idx, word_idx);
> +				__this_cpu_write(last_bit_idx, bit_idx);
>  			} while (bit_idx != BITS_PER_LONG - 1);
>  
>  			pending_bits = active_evtchns(cpu, s, word_idx);
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Ian Campbell]

On Wed, 2011-03-09 at 20:32 +0000, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 03, 2011 at 05:10:12PM +0000, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> You cloned yourself?

The first is from the original linux-2.6.18-xen.hg commit.

The second is from today as I forward the change upstream.

I wasn't really sure what to do with the historical signed-off-by so I
decided to leave it, just as I would have done if it had been someone
else's.

Ian.

> 
> > [ijc: forward ported from linux-2.6.18-xen.hg 325:b2768401db94]
> > ---
> >  drivers/xen/events.c |   13 ++++++-------
> >  1 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 75cc6f5..1fc3192 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -1026,6 +1026,8 @@ irqreturn_t xen_debug_interrupt(int irq, void *dev_id)
> >  }
> >  
> >  static DEFINE_PER_CPU(unsigned, xed_nesting_count);
> > +static DEFINE_PER_CPU(unsigned int, last_word_idx) = { BITS_PER_LONG - 1 };
> > +static DEFINE_PER_CPU(unsigned int, last_bit_idx) = { BITS_PER_LONG - 1 };
> >  
> >  /*
> >   * Mask out the i least significant bits of w
> > @@ -1043,8 +1045,6 @@ static DEFINE_PER_CPU(unsigned, xed_nesting_count);
> >   */
> >  static void __xen_evtchn_do_upcall(void)
> >  {
> > -	static unsigned int last_word_idx = BITS_PER_LONG - 1;
> > -	static unsigned int last_bit_idx = BITS_PER_LONG - 1;
> >  	int word_idx, bit_idx;
> >  	int cpu = get_cpu();
> >  	struct shared_info *s = HYPERVISOR_shared_info;
> > @@ -1065,8 +1065,8 @@ static void __xen_evtchn_do_upcall(void)
> >  #endif
> >  		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
> >  
> > -		word_idx = last_word_idx;
> > -		bit_idx = last_bit_idx;
> > +		word_idx = __this_cpu_read(last_word_idx);
> > +		bit_idx = __this_cpu_read(last_bit_idx);
> >  
> >  		while (pending_words != 0) {
> >  			unsigned long pending_bits;
> > @@ -1121,9 +1121,8 @@ static void __xen_evtchn_do_upcall(void)
> >  				 * If this is the final port processed, we'll
> >  				 * pick up here+1 next time.
> >  				 */
> > -				last_word_idx = word_idx;
> > -				last_bit_idx = bit_idx;
> > -
> > +				__this_cpu_write(last_word_idx, word_idx);
> > +				__this_cpu_write(last_bit_idx, bit_idx);
> >  			} while (bit_idx != BITS_PER_LONG - 1);
> >  
> >  			pending_bits = active_evtchns(cpu, s, word_idx);
> > -- 
> > 1.5.6.5
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel

Re: [PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Jeremy Fitzhardinge]

On 03/09/2011 12:40 PM, Ian Campbell wrote:
> On Wed, 2011-03-09 at 20:32 +0000, Konrad Rzeszutek Wilk wrote:
>> On Thu, Mar 03, 2011 at 05:10:12PM +0000, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> You cloned yourself?
> The first is from the original linux-2.6.18-xen.hg commit.
>
> The second is from today as I forward the change upstream.
>
> I wasn't really sure what to do with the historical signed-off-by so I
> decided to leave it, just as I would have done if it had been someone
> else's.

Since Citrix and Xensource are logically the same entity, just the
Citrix one should be fine.

    J

Re: [PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Ian Campbell]

On Wed, 2011-03-09 at 20:47 +0000, Jeremy Fitzhardinge wrote:
> On 03/09/2011 12:40 PM, Ian Campbell wrote:
> > On Wed, 2011-03-09 at 20:32 +0000, Konrad Rzeszutek Wilk wrote:
> >> On Thu, Mar 03, 2011 at 05:10:12PM +0000, Ian Campbell wrote:
> >>> Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> You cloned yourself?
> > The first is from the original linux-2.6.18-xen.hg commit.
> >
> > The second is from today as I forward the change upstream.
> >
> > I wasn't really sure what to do with the historical signed-off-by so I
> > decided to leave it, just as I would have done if it had been someone
> > else's.
> 
> Since Citrix and Xensource are logically the same entity, just the
> Citrix one should be fine.

You are almost certainly right, but the xensource one is harmless and
accurately describes the provenance of the patch.

More importantly I don't think it's a big enough deal to be worth
respinning the series

Ian.

Re: [PATCH 2/5] xen: events: Make last processed event channel a per-cpu variable.

[Jeremy Fitzhardinge]

On 03/10/2011 12:30 AM, Ian Campbell wrote:
> More importantly I don't think it's a big enough deal to be worth
> respinning the series

Definitely not.

    J