[Qemu-devel] [PATCH 0/3] slirp: fill mainloop with more precise timeout value

[Qemu-devel] [PATCH 0/3] slirp: fill mainloop with more precise timeout value

[Liu Ping Fan]

These patches is separated from "[PATCH v1 0/5] make slirp subsystem self-contained", as Paolo suggested.
    lists.nongnu.org/archive/html/qemu-devel/2013-08/msg00980.html

With them, we can fill the timeout of mainloop more precise, when slirp has to emulate tcp timeout
problem.

Liu Ping Fan (3):
  slirp: make timeout local
  slirp: define timeout as macro
  slirp: fill mainloop timeout with more precise value

 main-loop.c   |  2 +-
 slirp/slirp.c | 43 +++++++++++++++++++++++++++++++------------
 slirp/slirp.h |  3 +++
 3 files changed, 35 insertions(+), 13 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] slirp: make timeout local

[Liu Ping Fan]

Each slirp has its own time to caculate timeout.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 slirp/slirp.c | 22 ++++++++++------------
 slirp/slirp.h |  3 +++
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 80b28ea..55654d5 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -40,8 +40,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
 static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
 
 u_int curtime;
-static u_int time_fasttimo, last_slowtimo;
-static int do_slowtimo;
 
 static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
     QTAILQ_HEAD_INITIALIZER(slirp_instances);
@@ -278,14 +276,13 @@ void slirp_pollfds_fill(GArray *pollfds)
     /*
      * First, TCP sockets
      */
-    do_slowtimo = 0;
 
     QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
         /*
          * *_slowtimo needs calling if there are IP fragments
          * in the fragment queue, or there are TCP connections active
          */
-        do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
+        slirp->do_slowtimo = ((slirp->tcb.so_next != &slirp->tcb) ||
                 (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
 
         for (so = slirp->tcb.so_next; so != &slirp->tcb;
@@ -299,8 +296,9 @@ void slirp_pollfds_fill(GArray *pollfds)
             /*
              * See if we need a tcp_fasttimo
              */
-            if (time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
-                time_fasttimo = curtime; /* Flag when we want a fasttimo */
+            if (slirp->time_fasttimo == 0 &&
+                so->so_tcpcb->t_flags & TF_DELACK) {
+                slirp->time_fasttimo = curtime; /* Flag when want a fasttimo */
             }
 
             /*
@@ -381,7 +379,7 @@ void slirp_pollfds_fill(GArray *pollfds)
                     udp_detach(so);
                     continue;
                 } else {
-                    do_slowtimo = 1; /* Let socket expire */
+                    slirp->do_slowtimo = 1; /* Let socket expire */
                 }
             }
 
@@ -422,7 +420,7 @@ void slirp_pollfds_fill(GArray *pollfds)
                     icmp_detach(so);
                     continue;
                 } else {
-                    do_slowtimo = 1; /* Let socket expire */
+                    slirp->do_slowtimo = 1; /* Let socket expire */
                 }
             }
 
@@ -454,14 +452,14 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
         /*
          * See if anything has timed out
          */
-        if (time_fasttimo && ((curtime - time_fasttimo) >= 2)) {
+        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
             tcp_fasttimo(slirp);
-            time_fasttimo = 0;
+            slirp->time_fasttimo = 0;
         }
-        if (do_slowtimo && ((curtime - last_slowtimo) >= 499)) {
+        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
             ip_slowtimo(slirp);
             tcp_slowtimo(slirp);
-            last_slowtimo = curtime;
+            slirp->last_slowtimo = curtime;
         }
 
         /*
diff --git a/slirp/slirp.h b/slirp/slirp.h
index fe0e65d..008360e 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -203,6 +203,9 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
 
 struct Slirp {
     QTAILQ_ENTRY(Slirp) entry;
+    u_int time_fasttimo;
+    u_int last_slowtimo;
+    int do_slowtimo;
 
     /* virtual network configuration */
     struct in_addr vnetwork_addr;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/3] slirp: make timeout local

[Jan Kiszka]

