Fix migration with lots of memory

From: Juan Quintela <[email protected]>

This time is each time that buffered_file ticks happen

Signed-off-by Juan Quintela <[email protected]>

Signed-off-by Juan Quintela <[email protected]>

---
 buffered_file.c |    6 ++++--
 buffered_file.h |    2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

buffered_file.c +4 -2

index 1836e7e..1f492e6 100644
@@ -20,6 +20,8 @@
 
 //#define DEBUG_BUFFERED_FILE
 
+const int buffered_file_interval = 100;
+
 typedef struct QEMUFileBuffered
 {
     BufferedPutFunc *put_buffer;

@@ -235,7 +237,7 @@ static void buffered_rate_tick(void *opaque)
         return;
     }
 
-    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 100);
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + buffered_file_interval);
 
     if (s->freeze_output)
         return;

@@ -273,7 +275,7 @@ QEMUFile *qemu_fopen_ops_buffered(void *opaque,
 
     s->timer = qemu_new_timer(rt_clock, buffered_rate_tick, s);
 
-    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 100);
+    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + buffered_file_interval);
 
     return s->file;
 }

buffered_file.h +0 -0

index 98d358b..a728316 100644

buffered_file.h +0 -0

 QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
                                   BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,

Signature · 1 line

1.7.3.2

Quoted 10 lines · Juan Quintela

From: Juan Quintela <[email protected]>

buffered_file.h +0 -0

index 98d358b..a728316 100644
@@ -21,6 +21,8 @@ typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);
 

buffered_file.h typedef void (BufferedPutReadyFunc)(void *opaque); · line 25

Quoted 2 lines

buffered_file.h +2 -0

+extern const int buffered_file_interval;
+

This shouldn't be exported, IMO.

Quoted 7 lines

buffered_file.h +0 -0

 QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
                                   BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,

Signature · 1 line

1.7.3.2

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Juan Quintela

From: Juan Quintela <[email protected]>

buffered_file.h +0 -0

index 98d358b..a728316 100644
@@ -21,6 +21,8 @@ typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);
 

Quoted 4 lines

buffered_file.h typedef void (BufferedPutReadyFunc)(void *opaque); · line 25

Quoted 2 lines

buffered_file.h +2 -0

+extern const int buffered_file_interval;
+

This shouldn't be exported, IMO.

What do you want? an accessor function?

Notice that it is a constant.
We need the value in other places, see the last patch.

Otherwise, I have to pick random numbers like ... 50ms.

Later, Juan.

Quoted 8 lines

Quoted 8 lines

buffered_file.h +0 -0

 QEMUFile *qemu_fopen_ops_buffered(void *opaque, size_t xfer_limit,
                                   BufferedPutFunc *put_buffer,
                                   BufferedPutReadyFunc *put_ready,

Signature · 1 line

1.7.3.2

Quoted 12 lines · Juan Quintela

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Juan Quintela

From: Juan Quintela <[email protected]>

buffered_file.h +0 -0

index 98d358b..a728316 100644
@@ -21,6 +21,8 @@ typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);
 

Quoted 6 lines

Quoted 4 lines

buffered_file.h typedef void (BufferedPutReadyFunc)(void *opaque); · line 25

Quoted 2 lines

buffered_file.h +2 -0

+extern const int buffered_file_interval;
+

This shouldn't be exported, IMO.

What do you want? an accessor function?

I mean an abstraction at qemu_file_ level.

Quoted 6 lines

Notice that it is a constant.
We need the value in other places, see the last patch.

Otherwise, I have to pick random numbers like ... 50ms.

Later, Juan.

If you need a random number, use a random number :)

Quoted 25 lines · Michael S. Tsirkin

Quoted 12 lines · Juan Quintela

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Juan Quintela

From: Juan Quintela <[email protected]>

buffered_file.h +0 -0

index 98d358b..a728316 100644
@@ -21,6 +21,8 @@ typedef void (BufferedPutReadyFunc)(void *opaque);
 typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
 typedef int (BufferedCloseFunc)(void *opaque);
 

Quoted 6 lines

Quoted 4 lines

buffered_file.h typedef void (BufferedPutReadyFunc)(void *opaque); · line 25

Quoted 2 lines

buffered_file.h +2 -0

+extern const int buffered_file_interval;
+

This shouldn't be exported, IMO.

What do you want? an accessor function?

I mean an abstraction at qemu_file_ level.

Quoted 3 lines

Notice that it is a constant.
We need the value in other places, see the last patch.

Quoted 5 lines

Quoted 3 lines

Otherwise, I have to pick random numbers like ... 50ms.

Later, Juan.

If you need a random number, use a random number :)

It is not random, it is the bigger number that lets the timer to run at each 100ms O:-)

I tried 100ms and 75ms, they got better throughput, but I got still stalls of 1.5s.

I could do some search between 50 and 75ms, but the advantage in bandwidth is not so big to complicate things. (150 vs 125s for total migration time or so, and system is quite irresponsive with the bigger value).

Later, Juan.

Quoted 20 lines · Juan Quintela

Quoted 18 lines · Michael S. Tsirkin

Quoted 8 lines · Juan Quintela

From: Juan Quintela<[email protected]>

buffered_file.h +0 -0

index 98d358b..a728316 100644

buffered_file.h typedef void (BufferedPutReadyFunc)(void *opaque); · line 26

Quoted 6 lines

buffered_file.h +2 -0

@@ -21,6 +21,8 @@ typedef void (BufferedPutReadyFunc)(void *opaque);
  typedef void (BufferedWaitForUnfreezeFunc)(void *opaque);
  typedef int (BufferedCloseFunc)(void *opaque);
 
+extern const int buffered_file_interval;
+
 

This shouldn't be exported, IMO.

Quoted 3 lines

What do you want? an accessor function? Notice that it is a constant. We need the value in other places, see the last patch.

That one's just as wrong as this one. TBH, this whole series is fundamentally the wrong approach because it's all ad hoc heuristics benchmarked against one workload.

There are three fundamental problems: 1) kvm.ko dirty bit tracking doesn't scale

2) we lose flow control information because of the multiple levels of buffering which means we move more data than we should move

3) migration prevents a guest from executing the device model because of qemu_mutex.

Those are the problems to fix.

Sprinkling the code with returns in
semi-random places because it benchmarked well for one particular test case is something we'll deeply regret down the road.

No, I benchmarked against two workloads:
a- idle guest (because it was faster to test)
b- busy guest (each test takes forever, that is the reason that I tested last).

So, I don't agree with that.

Quoted 3 lines

There are three fundamental problems: 1) kvm.ko dirty bit tracking doesn't scale

Fully agree, but this patch don't took that.

Quoted 4 lines

2) we lose flow control information because of the multiple levels of buffering which means we move more data than we should move

Fully agree here, but this is a "massive change" to fix it correctly.

Quoted 3 lines

3) migration prevents a guest from executing the device model because of qemu_mutex.

This is a different problem.

Quoted 2 lines

Those are the problems to fix.

This still don't fix the stalls on the main_loop.

So, you are telling me, there are this list of problems that you need to fix. They are not enough to fix the problem, and their imply massive changes.

In the middle time, everybody in stable and 0.14 is not going to be able to use migration with more than 2GB/4GB guest.

Quoted 4 lines

Sprinkling the code with returns in
semi-random places because it benchmarked well for one particular test case is something we'll deeply regret down the road.

This was mean :(

There are two returns and one heuristic.

- return a) we try to migrate when we know that there is no space, obvious optimazation/bug (deppends on how to look at it).

- return b) we don't need to handle TLB bitmap code for kvm. I fully agree that we need to split the bitmaps in something more sensible, but change is quite invasible, and simple fix works for the while.

- heuristic: if you really think that an io_handler should be able to stall the main loop for almost 4 seconds, sorry, I don't agree.

