From: Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>
To: Anton Blanchard <anton@samba.org>
Cc: Linux Kernel <linux-kernel@vger.kernel.org>,
Milton Miller <miltonm@bga.com>,
Michael Ellerman <michael@ellerman.id.au>,
"Eric W. Biederman" <ebiederm@xmission.com>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [RFC PATCH 02/10] fadump: Reserve the memory for firmware assisted dump.
Date: Tue, 06 Sep 2011 17:29:55 +0530 [thread overview]
Message-ID: <4E660B3B.3070608@linux.vnet.ibm.com> (raw)
In-Reply-To: <20110831141134.590c4f4e@kryten>
Hi Anton,
On 08/31/2011 09:41 AM, Anton Blanchard wrote:
>
> Hi Mahesh,
>
> Just a few comments.
>
>> +#define RMR_START 0x0
>> +#define RMR_END (0x1UL << 28) /* 256 MB */
>
> What if the RMO is bigger than 256MB? Should we be using ppc64_rma_size?
The idea was to have a minimum memory threshold that requires for a
kernel to boot successfully. On some Power systems where RMO is 128MB,
it still requires minimum of 256MB for kernel to boot successfully.
I think we can rename above #defines as BOOT_MEM_START and BOOT_MEM_END
respectively and have BOOT_MEM_END defined as below:
#define BOOT_MEM_END ((ppc64_rma_size < (0x1UL << 28)) ? \
(0x1UL << 28) : ppc64_rma_size)
What do you think?
>
>> +#ifdef DEBUG
>> +#define PREFIX "fadump: "
>> +#define DBG(fmt...) printk(KERN_ERR PREFIX fmt)
>> +#else
>> +#define DBG(fmt...)
>> +#endif
>
> We should use the standard debug macros (pr_debug etc).
Sure will do that.
>
>> +/* Global variable to hold firmware assisted dump configuration info. */
>> +static struct fw_dump fw_dump;
>
> You can remove this comment, especially because the variable isn't global :)
Agree.
>
>> + sections = of_get_flat_dt_prop(node, "ibm,configure-kernel-dump-sizes",
>> + NULL);
>> +
>> + if (!sections)
>> + return 0;
>> +
>> + for (i = 0; i < FW_DUMP_NUM_SECTIONS; i++) {
>> + switch (sections[i].dump_section) {
>> + case FADUMP_CPU_STATE_DATA:
>> + fw_dump.cpu_state_data_size =
>> sections[i].section_size;
>> + break;
>> + case FADUMP_HPTE_REGION:
>> + fw_dump.hpte_region_size =
>> sections[i].section_size;
>> + break;
>> + }
>> + }
>> + return 1;
>> +}
>
> This makes me a bit nervous. We should really get the size of the property
> and use it to iterate through the array. I saw no requirement in the PAPR
> that the array had to be 2 entries long.
>
Agree. Will make the change.
>> +static inline unsigned long calculate_reserve_size(void)
>> +{
>> + unsigned long size;
>> +
>> + /* divide by 20 to get 5% of value */
>> + size = memblock_end_of_DRAM();
>> + do_div(size, 20);
>> +
>> + /* round it down in multiples of 256 */
>> + size = size & ~0x0FFFFFFFUL;
>> +
>> + /* Truncate to memory_limit. We don't want to over reserve
>> the memory.*/
>> + if (memory_limit && size > memory_limit)
>> + size = memory_limit;
>> +
>> + return (size > RMR_END ? size : RMR_END);
>> +}
>
> 5% is pretty aribitrary, that's 400GB on an 8TB box. Also our experience
> with kdump is that 256MB is too small. Is there any reason to scale it
> with memory size? Can we do what kdump does and set it to a single
> value (eg 512MB)?
I have picked up this heuristic from the phyp-assisted dump code. I am
yet to figure out a fool-proof method to calculate the minimum memory
needed for any Power box to successfully boot. Till then, I presume we
can use this heuristic based approach?
While testing these patches on huge power system with 1TB RAM and 896
CPUs, I found that even 512MB is small. Hence setting it to a single
value may not work for all system configuration.
>
> We could override the default with a boot option, which is similar to
> how kdump specifies the region to reserve.
Agree, will work on the change.
Thanks,
-Mahesh.
next prev parent reply other threads:[~2011-09-06 12:00 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-13 18:06 [RFC PATCH 00/10] fadump: Firmware-assisted dump support for Powerpc Mahesh J Salgaonkar
2011-07-13 18:06 ` [RFC PATCH 01/10] fadump: Add documentation for firmware-assisted dump Mahesh J Salgaonkar
2011-07-13 18:06 ` [RFC PATCH 02/10] fadump: Reserve the memory for firmware assisted dump Mahesh J Salgaonkar
2011-08-31 4:11 ` Anton Blanchard
2011-09-06 11:59 ` Mahesh Jagannath Salgaonkar [this message]
2011-07-13 18:07 ` [RFC PATCH 03/10] fadump: Register " Mahesh J Salgaonkar
2011-08-31 4:20 ` Anton Blanchard
2011-09-07 7:20 ` Mahesh J Salgaonkar
2011-09-08 18:34 ` Kumar Gala
2011-07-13 18:07 ` [RFC PATCH 04/10] fadump: Initialize elfcore header and add PT_LOAD program headers Mahesh J Salgaonkar
2011-07-13 18:07 ` [RFC PATCH 05/10] fadump: Convert firmware-assisted cpu state dump data into elf notes Mahesh J Salgaonkar
2011-08-31 4:23 ` Anton Blanchard
2011-07-13 18:07 ` [RFC PATCH 06/10] fadump: Add PT_NOTE program header for vmcoreinfo Mahesh J Salgaonkar
2011-07-13 18:08 ` [RFC PATCH 07/10] fadump: Introduce cleanup routine to invalidate /proc/vmcore Mahesh J Salgaonkar
2011-07-13 18:08 ` [RFC PATCH 08/10] fadump: Invalidate registration and release reserved memory for general use Mahesh J Salgaonkar
2011-07-13 18:08 ` [RFC PATCH 09/10] fadump: Invalidate the fadump registration during machine shutdown Mahesh J Salgaonkar
2011-07-13 18:08 ` [RFC PATCH 10/10] fadump: Introduce config option for firmware assisted dump feature Mahesh J Salgaonkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E660B3B.3070608@linux.vnet.ibm.com \
--to=mahesh@linux.vnet.ibm.com \
--cc=anton@samba.org \
--cc=ebiederm@xmission.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=michael@ellerman.id.au \
--cc=miltonm@bga.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).