8:38 a.m.
----- Original Message -----
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()".
Correct. ACTIVE() is used ~100 times, and in the vast majority of cases, its use
applies to a live QEMU/KVM session. In the few circumstances that it doesn't, then
ACTIVE_QEMU() should be applied so that it's obvious to the maintainer (me), what
the issue is. Who know what "live" mechanism may come about in the future that
may also have its own quirks? I don't want to hide it, but rather make it strikingly
obvious.
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.
Correct. So restrict it meaningfully (to me anyway).
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()".
I agree that there are a handful of circumstances that you have run into where
ACTIVE() may not apply, such as the case where /proc was accessed. But I don't
understand why you say "almost every" instance?
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.
Why? If the target is live, then all of the above should be called as-is. Each
of them returns if the target is a dumpfile.
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.
I just suggested qemu-live.c simply because the facility is for accessing a live
QEMU/KVM instance. Name it whatever you'd like.
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.
I guess I've got some basic misunderstandings here...
If it's a live system, why is necessary to specify RAM offsets?
And if you're just emulating the ramdump facility by first dumping the guest's
memory
into a dumpfile, why isn't it just a ramdump clone?
Dave