Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751176AbdHRKwF (ORCPT ); Fri, 18 Aug 2017 06:52:05 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:37686 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751013AbdHRKwC (ORCPT ); Fri, 18 Aug 2017 06:52:02 -0400 Subject: Re: [PATCH v7 1/4] powerpc/fadump: reduce memory consumption for capture kernel To: Michal Suchanek , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Jonathan Corbet , Jessica Yu , Rusty Russell , Jason Baron , Mahesh J Salgaonkar , Daniel Axtens , Andrew Morton , Thiago Jung Bauermann , Balbir Singh , Nicholas Piggin , Michael Neuling , "Aneesh Kumar K.V" , Christophe Leroy , Scott Wood , "Oliver O'Halloran" , David Howells , "Sylvain 'ythier' Hitier" , Ingo Molnar , Kees Cook , Thomas Gleixner Cc: Baoquan He , linux-doc@vger.kernel.org, Lokesh Vutla , Viresh Kumar , linux-kernel@vger.kernel.org, "Steven Rostedt," , Tejun Heo , Ilya Matveychikov , linuxppc-dev@lists.ozlabs.org References: <78655b3689f6cd0c76b787b8ef2c824ef75a99a5.1503000577.git.msuchanek@suse.de> From: Hari Bathini Date: Fri, 18 Aug 2017 16:20:53 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <78655b3689f6cd0c76b787b8ef2c824ef75a99a5.1503000577.git.msuchanek@suse.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-MML: disable x-cbid: 17081810-0012-0000-0000-00000259EC31 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17081810-0013-0000-0000-000007758A78 Message-Id: <975190aa-fff4-6a1f-70b5-27341cb70f97@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-18_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1708180172 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8855 Lines: 242 Hi Michal, Thanks for the patches. I tried testing with the patches: [ 0.000000] fadump: Firmware-assisted dump is active. [ 0.000000] fadump: Modifying command line to enforce the additional parameters passed through 'fadump_extra_args=' [ 0.000000] fadump: Original command line: BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M "fadump_extra_args=nr_cpus=1 numa=off udev.childern-max=2" rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap [ 0.000000] fadump: Modified command line: BOOT_IMAGE=/vmlinux-4.13.0-rc1-bz155783+ root=/dev/mapper/rhel-root ro crashkernel=2048M fadump=on fadump_reserve_mem=1024M "fadump_extra_args nr_cpus=1 numa=off udev.childern-max=2" rd.lvm.lv=rhel/root rd.lvm.lv=rhel/swap Looks like the quotes are retained not enforcing the parameters... I am yet to test the patches in other scenarios though.. Thanks Hari On Friday 18 August 2017 01:44 AM, Michal Suchanek wrote: > From: Hari Bathini > > With fadump (dump capture) kernel booting like a regular kernel, it needs > almost the same amount of memory to boot as the production kernel, which is > unwarranted for a dump capture kernel. But with no option to disable some > of the unnecessary subsystems in fadump kernel, that much memory is wasted > on fadump, depriving the production kernel of that memory. > > Introduce kernel parameter 'fadump_extra_args=' that would take regular > parameters as a space separated quoted string, to be enforced when fadump > is active. This 'fadump_extra_args=' parameter can be leveraged to pass > parameters like nr_cpus=1, cgroup_disable=memory and numa=off, to disable > unwarranted resources/subsystems. > > Also, ensure the log "Firmware-assisted dump is active" is printed early > in the boot process to put the subsequent fadump messages in context. > > Suggested-by: Michael Ellerman > Signed-off-by: Hari Bathini > Signed-off-by: Michal Suchanek > --- > Changes from v6: > Correct and simplify quote handling. Ideally I would like to extend > parse_args to give the length of the original quoted value to callback. > However, parse_args removes at most one doubel-quote from the start and > one from the end so that is easy to detect. Otherwise all other users > will have to be updated to trash the new argument. > --- > arch/powerpc/include/asm/fadump.h | 2 + > arch/powerpc/kernel/fadump.c | 109 ++++++++++++++++++++++++++++++++++++-- > arch/powerpc/kernel/prom.c | 7 +++ > 3 files changed, 115 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h > index ce88bbe1d809..98ae00943fb3 100644 > --- a/arch/powerpc/include/asm/fadump.h > +++ b/arch/powerpc/include/asm/fadump.h > @@ -208,11 +208,13 @@ extern int early_init_dt_scan_fw_dump(unsigned long node, > const char *uname, int depth, void *data); > extern int fadump_reserve_mem(void); > extern int setup_fadump(void); > +extern void enforce_fadump_extra_args(char *cmdline); > extern int is_fadump_active(void); > extern void crash_fadump(struct pt_regs *, const char *); > extern void fadump_cleanup(void); > > #else /* CONFIG_FA_DUMP */ > +static inline void enforce_fadump_extra_args(char *cmdline) { } > static inline int is_fadump_active(void) { return 0; } > static inline void crash_fadump(struct pt_regs *regs, const char *str) { } > #endif > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index dc0c49cfd90a..a1614d9b8a21 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -78,8 +78,10 @@ int __init early_init_dt_scan_fw_dump(unsigned long node, > * dump data waiting for us. > */ > fdm_active = of_get_flat_dt_prop(node, "ibm,kernel-dump", NULL); > - if (fdm_active) > + if (fdm_active) { > + pr_info("Firmware-assisted dump is active.\n"); > fw_dump.dump_active = 1; > + } > > /* Get the sizes required to store dump data for the firmware provided > * dump sections. > @@ -332,8 +334,11 @@ int __init fadump_reserve_mem(void) > { > unsigned long base, size, memory_boundary; > > - if (!fw_dump.fadump_enabled) > + if (!fw_dump.fadump_enabled) { > + if (fw_dump.dump_active) > + pr_warn("Firmware-assisted dump was active but kernel booted with fadump disabled!\n"); > return 0; > + } > > if (!fw_dump.fadump_supported) { > printk(KERN_INFO "Firmware-assisted dump is not supported on" > @@ -373,7 +378,6 @@ int __init fadump_reserve_mem(void) > memory_boundary = memblock_end_of_DRAM(); > > if (fw_dump.dump_active) { > - printk(KERN_INFO "Firmware-assisted dump is active.\n"); > /* > * If last boot has crashed then reserve all the memory > * above boot_memory_size so that we don't touch it until > @@ -460,6 +464,105 @@ static int __init early_fadump_reserve_mem(char *p) > } > early_param("fadump_reserve_mem", early_fadump_reserve_mem); > > +#define FADUMP_EXTRA_ARGS_PARAM "fadump_extra_args=" > +#define FADUMP_EXTRA_ARGS_LEN (strlen(FADUMP_EXTRA_ARGS_PARAM) - 1) > + > +struct param_info { > + char *cmdline; > + char *tmp_cmdline; > + int shortening; > +}; > + > +static void __init fadump_update_params(struct param_info *param_info, > + char *param, char *val) > +{ > + ptrdiff_t param_offset = param - param_info->tmp_cmdline; > + size_t vallen = val ? strlen(val) : 0; > + char *tgt = param_info->cmdline + param_offset + > + FADUMP_EXTRA_ARGS_LEN - param_info->shortening; > + int shortening = 0; > + > + if (!val) > + return; > + > + /* remove '=' */ > + *tgt++ = ' '; > + > + /* next_arg removes one leading and one trailing '"' */ > + if (*tgt == '"') > + shortening += 1; > + if (*(tgt + vallen + shortening) == '"') > + shortening += 1; > + > + /* remove one leading and one trailing quote if both are present */ > + if ((val[0] == '"') && (val[vallen - 1] == '"')) { > + shortening += 2; > + vallen -= 2; > + val++; > + } > + > + /* some characters were removed - move the trailing part of cmdline */ > + if (shortening) { > + char *src; > + > + strncpy(tgt, val, vallen); > + tgt += vallen; > + src = tgt + shortening; > + memmove(tgt, src, strlen(src) + 1); > + } > + > + param_info->shortening += shortening; > +} > + > +/* > + * Reworks command line parameters and splits 'fadump_extra_args=' param > + * to enforce the parameters passed through it > + */ > +static int __init fadump_rework_cmdline_params(char *param, char *val, > + const char *unused, void *arg) > +{ > + struct param_info *param_info = (struct param_info *)arg; > + > + if (strncmp(param, FADUMP_EXTRA_ARGS_PARAM, > + strlen(FADUMP_EXTRA_ARGS_PARAM) - 1)) > + return 0; > + > + fadump_update_params(param_info, param, val); > + > + return 0; > +} > + > +/* > + * Replace every occurrence of 'fadump_extra_args="param1 param2 param3"' > + * in cmdline with 'fadump_extra_args param1 param2 param3' by stripping > + * off '=' and quotes, if any. This ensures that the additional parameters > + * passed with 'fadump_extra_args=' are enforced. > + */ > +void __init enforce_fadump_extra_args(char *cmdline) > +{ > + static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata; > + static char init_cmdline[COMMAND_LINE_SIZE] __initdata; > + struct param_info param_info; > + > + if (strstr(cmdline, FADUMP_EXTRA_ARGS_PARAM) == NULL) > + return; > + > + pr_info("Modifying command line to enforce the additional parameters passed through 'fadump_extra_args='"); > + > + param_info.cmdline = cmdline; > + param_info.tmp_cmdline = tmp_cmdline; > + param_info.shortening = 0; > + > + strlcpy(init_cmdline, cmdline, COMMAND_LINE_SIZE); > + > + strlcpy(tmp_cmdline, cmdline, COMMAND_LINE_SIZE); > + parse_args("fadump params", tmp_cmdline, NULL, 0, 0, 0, > + ¶m_info, &fadump_rework_cmdline_params); > + > + pr_info("Original command line: %s\n", init_cmdline); > + pr_info("Modified command line: %s\n", cmdline); > +} > + > static int register_fw_dump(struct fadump_mem_struct *fdm) > { > int rc, err; > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index f83056297441..2e6f40217266 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -693,6 +693,13 @@ void __init early_init_devtree(void *params) > of_scan_flat_dt(early_init_dt_scan_root, NULL); > of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL); > > + /* > + * Look for 'fadump_extra_args=' parameter and enfore the additional > + * parameters passed to it if fadump is active. > + */ > + if (is_fadump_active()) > + enforce_fadump_extra_args(boot_command_line); > + > parse_early_param(); > > /* make sure we've parsed cmdline for mem= before this */