Re: [Qemu-devel] [PATCH REPOST 0/2] Add basic "detach" support for dump-guest-memory

[Laszlo Ersek]

On 11/24/15 04:10, Fam Zheng wrote:
> On Tue, 11/24 09:57, Peter Xu wrote:
>> On 11/24/2015 01:57 AM, Andrew Jones wrote:
>>> On Mon, Nov 23, 2015 at 05:22:29PM +0100, Laszlo Ersek wrote:
>>>> On 11/23/15 11:07, Peter Xu wrote:
>>>>> Currently, dump-guest-memory supports synchronous operation only. This 
>>>>> patch
>>>>> sets are adding "detach" support for it (just like "migrate -d" for
>>>>> migration). When "-d" is provided, dump-guest-memory command will return
>>>>> immediately without hanging user. This should be useful when the backend
>>>>> storage for the dump file is very slow.
>>>>>
>>>>> Peter Xu (2):
>>>>>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces
>>>>>   dump-guest-memory: add basic "detach" support.
>>>>>
>>>>>  dump.c                | 62 
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>>>  hmp-commands.hx       |  5 +++--
>>>>>  hmp.c                 |  3 ++-
>>>>>  include/sysemu/dump.h |  4 ++++
>>>>>  qapi-schema.json      |  3 ++-
>>>>>  qmp-commands.hx       |  4 ++--
>>>>>  qmp.c                 |  9 ++++++++
>>>>>  vl.c                  |  3 +++
>>>>>  8 files changed, 81 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> I'm not seeing anything that would prevent races between the new thread
>>>> and any other existing threads that manipulate the MemoryRegion objects
>>>> (in response to guest actions), or the guest RAM contents (by way of
>>>> executing guest code).
>>>>
>>>> The dump_init() function has
>>>>
>>>>     if (runstate_is_running()) {
>>>>         vm_stop(RUN_STATE_SAVE_VM);
>>>>         s->resume = true;
>>>>     } else {
>>>>         s->resume = false;
>>>>     }
>>>>
>>>> Whereas dump_cleanup() has:
>>>>
>>>>     if (s->resume) {
>>>>         vm_start();
>>>>     }
>>>>
>>>> If you return control to the QEMU monitor's user before the dump
>>>> completes, they could issue the "cont" command, and unleash the VCPU
>>>> threads again. (Of course, this is just one example where things could
>>>> go wrong.)
>>
>> Yes, I added the global flag "dump_in_progress_p" to do this. For
>> now, what I found might be affected was "dump-guest-memory" itself,
>> and "cont". Please check patch 2/2 modification for qmp_cont(). I
>> failed to find any other place that might be influenced by this
>> asynchronous operation (you are right, maybe it still exists, and it
>> might introduce extra bugs, actually that's what I was looking for
>> to see whether I missed something in the review session).
> 
> What about all the hot-plug commands that changes the memory layout?
> 
> Another question is what if user issued "stop" during dump, should you still
> resume when dump completes?

I agree completely. Synchronization requires design; it is not (well,
should not be) a whack-a-mole game where you smack down races as they
pop up. The possible interactions here are just to complex, and making
the dump feature truly multi-threaded would require a lot of work. I
don't think that is warranted for in this instance (see below).

> 
>>
>>>>
>>>> Also, the live migration analogy is not a good one IMO. For live
>>>> migration, a whole infrastructure exists for tracking asynchronous guest
>>>> state changes (dirty bitmap, locking, whatever).
>>>>
>>>> The good analogy with live migration would be continuous streaming of
>>>> guest memory changes into the dump file, until it converges, or a cutoff
>>>> is reached (at which point the guest would be frozen, same as now). Of
>>>> course, such streaming could generate huge amounts of traffic and
>>>> entirely defeat the original purpose.
>>
>> Yes, I see that migration is much more complex scenario, so that's
>> why I am trying to add "basic detach" support, just as I mentioned
>> in the patch title. :)
>>
>> Before doing anything like that complex, I will send a mail asking
>> about it, to first know whether we need to do that.
>>
>>>>
>>>> In total, I don't think this is a good idea. I find it possible that
>>>> this would lead to QEMU crashes, and/or wildly inconsistent guest memory
>>>> images.
>>>
>>> Despite having already run through both patches giving review comments,
>>> I agree with Laszlo. At first blush it seems like a good idea, but I
>>> can't think of how it would be safe. Also, an admin can always background
>>> the task that invokes the dump if they need that particular terminal
>>> back. So, this looks more like a management tool problem to solve, if
>>> anything.
>>>
>>>>
>>>> As for the goal itself... People also tend to cope with *kdump* being
>>>> slow on physical machines.
>>>>
>>>> My recommendation would be to use the dump compression feature
>>>> (especially lzo and snappy). In my experience, even when people are
>>>> aware of their existence, they don't always realize that shrinking the
>>>> dump file size by a given factor also shrinks the dumping *time* by the
>>>> same factor, provided that the dumping process remains IO-bound even in
>>>> the compressed case.
>>>>
>>>> Which it should, assuming a "very slow storage" -- lzo and snappy are
>>>> very CPU-efficient.
>>>
>>> This has been my experience, i.e. using lzo or snappy tends to be much,
>>> much faster.
>>
>> Sorry that I am not the daily user of dump-guest-memory, so I may
>> have not tried to compare how time would save when compression
>> techniques are used. Thanks (Drew & Laszlo) to let me know this.
>> Actually, what I am coping with is the bz:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1193826
> 
> I don't think this mode is very helpful to fix this issue unless there is a 
> way
> to query the dump progress.

Ah, so all it is about is progress information for the interactive user.
I think that should be doable with periodic messages written to the
monitor. I can think of some well-placed error_printf() calls that
report progress info to the monitor, after every N megabytes of guest
RAM processed (or something similar), or perhaps even a new QEMU event.

I think the libvirt and QMP developers should be able to help with this.

> 
>>
>> I just feel like it would be nice to offer something extra, when
>> people are using the stdio monitor, they could have another choice
>> when dump. Also, this is my first patch to QEMU. That's all I
>> thought about.
>>
>> Thanks you all (especially Drew and Laszlo) for leaving mass review
>> comments. After knowing that more than one of you would suggest not
>> taking the risk comparing to the feature it brings, I'd totally
>> agree to drop this patch.

I think the patch should be dropped, and periodic progress reports
should be emitted from within the dump loops that do the heavy lifting.

For the ELF format dumps, that loop appears to reside in dump_iterate()
[dump.c].

For the compressed format dumps, the loop seems to live in
write_dump_pages() [dump.c].

Thanks!
Laszlo

>>
>> Thanks.
>> Peter
>>
>>>
>>> Thanks,
>>> drew
>>>
>>