[Qemu-devel] Re: [PATCH 02/10] Add buffered_file_internal constant

[Juan Quintela <quintela@redhat.com>]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 11/24/2010 04:52 AM, Juan Quintela wrote:
>> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>>    
>>> On Wed, Nov 24, 2010 at 12:02:59AM +0100, Juan Quintela wrote:
>>>      
>>>> From: Juan Quintela<quintela@trasno.org>
>>>>        
>>    
>>>> diff --git a/buffered_file.h b/buffered_file.h
>>>> index 98d358b..a728316 100644
>>>> --- a/buffered_file.h
>>>> +++ b/buffered_file.h
>>>> @@ -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.
>>>      
>> 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.

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.

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

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

> 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.

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

This is a different problem.

> 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.

>  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.