linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Baoquan He <bhe@redhat.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Colin King <colin.king@canonical.com>,
	Jonathan Corbet <corbet@lwn.net>,
	dyoung@redhat.com, Frederic Weisbecker <frederic@kernel.org>,
	gpiccoli@canonical.com, john.p.donnelly@oracle.com,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Kees Cook <keescook@chromium.org>, Linux-MM <linux-mm@kvack.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Ingo Molnar <mingo@kernel.org>,
	mm-commits@vger.kernel.org,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mike Rapoport <rppt@kernel.org>,
	saeed.mirzamohammadi@oracle.com,
	Sami Tolvanen <samitolvanen@google.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vivek Goyal <vgoyal@redhat.com>,
	yifeifz2@illinois.edu, Hari Bathini <hbathini@linux.ibm.com>,
	piliu@redhat.com, kasong@redhat.com
Subject: Re: [patch 48/91] kernel/crash_core: add crashkernel=auto for vmcore creation
Date: Sat, 8 May 2021 11:29:36 +0800	[thread overview]
Message-ID: <20210508032936.GC23668@MiWiFi-R3L-srv> (raw)
In-Reply-To: <20210508031309.GD2834@localhost.localdomain>

Add Kairui to CC since he is taking care of the crashkernel=auto code in
our Distros.

