| Message ID | 1365311880-11800-1-git-send-email-peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > Modify Anthony's starvation detection logic to keep the BQL unlocked > until the starvation condition goes away. Otherwise the counter has to > count up to 1000 for each needed iteration until the busy-wait is > lifted. > > Reset the counter back to zero once glib_pollfds_fill returns with a > non-zero timout, (indicating a return to normality). The 1000 iteration > wait now only happens once on the transition from normal operation to > busy-wait starvation. > > Anthony's original patch fixed the serial paste bug, but this patch is > also needed to restore performance. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> I'm going through patches for 1.5 candidates. I believe the paste performance issue has been resolved now and this patch is no longer needed. I can't find a definitive statement on the list for that though. Peter, can you confirm? Regards, Anthony Liguori > --- > main-loop.c | 20 ++++++++++---------- > 1 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/main-loop.c b/main-loop.c > index f46aece..93d2917 100644 > --- a/main-loop.c > +++ b/main-loop.c > @@ -200,10 +200,13 @@ static int os_host_main_loop_wait(uint32_t timeout) > /* If the I/O thread is very busy or we are incorrectly busy waiting in > * the I/O thread, this can lead to starvation of the BQL such that the > * VCPU threads never run. To make sure we can detect the later case, > - * print a message to the screen. If we run into this condition, create > - * a fake timeout in order to give the VCPU threads a chance to run. > + * print a message to the screen. If we run into this condition, unlock > + * the BQL until a non-zero timout is given (indicating the starvation > + * issue has gome away). > */ > - if (spin_counter > MAX_MAIN_LOOP_SPIN) { > + if (timeout > 0) { > + spin_counter = 0; > + } else if (spin_counter > MAX_MAIN_LOOP_SPIN) { > static bool notified; > > if (!notified) { > @@ -212,21 +215,18 @@ static int os_host_main_loop_wait(uint32_t timeout) > MAX_MAIN_LOOP_SPIN); > notified = true; > } > - > - timeout = 1; > } > > - if (timeout > 0) { > - spin_counter = 0; > + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { > qemu_mutex_unlock_iothread(); > - } else { > - spin_counter++; > } > > ret = g_poll((GPollFD *)gpollfds->data, gpollfds->len, timeout); > > - if (timeout > 0) { > + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { > qemu_mutex_lock_iothread(); > + } else { > + spin_counter++; > } > > glib_pollfds_poll(); > -- > 1.7.0.4
Il 15/04/2013 15:08, Anthony Liguori ha scritto: >> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > I'm going through patches for 1.5 candidates. > > I believe the paste performance issue has been resolved now and this > patch is no longer needed. I can't find a definitive statement on the > list for that though. > > Peter, can you confirm? Kind of. There is one remaining patch, http://article.gmane.org/gmane.comp.emulators.qemu/205376. Amit says he sees a problem with it, but I cannot reproduce. But the patch is obvious and the problem must be latent, so I would apply it anyway. There is another issue pointed out by Michael Hines with "-serial pty". I couldn't reproduce it, but I haven't tried very hard. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 15/04/2013 15:08, Anthony Liguori ha scritto: >>> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> I'm going through patches for 1.5 candidates. >> >> I believe the paste performance issue has been resolved now and this >> patch is no longer needed. I can't find a definitive statement on the >> list for that though. >> >> Peter, can you confirm? > > Kind of. > > There is one remaining patch, Yeah, I'm testing this right now. > http://article.gmane.org/gmane.comp.emulators.qemu/205376. Amit says he > sees a problem with it, but I cannot reproduce. But the patch is > obvious and the problem must be latent, so I would apply it anyway. So far, I haven't encountered a problem with it and that includes doing virtio-serial testing. If I don't see a problem, I'll push it as I agree the patch looks obviously correct to me. > There is another issue pointed out by Michael Hines with "-serial pty". > I couldn't reproduce it, but I haven't tried very hard. There's also a qtest problem that Andreas reported that I can reproduce that I have already begun looking into. Regards, Anthony Liguori > > Paolo
On 15.04.2013 15:08, Anthony Liguori wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> Modify Anthony's starvation detection logic to keep the BQL unlocked >> until the starvation condition goes away. Otherwise the counter has to >> count up to 1000 for each needed iteration until the busy-wait is >> lifted. >> >> Reset the counter back to zero once glib_pollfds_fill returns with a >> non-zero timout, (indicating a return to normality). The 1000 iteration >> wait now only happens once on the transition from normal operation to >> busy-wait starvation. >> >> Anthony's original patch fixed the serial paste bug, but this patch is >> also needed to restore performance. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > I'm going through patches for 1.5 candidates. > > I believe the paste performance issue has been resolved now and this > patch is no longer needed. I can't find a definitive statement on the > list for that though. I am also hitting a problem that occured first after Anthonys original patch. In my testing environment I had 3 vServers that indepently of the load became heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun for 1000 iterations". Even QMP is not responsing from time to time. But I am not using serial. From the load statistics it seems that the vServers is using one complete core busy waiting. I haven't seen this before this patch. Peter
On Mon, Apr 15, 2013 at 11:08 PM, Anthony Liguori <aliguori@us.ibm.com> wrote: > Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > >> Modify Anthony's starvation detection logic to keep the BQL unlocked >> until the starvation condition goes away. Otherwise the counter has to >> count up to 1000 for each needed iteration until the busy-wait is >> lifted. >> >> Reset the counter back to zero once glib_pollfds_fill returns with a >> non-zero timout, (indicating a return to normality). The 1000 iteration >> wait now only happens once on the transition from normal operation to >> busy-wait starvation. >> >> Anthony's original patch fixed the serial paste bug, but this patch is >> also needed to restore performance. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > I'm going through patches for 1.5 candidates. > > I believe the paste performance issue has been resolved now and this > patch is no longer needed. I can't find a definitive statement on the > list for that though. > > Peter, can you confirm? > Paos patches cleared it all up for me, so this patch is not needed for the paste bug specifically. Regards, Peter > Regards, > > Anthony Liguori > >> --- >> main-loop.c | 20 ++++++++++---------- >> 1 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/main-loop.c b/main-loop.c >> index f46aece..93d2917 100644 >> --- a/main-loop.c >> +++ b/main-loop.c >> @@ -200,10 +200,13 @@ static int os_host_main_loop_wait(uint32_t timeout) >> /* If the I/O thread is very busy or we are incorrectly busy waiting in >> * the I/O thread, this can lead to starvation of the BQL such that the >> * VCPU threads never run. To make sure we can detect the later case, >> - * print a message to the screen. If we run into this condition, create >> - * a fake timeout in order to give the VCPU threads a chance to run. >> + * print a message to the screen. If we run into this condition, unlock >> + * the BQL until a non-zero timout is given (indicating the starvation >> + * issue has gome away). >> */ >> - if (spin_counter > MAX_MAIN_LOOP_SPIN) { >> + if (timeout > 0) { >> + spin_counter = 0; >> + } else if (spin_counter > MAX_MAIN_LOOP_SPIN) { >> static bool notified; >> >> if (!notified) { >> @@ -212,21 +215,18 @@ static int os_host_main_loop_wait(uint32_t timeout) >> MAX_MAIN_LOOP_SPIN); >> notified = true; >> } >> - >> - timeout = 1; >> } >> >> - if (timeout > 0) { >> - spin_counter = 0; >> + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { >> qemu_mutex_unlock_iothread(); >> - } else { >> - spin_counter++; >> } >> >> ret = g_poll((GPollFD *)gpollfds->data, gpollfds->len, timeout); >> >> - if (timeout > 0) { >> + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { >> qemu_mutex_lock_iothread(); >> + } else { >> + spin_counter++; >> } >> >> glib_pollfds_poll(); >> -- >> 1.7.0.4 > >
Hi Peter, On Fri, Apr 19, 2013 at 12:22 AM, Peter Lieven <lieven-lists@dlhnet.de> wrote: > On 15.04.2013 15:08, Anthony Liguori wrote: >> >> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >> >>> Modify Anthony's starvation detection logic to keep the BQL unlocked >>> until the starvation condition goes away. Otherwise the counter has to >>> count up to 1000 for each needed iteration until the busy-wait is >>> lifted. >>> >>> Reset the counter back to zero once glib_pollfds_fill returns with a >>> non-zero timout, (indicating a return to normality). The 1000 iteration >>> wait now only happens once on the transition from normal operation to >>> busy-wait starvation. >>> >>> Anthony's original patch fixed the serial paste bug, but this patch is >>> also needed to restore performance. >>> >>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> I'm going through patches for 1.5 candidates. >> >> I believe the paste performance issue has been resolved now and this >> patch is no longer needed. I can't find a definitive statement on the >> list for that though. > > > I am also hitting a problem that occured first after Anthonys original > patch. > In my testing environment I had 3 vServers that indepently of the load > became > heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun > for 1000 iterations". > Even QMP is not responsing from time to time. But I am not using serial. > From the load statistics > it seems that the vServers is using one complete core busy waiting. > > I haven't seen this before this patch. Are you referring to my patch or Anthonys patch here? Does this patch introduce a regression (or even a change in behaviour) for you? Regards, Peter > > Peter > >
On 18.04.2013 16:35, Peter Crosthwaite wrote: > Hi Peter, > > On Fri, Apr 19, 2013 at 12:22 AM, Peter Lieven <lieven-lists@dlhnet.de> wrote: >> On 15.04.2013 15:08, Anthony Liguori wrote: >>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >>> >>>> Modify Anthony's starvation detection logic to keep the BQL unlocked >>>> until the starvation condition goes away. Otherwise the counter has to >>>> count up to 1000 for each needed iteration until the busy-wait is >>>> lifted. >>>> >>>> Reset the counter back to zero once glib_pollfds_fill returns with a >>>> non-zero timout, (indicating a return to normality). The 1000 iteration >>>> wait now only happens once on the transition from normal operation to >>>> busy-wait starvation. >>>> >>>> Anthony's original patch fixed the serial paste bug, but this patch is >>>> also needed to restore performance. >>>> >>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>> I'm going through patches for 1.5 candidates. >>> >>> I believe the paste performance issue has been resolved now and this >>> patch is no longer needed. I can't find a definitive statement on the >>> list for that though. >> >> I am also hitting a problem that occured first after Anthonys original >> patch. >> In my testing environment I had 3 vServers that indepently of the load >> became >> heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun >> for 1000 iterations". >> Even QMP is not responsing from time to time. But I am not using serial. >> From the load statistics >> it seems that the vServers is using one complete core busy waiting. >> >> I haven't seen this before this patch. > Are you referring to my patch or Anthonys patch here? Does this patch > introduce a regression (or even a change in behaviour) for you? I only noticed that after Anthonys patch (or at that time) I got VMs that became unresponsive after the thread spun notification. Can you imagine that the fake timeout that is introduced by this patch can somehow itself can cause a problem? Peter
Hi Peter, On Fri, Apr 19, 2013 at 5:01 PM, Peter Lieven <lieven-lists@dlhnet.de> wrote: > On 18.04.2013 16:35, Peter Crosthwaite wrote: >> >> Hi Peter, >> >> On Fri, Apr 19, 2013 at 12:22 AM, Peter Lieven <lieven-lists@dlhnet.de> >> wrote: >>> >>> On 15.04.2013 15:08, Anthony Liguori wrote: >>>> >>>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >>>> >>>>> Modify Anthony's starvation detection logic to keep the BQL unlocked >>>>> until the starvation condition goes away. Otherwise the counter has to >>>>> count up to 1000 for each needed iteration until the busy-wait is >>>>> lifted. >>>>> >>>>> Reset the counter back to zero once glib_pollfds_fill returns with a >>>>> non-zero timout, (indicating a return to normality). The 1000 iteration >>>>> wait now only happens once on the transition from normal operation to >>>>> busy-wait starvation. >>>>> >>>>> Anthony's original patch fixed the serial paste bug, but this patch is >>>>> also needed to restore performance. >>>>> >>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>> >>>> I'm going through patches for 1.5 candidates. >>>> >>>> I believe the paste performance issue has been resolved now and this >>>> patch is no longer needed. I can't find a definitive statement on the >>>> list for that though. >>> >>> >>> I am also hitting a problem that occured first after Anthonys original >>> patch. >>> In my testing environment I had 3 vServers that indepently of the load >>> became >>> heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun >>> for 1000 iterations". >>> Even QMP is not responsing from time to time. But I am not using serial. >>> From the load statistics >>> it seems that the vServers is using one complete core busy waiting. >>> >>> I haven't seen this before this patch. >> >> Are you referring to my patch or Anthonys patch here? Does this patch >> introduce a regression (or even a change in behaviour) for you? > > I only noticed that after Anthonys patch (or at that time) I got Can you re-confirm with a quick test? bisect could work, or more quickly a straight up git-revert of Anthonys patch ontop of whatever your are currently building to see if it makes any difference. Also try applying this patch. See if it clears up your issue as this patch is designed to improve performance in the spin-out case. > VMs that became unresponsive after the thread spun notification. > Can you imagine that the fake timeout that is introduced by this > patch can somehow itself can cause a problem? > No I cannot. Unless somehow there is a mutex issue with the lock, which would probably indicate a bug elsewhere in the code base. The affected unlock-relock is purely for optimization purposes and the QEMU should be able to work without it at all. Regards, Peter > Peter >
On 21.04.2013 16:21, Peter Crosthwaite wrote: > Hi Peter, > > On Fri, Apr 19, 2013 at 5:01 PM, Peter Lieven <lieven-lists@dlhnet.de> wrote: >> On 18.04.2013 16:35, Peter Crosthwaite wrote: >>> Hi Peter, >>> >>> On Fri, Apr 19, 2013 at 12:22 AM, Peter Lieven <lieven-lists@dlhnet.de> >>> wrote: >>>> On 15.04.2013 15:08, Anthony Liguori wrote: >>>>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >>>>> >>>>>> Modify Anthony's starvation detection logic to keep the BQL unlocked >>>>>> until the starvation condition goes away. Otherwise the counter has to >>>>>> count up to 1000 for each needed iteration until the busy-wait is >>>>>> lifted. >>>>>> >>>>>> Reset the counter back to zero once glib_pollfds_fill returns with a >>>>>> non-zero timout, (indicating a return to normality). The 1000 iteration >>>>>> wait now only happens once on the transition from normal operation to >>>>>> busy-wait starvation. >>>>>> >>>>>> Anthony's original patch fixed the serial paste bug, but this patch is >>>>>> also needed to restore performance. >>>>>> >>>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>> I'm going through patches for 1.5 candidates. >>>>> >>>>> I believe the paste performance issue has been resolved now and this >>>>> patch is no longer needed. I can't find a definitive statement on the >>>>> list for that though. >>>> >>>> I am also hitting a problem that occured first after Anthonys original >>>> patch. >>>> In my testing environment I had 3 vServers that indepently of the load >>>> became >>>> heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun >>>> for 1000 iterations". >>>> Even QMP is not responsing from time to time. But I am not using serial. >>>> From the load statistics >>>> it seems that the vServers is using one complete core busy waiting. >>>> >>>> I haven't seen this before this patch. >>> Are you referring to my patch or Anthonys patch here? Does this patch >>> introduce a regression (or even a change in behaviour) for you? >> I only noticed that after Anthonys patch (or at that time) I got > Can you re-confirm with a quick test? bisect could work, or more > quickly a straight up git-revert of Anthonys patch ontop of whatever > your are currently building to see if it makes any difference. Also > try applying this patch. See if it clears up your issue as this patch > is designed to improve performance in the spin-out case. Of course, I can, but I have no way to reproduce this issue. Just seeing it doesn't happen does not mean it is away... Peter
On 21.04.2013 16:21, Peter Crosthwaite wrote: > Hi Peter, > > On Fri, Apr 19, 2013 at 5:01 PM, Peter Lieven <lieven-lists@dlhnet.de> wrote: >> On 18.04.2013 16:35, Peter Crosthwaite wrote: >>> Hi Peter, >>> >>> On Fri, Apr 19, 2013 at 12:22 AM, Peter Lieven <lieven-lists@dlhnet.de> >>> wrote: >>>> On 15.04.2013 15:08, Anthony Liguori wrote: >>>>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: >>>>> >>>>>> Modify Anthony's starvation detection logic to keep the BQL unlocked >>>>>> until the starvation condition goes away. Otherwise the counter has to >>>>>> count up to 1000 for each needed iteration until the busy-wait is >>>>>> lifted. >>>>>> >>>>>> Reset the counter back to zero once glib_pollfds_fill returns with a >>>>>> non-zero timout, (indicating a return to normality). The 1000 iteration >>>>>> wait now only happens once on the transition from normal operation to >>>>>> busy-wait starvation. >>>>>> >>>>>> Anthony's original patch fixed the serial paste bug, but this patch is >>>>>> also needed to restore performance. >>>>>> >>>>>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >>>>> I'm going through patches for 1.5 candidates. >>>>> >>>>> I believe the paste performance issue has been resolved now and this >>>>> patch is no longer needed. I can't find a definitive statement on the >>>>> list for that though. >>>> >>>> I am also hitting a problem that occured first after Anthonys original >>>> patch. >>>> In my testing environment I had 3 vServers that indepently of the load >>>> became >>>> heavily unresponsive after reporting "main-loop: WARNING: I/O thread spun >>>> for 1000 iterations". >>>> Even QMP is not responsing from time to time. But I am not using serial. >>>> From the load statistics >>>> it seems that the vServers is using one complete core busy waiting. >>>> >>>> I haven't seen this before this patch. >>> Are you referring to my patch or Anthonys patch here? Does this patch >>> introduce a regression (or even a change in behaviour) for you? >> I only noticed that after Anthonys patch (or at that time) I got > Can you re-confirm with a quick test? bisect could work, or more > quickly a straight up git-revert of Anthonys patch ontop of whatever > your are currently building to see if it makes any difference. Also > try applying this patch. See if it clears up your issue as this patch > is designed to improve performance in the spin-out case. > With your patch I have not seen this unresponsiveness again. Peter
diff --git a/main-loop.c b/main-loop.c index f46aece..93d2917 100644 --- a/main-loop.c +++ b/main-loop.c @@ -200,10 +200,13 @@ static int os_host_main_loop_wait(uint32_t timeout) /* If the I/O thread is very busy or we are incorrectly busy waiting in * the I/O thread, this can lead to starvation of the BQL such that the * VCPU threads never run. To make sure we can detect the later case, - * print a message to the screen. If we run into this condition, create - * a fake timeout in order to give the VCPU threads a chance to run. + * print a message to the screen. If we run into this condition, unlock + * the BQL until a non-zero timout is given (indicating the starvation + * issue has gome away). */ - if (spin_counter > MAX_MAIN_LOOP_SPIN) { + if (timeout > 0) { + spin_counter = 0; + } else if (spin_counter > MAX_MAIN_LOOP_SPIN) { static bool notified; if (!notified) { @@ -212,21 +215,18 @@ static int os_host_main_loop_wait(uint32_t timeout) MAX_MAIN_LOOP_SPIN); notified = true; } - - timeout = 1; } - if (timeout > 0) { - spin_counter = 0; + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { qemu_mutex_unlock_iothread(); - } else { - spin_counter++; } ret = g_poll((GPollFD *)gpollfds->data, gpollfds->len, timeout); - if (timeout > 0) { + if (timeout > 0 || spin_counter > MAX_MAIN_LOOP_SPIN) { qemu_mutex_lock_iothread(); + } else { + spin_counter++; } glib_pollfds_poll();
Modify Anthony's starvation detection logic to keep the BQL unlocked until the starvation condition goes away. Otherwise the counter has to count up to 1000 for each needed iteration until the busy-wait is lifted. Reset the counter back to zero once glib_pollfds_fill returns with a non-zero timout, (indicating a return to normality). The 1000 iteration wait now only happens once on the transition from normal operation to busy-wait starvation. Anthony's original patch fixed the serial paste bug, but this patch is also needed to restore performance. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- main-loop.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)