Remove static variables from xc_domain_{save, restore}.c

PATCH In review Ian Campbell 5 participants 15 messages May 24, 2011

Share & tools

Ian Campbell

author

2011-05-24 09:14 UTC #0 lore raw

PATCH 1/1 Remove static variables from xc_domain_{save, restore}.c

Otherwise users which deal with multiple domains need to do their own locking and cannot save/migrate multiple domains in parallel (should they want to).

Also made a bunch of cleanup along the way, mainly to make it easier to figure out what was going on with the twisty maze of macros redefining functions as macros and redefining the macros etc. I'm sure there is plenty more straightening out which could be done but I don't have the stomach for it this morning.

Shriram, does this have any impact on Remus?

Ian Campbell

author

2011-05-24 09:14 UTC #1 lore raw

PATCH 1/1 libxc: save/restore: remove static context variables

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1306228450 -3600
# Node ID c43bf40e2f29eb02c7dcf1a2317f243f1af5f659 # Parent 0727b2fcc33fd5cdf847fde0d2be261be3d6b279
libxc: save/restore: remove static context variables

20544:ad9d75d74bd5 and 20545:cc7d66ba0dad seemingly intended to change these global static variables into stack variables but didn't remove the static qualifier.

Also zero the entire struct once with memset rather than clearing fields piecemeal in two different places.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_restore.c +4 -8

