[Crash-utility] PATCH 00/10] teach crash to work with "live" ramdump

[Oleg Nesterov]

On 04/25, Dave Anderson wrote:
>
> > But let me repeat, I believe that crash needs something like 1-7 (and probably
> > more) anyway. Otherwise it can't work with remote LIVE_SYSTEM correctly.
>
> But the ACTIVE() macro has been the standard since forever, external extension modules
> depend upon it, etc., and so I'd strongly prefer to not change it to LOCAL_ACTIVE().

but I didn't change the ACTIVE() macro. My point is, it is not always correct to
use it in live/remote case.

> I'd rather keep the handling of this new facility segregated, maybe create something
> like an ACTIVE_QEMU macro/define that can be plugged in wherever you've modified the
> ACTIVE() callers where ACTIVE() alone is not enough.

Hmm. And this is what I strongly disagree with... Or I misunderstood.

OK. Suppose we add ACTIVE_QEMU() helper. IMO this is a bad idea in any case, the core
code should not even know that this kernel runs under qemu. Nevermind, suppose we have
say

	#define ACTIVE_QEMU() ((pc->flags & LIVE_SYSTEM) && (pc->flags2 && QEMU))

Now what? We need the same 1-7 patches, just LOCAL_ACTIVE() should be replaced
with "ACTIVE() && !QEMU_ACTIVE()".


Let's look at 03/10,

	--- a/kernel.c
	+++ b/kernel.c
	@@ -2900,7 +2900,7 @@ back_trace(struct bt_info *bt)
			return;
		}
	 
	-	if (ACTIVE() && !INSTACK(esp, bt)) {
	+	if (LOCAL_ACTIVE() && !INSTACK(esp, bt)) {
			sprintf(buf, "/proc/%ld", bt->tc->pid); 
			if (!file_exists(buf, NULL))
				error(INFO, "task no longer exists\n");

The usage of ACTIVE() is obviously wrong if this is the live (so that ACTIVE()
is true) but remote kernel. We should not even try to look at /proc files on
the local system in this case.

Or perhaps you mean that ACTIVE_QEMU() should be defined as

	#define ACTIVE_QEMU()	(pc->flags2 & QEMU_LIVE)

? iow, it should not imply ACTIVE() ? This would be even worse, in this case we
would neet to replace almost every ACTIVE() with "ACTIVE() || ACTIVE_QEMU()".
And in fact most of DUMPFILE() users should be updated too. Just for example,
restore_sanity() does

	/*
	 *  Clear the structure cache references -- no-ops if DUMPFILE().
	 */
	clear_task_cache();
	clear_machdep_cache();
	clear_swap_info_cache();
	clear_file_cache();
	clear_dentry_cache();
	clear_inode_cache();
	clear_vma_cache();
	clear_active_set();

yes, and every function above will need the fix.

So I strongly disagree, but perhaps I misunderstood you...

If you meant that its name is ugly, or it should not abuse MEMSRC_LOCAL - I won't
argue. But this is minor.

> > And while I think that 10/10 can be useful by itself (and least for me), this
> > is mostly POC which which allows to test/justify these LOCAL_ACTIVE changes
> > in 1-7.
>
> OK good, so you can keep your stuff completely outside of ramdump.c, and not use
> is_ramdump(), etc., and then place as many of your changes as possible in a new
> file,

OK, I won't argue. And of course I won't insist if you simply don't like this new
feauture, I agree it is not that useful.

> say something like qemu-live.c.

Again, this has almost nothing to do with qemu in particular, but this doesn't
matter.

OK, lets suppose we add this feature... How do you think the command line should
look?

I mean, after this series we can do, say,

	./crash vmlinux raw:DUMP_1 at OFFSET_1,DUMP_2 at OFFSET_2

if we have 2 ramdump's which should be used together. How do you think the new
syntax should look? I am fine either way.

Oleg.