On 05/08/21 at 11:13am, Baoquan He wrote:
> Hi Linus,
> 
> On 05/07/21 at 12:25am, Linus Torvalds wrote:
> > On Thu, May 6, 2021 at 6:04 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> > >
> > > From: Saeed Mirzamohammadi <saeed.mirzamohammadi@oracle.com>
> > > Subject: kernel/crash_core: add crashkernel=auto for vmcore creation
> > >
> > > This adds crashkernel=auto feature to configure reserved memory for vmcore
> > > creation.  CONFIG_CRASH_AUTO_STR is defined to be set for different kernel
> > > distributions and different archs based on their needs.
> 
> 
> > 
> > Ugh. I didn't realize how nasty this was until after I'd applied this patch.
> > 
> > I'm going to drop this patch, because the Kconfig thing for it is an
> > unmitigated mess. I was confused by the question, and then the help
> > message was actively misleading.
> > 
> > This is wrong for so many reasons:
> > 
> >  - this is a classic case of "you shouldn't ask a user this".
> > 
> >    The question makes no sense to any normal person, it certainly
> > didn't to me. Don't ask questions that don't have sane answers.
> > 
> >  - the config help text is actively misleading, and claims that the
> > option is about how much memory is reserved for a crash kernel
> > 
> >    Not so. It's the default string for when somebody uses "crashkernel=auto"
> 
> Sorry for the confusion, we should have been more careful to reivew and
> add the commit log and kernel config description.
> > 
> >  - this shouldn't be a config option at all, it's clearly a distro
> > setting, and should be on the kernel command line with the other
> > distro settings.
> 
> Don't know kernel config is disliked sometime, will remember it in the
> future and more cautiously to add. 
> 
> Crashkernel=auto exists in our distros for many years, and as David
> mentioned in other thread, we have been trying to adding rashkernel=auto
> support into upstream. We pursue crashkernel=auto being added to upstream
> because:
> 
> 1) Empirical value is given to user by default;
> 
> It was required by customer originally, now has been an important part
> of kdump feature and supported in several main ARCHes. With crashkernel=auto,
> people w/o much knowledge of kdump details can use kdump to debug. Distros
> can provide the suggested values with crashkernel=auto which are got by
> investigation, analysis and tested widely on test environment. 
> 
> 2) Cover corner case/special case;
> 
> In some cases, kernel may need extra memory to handle, kdump kernel is
> not exceptional. E.g when sme/sev enabled, SWIOTLB will be enabled
> necessarily, even in kdump kernel. (Below sme/sev related commits for
> reference). Then extra 64M need be reserved for crashkernel. User
> doesn't need to know this, we already have done it for them.
> 
> commit c7753208a94c ("x86, swiotlb: Add memory encryption support")
> commit aba2d9a6385a ("iommu/amd: Do not disable SWIOTLB if SME is active")
> commit d7b417fa08d1 ("x86/mm: Add DMA support for SEV memory encryption")
> 
> We are eager to push crashkernel=auto to upstream becasue of our
> UPSTREAM FIRST rule. Since it has been in RHEL for many years, each time
> a new RHEL main release anchor a upstream kernel release and is prepared,
> these crashkernel=auto RHEL-only patches need be reviewed inside Redhat,
> then we will be questioned and challenged why they are not in upstream.
> 
> As for how to implement crashkernel=auto, we have tried several ways.
> 
> 1) Add into kernel command line
> 
> The suggested value need be stored in user space if added into kernel
> command line, then added into kernel. This makes the suggested value
> separated from kernel itself. It's not what we expect to see. Because
> the suggested crashkernel value is strongly related to distros release.
> We could adjust the value between sub-releases of kernel because of
> of kernel change. Adding them into kernel command line make us lose the
> track of them in kernel.
> 
> 2) Add a weak generic function and several arch dependent functions
> 3) Hardcode values in __parse_crashkernel()
> 
> Method 2) is taken in our RHEL7, 3) is used in RHEL8, RHEL-only patches
> add them. If we try to push them into upstram, any later value
> adjustment need a upstream patch posting. Otherwise, RHEL-only patch
> need be introduced again, Redhat internal reviewer will challenge us
> again. (Put the value hard coding pieces at bottom for reference).
> 
> 4) Add kernel config to add default value
> 
> It's done in this patch. With the kernel config CRASH_AUTO_STR, Distros can
> add default value, and adjust it anytime in the future w/o bothering
> upstream. If crashkernel=auto is specified, only below 3 LOC added, to
> go to parse the CONFIG_CRASH_AUTO_STR directly.
> 
> @@ -250,6 +251,12 @@ static int __init __parse_crashkernel(ch
>         if (suffix)
>                 return parse_crashkernel_suffix(ck_cmdline, crash_size,
>                                 suffix);
> +#ifdef CONFIG_CRASH_AUTO_STR
> +       if (strncmp(ck_cmdline, "auto", 4) == 0) {
> +               ck_cmdline = CONFIG_CRASH_AUTO_STR;
> +               pr_info("Using crashkernel=auto, the size chosen is a best effort estimation.\n");
> +       }
> +#endif
> 
> 
> Before this, we don't know Saeed Mirzamohammadi, the patch author. He
> could experience the same torture. We were wild with joy when noticing
> his patch. We were planning to launch new round of post to add
> crashkernel=auto, kernel config is our final option too. We could be too
> happy to forget polishing the commit log.
> 
> Not sure if I make myself clear. Basically, we expect crashkernel=auto
> to be added in upstream kernel. About how to implement it in kernel, we would
> like to hear upstream people's suggestion.
> 
> Thanks
> Baoquan
> 
> 
> Hard code crashkernel=auto values in __parse_crashkernel()
> ===========================================================
> static int __init __parse_crashkernel(char *cmdline,
>                              unsigned long long system_ram,
>                              unsigned long long *crash_size,
>                              unsigned long long *crash_base,
>                              const char *name,
>                              const char *suffix)
> {
> ......
>         if (strncmp(ck_cmdline, "auto", 4) == 0) {
> #if defined(CONFIG_X86_64) || defined(CONFIG_S390)
>                 ck_cmdline = "1G-4G:160M,4G-64G:192M,64G-1T:256M,1T-:512M";
> #elif defined(CONFIG_ARM64)
>                 ck_cmdline = "2G-:448M";
> #elif defined(CONFIG_PPC64)
>                 char *fadump_cmdline;
> 
>                 fadump_cmdline = get_last_crashkernel(cmdline, "fadump=", NULL);
>                 fadump_cmdline = fadump_cmdline ?
>                                 fadump_cmdline + strlen("fadump=") : NULL;
>                 if (!fadump_cmdline || (strncmp(fadump_cmdline, "off", 3) == 0))
>                         ck_cmdline = "2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G";
>                 else
>                         ck_cmdline = "4G-16G:768M,16G-64G:1G,64G-128G:2G,128G-1T:4G,1T-2T:6G,2T-4T:12G,4T-8T:20G,8T-16T:36G,16T-32T:64G,32T-64T:128G,64T-:180G";
> #endif
>                 pr_info("Using crashkernel=auto, the size chosen is a best effort estimation.\n");
>         }
> 
> ......
> }
> ==================================================================
> 
> 
> 
> 



  reply	other threads:[~2021-05-08  3:29 UTC|newest]

Thread overview: 119+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-07  1:01 incoming Andrew Morton
2021-05-07  1:02 ` [patch 01/91] alpha: eliminate old-style function definitions Andrew Morton
2021-05-07  1:02 ` [patch 02/91] alpha: csum_partial_copy.c: add function prototypes from <net/checksum.h> Andrew Morton
2021-05-07  1:02 ` [patch 03/91] fs/proc/generic.c: fix incorrect pde_is_permanent check Andrew Morton
2021-05-07  1:02 ` [patch 04/91] proc: save LOC in __xlate_proc_name() Andrew Morton
2021-05-07  2:24   ` Linus Torvalds
2021-05-07  1:02 ` [patch 05/91] proc: mandate ->proc_lseek in "struct proc_ops" Andrew Morton
2021-05-07  1:02 ` [patch 06/91] proc: delete redundant subset=pid check Andrew Morton
2021-05-07  1:02 ` [patch 07/91] selftests: proc: test subset=pid Andrew Morton
2021-05-07  1:02 ` [patch 08/91] proc/sysctl: fix function name error in comments Andrew Morton
2021-05-07  1:02 ` [patch 09/91] include: remove pagemap.h from blkdev.h Andrew Morton
2021-05-07  1:02 ` [patch 10/91] kernel.h: drop inclusion in bitmap.h Andrew Morton
2021-05-07  1:02 ` [patch 11/91] linux/profile.h: remove unnecessary declaration Andrew Morton
2021-05-07  1:02 ` [patch 12/91] kernel/async.c: fix pr_debug statement Andrew Morton
2021-05-07  1:02 ` [patch 13/91] kernel/cred.c: make init_groups static Andrew Morton
2021-05-07  1:02 ` [patch 14/91] tools: disable -Wno-type-limits Andrew Morton
2021-05-07  1:02 ` [patch 15/91] tools: bitmap: sync function declarations with the kernel Andrew Morton
2021-05-07  1:02 ` [patch 16/91] tools: sync BITMAP_LAST_WORD_MASK() macro " Andrew Morton
2021-05-07  1:02 ` [patch 17/91] arch: rearrange headers inclusion order in asm/bitops for m68k, sh and h8300 Andrew Morton
2021-05-07  1:02 ` [patch 18/91] lib: extend the scope of small_const_nbits() macro Andrew Morton
2021-05-07  1:03 ` [patch 19/91] tools: sync small_const_nbits() macro with the kernel Andrew Morton
2021-05-07  1:03 ` [patch 20/91] lib: inline _find_next_bit() wrappers Andrew Morton
2021-05-07  1:03 ` [patch 21/91] tools: sync find_next_bit implementation Andrew Morton
2021-05-07  1:03 ` [patch 22/91] lib: add fast path for find_next_*_bit() Andrew Morton
2021-05-07  1:03 ` [patch 23/91] lib: add fast path for find_first_*_bit() and find_last_bit() Andrew Morton
2021-05-07  1:03 ` [patch 24/91] tools: sync lib/find_bit implementation Andrew Morton
2021-05-07  1:03 ` [patch 25/91] MAINTAINERS: add entry for the bitmap API Andrew Morton
2021-05-07  1:03 ` [patch 26/91] lib/bch.c: fix a typo in the file bch.c Andrew Morton
2021-05-07  1:03 ` [patch 27/91] lib: fix inconsistent indenting in process_bit1() Andrew Morton
2021-05-07  1:03 ` [patch 28/91] lib/list_sort.c: fix typo in function description Andrew Morton
2021-05-07  1:03 ` [patch 29/91] lib/genalloc.c: fix a typo Andrew Morton
2021-05-07  1:03 ` [patch 30/91] lib: crc8: pointer to data block should be const Andrew Morton
2021-05-07  1:03 ` [patch 31/91] lib: stackdepot: turn depot_lock spinlock to raw_spinlock Andrew Morton
2021-05-07  1:03 ` [patch 32/91] lib/percpu_counter: tame kernel-doc compile warning Andrew Morton
2021-05-07  1:03 ` [patch 33/91] lib/genalloc: add parameter description to fix doc " Andrew Morton
2021-05-07  1:03 ` [patch 34/91] lib: parser: clean up kernel-doc Andrew Morton
2021-05-07  1:03 ` [patch 35/91] include/linux/compat.h: remove unneeded declaration from COMPAT_SYSCALL_DEFINEx() Andrew Morton
2021-05-07  1:03 ` [patch 36/91] checkpatch: warn when missing newline in return sysfs_emit() formats Andrew Morton
2021-05-07  1:03 ` [patch 37/91] checkpatch: exclude four preprocessor sub-expressions from MACRO_ARG_REUSE Andrew Morton
2021-05-07  1:04 ` [patch 38/91] checkpatch: improve ALLOC_ARRAY_ARGS test Andrew Morton
2021-05-07  1:04 ` [patch 39/91] kselftest: introduce new epoll test case Andrew Morton
2021-05-07  1:04 ` [patch 40/91] fs/epoll: restore waking from ep_done_scan() Andrew Morton
2021-05-07  1:04 ` [patch 41/91] isofs: fix fall-through warnings for Clang Andrew Morton
2021-05-07  1:04 ` [patch 42/91] fs/nilfs2: fix misspellings using codespell tool Andrew Morton
2021-05-07  1:04 ` [patch 43/91] nilfs2: fix typos in comments Andrew Morton
2021-05-07  1:04 ` [patch 44/91] hpfs: replace one-element array with flexible-array member Andrew Morton
2021-05-07  1:04 ` [patch 45/91] do_wait: make PIDTYPE_PID case O(1) instead of O(n) Andrew Morton
2021-05-07  1:04 ` [patch 46/91] kernel/fork.c: simplify copy_mm() Andrew Morton
2021-05-07  1:04 ` [patch 47/91] kernel/fork.c: fix typos Andrew Morton
2021-05-07  1:04 ` [patch 48/91] kernel/crash_core: add crashkernel=auto for vmcore creation Andrew Morton
2021-05-07  7:25   ` Linus Torvalds
2021-05-08  3:13     ` Baoquan He
2021-05-08  3:29       ` Baoquan He [this message]
2021-05-07  8:16   ` David Hildenbrand
2021-05-08  8:51     ` Baoquan He
2021-05-08  9:22       ` David Hildenbrand
2021-05-10  4:53         ` Baoquan He
2021-05-10  8:32           ` David Hildenbrand
2021-05-10 10:43             ` Baoquan He
2021-05-10 11:01               ` David Hildenbrand
2021-05-10 11:44                 ` Dave Young
2021-05-10 11:56                   ` David Hildenbrand
2021-05-11 13:36                     ` Baoquan He
2021-05-11 16:31                       ` Mike Rapoport
2021-05-11 17:07                         ` David Hildenbrand
2021-05-12 14:51                           ` Baoquan He
2021-05-12 15:07                             ` David Hildenbrand
2021-05-13  5:04                               ` Baoquan He
2021-05-12 19:03                             ` Kairui Song
2021-05-17  8:22                             ` David Hildenbrand
2021-05-18  8:49                               ` Baoquan He
2021-05-18  8:51                                 ` David Hildenbrand
2021-05-18  9:24                                   ` Dave Young
2021-05-12 14:13                         ` Baoquan He
2021-05-12  7:42                     ` Dave Young
2021-05-07  1:04 ` [patch 49/91] kexec: add kexec reboot string Andrew Morton
2021-05-07  1:04 ` [patch 50/91] kernel: kexec_file: fix error return code of kexec_calculate_store_digests() Andrew Morton
2021-05-07  1:04 ` [patch 51/91] kexec: dump kmessage before machine_kexec Andrew Morton
2021-05-07  1:04 ` [patch 52/91] gcov: combine common code Andrew Morton
2021-05-07  1:04 ` [patch 53/91] gcov: simplify buffer allocation Andrew Morton
2021-05-07  1:04 ` [patch 54/91] gcov: use kvmalloc() Andrew Morton
2021-05-07  1:04 ` [patch 55/91] gcov: clang: drop support for clang-10 and older Andrew Morton
2021-05-07  1:04 ` [patch 56/91] smp: kernel/panic.c - silence warnings Andrew Morton
2021-05-07  1:05 ` [patch 57/91] delayacct: clear right task's flag after blkio completes Andrew Morton
2021-05-07  1:05 ` [patch 58/91] gdb: lx-symbols: store the abspath() Andrew Morton
2021-05-07  1:05 ` [patch 59/91] scripts/gdb: document lx_current is only supported by x86 Andrew Morton
2021-05-07  1:05 ` [patch 60/91] scripts/gdb: add lx_current support for arm64 Andrew Morton
2021-05-07  1:05 ` [patch 61/91] kernel/resource: make walk_system_ram_res() find all busy IORESOURCE_SYSTEM_RAM resources Andrew Morton
2021-05-07  1:05 ` [patch 62/91] kernel/resource: make walk_mem_res() find all busy IORESOURCE_MEM resources Andrew Morton
2021-05-07  1:05 ` [patch 63/91] kernel/resource: remove first_lvl / siblings_only logic Andrew Morton
2021-05-07  1:05 ` [patch 64/91] kernel/resource: allow region_intersects users to hold resource_lock Andrew Morton
2021-05-07  1:05 ` [patch 65/91] kernel/resource: refactor __request_region to allow external locking Andrew Morton
2021-05-07  1:05 ` [patch 66/91] kernel/resource: fix locking in request_free_mem_region Andrew Morton
2021-05-07  1:05 ` [patch 67/91] selftests: remove duplicate include Andrew Morton
2021-05-07  1:05 ` [patch 68/91] kernel/async.c: stop guarding pr_debug() statements Andrew Morton
2021-05-07  1:05 ` [patch 69/91] kernel/async.c: remove async_unregister_domain() Andrew Morton
2021-05-07  1:05 ` [patch 70/91] init/initramfs.c: do unpacking asynchronously Andrew Morton
2021-05-07  1:05 ` [patch 71/91] modules: add CONFIG_MODPROBE_PATH Andrew Morton
2021-05-07  1:05 ` [patch 72/91] ipc/sem.c: mundane typo fixes Andrew Morton
2021-05-07  1:05 ` [patch 73/91] mm: fix some typos and code style problems Andrew Morton
2021-05-07  1:05 ` [patch 74/91] drivers/char: remove /dev/kmem for good Andrew Morton
2021-05-07  1:06 ` [patch 75/91] mm: remove xlate_dev_kmem_ptr() Andrew Morton
2021-05-07  1:06 ` [patch 76/91] mm/vmalloc: remove vwrite() Andrew Morton
2021-05-07  1:06 ` [patch 77/91] arm: print alloc free paths for address in registers Andrew Morton
2021-05-07  1:06 ` [patch 78/91] scripts/spelling.txt: add "overlfow" Andrew Morton
2021-05-07  1:06 ` [patch 79/91] scripts/spelling.txt: add "diabled" typo Andrew Morton
2021-05-07  1:06 ` [patch 80/91] scripts/spelling.txt: add "overflw" Andrew Morton
2021-05-07  1:06 ` [patch 81/91] mm/slab.c: fix spelling mistake "disired" -> "desired" Andrew Morton
2021-05-07  1:06 ` [patch 82/91] include/linux/pgtable.h: few spelling fixes Andrew Morton
2021-05-07  1:06 ` [patch 83/91] kernel/umh.c: fix some spelling mistakes Andrew Morton
2021-05-07  1:06 ` [patch 84/91] kernel/user_namespace.c: fix typos Andrew Morton
2021-05-07  1:06 ` [patch 85/91] kernel/up.c: fix typo Andrew Morton
2021-05-07  1:06 ` [patch 86/91] kernel/sys.c: " Andrew Morton
2021-05-07  1:06 ` [patch 87/91] fs: fat: fix spelling typo of values Andrew Morton
2021-05-07  1:06 ` [patch 88/91] ipc/sem.c: spelling fix Andrew Morton
2021-05-07  1:06 ` [patch 89/91] treewide: remove editor modelines and cruft Andrew Morton
2021-05-07  1:06 ` [patch 90/91] mm: fix typos in comments Andrew Morton
2021-05-07  1:06 ` [patch 91/91] " Andrew Morton
2021-05-07  7:12 ` incoming Linus Torvalds

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=20210508032936.GC23668@MiWiFi-R3L-srv \
    --to=bhe@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=colin.king@canonical.com \
    --cc=corbet@lwn.net \
    --cc=dyoung@redhat.com \
    --cc=frederic@kernel.org \
    --cc=gpiccoli@canonical.com \
    --cc=hbathini@linux.ibm.com \
    --cc=john.p.donnelly@oracle.com \
    --cc=jpoimboe@redhat.com \
    --cc=kasong@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-mm@kvack.org \
    --cc=masahiroy@kernel.org \
    --cc=mchehab+huawei@kernel.org \
    --cc=mike.kravetz@oracle.com \
    --cc=mingo@kernel.org \
    --cc=mm-commits@vger.kernel.org \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=piliu@redhat.com \
    --cc=rdunlap@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=rppt@kernel.org \
    --cc=saeed.mirzamohammadi@oracle.com \
    --cc=samitolvanen@google.com \
    --cc=sboyd@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=vgoyal@redhat.com \
    --cc=yifeifz2@illinois.edu \
    --subject='Re: [patch 48/91] kernel/crash_core: add crashkernel=auto for vmcore creation' \
    /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

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).