diff -r 0727b2fcc33f -r c43bf40e2f29 tools/libxc/xc_domain_restore.c
@@ -1133,23 +1133,19 @@ int xc_domain_restore(xc_interface *xch,
 
     int orig_io_fd_flags;
 
-    static struct restore_ctx _ctx = {
-        .live_p2m = NULL,
-        .p2m = NULL,
-    };
-    static struct restore_ctx *ctx = &_ctx;
+    struct restore_ctx _ctx;
+    struct restore_ctx *ctx = &_ctx;
     struct domain_info_context *dinfo = &ctx->dinfo;
 
     pagebuf_init(&pagebuf);
     memset(&tailbuf, 0, sizeof(tailbuf));
     tailbuf.ishvm = hvm;
 
-    /* For info only */
-    ctx->nr_pfns = 0;
-
     if ( superpages )
         return 1;
 
+    memset(ctx, 0, sizeof(*ctx));
+
     ctxt = xc_hypercall_buffer_alloc(xch, ctxt, sizeof(*ctxt));
 
     if ( ctxt == NULL )

tools/libxc/xc_domain_save.c +4 -5

diff -r 0727b2fcc33f -r c43bf40e2f29 tools/libxc/xc_domain_save.c
@@ -958,11 +958,8 @@ int xc_domain_save(xc_interface *xch, in
     unsigned long mfn;
 
     struct outbuf ob;
-    static struct save_ctx _ctx = {
-        .live_p2m = NULL,
-        .live_m2p = NULL,
-    };
-    static struct save_ctx *ctx = &_ctx;
+    struct save_ctx _ctx;
+    struct save_ctx *ctx = &_ctx;
     struct domain_info_context *dinfo = &ctx->dinfo;
 
     int completed = 0;

@@ -976,6 +973,8 @@ int xc_domain_save(xc_interface *xch, in
 
     outbuf_init(xch, &ob, OUTBUF_SIZE);
 
+    memset(ctx, 0, sizeof(*ctx));
+
     /* If no explicit control parameters given, use defaults */
     max_iters  = max_iters  ? : DEF_MAX_ITERS;
     max_factor = max_factor ? : DEF_MAX_FACTOR;

Vincent Hanquez

2011-05-24 10:33 UTC #2 lore raw

Re: [PATCH 1 of 8] libxc: save/restore: remove static context variables

Quoted 7 lines · Ian Campbell

# HG changeset patch
# User Ian Campbell<[email protected]> # Date 1306228450 -3600
# Node ID c43bf40e2f29eb02c7dcf1a2317f243f1af5f659 # Parent 0727b2fcc33fd5cdf847fde0d2be261be3d6b279
libxc: save/restore: remove static context variables

Quoted 6 lines

20544:ad9d75d74bd5 and 20545:cc7d66ba0dad seemingly intended to change these global static variables into stack variables but didn't remove the static qualifier.
Also zero the entire struct once with memset rather than clearing fields piecemeal in two different places.

Good catch.
Acked-by-self.

Signature · 1 line

Vincent

Ian Campbell

author

2011-05-24 09:14 UTC #3 lore raw

PATCH 1/1 libxc: save: drop code under ADAPTIVE_SAVE ifdef

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1306228450 -3600
# Node ID 0a7ee235f5139cce2a3c1220086d16b847279dc6 # Parent c43bf40e2f29eb02c7dcf1a2317f243f1af5f659
libxc: save: drop code under ADAPTIVE_SAVE ifdef.

The ifdef was added in 2005 (7702:b3c2bc39d815) but, as far as I can see, was never enabled by default. Dropping it will help untangle some macros redefining functions etc.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +2 ~~-110~~

diff -r c43bf40e2f29 -r 0a7ee235f513 tools/libxc/xc_domain_save.c
@@ -252,98 +252,6 @@ static inline int write_buffer(xc_interf
         return write_exact(fd, buf, len);
 }
 
-#ifdef ADAPTIVE_SAVE
-
-/*
-** We control the rate at which we transmit (or save) to minimize impact
-** on running domains (including the target if we're doing live migrate).
-*/
-
-#define MAX_MBIT_RATE    500      /* maximum transmit rate for migrate */
-#define START_MBIT_RATE  100      /* initial transmit rate for migrate */
-
-/* Scaling factor to convert between a rate (in Mb/s) and time (in usecs) */
-#define RATE_TO_BTU      781250
-
-/* Amount in bytes we allow ourselves to send in a burst */
-#define BURST_BUDGET (100*1024)
-
-/* We keep track of the current and previous transmission rate */
-static int mbit_rate, ombit_rate = 0;
-
-/* Have we reached the maximum transmission rate? */
-#define RATE_IS_MAX() (mbit_rate == MAX_MBIT_RATE)
-
-static inline void initialize_mbit_rate()
-{
-    mbit_rate = START_MBIT_RATE;
-}
-
-static int ratewrite(xc_interface *xch, int io_fd, int live, void *buf, int n)
-{
-    static int budget = 0;
-    static int burst_time_us = -1;
-    static struct timeval last_put = { 0 };
-    struct timeval now;
-    struct timespec delay;
-    long long delta;
-
-    if ( START_MBIT_RATE == 0 )
-        return noncached_write(io_fd, live, buf, n);
-
-    budget -= n;
-    if ( budget < 0 )
-    {
-        if ( mbit_rate != ombit_rate )
-        {
-            burst_time_us = RATE_TO_BTU / mbit_rate;
-            ombit_rate = mbit_rate;
-            DPRINTF("rate limit: %d mbit/s burst budget %d slot time %d\n",
-                    mbit_rate, BURST_BUDGET, burst_time_us);
-        }
-        if ( last_put.tv_sec == 0 )
-        {
-            budget += BURST_BUDGET;
-            gettimeofday(&last_put, NULL);
-        }
-        else
-        {
-            while ( budget < 0 )
-            {
-                gettimeofday(&now, NULL);
-                delta = tv_delta(&now, &last_put);
-                while ( delta > burst_time_us )
-                {
-                    budget += BURST_BUDGET;
-                    last_put.tv_usec += burst_time_us;
-                    if ( last_put.tv_usec > 1000000 )
-                    {
-                        last_put.tv_usec -= 1000000;
-                        last_put.tv_sec++;
-                    }
-                    delta -= burst_time_us;
-                }
-                if ( budget > 0 )
-                    break;
-                delay.tv_sec = 0;
-                delay.tv_nsec = 1000 * (burst_time_us - delta);
-                while ( delay.tv_nsec > 0 )
-                    if ( nanosleep(&delay, &delay) == 0 )
-                        break;
-            }
-        }
-    }
-    return noncached_write(io_fd, live, buf, n);
-}
-
-#else /* ! ADAPTIVE SAVE */
-
-#define RATE_IS_MAX() (0)
-#define ratewrite(xch, _io_fd, _live, _buf, _n) noncached_write((xch), (_io_fd), (_live), (_buf), (_n))
-#define initialize_mbit_rate()
-
-#endif
-
 /* like write_buffer for ratewrite, which returns number of bytes written */
 static inline int ratewrite_buffer(xc_interface *xch,
                                    int dobuf, struct outbuf* ob, int fd,

@@ -352,7 +260,7 @@ static inline int ratewrite_buffer(xc_in
     if ( dobuf )
         return outbuf_hardwrite(xch, ob, fd, buf, len) ? -1 : len;
     else
-        return ratewrite(xch, fd, live, buf, len);
+        return noncached_write(xch, fd, live, buf, len);
 }
 
 static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,

@@ -392,16 +300,6 @@ static int print_stats(xc_interface *xch
                 (int)((stats->dirty_count*PAGE_SIZE)/(wall_delta*(1000/8))),
                 stats->dirty_count);
 
-#ifdef ADAPTIVE_SAVE
-    if ( ((stats->dirty_count*PAGE_SIZE)/(wall_delta*(1000/8))) > mbit_rate )
-    {
-        mbit_rate = (int)((stats->dirty_count*PAGE_SIZE)/(wall_delta*(1000/8)))
-            + 50;
-        if ( mbit_rate > MAX_MBIT_RATE )
-            mbit_rate = MAX_MBIT_RATE;
-    }
-#endif
-
     d0_cpu_last = d0_cpu_now;
     d1_cpu_last = d1_cpu_now;
     wall_last   = wall_now;

@@ -979,8 +877,6 @@ int xc_domain_save(xc_interface *xch, in
     max_iters  = max_iters  ? : DEF_MAX_ITERS;
     max_factor = max_factor ? : DEF_MAX_FACTOR;
 
-    initialize_mbit_rate();
-
     if ( !get_platform_info(xch, dom,
                             &ctx->max_mfn, &ctx->hvirt_start, &ctx->pt_levels, &dinfo->guest_width) )
     {

@@ -1170,9 +1066,6 @@ int xc_domain_save(xc_interface *xch, in
 
   copypages:
 #define wrexact(fd, buf, len) write_buffer(xch, last_iter, &ob, (fd), (buf), (len))
-#ifdef ratewrite
-#undef ratewrite
-#endif
 #define ratewrite(fd, live, buf, len) ratewrite_buffer(xch, last_iter, &ob, (fd), (live), (buf), (len))
 
     /* Now write out each data page, canonicalising page tables as we go... */

@@ -1509,8 +1402,7 @@ int xc_domain_save(xc_interface *xch, in
 
         if ( live )
         {
-            if ( ((sent_this_iter > sent_last_iter) && RATE_IS_MAX()) ||
-                 (iter >= max_iters) ||
+            if ( (iter >= max_iters) ||
                  (sent_this_iter+skip_this_iter < 50) ||
                  (total_sent > dinfo->p2m_size*max_factor) )
             {

Ian Campbell

author

2011-05-24 09:14 UTC #4 lore raw

PATCH 1/1 libxc: save: rename ratewrite to uncached

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1306228450 -3600
# Node ID 0d97d90876b0e59648bd403794812941bcb8d13e # Parent 0a7ee235f5139cce2a3c1220086d16b847279dc6
libxc: save: rename ratewrite to uncached.

It doesn't do any ratelimiting...

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +3 -3

diff -r 0a7ee235f513 -r 0d97d90876b0 tools/libxc/xc_domain_save.c
@@ -252,8 +252,8 @@ static inline int write_buffer(xc_interf
         return write_exact(fd, buf, len);
 }
 
-/* like write_buffer for ratewrite, which returns number of bytes written */
-static inline int ratewrite_buffer(xc_interface *xch,
+/* like write_buffer for noncached, which returns number of bytes written */
+static inline int write_uncached(xc_interface *xch,
                                    int dobuf, struct outbuf* ob, int fd,
                                    int live, void* buf, size_t len)
 {

@@ -1066,7 +1066,7 @@ int xc_domain_save(xc_interface *xch, in
 
   copypages:
 #define wrexact(fd, buf, len) write_buffer(xch, last_iter, &ob, (fd), (buf), (len))
-#define ratewrite(fd, live, buf, len) ratewrite_buffer(xch, last_iter, &ob, (fd), (live), (buf), (len))
+#define ratewrite(fd, live, buf, len) write_uncached(xch, last_iter, &ob, (fd), (live), (buf), (len))
 
     /* Now write out each data page, canonicalising page tables as we go... */
     for ( ; ; )

Ian Campbell

author

2011-05-24 09:14 UTC #5 lore raw

PATCH 1/1 libxc: save: noncached write doesn't use live parameter

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1306228450 -3600
# Node ID d6eb4fc290ba248e6830c5df11c8cb511bfd6db9 # Parent 0d97d90876b0e59648bd403794812941bcb8d13e
libxc: save: noncached write doesn't use live parameter.

so drop it.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +7 -7

diff -r 0d97d90876b0 -r d6eb4fc290ba tools/libxc/xc_domain_save.c
@@ -152,7 +152,7 @@ static uint64_t tv_delta(struct timeval 
 }
 
 static int noncached_write(xc_interface *xch,
-                           int fd, int live, void *buffer, int len) 
+                           int fd, void *buffer, int len) 
 {
     static int write_count = 0;
     int rc = (write_exact(fd, buffer, len) == 0) ? len : -1;

@@ -255,12 +255,12 @@ static inline int write_buffer(xc_interf
 /* like write_buffer for noncached, which returns number of bytes written */
 static inline int write_uncached(xc_interface *xch,
                                    int dobuf, struct outbuf* ob, int fd,
-                                   int live, void* buf, size_t len)
+                                   void* buf, size_t len)
 {
     if ( dobuf )
         return outbuf_hardwrite(xch, ob, fd, buf, len) ? -1 : len;
     else
-        return noncached_write(xch, fd, live, buf, len);
+        return noncached_write(xch, fd, buf, len);
 }
 
 static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,

@@ -1066,7 +1066,7 @@ int xc_domain_save(xc_interface *xch, in
 
   copypages:
 #define wrexact(fd, buf, len) write_buffer(xch, last_iter, &ob, (fd), (buf), (len))
-#define ratewrite(fd, live, buf, len) write_uncached(xch, last_iter, &ob, (fd), (live), (buf), (len))
+#define wruncached(fd, live, buf, len) write_uncached(xch, last_iter, &ob, (fd), (buf), (len))
 
     /* Now write out each data page, canonicalising page tables as we go... */
     for ( ; ; )

@@ -1300,7 +1300,7 @@ int xc_domain_save(xc_interface *xch, in
                        run of pages we may have previously acumulated */
                     if ( run )
                     {
-                        if ( ratewrite(io_fd, live, 
+                        if ( wruncached(io_fd, live,
                                        (char*)region_base+(PAGE_SIZE*(j-run)), 
                                        PAGE_SIZE*run) != PAGE_SIZE*run )
                         {

@@ -1332,7 +1332,7 @@ int xc_domain_save(xc_interface *xch, in
                         goto out;
                     }
 
-                    if ( ratewrite(io_fd, live, page, PAGE_SIZE) != PAGE_SIZE )
+                    if ( wruncached(io_fd, live, page, PAGE_SIZE) != PAGE_SIZE )
                     {
                         PERROR("Error when writing to state file (4b)"
                               " (errno %d)", errno);

@@ -1349,7 +1349,7 @@ int xc_domain_save(xc_interface *xch, in
             if ( run )
             {
                 /* write out the last accumulated run of pages */
-                if ( ratewrite(io_fd, live, 
+                if ( wruncached(io_fd, live,
                                (char*)region_base+(PAGE_SIZE*(j-run)), 
                                PAGE_SIZE*run) != PAGE_SIZE*run )
                 {

Ian Campbell

author

2011-05-24 09:14 UTC #6 lore raw

PATCH 1/1 libxc: save: move static "write_count" variable into outbuf

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1306228450 -3600
# Node ID da70ac9b134d29b09a887ee0ad9a5dc4a5531b3b # Parent d6eb4fc290ba248e6830c5df11c8cb511bfd6db9
libxc: save: move static "write_count" variable into outbuf.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +6 -5

diff -r d6eb4fc290ba -r da70ac9b134d tools/libxc/xc_domain_save.c
@@ -59,6 +59,7 @@ struct outbuf {
     void* buf;
     size_t size;
     size_t pos;
+    int write_count;
 };
 
 #define OUTBUF_SIZE (16384 * 1024)

@@ -152,19 +153,19 @@ static uint64_t tv_delta(struct timeval 
 }
 
 static int noncached_write(xc_interface *xch,
+                           struct outbuf* ob,
                            int fd, void *buffer, int len) 
 {
-    static int write_count = 0;
     int rc = (write_exact(fd, buffer, len) == 0) ? len : -1;
 
-    write_count += len;
-    if ( write_count >= (MAX_PAGECACHE_USAGE * PAGE_SIZE) )
+    ob->write_count += len;
+    if ( ob->write_count >= (MAX_PAGECACHE_USAGE * PAGE_SIZE) )
     {
         /* Time to discard cache - dont care if this fails */
         int saved_errno = errno;
         discard_file_cache(xch, fd, 0 /* no flush */);
         errno = saved_errno;
-        write_count = 0;
+        ob->write_count = 0;
     }
 
     return rc;

@@ -260,7 +261,7 @@ static inline int write_uncached(xc_inte
     if ( dobuf )
         return outbuf_hardwrite(xch, ob, fd, buf, len) ? -1 : len;
     else
-        return noncached_write(xch, fd, buf, len);
+        return noncached_write(xch, ob, fd, buf, len);
 }
 
 static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,

Ian Campbell

author

2011-05-24 09:14 UTC #7 lore raw

PATCH 1/1 libxc: save: encapsulate time stats in a struct

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1306228450 -3600
# Node ID c6265f8b07948c5e5042b18f612e17f187921229 # Parent da70ac9b134d29b09a887ee0ad9a5dc4a5531b3b
libxc: save: encapsulate time stats in a struct

As a precursor to making non-static.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +18 ~~-17~~

diff -r da70ac9b134d -r c6265f8b0794 tools/libxc/xc_domain_save.c
@@ -264,32 +264,35 @@ static inline int write_uncached(xc_inte
         return noncached_write(xch, ob, fd, buf, len);
 }
 
+struct time_stats {
+    struct timeval wall;
+    long long d0_cpu, d1_cpu;
+};
+
 static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,
                        xc_shadow_op_stats_t *stats, int print)
 {
-    static struct timeval wall_last;
-    static long long      d0_cpu_last;
-    static long long      d1_cpu_last;
+    static struct time_stats last;
+    struct time_stats now;
 
-    struct timeval        wall_now;
-    long long             wall_delta;
-    long long             d0_cpu_now, d0_cpu_delta;
-    long long             d1_cpu_now, d1_cpu_delta;
+    long long wall_delta;
+    long long d0_cpu_delta;
+    long long d1_cpu_delta;
 
-    gettimeofday(&wall_now, NULL);
+    gettimeofday(&now.wall, NULL);
 
-    d0_cpu_now = xc_domain_get_cpu_usage(xch, 0, /* FIXME */ 0)/1000;
-    d1_cpu_now = xc_domain_get_cpu_usage(xch, domid, /* FIXME */ 0)/1000;
+    now.d0_cpu = xc_domain_get_cpu_usage(xch, 0, /* FIXME */ 0)/1000;
+    now.d1_cpu = xc_domain_get_cpu_usage(xch, domid, /* FIXME */ 0)/1000;
 
-    if ( (d0_cpu_now == -1) || (d1_cpu_now == -1) )
+    if ( (now.d0_cpu == -1) || (now.d1_cpu == -1) )
         DPRINTF("ARRHHH!!\n");
 
-    wall_delta = tv_delta(&wall_now,&wall_last)/1000;
+    wall_delta = tv_delta(&now.wall,&last.wall)/1000;
     if ( wall_delta == 0 )
         wall_delta = 1;
 
-    d0_cpu_delta = (d0_cpu_now - d0_cpu_last)/1000;
-    d1_cpu_delta = (d1_cpu_now - d1_cpu_last)/1000;
+    d0_cpu_delta = (now.d0_cpu - last.d0_cpu)/1000;
+    d1_cpu_delta = (now.d1_cpu - last.d1_cpu)/1000;
 
     if ( print )
         DPRINTF("delta %lldms, dom0 %d%%, target %d%%, sent %dMb/s, "

@@ -301,9 +304,7 @@ static int print_stats(xc_interface *xch
                 (int)((stats->dirty_count*PAGE_SIZE)/(wall_delta*(1000/8))),
                 stats->dirty_count);
 
-    d0_cpu_last = d0_cpu_now;
-    d1_cpu_last = d1_cpu_now;
-    wall_last   = wall_now;
+    last = now;
 
     return 0;
 }

Ian Campbell

author

2011-05-24 09:14 UTC #8 lore raw

PATCH 1/1 libxc: save: don't bother calculating stat's deltas unless we are going to print them

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1306228450 -3600
# Node ID 5463bdc1d77942b50b28eea059eaba4d1ec7d2ac
# Parent c6265f8b07948c5e5042b18f612e17f187921229
libxc: save: don't bother calculating stat's deltas unless we are going to print them

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +12 ~~-10~~

diff -r c6265f8b0794 -r 5463bdc1d779 tools/libxc/xc_domain_save.c
@@ -275,10 +275,6 @@ static int print_stats(xc_interface *xch
     static struct time_stats last;
     struct time_stats now;
 
-    long long wall_delta;
-    long long d0_cpu_delta;
-    long long d1_cpu_delta;
-
     gettimeofday(&now.wall, NULL);
 
     now.d0_cpu = xc_domain_get_cpu_usage(xch, 0, /* FIXME */ 0)/1000;

@@ -287,14 +283,19 @@ static int print_stats(xc_interface *xch
     if ( (now.d0_cpu == -1) || (now.d1_cpu == -1) )
         DPRINTF("ARRHHH!!\n");
 
-    wall_delta = tv_delta(&now.wall,&last.wall)/1000;
-    if ( wall_delta == 0 )
-        wall_delta = 1;
+    if ( print )
+    {
+        long long wall_delta;
+        long long d0_cpu_delta;
+        long long d1_cpu_delta;
 
-    d0_cpu_delta = (now.d0_cpu - last.d0_cpu)/1000;
-    d1_cpu_delta = (now.d1_cpu - last.d1_cpu)/1000;
+        wall_delta = tv_delta(&now.wall,&last.wall)/1000;
+        if ( wall_delta == 0 )
+            wall_delta = 1;
 
-    if ( print )
+        d0_cpu_delta = (now.d0_cpu - last.d0_cpu)/1000;
+        d1_cpu_delta = (now.d1_cpu - last.d1_cpu)/1000;
+
         DPRINTF("delta %lldms, dom0 %d%%, target %d%%, sent %dMb/s, "
                 "dirtied %dMb/s %" PRId32 " pages\n",
                 wall_delta,

@@ -303,6 +304,7 @@ static int print_stats(xc_interface *xch
                 (int)((pages_sent*PAGE_SIZE)/(wall_delta*(1000/8))),
                 (int)((stats->dirty_count*PAGE_SIZE)/(wall_delta*(1000/8))),
                 stats->dirty_count);
+    }
 
     last = now;

Ian Campbell

author

2011-05-24 09:14 UTC #9 lore raw

PATCH 1/1 libxc: save: move static stats variable to stack variable

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1306228450 -3600
# Node ID 32d62506e3be95124097775dc79c42304a18084c # Parent 5463bdc1d77942b50b28eea059eaba4d1ec7d2ac
libxc: save: move static stats variable to stack variable.

Signed-off-by Ian Campbell <[email protected]>

tools/libxc/xc_domain_save.c +14 ~~-13~~

diff -r 5463bdc1d779 -r 32d62506e3be tools/libxc/xc_domain_save.c
@@ -270,9 +270,9 @@ struct time_stats {
 };
 
 static int print_stats(xc_interface *xch, uint32_t domid, int pages_sent,
+                       struct time_stats *last,
                        xc_shadow_op_stats_t *stats, int print)
 {
-    static struct time_stats last;
     struct time_stats now;
 
     gettimeofday(&now.wall, NULL);

@@ -289,12 +289,12 @@ static int print_stats(xc_interface *xch
         long long d0_cpu_delta;
         long long d1_cpu_delta;
 
-        wall_delta = tv_delta(&now.wall,&last.wall)/1000;
+        wall_delta = tv_delta(&now.wall,&last->wall)/1000;
         if ( wall_delta == 0 )
             wall_delta = 1;
 
-        d0_cpu_delta = (now.d0_cpu - last.d0_cpu)/1000;
-        d1_cpu_delta = (now.d1_cpu - last.d1_cpu)/1000;
+        d0_cpu_delta = (now.d0_cpu - last->d0_cpu)/1000;
+        d1_cpu_delta = (now.d1_cpu - last->d1_cpu)/1000;
 
         DPRINTF("delta %lldms, dom0 %d%%, target %d%%, sent %dMb/s, "
                 "dirtied %dMb/s %" PRId32 " pages\n",

@@ -306,7 +306,7 @@ static int print_stats(xc_interface *xch
                 stats->dirty_count);
     }
 
-    last = now;
+    *last = now;
 
     return 0;
 }

@@ -843,7 +843,8 @@ int xc_domain_save(xc_interface *xch, in
     DECLARE_HYPERCALL_BUFFER(unsigned long, to_send);
     unsigned long *to_fix = NULL;
 
-    xc_shadow_op_stats_t stats;
+    struct time_stats time_stats;
+    xc_shadow_op_stats_t shadow_stats;
 
     unsigned long needed_to_fix = 0;
     unsigned long total_sent    = 0;

@@ -1053,7 +1054,7 @@ int xc_domain_save(xc_interface *xch, in
         DPRINTF("Had %d unexplained entries in p2m table\n", err);
     }
 
-    print_stats(xch, dom, 0, &stats, 0);
+    print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
 
     tmem_saved = xc_tmem_save(xch, dom, io_fd, live, XC_SAVE_ID_TMEM);
     if ( tmem_saved == -1 )

@@ -1377,7 +1378,7 @@ int xc_domain_save(xc_interface *xch, in
 
         if ( last_iter )
         {
-            print_stats( xch, dom, sent_this_iter, &stats, 1);
+            print_stats( xch, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
 
             DPRINTF("Total pages sent= %ld (%.2fx)\n",
                     total_sent, ((float)total_sent)/dinfo->p2m_size );

@@ -1439,7 +1440,7 @@ int xc_domain_save(xc_interface *xch, in
 
             if ( xc_shadow_control(xch, dom,
                                    XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(to_send),
-                                   dinfo->p2m_size, NULL, 0, &stats) != dinfo->p2m_size )
+                                   dinfo->p2m_size, NULL, 0, &shadow_stats) != dinfo->p2m_size )
             {
                 PERROR("Error flushing shadow PT");
                 goto out;

@@ -1447,7 +1448,7 @@ int xc_domain_save(xc_interface *xch, in
 
             sent_last_iter = sent_this_iter;
 
-            print_stats(xch, dom, sent_this_iter, &stats, 1);
+            print_stats(xch, dom, sent_this_iter, &time_stats, &shadow_stats, 1);
 
         }
     } /* end of infinite for loop */

@@ -1810,7 +1811,7 @@ int xc_domain_save(xc_interface *xch, in
         callbacks->checkpoint(callbacks->data) > 0)
     {
         /* reset stats timer */
-        print_stats(xch, dom, 0, &stats, 0);
+        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 0);
 
         rc = 1;
         /* last_iter = 1; */

@@ -1821,11 +1822,11 @@ int xc_domain_save(xc_interface *xch, in
             goto out;
         }
         DPRINTF("SUSPEND shinfo %08lx\n", info.shared_info_frame);
-        print_stats(xch, dom, 0, &stats, 1);
+        print_stats(xch, dom, 0, &time_stats, &shadow_stats, 1);
 
         if ( xc_shadow_control(xch, dom,
                                XEN_DOMCTL_SHADOW_OP_CLEAN, HYPERCALL_BUFFER(to_send),
-                               dinfo->p2m_size, NULL, 0, &stats) != dinfo->p2m_size )
+                               dinfo->p2m_size, NULL, 0, &shadow_stats) != dinfo->p2m_size )
         {
             PERROR("Error flushing shadow PT");
         }

Ian Jackson

Acked-by on v1 1/1

2011-05-24 16:45 UTC #10 lore raw

Re: [PATCH 8 of 8] libxc: save: move static stats variable to stack variable

Ian Campbell writes ("[Xen-devel] [PATCH 8 of 8] libxc: save: move static stats variable to stack variable"):

Quoted 1 line

libxc: save: move static stats variable to stack variable.

Cor what a pile of poo you discovered. Thanks for cleaning it up!

Acked-by Ian Jackson <[email protected]>

Committed-by Ian Jackson <[email protected]>

Shriram Rajagopalan

2011-05-24 13:47 UTC #11 lore raw

In reply to Ian Campbell

Quoted 11 lines · Ian Campbell

Otherwise users which deal with multiple domains need to do their own locking and cannot save/migrate multiple domains in parallel (should they want to).
Also made a bunch of cleanup along the way, mainly to make it easier to figure out what was going on with the twisty maze of macros redefining functions as macros and redefining the macros etc. I'm sure there is plenty more straightening out which could be done but I don't have the stomach for it this morning.
I ll do it!!.. I have been waiting for this. Thanks a lot for cleaning up

this chaff!
I was under the impression that this was some arcane legacy code that shouldnt
be touched. One particular thing that I would like to do is to factor out the write functions
(outbuf_*, noncached_write, ratewrite*, etc) into a separate file and make it sort of pluggable.

(selfish :P) I wanted to introduce a patch that would overlap outbuf flush operation
and guest memory copy operation instead of the current model <flush,copy,flush,copy..>.
This might be helpful for both Remus and live migration of large domains.

Shriram, does this have any impact on Remus?

Quoted 2 lines

From a cursory look, it doesnt look like it would impact Remus. I ll test

it
in my setup and revert soon.

shriram

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Ian Jackson

2011-05-24 17:02 UTC #12 lore raw

Shriram Rajagopalan writes ("[Xen-devel] Re: [PATCH 0 of 8] Remove static variables from xc_domain_{save, restore}.c"):

Quoted 3 lines

I ll do it!!.. I have been waiting for this. Thanks a lot for cleaning up this chaff! I was under the impression that this was some arcane legacy code that shouldnt be touched.

No, it's arcane legacy code that we have been gradually cleaning up :-).

Quoted 4 lines

One particular
thing that I would like to do is to factor out the write functions (outbuf_*, noncached_write, ratewrite*, etc) into a separate file and make it sort of pluggable.

Do you have a particular use case fot that ? Without a different set of implementations I'm not sure that we need it to be pluggable.

Quoted 4 lines

(selfish :P) I wanted to introduce a patch that would overlap outbuf flush operation and guest memory copy operation instead of the current model <flush,copy,flush,copy..>. This might be helpful for both Remus and live migration of large domains.

But yes, if that produces a speedup, certainly.

Quoted 1 line

Shriram, does this have any impact on Remus?

I think it should be OK but we should hear what Shriram has to say (CC'd).

Ian.

Ian Campbell

2011-05-24 17:52 UTC #13 lore raw

Quoted 21 lines · Ian Jackson

Shriram Rajagopalan writes ("[Xen-devel] Re: [PATCH 0 of 8] Remove static variables from xc_domain_{save, restore}.c"):
Quoted 3 lines
I ll do it!!.. I have been waiting for this. Thanks a lot for cleaning up this chaff! I was under the impression that this was some arcane legacy code that shouldnt be touched.
No, it's arcane legacy code that we have been gradually cleaning up :-).
Quoted 4 lines
One particular
thing that I would like to do is to factor out the write functions (outbuf_*, noncached_write, ratewrite*, etc) into a separate file and make it sort of pluggable.
Do you have a particular use case fot that ? Without a different set of implementations I'm not sure that we need it to be pluggable.
Quoted 4 lines
(selfish :P) I wanted to introduce a patch that would overlap outbuf flush operation and guest memory copy operation instead of the current model <flush,copy,flush,copy..>. This might be helpful for both Remus and live migration of large domains.

Quoted 6 lines

But yes, if that produces a speedup, certainly.
Quoted 1 line
Shriram, does this have any impact on Remus?
I think it should be OK but we should hear what Shriram has to say (CC'd).

You were replying to Shriram ;-)

Ian.

Shriram Rajagopalan

Acked-by

2011-05-24 18:57 UTC #14 lore raw

Quoted 25 lines · Ian Campbell

Quoted 23 lines · Ian Jackson
Shriram Rajagopalan writes ("[Xen-devel] Re: [PATCH 0 of 8] Remove static variables from xc_domain_{save, restore}.c"):
Quoted 3 lines
I ll do it!!.. I have been waiting for this. Thanks a lot for cleaning up this chaff! I was under the impression that this was some arcane legacy code that shouldnt be touched.
No, it's arcane legacy code that we have been gradually cleaning up :-).
Quoted 4 lines
One particular
thing that I would like to do is to factor out the write functions (outbuf_*, noncached_write, ratewrite*, etc) into a separate file and make it sort of pluggable.
Do you have a particular use case fot that ? Without a different set of implementations I'm not sure that we need it to be pluggable.
Quoted 4 lines
(selfish :P) I wanted to introduce a patch that would overlap outbuf flush operation and guest memory copy operation instead of the current model <flush,copy,flush,copy..>. This might be helpful for both Remus and live migration of large domains.
But yes, if that produces a speedup, certainly.

Quoted 11 lines

Quoted 4 lines
Quoted 1 line
Shriram, does this have any impact on Remus?
I think it should be OK but we should hear what Shriram has to say (CC'd).
You were replying to Shriram ;-)
Ian.

Ok. I ve tested it with remus. works properly :).

Acked-by Shriram Rajagopalan <[email protected]>

thanks
shriram