Again, we can fix this much better, but it needs lots of changes:
* I asked you if there is a "maximum" value that one io_handler can hog the main loop. Answer: dunno/deppends.
* Long term: migration should be its own thread, have I told thread? This is qemu, no thread.
* qemu_mutex: this is the holy grail, if we drop qemu_mutex in ram_save_live(), we left the guest cpu's to continue running. But, and this is a big but, we still stuck the main_loop for 4 seconds, so this solution is at least partial.

And that is it. To improve things, I receive complains left and right that exporting buffered_file_period/timeout is BAD, very BAD. Because that exposes buffered_file.c internal state.

But I am given the suggestion that I should just create a buffered_file_write_nop() that just increases the number of bytes transferred for normal pages. I agree that this "could" also work, but that is indeed worse in the sense that we are exposing yet more internals of buffered_file.c. In the 1st case, we are only exposing the periof of a timer, in the second, we are hijaking how qemu_file_rate_limit() works + how we account for written memory + how it calculates the ammount of staff that it have sent, and with this we "knownly" lie about how much stuff we have write.

How this can be "cleaner" than a timeout of 50ms is beyond me.

On the other hand, this is the typical case where you need a lot of testing for each change that you did. I thought several times that I had found the "guilty" bit for the stalls, and no luck, there was yet another problem.

I also thought at points, ok, I can now drop the previous attempts, and no, that didn't work either. This was the minimal set of patches able to fix the stalls.

This is just very depressing.

Later, Juan.

Quoted 7 lines · Juan Quintela

No, I benchmarked against two workloads:
a- idle guest (because it was faster to test)
b- busy guest (each test takes forever, that is the reason that I tested last).

So, I don't agree with that.

But in both cases, it's a large memory guest where the RSS size is <<< than the allocated memory size. This is simply an unrealistic scenario. Migrating immediately after launch may be common among developers but users typically run useful workloads in their guests and run migration after the guest has been running for quite some time.

So the scenario I'm much more interested in, is trying to migrate a 400GB guest after the guest has been doing useful work and has brought in most of it's RSS. Also with a box that big, you're going to be on 10gbit.

That's going to introduce a whole other set of problems that this series is potentially just going to exacerbate even more.

Quoted 12 lines

Quoted 3 lines

There are three fundamental problems: 1) kvm.ko dirty bit tracking doesn't scale

Fully agree, but this patch don't took that.

Quoted 4 lines

2) we lose flow control information because of the multiple levels of buffering which means we move more data than we should move

Fully agree here, but this is a "massive change" to fix it correctly.

It's really not massive.

Quoted 5 lines

Quoted 3 lines

3) migration prevents a guest from executing the device model because of qemu_mutex.

This is a different problem.

No, this is really the fundamental one.

Quoted 8 lines

Quoted 2 lines

Those are the problems to fix.

This still don't fix the stalls on the main_loop.

So, you are telling me, there are this list of problems that you need to fix. They are not enough to fix the problem, and their imply massive changes.

Quoted 3 lines

In the middle time, everybody in stable and 0.14 is not going to be able to use migration with more than 2GB/4GB guest.

That's simply not true. You started this series with a statement that migration is broken.

It's not, it works perfectly fine. Migration
with 8GB guests work perfectly fine.

You've identified a corner case for which we have suboptimal behavior, and are now declaring that migration is "totally broken".

Quoted 6 lines

Quoted 4 lines

Sprinkling the code with returns in
semi-random places because it benchmarked well for one particular test case is something we'll deeply regret down the road.

This was mean :(

It wasn't intended to be mean but it is the truth. We need to approach these sort of problems in a more systematic way. Systematic means identifying what the fundamental problems are and fixing them in a proper way.

Throwing a magic number in the iteration path after which we let the main loop run for a little bit is simply not a solution. You're still going to get main loop starvation.

Quoted 9 lines

There are two returns and one heuristic.

- return a) we try to migrate when we know that there is no space, obvious optimazation/bug (deppends on how to look at it).

- return b) we don't need to handle TLB bitmap code for kvm. I fully agree that we need to split the bitmaps in something more sensible, but change is quite invasible, and simple fix works for the while.

Quoted 3 lines

- heuristic: if you really think that an io_handler should be able to stall the main loop for almost 4 seconds, sorry, I don't agree.

But fundamentally, why would an iteration of migration take 4 seconds?

This is really my fundamental object, the migration loop should, at most take as much time as it takes to fill up an empty socket buffer or until it hits the bandwidth limit.

The bandwidth limit offers a fine-grain control over exactly how long it should take.

