Message ID | 1377051352-23499-4-git-send-email-pingfank@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
--On 21 August 2013 10:15:52 +0800 Liu Ping Fan <qemulist@gmail.com> wrote: > -void slirp_update_timeout(uint32_t *timeout) > +static void slirp_update_timeout(uint32_t *timeout) > { > - if (!QTAILQ_EMPTY(&slirp_instances)) { > - *timeout = MIN(1000, *timeout); If you are putting things in macros, you might as well change that 1000 as well, and hopefully comment why that particular magic value is there. > + Slirp *slirp; > + uint32_t t; > + > + *timeout = MIN(1000, *timeout); > + if (*timeout <= TIMEOUT_FAST) { > + return; > + } > + t = *timeout;
On Wed, Aug 21, 2013 at 3:36 PM, Alex Bligh <alex@alex.org.uk> wrote: > > > --On 21 August 2013 10:15:52 +0800 Liu Ping Fan <qemulist@gmail.com> wrote: > >> -void slirp_update_timeout(uint32_t *timeout) >> +static void slirp_update_timeout(uint32_t *timeout) >> { >> - if (!QTAILQ_EMPTY(&slirp_instances)) { >> - *timeout = MIN(1000, *timeout); > > > If you are putting things in macros, you might as well change that TIMEOUT_FAST/SLOW have definite meaning, and used more than one place in the code. For 1000ms, I do not know this magic value's meaning, but whatever, it just occurs once. So there is no trouble to read the code. > 1000 as well, and hopefully comment why that particular magic value > is there. > > >> + Slirp *slirp; >> + uint32_t t; >> + >> + *timeout = MIN(1000, *timeout); >> + if (*timeout <= TIMEOUT_FAST) { >> + return; >> + } >> + t = *timeout; > > > > > -- > Alex Bligh
On 2013-08-21 10:07, liu ping fan wrote: > On Wed, Aug 21, 2013 at 3:36 PM, Alex Bligh <alex@alex.org.uk> wrote: >> >> >> --On 21 August 2013 10:15:52 +0800 Liu Ping Fan <qemulist@gmail.com> wrote: >> >>> -void slirp_update_timeout(uint32_t *timeout) >>> +static void slirp_update_timeout(uint32_t *timeout) >>> { >>> - if (!QTAILQ_EMPTY(&slirp_instances)) { >>> - *timeout = MIN(1000, *timeout); >> >> >> If you are putting things in macros, you might as well change that > > TIMEOUT_FAST/SLOW have definite meaning, and used more than one place > in the code. For 1000ms, I do not know this magic value's meaning, but > whatever, it just occurs once. So there is no trouble to read the > code. You could name it ONE_SEC or so. Can be done as trivial patch on top. IIRC, slirp requires regular polling for the aging of certain requests like DNS. Jan > >> 1000 as well, and hopefully comment why that particular magic value >> is there. >> >> >>> + Slirp *slirp; >>> + uint32_t t; >>> + >>> + *timeout = MIN(1000, *timeout); >>> + if (*timeout <= TIMEOUT_FAST) { >>> + return; >>> + } >>> + t = *timeout; >> >> >> >> >> -- >> Alex Bligh
On 2013-08-21 04:15, Liu Ping Fan wrote: > If slirp needs to emulate tcp timeout, then the timeout value > for mainloop should be more precise, which is determined by > slirp's fasttimo or slowtimo. Achieve this by swap the logic > sequence of slirp_pollfds_fill and slirp_update_timeout. > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > --- > main-loop.c | 3 +-- > slirp/libslirp.h | 3 +-- > slirp/slirp.c | 28 ++++++++++++++++++++++++---- > stubs/slirp.c | 6 +----- > 4 files changed, 27 insertions(+), 13 deletions(-) > > diff --git a/main-loop.c b/main-loop.c > index a44fff6..e258567 100644 > --- a/main-loop.c > +++ b/main-loop.c > @@ -458,8 +458,7 @@ int main_loop_wait(int nonblocking) > g_array_set_size(gpollfds, 0); /* reset for new iteration */ > /* XXX: separate device handlers from system ones */ > #ifdef CONFIG_SLIRP > - slirp_update_timeout(&timeout); > - slirp_pollfds_fill(gpollfds); > + slirp_pollfds_fill(gpollfds, &timeout); > #endif > qemu_iohandler_fill(gpollfds); > ret = os_host_main_loop_wait(timeout); > diff --git a/slirp/libslirp.h b/slirp/libslirp.h > index ceabff8..5bdcbd5 100644 > --- a/slirp/libslirp.h > +++ b/slirp/libslirp.h > @@ -16,8 +16,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork, > void *opaque); > void slirp_cleanup(Slirp *slirp); > > -void slirp_update_timeout(uint32_t *timeout); > -void slirp_pollfds_fill(GArray *pollfds); > +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout); > > void slirp_pollfds_poll(GArray *pollfds, int select_error); > > diff --git a/slirp/slirp.c b/slirp/slirp.c > index 1e8983e..f312a7d 100644 > --- a/slirp/slirp.c > +++ b/slirp/slirp.c > @@ -260,14 +260,33 @@ void slirp_cleanup(Slirp *slirp) > #define CONN_CANFSEND(so) (((so)->so_state & (SS_FCANTSENDMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) > #define CONN_CANFRCV(so) (((so)->so_state & (SS_FCANTRCVMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) > > -void slirp_update_timeout(uint32_t *timeout) > +static void slirp_update_timeout(uint32_t *timeout) > { > - if (!QTAILQ_EMPTY(&slirp_instances)) { > - *timeout = MIN(1000, *timeout); > + Slirp *slirp; > + uint32_t t; > + > + *timeout = MIN(1000, *timeout); > + if (*timeout <= TIMEOUT_FAST) { Nitpicking, sorry, but TIMEOUT_FAST is always smaller than 1000. So this check should come first, and then the MIN assignment (to t). Jan > + return; > + } > + t = *timeout; > + > + /* If we have tcp timeout with slirp, then we will fill @timeout with > + * more precise value. > + */ > + QTAILQ_FOREACH(slirp, &slirp_instances, entry) { > + if (slirp->time_fasttimo) { > + *timeout = TIMEOUT_FAST; > + return; > + } > + if (slirp->do_slowtimo) { > + t = MIN(TIMEOUT_SLOW, t); > + } > } > + *timeout = t; > } > > -void slirp_pollfds_fill(GArray *pollfds) > +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout) > { > Slirp *slirp; > struct socket *so, *so_next; > @@ -437,6 +456,7 @@ void slirp_pollfds_fill(GArray *pollfds) > } > } > } > + slirp_update_timeout(timeout); > } > > void slirp_pollfds_poll(GArray *pollfds, int select_error) > diff --git a/stubs/slirp.c b/stubs/slirp.c > index f1fc833..bd0ac7f 100644 > --- a/stubs/slirp.c > +++ b/stubs/slirp.c > @@ -1,11 +1,7 @@ > #include "qemu-common.h" > #include "slirp/slirp.h" > > -void slirp_update_timeout(uint32_t *timeout) > -{ > -} > - > -void slirp_pollfds_fill(GArray *pollfds) > +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout) > { > } > >
On Sat, Aug 24, 2013 at 12:49 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2013-08-21 10:07, liu ping fan wrote: >> On Wed, Aug 21, 2013 at 3:36 PM, Alex Bligh <alex@alex.org.uk> wrote: >>> >>> >>> --On 21 August 2013 10:15:52 +0800 Liu Ping Fan <qemulist@gmail.com> wrote: >>> >>>> -void slirp_update_timeout(uint32_t *timeout) >>>> +static void slirp_update_timeout(uint32_t *timeout) >>>> { >>>> - if (!QTAILQ_EMPTY(&slirp_instances)) { >>>> - *timeout = MIN(1000, *timeout); >>> >>> >>> If you are putting things in macros, you might as well change that >> >> TIMEOUT_FAST/SLOW have definite meaning, and used more than one place >> in the code. For 1000ms, I do not know this magic value's meaning, but >> whatever, it just occurs once. So there is no trouble to read the >> code. > > You could name it ONE_SEC or so. Can be done as trivial patch on top. > > IIRC, slirp requires regular polling for the aging of certain requests > like DNS. > Thanks for the explanation. Will fix and document it. Regards, Pingfan
On Sat, Aug 24, 2013 at 12:54 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2013-08-21 04:15, Liu Ping Fan wrote: >> If slirp needs to emulate tcp timeout, then the timeout value >> for mainloop should be more precise, which is determined by >> slirp's fasttimo or slowtimo. Achieve this by swap the logic >> sequence of slirp_pollfds_fill and slirp_update_timeout. >> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> >> --- >> main-loop.c | 3 +-- >> slirp/libslirp.h | 3 +-- >> slirp/slirp.c | 28 ++++++++++++++++++++++++---- >> stubs/slirp.c | 6 +----- >> 4 files changed, 27 insertions(+), 13 deletions(-) >> >> diff --git a/main-loop.c b/main-loop.c >> index a44fff6..e258567 100644 >> --- a/main-loop.c >> +++ b/main-loop.c >> @@ -458,8 +458,7 @@ int main_loop_wait(int nonblocking) >> g_array_set_size(gpollfds, 0); /* reset for new iteration */ >> /* XXX: separate device handlers from system ones */ >> #ifdef CONFIG_SLIRP >> - slirp_update_timeout(&timeout); >> - slirp_pollfds_fill(gpollfds); >> + slirp_pollfds_fill(gpollfds, &timeout); >> #endif >> qemu_iohandler_fill(gpollfds); >> ret = os_host_main_loop_wait(timeout); >> diff --git a/slirp/libslirp.h b/slirp/libslirp.h >> index ceabff8..5bdcbd5 100644 >> --- a/slirp/libslirp.h >> +++ b/slirp/libslirp.h >> @@ -16,8 +16,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork, >> void *opaque); >> void slirp_cleanup(Slirp *slirp); >> >> -void slirp_update_timeout(uint32_t *timeout); >> -void slirp_pollfds_fill(GArray *pollfds); >> +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout); >> >> void slirp_pollfds_poll(GArray *pollfds, int select_error); >> >> diff --git a/slirp/slirp.c b/slirp/slirp.c >> index 1e8983e..f312a7d 100644 >> --- a/slirp/slirp.c >> +++ b/slirp/slirp.c >> @@ -260,14 +260,33 @@ void slirp_cleanup(Slirp *slirp) >> #define CONN_CANFSEND(so) (((so)->so_state & (SS_FCANTSENDMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) >> #define CONN_CANFRCV(so) (((so)->so_state & (SS_FCANTRCVMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) >> >> -void slirp_update_timeout(uint32_t *timeout) >> +static void slirp_update_timeout(uint32_t *timeout) >> { >> - if (!QTAILQ_EMPTY(&slirp_instances)) { >> - *timeout = MIN(1000, *timeout); >> + Slirp *slirp; >> + uint32_t t; >> + >> + *timeout = MIN(1000, *timeout); >> + if (*timeout <= TIMEOUT_FAST) { > > Nitpicking, sorry, but TIMEOUT_FAST is always smaller than 1000. So this > check should come first, and then the MIN assignment (to t). > Will fix. Regards, Pingfan
diff --git a/main-loop.c b/main-loop.c index a44fff6..e258567 100644 --- a/main-loop.c +++ b/main-loop.c @@ -458,8 +458,7 @@ int main_loop_wait(int nonblocking) g_array_set_size(gpollfds, 0); /* reset for new iteration */ /* XXX: separate device handlers from system ones */ #ifdef CONFIG_SLIRP - slirp_update_timeout(&timeout); - slirp_pollfds_fill(gpollfds); + slirp_pollfds_fill(gpollfds, &timeout); #endif qemu_iohandler_fill(gpollfds); ret = os_host_main_loop_wait(timeout); diff --git a/slirp/libslirp.h b/slirp/libslirp.h index ceabff8..5bdcbd5 100644 --- a/slirp/libslirp.h +++ b/slirp/libslirp.h @@ -16,8 +16,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork, void *opaque); void slirp_cleanup(Slirp *slirp); -void slirp_update_timeout(uint32_t *timeout); -void slirp_pollfds_fill(GArray *pollfds); +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout); void slirp_pollfds_poll(GArray *pollfds, int select_error); diff --git a/slirp/slirp.c b/slirp/slirp.c index 1e8983e..f312a7d 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -260,14 +260,33 @@ void slirp_cleanup(Slirp *slirp) #define CONN_CANFSEND(so) (((so)->so_state & (SS_FCANTSENDMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) #define CONN_CANFRCV(so) (((so)->so_state & (SS_FCANTRCVMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) -void slirp_update_timeout(uint32_t *timeout) +static void slirp_update_timeout(uint32_t *timeout) { - if (!QTAILQ_EMPTY(&slirp_instances)) { - *timeout = MIN(1000, *timeout); + Slirp *slirp; + uint32_t t; + + *timeout = MIN(1000, *timeout); + if (*timeout <= TIMEOUT_FAST) { + return; + } + t = *timeout; + + /* If we have tcp timeout with slirp, then we will fill @timeout with + * more precise value. + */ + QTAILQ_FOREACH(slirp, &slirp_instances, entry) { + if (slirp->time_fasttimo) { + *timeout = TIMEOUT_FAST; + return; + } + if (slirp->do_slowtimo) { + t = MIN(TIMEOUT_SLOW, t); + } } + *timeout = t; } -void slirp_pollfds_fill(GArray *pollfds) +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout) { Slirp *slirp; struct socket *so, *so_next; @@ -437,6 +456,7 @@ void slirp_pollfds_fill(GArray *pollfds) } } } + slirp_update_timeout(timeout); } void slirp_pollfds_poll(GArray *pollfds, int select_error) diff --git a/stubs/slirp.c b/stubs/slirp.c index f1fc833..bd0ac7f 100644 --- a/stubs/slirp.c +++ b/stubs/slirp.c @@ -1,11 +1,7 @@ #include "qemu-common.h" #include "slirp/slirp.h" -void slirp_update_timeout(uint32_t *timeout) -{ -} - -void slirp_pollfds_fill(GArray *pollfds) +void slirp_pollfds_fill(GArray *pollfds, uint32_t *timeout) { }
If slirp needs to emulate tcp timeout, then the timeout value for mainloop should be more precise, which is determined by slirp's fasttimo or slowtimo. Achieve this by swap the logic sequence of slirp_pollfds_fill and slirp_update_timeout. Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> --- main-loop.c | 3 +-- slirp/libslirp.h | 3 +-- slirp/slirp.c | 28 ++++++++++++++++++++++++---- stubs/slirp.c | 6 +----- 4 files changed, 27 insertions(+), 13 deletions(-)