[PATCH][RFC] vmcore: Remove saved_max_pfn check

[Magnus Damm]

On Mon, Sep 8, 2008 at 10:04 PM, Vivek Goyal <vgoyal at redhat.com> wrote:
> On Mon, Sep 08, 2008 at 12:21:32PM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm at igel.co.jp>
>>
>> This patch removes the saved_max_pfn check from the /proc/vmcore
>> function read_from_oldmem(). No need to verify, we should be able
>> to just trust that "elfcorehdr=" is correctly passed to the crash
>> kernel on the kernel command line like we do with other parameters.
>>
>> The read_from_oldmem() function in fs/proc/vmcore.c is quite similar
>> to read_from_oldmem() in drivers/char/mem.c, but only in the latter
>> it makes sense to use saved_max_pfn. For oldmem it is used to determine
>> when to stop reading. For vmcore we already have the elf header info
>> pointing out the physical memory regions, no need to pass the end-of-
>> old-memory twice.
>>
>> Removing the saved_max_pfn check from vmcore makes it possible for
>> architectures to skip oldmem but still support crash dump through
>> vmcore - without the need for the old saved_max_pfn cruft.
>>
>> Architectures that want to play safe can do the saved_max_pfn check
>> in copy_oldmem_page(). Not sure why anyone would want to do that,
>> but that's even safer than today - the saved_max_pfn check in vmcore
>> removed by this patch only checks the first page.

Hi Vivek,

> Though I don't feel very strongly for saved_max_pfn check in vmcore.c,
> but at the same time I don't understand what are you gaining by removing
> this check. Any way we are not getting rid of this symbol altogether
> because /dev/oldmem needs it.

Let me try to be a bit more clear. =)

> Is it sh arch for which you want to disable /dev/oldmem and only enable
> /proc/vmcore and hence want to get rid of saved_max_pfn?

Yes, exactly. I could do as powerpc and just pass the saved_max_pfn on
the command line together with the elfcorehdr pointer, but why? For
vmcore we only need the elfcorehdr pointer.

> Though I agree that we should elfcorehdrs but at the same time it does not
> hard doing additional check (We are anyway carrying saved_max_pfn for
> /dev/oldmem). We can always extend current code to check for end page also
> to make sure we are not reading beyond saved_max_pfn.

Maybe it doesn't harm, but it doesn't do any good either. Relying on
saved_max_pfn just requires more architecture specific code. The
existing vmcore.c code can of course be fixed up to handle the end
page, but what is the exact point with using saved_max_pfn in
vmcore.c?

I understand it is needed for oldmem, so I'm not saying that we should
remove the symbol all together.

> How much code is it to set value of saved_max_pfn in sh that you want to
> completely get rid of it. My feeling is that it should be just few lines.

You are correct. We just need to add a kernel command line parameter
and change kexec-tools to pass along that value. Like powerpc does
today. I did of course do just that in my first iteration of crash
support for SuperH and it works just fine. But why should we pass more
information than we actually need?

So it's not a matter of coding effort. It's more about passing just
the information that is needed to the secondary kernel. And if we want
to use vmcore but not oldmem, saved_max_pfn isn't needed. Unless I'm
mistaken that is. =)

> So though I don't feel strongly for saved_max_pfn check  in vmcore.c, at
> the same time I don't see you are gaining anything significant by removing
> it. We can just introduce saved_max_pfn in sh also.

Yep, we could introduce that on SuperH, but pruning code that is not
really needed is a step in the right direction IMO.

Thanks for your comments!

/ magnus