If we're burning excess CPU walking a 100MB bitmap, then let's fix that problem. Stopping every 1MB worth of the bitmap to do other work just papers over the real problem (that we're walking 100MB bitmap).

Basically our bitmap handling code is "exponential" on memory size,

Quoted 1 line · Juan Quintela

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Juan Quintela

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Well, 1st of all, it is "exponential" as you measure it.

stalls by default are:

1-2GB: milliseconds
2-4GB: 100-200ms
4-8GB: 1s
64GB: 59s
400GB: 24m (yes, minutes)

That sounds really exponential.

Now the other thing is the cache size.

with 64GB of RAM, we basically have a 16MB bitmap size, i.e. we blow the cache each time that we run ram_save_live().

This is one of the reasons why I don't want to walk the bitmap on ram_save_remaining().

ram_save_remaining() is linear with memory size (previous my changes). ram_save_live is also linear on the memory size, so we are in a worse case of n^n (notice that this is the worst case, we normally do much better n^2, n^3 or so).

Add to this that we are blowing the caches with big amounts of memory (we don't do witch smaller sizes), and you can see that our behaviour is clearly exponential.

As I need to fix them, I will work on them today/tomorrow.

Later, Juan.

Quoted 18 lines · Juan Quintela

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Juan Quintela

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Well, 1st of all, it is "exponential" as you measure it.

stalls by default are:

1-2GB: milliseconds
2-4GB: 100-200ms
4-8GB: 1s
64GB: 59s
400GB: 24m (yes, minutes)

Quoted 2 lines

That sounds really exponential.

How are you measuring stalls btw?

Regards,

Anthony Liguori

Quoted 21 lines

Now the other thing is the cache size.

with 64GB of RAM, we basically have a 16MB bitmap size, i.e. we blow the cache each time that we run ram_save_live().

This is one of the reasons why I don't want to walk the bitmap on ram_save_remaining().

ram_save_remaining() is linear with memory size (previous my changes). ram_save_live is also linear on the memory size, so we are in a worse case of n^n (notice that this is the worst case, we normally do much better n^2, n^3 or so).

Add to this that we are blowing the caches with big amounts of memory (we don't do witch smaller sizes), and you can see that our behaviour is clearly exponential.

As I need to fix them, I will work on them today/tomorrow.

Later, Juan.

Quoted 24 lines · Anthony Liguori

Quoted 22 lines · Juan Quintela

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Juan Quintela

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Well, 1st of all, it is "exponential" as you measure it.

stalls by default are:

1-2GB: milliseconds
2-4GB: 100-200ms
4-8GB: 1s
64GB: 59s
400GB: 24m (yes, minutes)

Quoted 5 lines

Quoted 3 lines

That sounds really exponential.

How are you measuring stalls btw?

At the end of the ram_save_live(). This was the reason that I put the information there.

for the 24mins stall (I don't have that machine anymore) I had less "exact" measurements. It was the amount that it "decided" to sent in the last non-live part of memory migration. With the stalls& zero page account, we just got to the point where we had basically infinity speed.

Later, Juan.

Quoted 27 lines

Regards,

Anthony Liguori

Quoted 22 lines

Now the other thing is the cache size.

with 64GB of RAM, we basically have a 16MB bitmap size, i.e. we blow the cache each time that we run ram_save_live().

This is one of the reasons why I don't want to walk the bitmap on ram_save_remaining().

ram_save_remaining() is linear with memory size (previous my changes). ram_save_live is also linear on the memory size, so we are in a worse case of n^n (notice that this is the worst case, we normally do much better n^2, n^3 or so).

Add to this that we are blowing the caches with big amounts of memory (we don't do witch smaller sizes), and you can see that our behaviour is clearly exponential.

As I need to fix them, I will work on them today/tomorrow.

Later, Juan.

Quoted 34 lines · Juan Quintela

Quoted 24 lines · Anthony Liguori

Quoted 22 lines · Juan Quintela

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Juan Quintela

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Well, 1st of all, it is "exponential" as you measure it.

stalls by default are:

1-2GB: milliseconds
2-4GB: 100-200ms
4-8GB: 1s
64GB: 59s
400GB: 24m (yes, minutes)

Quoted 5 lines

Quoted 3 lines

That sounds really exponential.

How are you measuring stalls btw?

At the end of the ram_save_live(). This was the reason that I put the information there.

Quoted 5 lines

for the 24mins stall (I don't have that machine anymore) I had less "exact" measurements. It was the amount that it "decided" to sent in the last non-live part of memory migration. With the stalls& zero page account, we just got to the point where we had basically infinity speed.

That's not quite guest visible.

It only is a "stall" if the guest is trying to access device emulation and acquiring the qemu_mutex. A more accurate measurement would be something that measured guest availability. For instance, I tight loop of while (1) { usleep(100); gettimeofday(); } that then recorded periods of unavailability > X.

Of course, it's critically important that a working version of pvclock be available int he guest for this to be accurate.

Quoted 50 lines · Anthony Liguori

Quoted 48 lines · Juan Quintela

Quoted 29 lines · Anthony Liguori

Quoted 26 lines · Juan Quintela

Quoted 4 lines · Michael S. Tsirkin

On Tue, Nov 30, 2010 at 04:40:41PM +0100, Juan Quintela wrote:

Quoted 8 lines

Quoted 4 lines

Basically our bitmap handling code is "exponential" on memory size,

I didn't realize this. What makes it exponential?

Well, 1st of all, it is "exponential" as you measure it.

stalls by default are:

1-2GB: milliseconds
2-4GB: 100-200ms
4-8GB: 1s
64GB: 59s
400GB: 24m (yes, minutes)

Quoted 7 lines

Quoted 4 lines

That sounds really exponential.

How are you measuring stalls btw?

At the end of the ram_save_live(). This was the reason that I put the information there.

for the 24mins stall (I don't have that machine anymore) I had less "exact" measurements. It was the amount that it "decided" to sent in
the last non-live part of memory migration. With the stalls& zero page
account, we just got to the point where we had basically infinity speed.

Quoted 2 lines

That's not quite guest visible.

Humm, guest don't answer in 24mins
monitor don't answer in 24mins
ping don't answer in 24mins

are you sure that this is not visible? Bug report put that guest had just died, it was me who waited to see that it took 24mins to end.

Quoted 6 lines

It only is a "stall" if the guest is trying to access device emulation and acquiring the qemu_mutex. A more accurate measurement would be something that measured guest availability. For instance, I tight loop of while (1) { usleep(100); gettimeofday(); } that then recorded periods of unavailability> X.

This is better, and this is what qemu_mutex change should fix.

Quoted 3 lines

Of course, it's critically important that a working version of pvclock be available int he guest for this to be accurate.

If the problem are 24mins, we don't need such an "exact" version O:-)

Later, Juan.

Quoted 9 lines · Juan Quintela

Quoted 9 lines

Quoted 9 lines

At the end of the ram_save_live(). This was the reason that I put the information there.

for the 24mins stall (I don't have that machine anymore) I had less "exact" measurements. It was the amount that it "decided" to sent in
the last non-live part of memory migration. With the stalls& zero page
account, we just got to the point where we had basically infinity speed.

Quoted 9 lines

Quoted 2 lines

That's not quite guest visible.

Humm, guest don't answer in 24mins
monitor don't answer in 24mins
ping don't answer in 24mins

are you sure that this is not visible? Bug report put that guest had just died, it was me who waited to see that it took 24mins to end.

I'm extremely sceptical that any of your patches would address this problem. Even if you had to scan every page in a 400GB guest, it
would not take 24 minutes. Something is not quite right here.

24 minutes suggests that there's another problem that is yet to be identified.

If we're burning excess CPU walking a 100MB bitmap, then let's fix that problem. Stopping every 1MB worth of the bitmap to do other work just papers over the real problem (that we're walking 100MB bitmap).

Quoted 5 lines · Anthony Liguori

If we're burning excess CPU walking a 100MB bitmap, then let's fix that problem. Stopping every 1MB worth of the bitmap to do other work just papers over the real problem (that we're walking 100MB bitmap).

Just using a bit per page will already make the problem 8x smaller, won't it? Is that enough to avoid the need for a new kernel interface?

Yeah, all of this should be done via tracing. Maybe Stefan can make some suggestions.

Quoted 2 lines · Anthony Liguori

Yeah, all of this should be done via tracing. Maybe Stefan can make some suggestions.

Here is an example of how to record savevm timestamps using tracing. Actually the timestamp is recorded automatically when a trace event fires.

Add these to the trace-events file:
savevm_start(void) ""
savevm_stop(unsigned int section_id) "section_id %u"

Then use trace_savevm_start() instead of START_SAVEVM_CLOCK() and trace_savevm_stop() instead of STOP_SAVEVM_CLOCK() in savevm.c. Also #include "trace.h".

All the macros and inline timestamp analysis can be removed from savevm.c.

./configure --trace-backend=simple [...]
make

After running savevm look for the trace-<pid> file that QEMU produces in its current working directory. You can pretty-print it like this:
./simpletrace.py trace-events trace-<pid>

The second field in the simpletrace.py output is the time delta (in microseconds) since the last trace event. So you can easily see how long start->stop took.

For more info see docs/tracing.txt. You might prefer to use SystemTap (./configure --trace-backend=dtrace) so you can write stap scripts to do more powerful analysis.

Stefan

Quoted 26 lines · Stefan Hajnoczi

Quoted 2 lines · Anthony Liguori

Yeah, all of this should be done via tracing. Maybe Stefan can make some suggestions.

Here is an example of how to record savevm timestamps using tracing. Actually the timestamp is recorded automatically when a trace event fires.

Add these to the trace-events file:
savevm_start(void) ""
savevm_stop(unsigned int section_id) "section_id %u"

Then use trace_savevm_start() instead of START_SAVEVM_CLOCK() and trace_savevm_stop() instead of STOP_SAVEVM_CLOCK() in savevm.c. Also #include "trace.h".

All the macros and inline timestamp analysis can be removed from savevm.c.

./configure --trace-backend=simple [...]
make

After running savevm look for the trace-<pid> file that QEMU produces in its current working directory. You can pretty-print it like this:
./simpletrace.py trace-events trace-<pid>

Quoted 7 lines

The second field in the simpletrace.py output is the time delta (in microseconds) since the last trace event. So you can easily see how long start->stop took.

For more info see docs/tracing.txt. You might prefer to use SystemTap (./configure --trace-backend=dtrace) so you can write stap scripts to do more powerful analysis.

Thanks.

Searching for guru, I basically want to only print the values when the difference is bigger that some values (number of calls is really big, I need to learn how to script the analisys).

Quoted 25 lines · Juan Quintela

From: Juan Quintela<[email protected]>

It returns a counter of things, not a ram address.

Signed-off-by Juan Quintela<[email protected]>

Signed-off-by Juan Quintela<[email protected]>

---
  arch_init.c |    4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

arch_init.c +2 -2

index df3d91f..9e941a0 100644
@@ -173,10 +173,10 @@ static int ram_save_block(QEMUFile *f)
 
  static uint64_t bytes_transferred;
 
-static ram_addr_t ram_save_remaining(void)
+static uint64_t ram_save_remaining(void)
  {
      RAMBlock *block;
-    ram_addr_t count = 0;
+    uint64_t count = 0;
 

arch_init.c static int ram_save_block(QEMUFile *f) · line 182

Quoted 3 lines

arch_init.c +0 -0

      QLIST_FOREACH(block,&ram_list.blocks, next) {
          ram_addr_t addr;

No, it returns a count of bytes of ram which is a subset of ram_addr_t's space. The unit is definitely right here.

Regards,

Anthony Liguori

Quoted 8 lines

arch_init.c +2 -2

-static ram_addr_t ram_save_remaining(void)
+static uint64_t ram_save_remaining(void)
  {
      RAMBlock *block;
-    ram_addr_t count = 0;
+    uint64_t count = 0;

Quoted 2 lines

      QLIST_FOREACH(block,&ram_list.blocks, next) {
          ram_addr_t addr;

No, it returns a count of bytes of ram which is a subset of ram_addr_t's space. The unit is definitely right here.

Quoted 8 lines · Anthony Liguori

Quoted 8 lines

arch_init.c +2 -2

-static ram_addr_t ram_save_remaining(void)
+static uint64_t ram_save_remaining(void)
  {
      RAMBlock *block;
-    ram_addr_t count = 0;
+    uint64_t count = 0;

Quoted 5 lines

Quoted 2 lines

      QLIST_FOREACH(block,&ram_list.blocks, next) {
          ram_addr_t addr;

No, it returns a count of bytes of ram which is a subset of ram_addr_t's space. The unit is definitely right here.

It returns a count of pages, actually, which is a different unit.

For example, in practice it can fit in 32 bits even (though I'm not
saying we should make it uint32_t; we'd be dangerously close to the limit for the guest sizes that Juan is handling).

Paolo

Quoted 5 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Signed-off-by Juan Quintela<[email protected]>

Signed-off-by Juan Quintela<[email protected]>

Why?

Regards,

Anthony Liguori

Quoted 37 lines

---
  arch_init.c |    7 ++++---
  1 files changed, 4 insertions(+), 3 deletions(-)

arch_init.c +4 -3

index 9e941a0..d32aaae 100644
@@ -216,6 +216,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
  {
      ram_addr_t addr;
      uint64_t bytes_transferred_last;
+    uint64_t t0;
      double bwidth = 0;
 
      if (stage<  0) {

@@ -258,7 +259,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
      }
 
      bytes_transferred_last = bytes_transferred;
-    bwidth = qemu_get_clock_ns(rt_clock);
+    t0 = qemu_get_clock_ns(rt_clock);
 
      while (!qemu_file_rate_limit(f)) {
          int bytes_sent;

@@ -270,8 +271,8 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
          }
      }
 
-    bwidth = qemu_get_clock_ns(rt_clock) - bwidth;
-    bwidth = (bytes_transferred - bytes_transferred_last) / bwidth;
+    t0 = qemu_get_clock_ns(rt_clock) - t0;
+    bwidth = (bytes_transferred - bytes_transferred_last) / t0;
 
      /* if we haven't transferred anything this round, force expected_time to a
       * a very high value, but without crashing */

From: Juan Quintela <[email protected]>

cheking each 64 pages is a random magic number as good as any other. We don't want to test too many times, but on the other hand, qemu_get_clock_ns() is not so expensive either.

Signed-off-by Juan Quintela <[email protected]>

Signed-off-by Juan Quintela <[email protected]>

---
 arch_init.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

arch_init.c +3 -0

index d32aaae..b463798 100644
@@ -40,6 +40,7 @@
 #include "net.h"
 #include "gdbstub.h"
 #include "hw/smbios.h"
+#include "buffered_file.h"
 
 #ifdef TARGET_SPARC
 int graphic_width = 1024;

@@ -218,6 +219,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     uint64_t bytes_transferred_last;
     uint64_t t0;
     double bwidth = 0;
+    int i;
 
     if (stage < 0) {
         cpu_physical_memory_set_dirty_tracking(0);

@@ -261,6 +263,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
     bytes_transferred_last = bytes_transferred;
     t0 = qemu_get_clock_ns(rt_clock);
 
+    i = 0;
     while (!qemu_file_rate_limit(f)) {
         int bytes_sent;
 

arch_init.c +4 -0

+		break;
+	    }
+	}
+        i++;
     }
 
     t0 = qemu_get_clock_ns(rt_clock) - t0;

Signature · 1 line

1.7.3.2

Quoted 6 lines · Juan Quintela

From: Juan Quintela <[email protected]>

cheking each 64 pages is a random magic number as good as any other. We don't want to test too many times, but on the other hand, qemu_get_clock_ns() is not so expensive either.

Could you please explain what's the problem this fixes? I would like to see an API that documents the contract we are making with the backend.

Quoted 6 lines

arch_init.c +2 -0

@@ -269,6 +272,19 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         if (bytes_sent == 0) { /* no more blocks */
             break;
         }
+	/* we want to check in the 1st loop, just in case it was the 1st time
+           and we had to sync the dirty bitmap.

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 281

Quoted 5 lines

arch_init.c +5 -0

+           qemu_get_clock_ns() is a bit expensive, so we only check each some
+           iterations
+	*/
+        if ((i & 63) == 0) {
+            uint64_t t1 = (qemu_get_clock_ns(rt_clock) - t0) / 1000000;

This adds even more non-determinism to savevm behaviour. If bandwidth limit is higth enough, I expect it to just keep going.

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 282

Quoted 1 line

arch_init.c +1 -0

+            if (t1 > buffered_file_interval/2) {

arch_init should not depend on buffered_file implementation IMO.

Also - / 2?

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 283

Quoted 1 line

arch_init.c +1 -0

+                printf("big delay %ld milliseconds, %d iterations\n", t1, i);

Is this a debugging aid?

Quoted 10 lines

arch_init.c +4 -0

+		break;
+	    }
+	}
+        i++;
     }
 
     t0 = qemu_get_clock_ns(rt_clock) - t0;

Signature · 1 line

1.7.3.2

Quoted 11 lines · Michael S. Tsirkin

Quoted 6 lines · Juan Quintela

From: Juan Quintela <[email protected]>

cheking each 64 pages is a random magic number as good as any other. We don't want to test too many times, but on the other hand, qemu_get_clock_ns() is not so expensive either.

Could you please explain what's the problem this fixes? I would like to see an API that documents the contract we are making with the backend.

buffered_file is an "abstraction" that uses a buffer.

live migration code (remember it can't sleep, it runs on the main loop) stores its "stuff" on that buffer. And a timer writes that buffer to the fd that is associated with migration.

This design is due to the main_loop/no threads qemu model.

buffered_file timer runs each 100ms. And we "try" to measure channel bandwidth from there. If we are not able to run the timer, all the calculations are wrong, and then stalls happens.

Quoted 16 lines

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 281

Quoted 13 lines

arch_init.c +7 -0

@@ -269,6 +272,19 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
         if (bytes_sent == 0) { /* no more blocks */
             break;
         }
+	/* we want to check in the 1st loop, just in case it was the 1st time
+           and we had to sync the dirty bitmap.
+           qemu_get_clock_ns() is a bit expensive, so we only check each some
+           iterations
+	*/
+        if ((i & 63) == 0) {
+            uint64_t t1 = (qemu_get_clock_ns(rt_clock) - t0) / 1000000;

This adds even more non-determinism to savevm behaviour. If bandwidth limit is higth enough, I expect it to just keep going.

If we find a row of 512MB of zero pages together (and that happens if you have a 64GB iddle guest, then you can spent more than 3seconds to fill the default bandwith). After that everything that uses the main loop has had stalls.

Quoted 5 lines

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 282

Quoted 1 line

arch_init.c +1 -0

+            if (t1 > buffered_file_interval/2) {

arch_init should not depend on buffered_file implementation IMO.

Also - / 2?

We need to run a timer each 100ms. For times look at the 0/6 patch. We can't spent more that 50ms in each function. It is something that should happen for all funnctions called from io_handlers.

Quoted 3 lines

arch_init.c int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque) · line 283

Quoted 1 line

arch_init.c +1 -0

+                printf("big delay %ld milliseconds, %d iterations\n", t1, i);

Is this a debugging aid?

I left that on purpose, to show that it happens a lot. There is no DEBUG_ARCH or DEBUG_RAM around, I can create them if you preffer. But notice that this is something that shouldn't happen (but it happens).

DPRINTF for that file should be a good idea, will do.

Quoted 10 lines

Quoted 10 lines

arch_init.c +4 -0

+		break;
+	    }
+	}
+        i++;
     }
 
     t0 = qemu_get_clock_ns(rt_clock) - t0;

Signature · 1 line

1.7.3.2

Quoted 3 lines

Quoted 3 lines

buffered_file timer runs each 100ms. And we "try" to measure channel bandwidth from there. If we are not able to run the timer, all the calculations are wrong, and then stalls happens.

So the problem is the timer in the buffered file abstraction? Why don't we just flush out data if the buffer is full?

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines

Quoted 3 lines

buffered_file timer runs each 100ms. And we "try" to measure channel bandwidth from there. If we are not able to run the timer, all the calculations are wrong, and then stalls happens.

Quoted 2 lines

So the problem is the timer in the buffered file abstraction? Why don't we just flush out data if the buffer is full?

It takes a lot to fill the buffer if you have many zero pages, and if that happens the guest is starved by the main loop filling the buffer.

Paolo

Quoted 9 lines · Paolo Bonzini

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines

Quoted 1 line

buffered_file timer runs each 100ms. And we "try" to measure

channel

Quoted 2 lines

bandwidth from there. If we are not able to run the timer, all the calculations are wrong, and then stalls happens.

So the problem is the timer in the buffered file abstraction? Why don't we just flush out data if the buffer is full?

Quoted 3 lines

It takes a lot to fill the buffer if you have many zero pages, and if that happens the guest is starved by the main loop filling the buffer.

Sounds like the sort of thing you'd only see if you created a guest a large guest that was mostly unallocated and then tried to migrate. That doesn't seem like a very real case to me though.

The best approach would be to drop qemu_mutex while processing this code instead of having an arbitrary back-off point. The later is deferring the problem to another day when it becomes the source of a future problem.

Regards,

Anthony Liguori

Quoted 2 lines

Paolo

Sounds like the sort of thing you'd only see if you created a guest a large guest that was mostly unallocated and then tried to migrate.

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

Quoted 3 lines · Anthony Liguori

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

The problem is that the file rate limit is not hit because work is done elsewhere. The rate can limit the bandwidth used and makes QEMU aware that socket operations may block (because that's what the buffered file freeze/unfreeze logic does); but it cannot be used to limit the _time_ spent in the migration code.

Paolo

Quoted 6 lines · Paolo Bonzini

Quoted 3 lines · Anthony Liguori

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

The problem is that the file rate limit is not hit because work is done elsewhere. The rate can limit the bandwidth used and makes QEMU aware that socket operations may block (because that's what the buffered file freeze/unfreeze logic does); but it cannot be used to limit the _time_ spent in the migration code.

Yes, it can, if you set the rate limit sufficiently low.

The caveats are 1) the kvm.ko interface for dirty bits doesn't scale for large memory guests so we spend a lot more CPU time walking it than we should 2) zero pages cause us to burn a lot more CPU time than we otherwise would because compressing them is so effective.

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

Quoted 6 lines · Anthony Liguori

Quoted 5 lines · Paolo Bonzini

Quoted 3 lines · Anthony Liguori

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

Quoted 5 lines

Quoted 1 line

The problem is that the file rate limit is not hit because work is done elsewhere. The rate can limit the bandwidth used and makes QEMU aware that socket operations may block (because that's what the buffered file freeze/unfreeze logic does); but it cannot be used to limit the _time_ spent in the migration code.

Yes, it can, if you set the rate limit sufficiently low.

The caveats are 1) the kvm.ko interface for dirty bits doesn't scale for large memory guests so we spend a lot more CPU time walking it than we should 2) zero pages cause us to burn a lot more CPU time than we otherwise would because compressing them is so effective.

What's the problem with burning that cpu? per guest page, compressing takes less than sending. Is it just an issue of qemu mutex hold time?

Quoted 3 lines

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

Why would a multi-level table help? (or rather, please explain what you mean by a multi-level table).

Something we could do is divide memory into more slots, and polling each slot when we start to scan its page range. That reduces the time between sampling a page's dirtiness and sending it off, and reduces the latency incurred by the sampling. There are also non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

Quoted 1 line

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

IMO that's the biggest hit currently.

Quoted 1 line

What's the problem with burning that cpu? per guest page, compressing takes less than sending. Is it just an issue of qemu mutex hold time?

If you have a 512GB guest, then you have a 16MB dirty bitmap which ends up being an 128MB dirty bitmap in QEMU because we represent dirty bits with 8 bits.

Walking 16mb (or 128mb) of memory just fine find a few pages to send over the wire is a big waste of CPU time. If kvm.ko used a multi-level table to represent dirty info, we could walk the memory mapping at 2MB chunks allowing us to skip a large amount of the comparisons.

Quoted 8 lines

Quoted 4 lines

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

Why would a multi-level table help? (or rather, please explain what you mean by a multi-level table).

Something we could do is divide memory into more slots, and polling each slot when we start to scan its page range. That reduces the time between sampling a page's dirtiness and sending it off, and reduces the latency incurred by the sampling. There are also non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

BTW, we should also refactor qemu to use the kvm dirty bitmap directly instead of mapping it to the main dirty bitmap.

Quoted 3 lines

Quoted 1 line

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

IMO that's the biggest hit currently.

Yup. That's the Correct solution to the problem.

Quoted 4 lines · Anthony Liguori

Quoted 1 line

What's the problem with burning that cpu? per guest page, compressing takes less than sending. Is it just an issue of qemu mutex hold time?

If you have a 512GB guest, then you have a 16MB dirty bitmap which ends up being an 128MB dirty bitmap in QEMU because we represent dirty bits with 8 bits.

Was there not a patchset to split each bit into its own bitmap? And then copy the kvm or qemu master bitmap into each client bitmap as it became needed?

Quoted 1 line

Walking 16mb (or 128mb) of memory just fine find a few pages to send over the wire is a big waste of CPU time. If kvm.ko used a multi-level table to represent dirty info, we could walk the memory mapping at 2MB chunks allowing us to skip a large amount of the comparisons.

There's no reason to assume dirty pages would be clustered. If 0.2% of memory were dirty, but scattered uniformly, there would be no win from the two-level bitmap. A loss, in fact: 2MB can be represented as 512 bits or 64 bytes, just one cache line. Any two-level thing will need more.

We might have a more compact encoding for sparse bitmaps, like run-length encoding.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

Quoted 5 lines

Quoted 3 lines

Why would a multi-level table help? (or rather, please explain what you mean by a multi-level table).

Something we could do is divide memory into more slots, and polling each slot when we start to scan its page range. That reduces the time between sampling a page's dirtiness and sending it off, and reduces the latency incurred by the sampling. There are also non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

BTW, we should also refactor qemu to use the kvm dirty bitmap directly instead of mapping it to the main dirty bitmap.

That's what the patch set I was alluding to did. Or maybe I imagined the whole thing.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

IMO that's the biggest hit currently.

Yup. That's the Correct solution to the problem.

Then let's just Do it.

Quoted 1 line

That's what the patch set I was alluding to did. Or maybe I imagined the whole thing.

No, it just split the main bitmap into three bitmaps. I'm suggesting that we have the dirty interface have two implementations, one that refers to the 8-bit bitmap when TCG in use and another one that uses the KVM representation.

TCG really needs multiple dirty bits but KVM doesn't. A shared implementation really can't be optimal.

Quoted 8 lines · Anthony Liguori

Quoted 3 lines

What's the problem with burning that cpu? per guest page, compressing takes less than sending. Is it just an issue of qemu mutex hold time?

If you have a 512GB guest, then you have a 16MB dirty bitmap which ends up being an 128MB dirty bitmap in QEMU because we represent dirty bits with 8 bits.

Was there not a patchset to split each bit into its own bitmap? And then copy the kvm or qemu master bitmap into each client bitmap as it became needed?

Quoted 5 lines

Walking 16mb (or 128mb) of memory just fine find a few pages to send over the wire is a big waste of CPU time. If kvm.ko used a multi-level table to represent dirty info, we could walk the memory mapping at 2MB chunks allowing us to skip a large amount of the comparisons.

There's no reason to assume dirty pages would be clustered. If 0.2% of memory were dirty, but scattered uniformly, there would be no win from the two-level bitmap. A loss, in fact: 2MB can be represented as 512 bits or 64 bytes, just one cache line. Any two-level thing will need more.

We might have a more compact encoding for sparse bitmaps, like run-length encoding.

Quoted 2 lines

BTW, we should also refactor qemu to use the kvm dirty bitmap directly instead of mapping it to the main dirty bitmap.

That's what the patch set I was alluding to did. Or maybe I imagined the whole thing.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

IMO that's the biggest hit currently.

Yup. That's the Correct solution to the problem.

Then let's just Do it.

Quoted 8 lines · Anthony Liguori

Quoted 3 lines

What's the problem with burning that cpu? per guest page, compressing takes less than sending. Is it just an issue of qemu mutex hold time?

If you have a 512GB guest, then you have a 16MB dirty bitmap which ends up being an 128MB dirty bitmap in QEMU because we represent dirty bits with 8 bits.

Was there not a patchset to split each bit into its own bitmap? And then copy the kvm or qemu master bitmap into each client bitmap as it became needed?

Quoted 5 lines

Walking 16mb (or 128mb) of memory just fine find a few pages to send over the wire is a big waste of CPU time. If kvm.ko used a multi-level table to represent dirty info, we could walk the memory mapping at 2MB chunks allowing us to skip a large amount of the comparisons.

There's no reason to assume dirty pages would be clustered. If 0.2% of memory were dirty, but scattered uniformly, there would be no win from the two-level bitmap. A loss, in fact: 2MB can be represented as 512 bits or 64 bytes, just one cache line. Any two-level thing will need more.

We might have a more compact encoding for sparse bitmaps, like run-length encoding.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

Quoted 7 lines

Quoted 7 lines

Why would a multi-level table help? (or rather, please explain what you mean by a multi-level table).

Something we could do is divide memory into more slots, and polling each slot when we start to scan its page range. That reduces the time between sampling a page's dirtiness and sending it off, and reduces the latency incurred by the sampling. There are also

Quoted 3 lines

Quoted 3 lines

non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

Quoted 3 lines

BTW, we should also refactor qemu to use the kvm dirty bitmap directly instead of mapping it to the main dirty bitmap.

That's what the patch set I was alluding to did. Or maybe I imagined the whole thing.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

IMO that's the biggest hit currently.

Yup. That's the Correct solution to the problem.

Then let's just Do it.

Signature · 6 lines

error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 30 Nov 2010 16:27:13 +0200
Avi Kivity <[email protected]> wrote:

Does anyone is profiling these dirty bitmap things?

- 512GB guest is really the target?

- how much cpu time can we use for these things?

- how many dirty pages do we have to care?

Since we are planning to do some profiling for these, taking into account Kemari, can you please share these information?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

In the short term, fixing (2) by accounting zero pages as full sized pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance.

Why would a multi-level table help? (or rather, please explain what you mean by a multi-level table).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Something we could do is divide memory into more slots, and polling each slot when we start to scan its page range. That reduces the time between sampling a page's dirtiness and sending it off, and reduces the latency incurred by the sampling. There are also

If we use rmap approach with one more interface, we can specify which range of dirty bitmap to get. This has the same effect to splitting into more slots.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

IIUC, O(1) will lazily write protect pages beggining from top level? Does this have any impact other than the timing of get_dirty_log()?

Quoted 2 lines

Since we are planning to do some profiling for these, taking into account Kemari, can you please share these information?

If you see the 0/10 email with this setup, you can see how much time are we spending on stuff. Just now (for migration, kemari is a bit different) we have to fix other things first.

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

non-interface-changing ways to reduce this latency, like O(1) write protection, or using dirty bits instead of write protection when available.

IIUC, O(1) will lazily write protect pages beggining from top level? Does this have any impact other than the timing of get_dirty_log()?

dunno.

At this point I am trying to:
- get migration with 16-64GB to not having stalls.
- get infrastructure to be able to know what is going on.

So far, bigger stalls are gone, and discusing what to do next. As Anthony suggested I run ram_save_live() loop without qemu_mutex, and now guests get much better interaction, but my current patch (for this) is just to put qemu_mutex_unlock_iothread()/qemu_mutex_lock_iothread() around it. I think that we are racey with teh access to the bitmap, but it was just a test.

With respect to Kemari, we can discuss what do you need and how you are going to test, just to not do overlapping work.

Quoted 1 line

- 512GB guest is really the target?

no, problems exist with smaller amounts of RAM. with 16GB guest it is trivial to get 1s stalls, 64GB guest, 3-4s, with more memory, migration is flaky to say the less.

Quoted 1 line

- how much cpu time can we use for these things?

the problem here is that we are forced to walk the bitmap too many times, we want to do it less times.

Quoted 1 line

- how many dirty pages do we have to care?

default values and assuming 1Gigabit ethernet for ourselves ~9.5MB of dirty pages to have only 30ms of downtime.

Quoted 6 lines · Juan Quintela

Quoted 1 line

- 512GB guest is really the target?

no, problems exist with smaller amounts of RAM. with 16GB guest it is trivial to get 1s stalls, 64GB guest, 3-4s, with more memory, migration is flaky to say the less.

Quoted 4 lines

Quoted 1 line

- how much cpu time can we use for these things?

the problem here is that we are forced to walk the bitmap too many times, we want to do it less times.

How much time is spent walking bitmaps? Are you sure this is the problem?

Quoted 4 lines

Quoted 1 line

- how many dirty pages do we have to care?

default values and assuming 1Gigabit ethernet for ourselves ~9.5MB of dirty pages to have only 30ms of downtime.

1Gb/s * 30ms = 100 MB/s * 30 ms = 3 MB.

Quoted 4 lines

Quoted 1 line

- how many dirty pages do we have to care?

default values and assuming 1Gigabit ethernet for ourselves ~9.5MB of dirty pages to have only 30ms of downtime.

1Gb/s * 30ms = 100 MB/s * 30 ms = 3 MB.

Thanks for the answers Avi, Juan,

Some FYI, (not about the bottleneck)

Quoted 7 lines · On Wed, 01 Dec 2010 14:35:57 +0200 Avi Kivity<[email protected]> wrote:

Quoted 4 lines

Quoted 1 line

- how many dirty pages do we have to care?

default values and assuming 1Gigabit ethernet for ourselves ~9.5MB of dirty pages to have only 30ms of downtime.

1Gb/s * 30ms = 100 MB/s * 30 ms = 3 MB.

3MB / 4KB/page = 750 pages.

Then, KVM side processing is near the theoretical goal!

In my framebuffer test, I tested

   nr_dirty_pages/npages = 576/4096

case with the rate of 20 updates/s (1updates/50ms).

Using rmap optimization, write protection only took 46,718 tsc time.

Bitmap copy was not a problem of course.

The display was working anyway at this rate!

In my guess, within 1,000 dirty pages, kvm_vm_ioctl_get_dirty_log() can be processed within 200us or so even for large RAM slot.
- rmap optimization depends mainly on nr_dirty_pages but npages.

Avi, can you guess the property of O(1) write protection? I want to test rmap optimization taking these issues into acount.

Of course, Kemari have to continue synchronization, and maybe see more dirty pages. This will be a future task!

Quoted 5 lines · Paolo Bonzini

Quoted 3 lines · Anthony Liguori

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

Quoted 5 lines

The problem is that the file rate limit is not hit because work is done elsewhere. The rate can limit the bandwidth used and makes QEMU aware that socket operations may block (because that's what the buffered file freeze/unfreeze logic does); but it cannot be used to limit the _time_ spent in the migration code.

Yes, it can, if you set the rate limit sufficiently low.

[..] accounting zero pages as full sized
pages should "fix" the problem.

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance. We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

Quoted 6 lines · Anthony Liguori

Quoted 5 lines · Paolo Bonzini

Quoted 3 lines · Anthony Liguori

BufferedFile should hit the qemu_file_rate_limit check when the socket buffer gets filled up.

Quoted 7 lines

Quoted 5 lines

The problem is that the file rate limit is not hit because work is done elsewhere. The rate can limit the bandwidth used and makes QEMU aware that socket operations may block (because that's what the buffered file freeze/unfreeze logic does); but it cannot be used to limit the _time_ spent in the migration code.

Yes, it can, if you set the rate limit sufficiently low.

You mean, just like you can drive a car without brakes by keeping the speed sufficiently low.

Quoted 2 lines

[..] accounting zero pages as full sized
pages should "fix" the problem.

I know you used quotes, but it's a very very generous definition of fix. Both these proposed "fixes" are nothing more than workarounds, and even particularly ugly ones. The worst thing about them is that there is no guarantee of migration finishing in a reasonable time, or at all.

If you account zero pages as full, you don't use effectively the bandwidth that was allotted to you, you use only 0.2% of it (8/4096). It then takes an exaggerate amount of time to start iteration on pages that matter. If you set the bandwidth low, instead, you do not have the bandwidth you need in order to converge.

Even from an aesthetic point of view, if there is such a thing, I don't understand why you advocate conflating network bandwidth and CPU usage into a single measurement. Nobody disagrees that all you propose is nice to have, and that what Juan sent is a stopgap measure (though a very effective one). However, this doesn't negate that Juan's accounting patches make a lot of sense in the current design.

Quoted 4 lines

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance. We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

This may be a good way to fix it, but it's also basically a rewrite.

Paolo

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 2 lines · Paolo Bonzini

Quoted 1 line · Anthony Liguori

On 11/30/2010 01:15 AM, Paolo Bonzini wrote:

Juan's patch, IIUC, does the following: If you've been iterating in a tight loop, return to the main loop for *one* iteration every 50ms.

But this means that during this 50ms period of time, a VCPU may be blocked from running. If the guest isn't doing a lot of device I/O *and* you're on a relatively low link speed, then this will mean that you don't hold qemu_mutex for more than 50ms at a time.

But in the degenerate case where you have a high speed link and you have a guest doing a lot of device I/O, you'll see the guest VCPU being blocked for 50ms, then getting to run for a very brief period of time, followed by another block for 50ms. The guest's execution will be extremely sporadic.

This isn't fixable with this approach. The only way to really fix this is to say that over a given period of time, migration may only consume XX amount of CPU time which guarantees the VCPUs get the qemu_mutex for the rest of the time.

This is exactly what rate limiting does. Yes, it results in a longer migration time but that's the trade-off we have to make if we want deterministic VCPU execution until we can implement threading properly.

If you want a simple example, doing I/O with the rtl8139 adapter while doing your migration test and run a tight loop in the get running gettimeofday(). Graph the results to see how much execution time the guest is actually getting.

Quoted 6 lines

Quoted 4 lines

In the long term, we need a new dirty bit interface from kvm.ko that uses a multi-level table. That should dramatically improve scan performance. We also need to implement live migration in a separate thread that doesn't carry qemu_mutex while it runs.

This may be a good way to fix it, but it's also basically a rewrite.

The only correct short term solution I can see if rate limiting unfortunately.

Regards,

Anthony Liguori

Quoted 5 lines

Paolo

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

There needs to be numbers that justify this as part of the commit message.

This only works for KVM because it happens to use set_dirty() whenever it dirties memory. I don't think will work with TCG.

I also think that the fundamental problem is the kernel dirty bitmap. Perhaps it should return a multiple level table instead of a simple linear bitmap.

Regards,

Anthony Liguori

Quoted 79 lines

Signed-off-by Juan Quintela<[email protected]>

Signed-off-by Juan Quintela<[email protected]>

---
  arch_init.c |   15 +--------------
  cpu-all.h   |    7 +++++++
  exec.c      |    1 +
  3 files changed, 9 insertions(+), 14 deletions(-)

arch_init.c +1 -14

index b463798..c2bc144 100644
@@ -176,20 +176,7 @@ static uint64_t bytes_transferred;
 
  static uint64_t ram_save_remaining(void)
  {
-    RAMBlock *block;
-    uint64_t count = 0;
-
-    QLIST_FOREACH(block,&ram_list.blocks, next) {
-        ram_addr_t addr;
-        for (addr = block->offset; addr<  block->offset + block->length;
-             addr += TARGET_PAGE_SIZE) {
-            if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
-                count++;
-            }
-        }
-    }
-
-    return count;
+    return ram_list.dirty_pages;
  }
 
  uint64_t ram_bytes_remaining(void)

cpu-all.h +7 -0

index 30ae17d..5dc27c6 100644
@@ -874,6 +874,7 @@ typedef struct RAMBlock {
 
  typedef struct RAMList {
      uint8_t *phys_dirty;
+    uint64_t dirty_pages;
      QLIST_HEAD(ram, RAMBlock) blocks;
  } RAMList;
  extern RAMList ram_list;

@@ -922,6 +923,9 @@ static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
 
  static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
  {
+    if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG))
+        ram_list.dirty_pages++;
+
      ram_list.phys_dirty[addr>>  TARGET_PAGE_BITS] = 0xff;
  }
 

@@ -942,6 +946,9 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
      mask = ~dirty_flags;
      p = ram_list.phys_dirty + (start>>  TARGET_PAGE_BITS);
      for (i = 0; i<  len; i++) {
+        if (cpu_physical_memory_get_dirty(start + i * TARGET_PAGE_SIZE,
+                                          MIGRATION_DIRTY_FLAG&  dirty_flags))
+            ram_list.dirty_pages--;
          p[i]&= mask;
      }
  }

exec.c +1 -0

index f5b2386..ca2506e 100644
@@ -2866,6 +2866,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(DeviceState *dev, const char *name,
                                         last_ram_offset()>>  TARGET_PAGE_BITS);
      memset(ram_list.phys_dirty + (new_block->offset>>  TARGET_PAGE_BITS),
             0xff, size>>  TARGET_PAGE_BITS);
+    ram_list.dirty_pages += size>>  TARGET_PAGE_BITS;
 
      if (kvm_enabled())
          kvm_setup_guest_memory(new_block->host, size);

Quoted 8 lines · Anthony Liguori

Quoted 6 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

Quoted 1 line

There needs to be numbers that justify this as part of the commit message.

They are on patch 0/6.

Additionally, with 64GB of RAM, this bitmap is HUGE, having to walk over it in each ram_save_live() call is too onerous.

Quoted 11 lines · Juan Quintela

Quoted 9 lines · Anthony Liguori

Quoted 6 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

There needs to be numbers that justify this as part of the commit message.

Quoted 4 lines

They are on patch 0/6.

Additionally, with 64GB of RAM, this bitmap is HUGE, having to walk over it in each ram_save_live() call is too onerous.

It's not so huge. It's scaled down by a factor of 8 * 4096 = 32K. So it's a 2MB bitmap. If kept as a bitmap and accessed in longs, it can be read in less than a millisecond.

An optimization can be to look at the previous ram_save_live (which had to walk the bitmap). If old_nr_dirty > 4*target_nr_dirty, assume we need one more pass and don't scan the bitmap.

Another optimization is to stop the count when we reach the target; instead of ram_save_remaining() have a ram_save_may_stop() which counts the number of dirty bits until it reaches target_nr_dirty or exhausts the bitmap.

Quoted 15 lines · Avi Kivity

Quoted 13 lines · Juan Quintela

Quoted 11 lines · Anthony Liguori

Quoted 7 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

There needs to be numbers that justify this as part of the commit message.

Quoted 8 lines

Quoted 5 lines

They are on patch 0/6.

Additionally, with 64GB of RAM, this bitmap is HUGE, having to walk over it in each ram_save_live() call is too onerous.

It's not so huge. It's scaled down by a factor of 8 * 4096 = 32K. So it's a 2MB bitmap. If kept as a bitmap and accessed in longs, it can be read in less than a millisecond.

Quoted 22 lines · Juan Quintela

Quoted 20 lines · Avi Kivity

Quoted 18 lines · Juan Quintela

Quoted 11 lines · Anthony Liguori

Quoted 7 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

There needs to be numbers that justify this as part of the commit message.

They are on patch 0/6.

Additionally, with 64GB of RAM, this bitmap is HUGE, having to walk over it in each ram_save_live() call is too onerous.

Quoted 3 lines

Quoted 3 lines

It's not so huge. It's scaled down by a factor of 8 * 4096 = 32K. So it's a 2MB bitmap. If kept as a bitmap and accessed in longs, it can be read in less than a millisecond.

BTW, by this logic, even a 1-byte dirty bitmap is only 16mb which can be read in less than 16ms so where is the reported 24 minute stall coming from?

Regards,

Anthony Liguori

Quoted 35 lines · Anthony Liguori

Quoted 33 lines · Juan Quintela

Quoted 30 lines · Avi Kivity

Quoted 14 lines · Juan Quintela

Quoted 11 lines · Anthony Liguori

Quoted 8 lines · Juan Quintela

From: Juan Quintela<[email protected]>

Calculate the number of dirty pages takes a lot on hosts with lots of memory. Just maintain how many pages are dirty. Only sync bitmaps if number is small enough.

Quoted 9 lines

Quoted 3 lines

There needs to be numbers that justify this as part of the commit message.

They are on patch 0/6.

Additionally, with 64GB of RAM, this bitmap is HUGE, having to walk over it in each ram_save_live() call is too onerous.

It's not so huge. It's scaled down by a factor of 8 * 4096 = 32K. So it's a 2MB bitmap. If kept as a bitmap and accessed in longs, it can be read in less than a millisecond.

Quoted 4 lines

BTW, by this logic, even a 1-byte dirty bitmap is only 16mb which can be read in less than 16ms so where is the reported 24 minute stall coming from?

a) we read the bitmap more than once

b) the 1ms is based on "we read" it with longs and optimized, just now we have to read it by byte and inside the byte.

Later, Juan.

Quoted 4 lines

Regards,

Anthony Liguori

Quoted 4 lines

Quoted 3 lines

BTW, by this logic, even a 1-byte dirty bitmap is only 16mb which can be read in less than 16ms so where is the reported 24 minute stall coming from?

a) we read the bitmap more than once

Not in a single iteration which is what the "stall" would consist of.

Quoted 2 lines

b) the 1ms is based on "we read" it with longs and optimized, just now we have to read it by byte and inside the byte.

Byte accesses verse long access doesn't turn 16ms into 24 minutes.

Quoted 5 lines · Anthony Liguori

Quoted 4 lines

Quoted 3 lines

BTW, by this logic, even a 1-byte dirty bitmap is only 16mb which can be read in less than 16ms so where is the reported 24 minute stall coming from?

a) we read the bitmap more than once

Quoted 6 lines

Not in a single iteration which is what the "stall" would consist of.

Quoted 2 lines

b) the 1ms is based on "we read" it with longs and optimized, just now we have to read it by byte and inside the byte.

Byte accesses verse long access doesn't turn 16ms into 24 minutes.

We need actual measurements instead of speculations.

Quoted 1 line

We need actual measurements instead of speculations.

Yes, I agree 100%. I think the place to start is what I suggested in a previous note in this thread, we need to measure actual stall time in the guest.

Quoted 4 lines · Anthony Liguori

Quoted 1 line

We need actual measurements instead of speculations.

Yes, I agree 100%. I think the place to start is what I suggested in a previous note in this thread, we need to measure actual stall time in the guest.

I'd actually start at the host. How much time does ioctl(KVM_GET_DIRTY_LOG) take? What's the percentage of time qemu_mutex is held?

Quoted 7 lines · Avi Kivity

Quoted 4 lines · Anthony Liguori

Quoted 1 line

We need actual measurements instead of speculations.

Yes, I agree 100%. I think the place to start is what I suggested in a previous note in this thread, we need to measure actual stall time in the guest.

I'd actually start at the host. How much time does ioctl(KVM_GET_DIRTY_LOG) take? What's the percentage of time qemu_mutex is held?

The question is, what really are the symptoms of the problem. It's not necessarily a bad thing if KVM_GET_DIRTY_LOG takes a long while qemu_mutex is held.

Is the problem that the monitor responds slowly? Is the problem that the guest isn't consistently getting execution time? Is the proper simply that the guest isn't getting enough total execution time?

I think we need to identify exactly what the problem is before we look for sources.

Quoted 5 lines · Anthony Liguori

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Anthony Liguori

Quoted 1 line

We need actual measurements instead of speculations.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Yes, I agree 100%. I think the place to start is what I suggested in a previous note in this thread, we need to measure actual stall time in the guest.

I'd actually start at the host. How much time does ioctl(KVM_GET_DIRTY_LOG) take? What's the percentage of time qemu_mutex is held?

The question is, what really are the symptoms of the problem. It's not necessarily a bad thing if KVM_GET_DIRTY_LOG takes a long while qemu_mutex is held.

Whether or not qemu_mutex is held, long KVM_GET_DIRTY_LONG runtimes are bad, since they are a lower bound on your downtime. And KVM_GET_DIRTY_LOG does a lot of work, and invokes synchronize_srcu_expedited(), which can be very slow.

Quoted 2 lines

Is the problem that the monitor responds slowly? Is the problem that the guest isn't consistently getting execution time? Is the proper simply that the guest isn't getting enough total execution time?

All three can happen if qemu_mutex is held too long.

The third can happen for other reasons (like KVM_GET_DIRTY_LOG
holding the mmu spinlock for too long, can fix with O(1) write protection).

Quoted 3 lines

I think we need to identify exactly what the problem is before we look for sources.

Quoted 14 lines · Avi Kivity

Quoted 13 lines · Anthony Liguori

Quoted 11 lines · Avi Kivity

Quoted 6 lines · Anthony Liguori

Quoted 1 line

We need actual measurements instead of speculations.

Yes, I agree 100%. I think the place to start is what I suggested in a previous note in this thread, we need to measure actual stall time in the guest.

I'd actually start at the host. How much time does ioctl(KVM_GET_DIRTY_LOG) take? What's the percentage of time qemu_mutex is held?

Quoted 8 lines

Quoted 3 lines

The question is, what really are the symptoms of the problem. It's not necessarily a bad thing if KVM_GET_DIRTY_LOG takes a long while qemu_mutex is held.

Whether or not qemu_mutex is held, long KVM_GET_DIRTY_LONG runtimes are bad, since they are a lower bound on your downtime. And KVM_GET_DIRTY_LOG does a lot of work, and invokes synchronize_srcu_expedited(), which can be very slow.

That's fine, and you're right, it's a useful thing to do, but this series originated because of a problem and we ought to make sure we capture what the actual problem is. That's not to say we shouldn't improve things that could stand to be improved.

Quoted 6 lines

Quoted 5 lines

Is the problem that the monitor responds slowly? Is the problem that the guest isn't consistently getting execution time? Is the proper simply that the guest isn't getting enough total execution time?

Quoted 1 line

All three can happen if qemu_mutex is held too long.

Right, but I'm starting to think that the root of the problem is not that it's being held too long but that it's being held too often.