On 2013-08-13 05:15, Liu Ping Fan wrote:
> Each slirp has its own time to caculate timeout.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  slirp/slirp.c | 22 ++++++++++------------
>  slirp/slirp.h |  3 +++
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 80b28ea..55654d5 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -40,8 +40,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
>  static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>  
>  u_int curtime;
> -static u_int time_fasttimo, last_slowtimo;
> -static int do_slowtimo;
>  
>  static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
>      QTAILQ_HEAD_INITIALIZER(slirp_instances);
> @@ -278,14 +276,13 @@ void slirp_pollfds_fill(GArray *pollfds)
>      /*
>       * First, TCP sockets
>       */
> -    do_slowtimo = 0;
>  
>      QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
>          /*
>           * *_slowtimo needs calling if there are IP fragments
>           * in the fragment queue, or there are TCP connections active
>           */
> -        do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
> +        slirp->do_slowtimo = ((slirp->tcb.so_next != &slirp->tcb) ||

What's the reason for changing the original logic for do_slowtimo as well?

>                  (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
>  
>          for (so = slirp->tcb.so_next; so != &slirp->tcb;
> @@ -299,8 +296,9 @@ void slirp_pollfds_fill(GArray *pollfds)
>              /*
>               * See if we need a tcp_fasttimo
>               */
> -            if (time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
> -                time_fasttimo = curtime; /* Flag when we want a fasttimo */
> +            if (slirp->time_fasttimo == 0 &&
> +                so->so_tcpcb->t_flags & TF_DELACK) {
> +                slirp->time_fasttimo = curtime; /* Flag when want a fasttimo */
>              }
>  
>              /*
> @@ -381,7 +379,7 @@ void slirp_pollfds_fill(GArray *pollfds)
>                      udp_detach(so);
>                      continue;
>                  } else {
> -                    do_slowtimo = 1; /* Let socket expire */
> +                    slirp->do_slowtimo = 1; /* Let socket expire */
>                  }
>              }
>  
> @@ -422,7 +420,7 @@ void slirp_pollfds_fill(GArray *pollfds)
>                      icmp_detach(so);
>                      continue;
>                  } else {
> -                    do_slowtimo = 1; /* Let socket expire */
> +                    slirp->do_slowtimo = 1; /* Let socket expire */
>                  }
>              }
>  
> @@ -454,14 +452,14 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>          /*
>           * See if anything has timed out
>           */
> -        if (time_fasttimo && ((curtime - time_fasttimo) >= 2)) {
> +        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
>              tcp_fasttimo(slirp);
> -            time_fasttimo = 0;
> +            slirp->time_fasttimo = 0;
>          }
> -        if (do_slowtimo && ((curtime - last_slowtimo) >= 499)) {
> +        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
>              ip_slowtimo(slirp);
>              tcp_slowtimo(slirp);
> -            last_slowtimo = curtime;
> +            slirp->last_slowtimo = curtime;
>          }
>  
>          /*
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index fe0e65d..008360e 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -203,6 +203,9 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>  
>  struct Slirp {
>      QTAILQ_ENTRY(Slirp) entry;
> +    u_int time_fasttimo;
> +    u_int last_slowtimo;
> +    int do_slowtimo;

"bool do_slowtimo" would be nicer.

>  
>      /* virtual network configuration */
>      struct in_addr vnetwork_addr;
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1/3] slirp: make timeout local

[liu ping fan]

