[Qemu-devel] [PATCH v2 0/3]

[Qemu-devel] [PATCH v2 0/3]

[Liu Ping Fan]

These patches are separated from "[PATCH v1 0/5] make slirp subsystem self-contained", as Paolo suggested.

With them, we can set the mainloop timeout more precisely when slirp has 
to emulate tcp timeout problem.

v2:
  fold slirp_update_timeout logic into slirp_pollfds_fill.

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

 main-loop.c      |  3 +--
 slirp/libslirp.h |  3 +--
 slirp/slirp.c    | 55 +++++++++++++++++++++++++++++++++++++++----------------
 slirp/slirp.h    |  3 +++
 stubs/slirp.c    |  6 +-----
 5 files changed, 45 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 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..b71c617 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 = true; /* 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 = true; /* 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..e4a1bd4 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;
+    bool do_slowtimo;
 
     /* virtual network configuration */
     struct in_addr vnetwork_addr;
-- 
1.8.1.4

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

[Liu Ping Fan]

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

diff --git a/slirp/slirp.c b/slirp/slirp.c
index b71c617..c224523 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,13 @@ 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

[Qemu-devel] [PATCH v2 3/3] slirp: set 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. 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 c224523..da759a7 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -260,18 +260,37 @@ 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;
+
+    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;
 
+    *timeout = MIN(1000, *timeout);
     if (QTAILQ_EMPTY(&slirp_instances)) {
         return;
     }
@@ -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)
 {
 }
 
-- 
1.8.1.4

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

[Jan Kiszka]

On 2013-08-14 05:02, 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 c224523..da759a7 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -260,18 +260,37 @@ 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;
> +
> +    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;
>  
> +    *timeout = MIN(1000, *timeout);

This should come after the QTAILQ_EMPTY check: no slirps, no timeout
modifications.

>      if (QTAILQ_EMPTY(&slirp_instances)) {
>          return;
>      }
> @@ -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)
>  {
>  }
>  
> 

The rest looks good!

Jan

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

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

[Stefan Hajnoczi]

On Wed, Aug 14, 2013 at 11:02:51AM +0800, Liu Ping Fan wrote:
> @@ -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

#define TIMEOUT_FAST 2 /* milliseconds */

Actually I'm not sure what the units are but please document them,
either as a comment or call the macro itself TIMEOUT_FAST_MS.

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

[liu ping fan]

On Wed, Aug 14, 2013 at 4:35 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2013-08-14 05:02, 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 c224523..da759a7 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -260,18 +260,37 @@ 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;
>> +
>> +    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;
>>
>> +    *timeout = MIN(1000, *timeout);
>
> This should come after the QTAILQ_EMPTY check: no slirps, no timeout
> modifications.
>
Will fix.

Thx,
Pingfan

>>      if (QTAILQ_EMPTY(&slirp_instances)) {
>>          return;
>>      }
>> @@ -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)
>>  {
>>  }
>>
>>
>
> The rest looks good!
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux

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

[liu ping fan]

On Wed, Aug 14, 2013 at 7:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Aug 14, 2013 at 11:02:51AM +0800, Liu Ping Fan wrote:
>> @@ -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
>
> #define TIMEOUT_FAST 2 /* milliseconds */
>
> Actually I'm not sure what the units are but please document them,
> either as a comment or call the macro itself TIMEOUT_FAST_MS.

Yes, its unit is milliseconds for g_poll(). Will document them.

Thx,
Pingfan