1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp

Igor Mammedov posted 1 patch 5 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1528143278-247932-1-git-send-email-imammedo@redhat.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
slirp/slirp.c    | 10 ++--------
util/main-loop.c | 13 ++-----------
2 files changed, 4 insertions(+), 19 deletions(-)
[Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Igor Mammedov 5 years, 10 months ago 
Since commit (047f7038f58 cli: add --preconfig option) QEMU is
stuck with indefinite timeout in os_host_main_loop_wait()
at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
when it's started with -nodefaults CLI option like this:

  ./s390x-softmmu/qemu-system-s390x -nodefaults

It's caused by the fact that slirp_pollfds_fill() bails out early
and slirp_update_timeout() won't be called to update timeout
to a reasonable value (1 sec) so timeout would be left with
infinite value (0xFFFFFFFF).

Default infinite timeout though doen't make sense and reducing
it to 1 second as in slirp_update_timeout() won't affect host.
Fix issue by simplifying default timeout to the same 1sec as it
is in slirp_update_timeout() and cleanup the later. It makes
default timeout the same regardless of slirp_pollfds_fill()
exited early or not (i.e. -nodefaults won't have any effect on
main_loop_wait() anymore, which would provide more consistent
behavior between both variants of startup).

Reported-by: Lukáš Doktor <ldoktor@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
it doesn't fix issue reported by Max where
  "echo foo | $QEMU"
is also broken due to commit 047f7038f58, but there is antoher fix
on the list to fix that (either Michal's or Daniel's).
---
 slirp/slirp.c    | 10 ++--------
 util/main-loop.c | 13 ++-----------
 2 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1cb6b07..1112f86 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -358,13 +358,8 @@ void slirp_cleanup(Slirp *slirp)
 static void slirp_update_timeout(uint32_t *timeout)
 {
     Slirp *slirp;
-    uint32_t t;
 
-    if (*timeout <= TIMEOUT_FAST) {
-        return;
-    }
-
-    t = MIN(1000, *timeout);
+    assert(*timeout > TIMEOUT_FAST);
 
     /* If we have tcp timeout with slirp, then we will fill @timeout with
      * more precise value.
@@ -375,10 +370,9 @@ static void slirp_update_timeout(uint32_t *timeout)
             return;
         }
         if (slirp->do_slowtimo) {
-            t = MIN(TIMEOUT_SLOW, t);
+            *timeout = MIN(TIMEOUT_SLOW, *timeout);
         }
     }
-    *timeout = t;
 }
 
 void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout)
diff --git a/util/main-loop.c b/util/main-loop.c
index 992f9b0..fd23166 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -497,25 +497,16 @@ static int os_host_main_loop_wait(int64_t timeout)
 void main_loop_wait(int nonblocking)
 {
     int ret;
-    uint32_t timeout = UINT32_MAX;
     int64_t timeout_ns;
+    uint32_t timeout = nonblocking ? 0 : 1000 /* milliseconds */;
 
-    if (nonblocking) {
-        timeout = 0;
-    }
 
     /* poll any events */
     g_array_set_size(gpollfds, 0); /* reset for new iteration */
     /* XXX: separate device handlers from system ones */
     slirp_pollfds_fill(gpollfds, &timeout);
 
-    if (timeout == UINT32_MAX) {
-        timeout_ns = -1;
-    } else {
-        timeout_ns = (uint64_t)timeout * (int64_t)(SCALE_MS);
-    }
-
-    timeout_ns = qemu_soonest_timeout(timeout_ns,
+    timeout_ns = qemu_soonest_timeout((uint64_t)timeout * (int64_t)(SCALE_MS),
                                       timerlistgroup_deadline_ns(
                                           &main_loop_tlg));
 
-- 
2.7.4
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Eduardo Habkost 5 years, 10 months ago 
On Mon, Jun 04, 2018 at 10:14:38PM +0200, Igor Mammedov wrote:
> Since commit (047f7038f58 cli: add --preconfig option) QEMU is
> stuck with indefinite timeout in os_host_main_loop_wait()
> at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
> when it's started with -nodefaults CLI option like this:
> 
>   ./s390x-softmmu/qemu-system-s390x -nodefaults
> 
> It's caused by the fact that slirp_pollfds_fill() bails out early
> and slirp_update_timeout() won't be called to update timeout
> to a reasonable value (1 sec) so timeout would be left with
> infinite value (0xFFFFFFFF).
> 
> Default infinite timeout though doen't make sense and reducing
> it to 1 second as in slirp_update_timeout() won't affect host.

I don't get this part.  Why default infinite timeout doesn't make
sense?  Why would a 1 second timeout make sense?


> Fix issue by simplifying default timeout to the same 1sec as it
> is in slirp_update_timeout() and cleanup the later. It makes
> default timeout the same regardless of slirp_pollfds_fill()
> exited early or not (i.e. -nodefaults won't have any effect on
> main_loop_wait() anymore, which would provide more consistent
> behavior between both variants of startup).
> 
> Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
> it doesn't fix issue reported by Max where
>   "echo foo | $QEMU"
> is also broken due to commit 047f7038f58, but there is antoher fix
> on the list to fix that (either Michal's or Daniel's).
[...]

-- 
Eduardo
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Igor Mammedov 5 years, 10 months ago 
On Mon, 4 Jun 2018 22:04:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 04, 2018 at 10:14:38PM +0200, Igor Mammedov wrote:
> > Since commit (047f7038f58 cli: add --preconfig option) QEMU is
> > stuck with indefinite timeout in os_host_main_loop_wait()
> > at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
> > when it's started with -nodefaults CLI option like this:
> > 
> >   ./s390x-softmmu/qemu-system-s390x -nodefaults
> > 
> > It's caused by the fact that slirp_pollfds_fill() bails out early
> > and slirp_update_timeout() won't be called to update timeout
> > to a reasonable value (1 sec) so timeout would be left with
> > infinite value (0xFFFFFFFF).
> > 
> > Default infinite timeout though doen't make sense and reducing
> > it to 1 second as in slirp_update_timeout() won't affect host.  
> 
> I don't get this part.  Why default infinite timeout doesn't make
> sense?  Why would a 1 second timeout make sense?
I've meant that there is no reason for infinite timeuot,
and 1sec is good as any other finite one.
Hence we can unify timeout with/without -nodefaults, by moving 1sec
constant from slirp to main_loop_wait() and simplify code a bit.
 
> 
> > Fix issue by simplifying default timeout to the same 1sec as it
> > is in slirp_update_timeout() and cleanup the later. It makes
> > default timeout the same regardless of slirp_pollfds_fill()
> > exited early or not (i.e. -nodefaults won't have any effect on
> > main_loop_wait() anymore, which would provide more consistent
> > behavior between both variants of startup).
> > 
> > Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > PS:
> > it doesn't fix issue reported by Max where
> >   "echo foo | $QEMU"
> > is also broken due to commit 047f7038f58, but there is antoher fix
> > on the list to fix that (either Michal's or Daniel's).  
> [...]
>
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Daniel P. Berrangé 5 years, 10 months ago 
On Tue, Jun 05, 2018 at 10:27:03AM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 22:04:13 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 04, 2018 at 10:14:38PM +0200, Igor Mammedov wrote:
> > > Since commit (047f7038f58 cli: add --preconfig option) QEMU is
> > > stuck with indefinite timeout in os_host_main_loop_wait()
> > > at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
> > > when it's started with -nodefaults CLI option like this:
> > > 
> > >   ./s390x-softmmu/qemu-system-s390x -nodefaults
> > > 
> > > It's caused by the fact that slirp_pollfds_fill() bails out early
> > > and slirp_update_timeout() won't be called to update timeout
> > > to a reasonable value (1 sec) so timeout would be left with
> > > infinite value (0xFFFFFFFF).
> > > 
> > > Default infinite timeout though doen't make sense and reducing
> > > it to 1 second as in slirp_update_timeout() won't affect host.  
> > 
> > I don't get this part.  Why default infinite timeout doesn't make
> > sense?  Why would a 1 second timeout make sense?
> I've meant that there is no reason for infinite timeuot,
> and 1sec is good as any other finite one.

I don't really agree - it is better to not wakeup at all if there's
no work todo rather than pointlessly wake up once a second, to deal
with a hack for the SLIRP feature that's almost never used in most
production scenarios..

> Hence we can unify timeout with/without -nodefaults, by moving 1sec
> constant from slirp to main_loop_wait() and simplify code a bit.
>  
> > 
> > > Fix issue by simplifying default timeout to the same 1sec as it
> > > is in slirp_update_timeout() and cleanup the later. It makes
> > > default timeout the same regardless of slirp_pollfds_fill()
> > > exited early or not (i.e. -nodefaults won't have any effect on
> > > main_loop_wait() anymore, which would provide more consistent
> > > behavior between both variants of startup).
> > > 
> > > Reported-by: Lukáš Doktor <ldoktor@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > PS:
> > > it doesn't fix issue reported by Max where
> > >   "echo foo | $QEMU"
> > > is also broken due to commit 047f7038f58, but there is antoher fix
> > > on the list to fix that (either Michal's or Daniel's).  
> > [...]
> > 
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Stefan Hajnoczi 5 years, 10 months ago 
On Tue, Jun 05, 2018 at 09:47:44AM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 05, 2018 at 10:27:03AM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 22:04:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Jun 04, 2018 at 10:14:38PM +0200, Igor Mammedov wrote:
> > > > Since commit (047f7038f58 cli: add --preconfig option) QEMU is
> > > > stuck with indefinite timeout in os_host_main_loop_wait()
> > > > at RUN_STATE_PRECONFIG even if --preconfig option wasn't used
> > > > when it's started with -nodefaults CLI option like this:
> > > > 
> > > >   ./s390x-softmmu/qemu-system-s390x -nodefaults
> > > > 
> > > > It's caused by the fact that slirp_pollfds_fill() bails out early
> > > > and slirp_update_timeout() won't be called to update timeout
> > > > to a reasonable value (1 sec) so timeout would be left with
> > > > infinite value (0xFFFFFFFF).
> > > > 
> > > > Default infinite timeout though doen't make sense and reducing
> > > > it to 1 second as in slirp_update_timeout() won't affect host.  
> > > 
> > > I don't get this part.  Why default infinite timeout doesn't make
> > > sense?  Why would a 1 second timeout make sense?
> > I've meant that there is no reason for infinite timeuot,
> > and 1sec is good as any other finite one.
> 
> I don't really agree - it is better to not wakeup at all if there's
> no work todo rather than pointlessly wake up once a second, to deal
> with a hack for the SLIRP feature that's almost never used in most
> production scenarios..

Right, a host with 1000 VMs should experience 1000 wakeups/second for no
good reason.  QEMU needs to be scalable.

Stefan
Re: [Qemu-devel] [PATCH] vl.c: make default main_loop_wait() timeout independed of slirp
Posted by Paolo Bonzini 5 years, 10 months ago 
On 05/06/2018 10:27, Igor Mammedov wrote:
>> I don't get this part.  Why default infinite timeout doesn't make
>> sense?  Why would a 1 second timeout make sense?
>
> I've meant that there is no reason for infinite timeout,
> and 1sec is good as any other finite one.
> Hence we can unify timeout with/without -nodefaults, by moving 1sec
> constant from slirp to main_loop_wait() and simplify code a bit.

The event loop is supposed to wake up when there is an event.  If there
is no event, the timeout should be infinite, otherwise you're just
introducing unnecessary BQL contention and/or papering over bugs.  So I
agree with Eduardo, I'm not sure what you mean when you say that the
infinite timeout makes no sense.

In fact, the 1-second limit on the SLIRP timeout was introduced in
commit a42e9c4188 ("slirp: set mainloop timeout with more precise
value", 2013-09-17) with no justification in the commit message.  The
desirable fix would be to remove that limit, not to push it up the stack
and actrivate it for all executions of QEMU.

Paolo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.