On Tue, Aug 13, 2013 at 4:03 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-08-13 05:15, Liu Ping Fan wrote:
>> Each slirp has its own time to caculate timeout.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  slirp/slirp.c | 22 ++++++++++------------
>>  slirp/slirp.h |  3 +++
>>  2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 80b28ea..55654d5 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -40,8 +40,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
>>  static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>>
>>  u_int curtime;
>> -static u_int time_fasttimo, last_slowtimo;
>> -static int do_slowtimo;
>>
>>  static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
>>      QTAILQ_HEAD_INITIALIZER(slirp_instances);
>> @@ -278,14 +276,13 @@ void slirp_pollfds_fill(GArray *pollfds)
>>      /*
>>       * First, TCP sockets
>>       */
>> -    do_slowtimo = 0;
>>
>>      QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
>>          /*
>>           * *_slowtimo needs calling if there are IP fragments
>>           * in the fragment queue, or there are TCP connections active
>>           */
>> -        do_slowtimo |= ((slirp->tcb.so_next != &slirp->tcb) ||
>> +        slirp->do_slowtimo = ((slirp->tcb.so_next != &slirp->tcb) ||
>
> What's the reason for changing the original logic for do_slowtimo as well?
>
Original code, do_slowtimo belong to the slirp system. After changing,
it is based on slirp instance. So no need to OR.

>>                  (&slirp->ipq.ip_link != slirp->ipq.ip_link.next));
>>
>>          for (so = slirp->tcb.so_next; so != &slirp->tcb;
>> @@ -299,8 +296,9 @@ void slirp_pollfds_fill(GArray *pollfds)
>>              /*
>>               * See if we need a tcp_fasttimo
>>               */
>> -            if (time_fasttimo == 0 && so->so_tcpcb->t_flags & TF_DELACK) {
>> -                time_fasttimo = curtime; /* Flag when we want a fasttimo */
>> +            if (slirp->time_fasttimo == 0 &&
>> +                so->so_tcpcb->t_flags & TF_DELACK) {
>> +                slirp->time_fasttimo = curtime; /* Flag when want a fasttimo */
>>              }
>>
>>              /*
>> @@ -381,7 +379,7 @@ void slirp_pollfds_fill(GArray *pollfds)
>>                      udp_detach(so);
>>                      continue;
>>                  } else {
>> -                    do_slowtimo = 1; /* Let socket expire */
>> +                    slirp->do_slowtimo = 1; /* Let socket expire */
>>                  }
>>              }
>>
>> @@ -422,7 +420,7 @@ void slirp_pollfds_fill(GArray *pollfds)
>>                      icmp_detach(so);
>>                      continue;
>>                  } else {
>> -                    do_slowtimo = 1; /* Let socket expire */
>> +                    slirp->do_slowtimo = 1; /* Let socket expire */
>>                  }
>>              }
>>
>> @@ -454,14 +452,14 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>>          /*
>>           * See if anything has timed out
>>           */
>> -        if (time_fasttimo && ((curtime - time_fasttimo) >= 2)) {
>> +        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
>>              tcp_fasttimo(slirp);
>> -            time_fasttimo = 0;
>> +            slirp->time_fasttimo = 0;
>>          }
>> -        if (do_slowtimo && ((curtime - last_slowtimo) >= 499)) {
>> +        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
>>              ip_slowtimo(slirp);
>>              tcp_slowtimo(slirp);
>> -            last_slowtimo = curtime;
>> +            slirp->last_slowtimo = curtime;
>>          }
>>
>>          /*
>> diff --git a/slirp/slirp.h b/slirp/slirp.h
>> index fe0e65d..008360e 100644
>> --- a/slirp/slirp.h
>> +++ b/slirp/slirp.h
>> @@ -203,6 +203,9 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>>
>>  struct Slirp {
>>      QTAILQ_ENTRY(Slirp) entry;
>> +    u_int time_fasttimo;
>> +    u_int last_slowtimo;
>> +    int do_slowtimo;
>
> "bool do_slowtimo" would be nicer.
>
Ok.

Regards,
Pingfan

[Qemu-devel] [PATCH 2/3] slirp: define timeout as macro

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 slirp/slirp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 55654d5..1deaad9 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -47,6 +47,9 @@ static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
 static struct in_addr dns_addr;
 static u_int dns_addr_time;
 
+#define TIMEOUT_FAST 2
+#define TIMEOUT_SLOW 499
+
 #ifdef _WIN32
 
 int get_dns_addr(struct in_addr *pdns_addr)
@@ -452,11 +455,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
         /*
          * See if anything has timed out
          */
