Re: [PATCH 0/3] vmcoreinfo support for dump filtering

Andrew Morton
2007-08-22T22:41:20Z

On Wed, 22 Aug 2007 21:08:38 +0900
"Ken'ichi Ohmichi" <oomichi@mxs.nes.nec.co.jp> wrote:

>
> Hi Andrew,
>
> This patch set frees the restriction that makedumpfile users should
> install a vmlinux file (including the debugging information) into
> each system.
>
> makedumpfile command is the dump filtering feature for kdump.
> It creates a small dumpfile by filtering unnecessary pages for the
> analysis. To distinguish unnecessary pages, it needs a vmlinux file
> including the debugging information. These days, the debugging package
> becomes a huge file, and it is hard to install it into each system.
>
> To solve the problem, kdump developers discussed it at lkml and kexec-ml.
> As the result, we reached the conclusion that necessary information
> for dump filtering (called "vmcoreinfo") should be embedded into the
> first kernel file and it should be accessed through /proc/vmcore
> during the second kernel.
> (http://www.uwsg.iu.edu/hypermail/linux/kernel/0707.0/1806.html)
>
> Dan Aloni created the patch set for the above implementation.
> (http://www.uwsg.iu.edu/hypermail/linux/kernel/0707.1/1053.html)
>
> And I updated it for multi architectures and memory models.
> (http://lists.infradead.org/pipermail/kexec/2007-August/000479.html)

so... will this permit us to generate kdump files whcih don't have any
pagecache or anonymous memory in them?

> +void vmcoreinfo_append_str(const char *fmt, ...);

This should have suitable __attribute__s so that the compiler can check its
use. See many examples in include/linux/kernel.h, around line 120.

> +/* vmcoreinfo stuff */
> +unsigned char vmcoreinfo_data[VMCOREINFO_BYTES];
> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
> +unsigned int vmcoreinfo_size = 0;

Please always run scripts/checkpatch.pl against your diffs.

> +unsigned int vmcoreinfo_max_size = sizeof(vmcoreinfo_data);

unsigned int = size_t? Perhaps vmcoreinfo_max_size should have size_t
type?

> +void crash_save_vmcoreinfo(void)
> +{
> + u32 *buf;
> +
> + if (!vmcoreinfo_size)
> + return;
> +
> + vmcoreinfo_append_str("CRASHTIME=%d", xtime.tv_sec);

open-coded access to xtime probably isn't appropriate here. Consider using
get_seconds(). That might be more accurate on tickless kernels, too.

> + buf = (u32 *)vmcoreinfo_note;
> +
> + buf = append_elf_note(buf, VMCOREINFO_NOTE_NAME, 0, vmcoreinfo_data,
> + vmcoreinfo_size);
> +
> + final_note(buf);
> +}
> +
> +}
> +
> +static int __init crash_save_vmcoreinfo_init(void)
> +{
> + vmcoreinfo_append_str("OSRELEASE=%s\n", UTS_RELEASE);
> + vmcoreinfo_append_str("PAGESIZE=%d\n", PAGE_SIZE);

I expect the virtualisation guys would be bothered by an open-coded access
to UTS_RELEASE. I guess it doesn't matter much here, but perhaps it'd be
setting a better example to use init_uts_ns.name.release?