Re: [Crash-utility] PATCH 00/10] teach crash to work with "live" ramdump
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@OFFSET_1,DUMP_2@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.