-        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
+        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= TIMEOUT_FAST)) {
             tcp_fasttimo(slirp);
             slirp->time_fasttimo = 0;
         }
-        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
+        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= TIMEOUT_SLOW)) {
             ip_slowtimo(slirp);
             tcp_slowtimo(slirp);
             slirp->last_slowtimo = curtime;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] slirp: define timeout as macro

[Jan Kiszka]

On 2013-08-13 05:15, Liu Ping Fan wrote:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  slirp/slirp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 55654d5..1deaad9 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -47,6 +47,9 @@ static QTAILQ_HEAD(slirp_instances, Slirp) slirp_instances =
>  static struct in_addr dns_addr;
>  static u_int dns_addr_time;
>  
> +#define TIMEOUT_FAST 2
> +#define TIMEOUT_SLOW 499
> +
>  #ifdef _WIN32
>  
>  int get_dns_addr(struct in_addr *pdns_addr)
> @@ -452,11 +455,11 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>          /*
>           * See if anything has timed out
>           */
> -        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= 2)) {
> +        if (slirp->time_fasttimo && ((curtime - slirp->time_fasttimo) >= TIMEOUT_FAST)) {

Overlong line.

>              tcp_fasttimo(slirp);
>              slirp->time_fasttimo = 0;
>          }
> -        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= 499)) {
> +        if (slirp->do_slowtimo && ((curtime - slirp->last_slowtimo) >= TIMEOUT_SLOW)) {

Here probably as well.

>              ip_slowtimo(slirp);
>              tcp_slowtimo(slirp);
>              slirp->last_slowtimo = curtime;
> 

JAn

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 3/3] slirp: fill mainloop timeout with more precise value

[Liu Ping Fan]

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.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 main-loop.c   |  2 +-
 slirp/slirp.c | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/main-loop.c b/main-loop.c
index a44fff6..04120d2 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -458,8 +458,8 @@ 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_update_timeout(&timeout);
 #endif
     qemu_iohandler_fill(gpollfds);
     ret = os_host_main_loop_wait(timeout);
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 1deaad9..af66006 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -262,9 +262,27 @@ void slirp_cleanup(Slirp *slirp)
 
 void slirp_update_timeout(uint32_t *timeout)
 {
+    Slirp *slirp;
+
     if (!QTAILQ_EMPTY(&slirp_instances)) {
         *timeout = MIN(1000, *timeout);
     }
+    if (*timeout <= TIMEOUT_FAST) {
+        return;
+    }
+
+    /* 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;
+            break;
+        }
+        if (slirp->do_slowtimo) {
+            *timeout = MIN(TIMEOUT_SLOW, *timeout);
+        }
+    }
 }
 
 void slirp_pollfds_fill(GArray *pollfds)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/3] slirp: fill mainloop timeout with more precise value

[Jan Kiszka]

On 2013-08-13 05: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.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  main-loop.c   |  2 +-
>  slirp/slirp.c | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/main-loop.c b/main-loop.c
> index a44fff6..04120d2 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -458,8 +458,8 @@ 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_update_timeout(&timeout);

Why this reordering?

>  #endif
>      qemu_iohandler_fill(gpollfds);
>      ret = os_host_main_loop_wait(timeout);
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 1deaad9..af66006 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -262,9 +262,27 @@ void slirp_cleanup(Slirp *slirp)
>  
>  void slirp_update_timeout(uint32_t *timeout)
>  {
> +    Slirp *slirp;
> +
>      if (!QTAILQ_EMPTY(&slirp_instances)) {

if (QTAILQ_EMPTY(&slirp_instances) || *timeout <= TIMEOUT_FAST) {
    return;
}

*timeout = MIN(1000, *timeout);
...

would be nicer.

>          *timeout = MIN(1000, *timeout);
>      }
> +    if (*timeout <= TIMEOUT_FAST) {
> +        return;
> +    }
> +
> +    /* 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;
> +            break;
> +        }
> +        if (slirp->do_slowtimo) {
> +            *timeout = MIN(TIMEOUT_SLOW, *timeout);
> +        }
> +    }

Not sure if the compiler is smart enough, but I would suggest to keep
timeout local until returning and only write it back by then.

Jan

>  }
>  
>  void slirp_pollfds_fill(GArray *pollfds)
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] slirp: fill mainloop timeout with more precise value

[liu ping fan]

On Tue, Aug 13, 2013 at 4:19 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-08-13 05: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.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  main-loop.c   |  2 +-
>>  slirp/slirp.c | 18 ++++++++++++++++++
>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/main-loop.c b/main-loop.c
>> index a44fff6..04120d2 100644
>> --- a/main-loop.c
>> +++ b/main-loop.c
>> @@ -458,8 +458,8 @@ 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_update_timeout(&timeout);
>
> Why this reordering?
>
In order to give timeout more precise value, which is based on the
result of fasttimo or slowtimo after slirp_pollfds_fill()
>>  #endif
>>      qemu_iohandler_fill(gpollfds);
>>      ret = os_host_main_loop_wait(timeout);
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index 1deaad9..af66006 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -262,9 +262,27 @@ void slirp_cleanup(Slirp *slirp)
>>
>>  void slirp_update_timeout(uint32_t *timeout)
>>  {
>> +    Slirp *slirp;
>> +
>>      if (!QTAILQ_EMPTY(&slirp_instances)) {
>
> if (QTAILQ_EMPTY(&slirp_instances) || *timeout <= TIMEOUT_FAST) {
>     return;
> }
>
> *timeout = MIN(1000, *timeout);
> ...
>
> would be nicer.
>
Ok.
>>          *timeout = MIN(1000, *timeout);
>>      }
>> +    if (*timeout <= TIMEOUT_FAST) {
>> +        return;
>> +    }
>> +
>> +    /* 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;
>> +            break;
>> +        }
>> +        if (slirp->do_slowtimo) {
>> +            *timeout = MIN(TIMEOUT_SLOW, *timeout);
>> +        }
>> +    }
>
> Not sure if the compiler is smart enough, but I would suggest to keep
> timeout local until returning and only write it back by then.
>
Ok, got it.

Thx,
Pingfan

Re: [Qemu-devel] [PATCH 3/3] slirp: fill mainloop timeout with more precise value

[Jan Kiszka]

On 2013-08-13 10:25, liu ping fan wrote:
> On Tue, Aug 13, 2013 at 4:19 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2013-08-13 05: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.
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  main-loop.c   |  2 +-
>>>  slirp/slirp.c | 18 ++++++++++++++++++
>>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/main-loop.c b/main-loop.c
>>> index a44fff6..04120d2 100644
>>> --- a/main-loop.c
>>> +++ b/main-loop.c
>>> @@ -458,8 +458,8 @@ 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_update_timeout(&timeout);
>>
>> Why this reordering?
>>
> In order to give timeout more precise value, which is based on the
> result of fasttimo or slowtimo after slirp_pollfds_fill()

OK. But to avoid that the caller has to know this dependency, better
merge the update timeout logic into pollfds_fill.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 3/3] slirp: fill mainloop timeout with more precise value

[liu ping fan]

On Tue, Aug 13, 2013 at 4:29 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-08-13 10:25, liu ping fan wrote:
>> On Tue, Aug 13, 2013 at 4:19 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2013-08-13 05: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.
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>  main-loop.c   |  2 +-
>>>>  slirp/slirp.c | 18 ++++++++++++++++++
>>>>  2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/main-loop.c b/main-loop.c
>>>> index a44fff6..04120d2 100644
>>>> --- a/main-loop.c
>>>> +++ b/main-loop.c
>>>> @@ -458,8 +458,8 @@ 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_update_timeout(&timeout);
>>>
>>> Why this reordering?
>>>
>> In order to give timeout more precise value, which is based on the
>> result of fasttimo or slowtimo after slirp_pollfds_fill()
>
> OK. But to avoid that the caller has to know this dependency, better
> merge the update timeout logic into pollfds_fill.
>
Fine. A good idea to the design.

Thx,
Pingfan