Hi Eric,
On 05/24/18 at 11:41am, Eric W. Biederman wrote:
> Dave Young <dyoung@xxxxxxxxxx> writes:
>
> > Hi Eric,
> > On 05/23/18 at 10:53am, Eric W. Biederman wrote:
> >> Dave Young <dyoung@xxxxxxxxxx> writes:
> >>
> >> > [snip]
> >> >
> >> >> >
> >> >> > +config CRASHKERNEL_DEFAULT_THRESHOLD_MB
> >> >> > + int "System memory size threshold for kdump memory default reserving"
> >> >> > + depends on CRASH_CORE
> >> >> > + default 0
> >> >> > + help
> >> >> > + CRASHKERNEL_DEFAULT_MB is used as default crashkernel value if
> >> >> > + the system memory size is equal or bigger than the threshold.
> >> >>
> >> >> "the threshold" is rather vague. Can it be clarified?
> >> >>
> >> >> In fact I'm really struggling to understand the logic here....
> >> >>
> >> >>
> >> >> > +config CRASHKERNEL_DEFAULT_MB
> >> >> > + int "Default crashkernel memory size reserved for kdump"
> >> >> > + depends on CRASH_CORE
> >> >> > + default 0
> >> >> > + help
> >> >> > + This is used as the default kdump reserved memory size in MB.
> >> >> > + crashkernel=X kernel cmdline can overwrite this value.
> >> >> > +
> >> >> > config HAVE_IMA_KEXEC
> >> >> > bool
> >> >> >
> >> >> > @@ -143,6 +144,24 @@ static int __init parse_crashkernel_simp
> >> >> > return 0;
> >> >> > }
> >> >> >
> >> >> > +static int __init get_crashkernel_default(unsigned long long system_ram,
> >> >> > + unsigned long long *size)
> >> >> > +{
> >> >> > + unsigned long long sz = CONFIG_CRASHKERNEL_DEFAULT_MB;
> >> >> > + unsigned long long thres = CONFIG_CRASHKERNEL_DEFAULT_THRESHOLD_MB;
> >> >> > +
> >> >> > + thres *= SZ_1M;
> >> >> > + sz *= SZ_1M;
> >> >> > +
> >> >> > + if (sz >= system_ram || system_ram < thres) {
> >> >> > + pr_debug("crashkernel default size can not be used.\n");
> >> >> > + return -EINVAL;
> >> >>
> >> >> In other words,
> >> >>
> >> >> if (system_ram <= CONFIG_CRASHKERNEL_DEFAULT_MB ||
> >> >> system_ram < CONFIG_CRASHKERNEL_DEFAULT_THRESHOLD_MB)
> >> >> fail;
> >> >>
> >> >> yes?
> >> >>
> >> >> How come? What's happening here? Perhaps a (good) explanatory comment
> >> >> is needed. And clearer Kconfig text.
> >> >>
> >> >> All confused :(
> >> >
> >> > Andrew, I tuned it a bit, removed the check of sz >= system_ram, so if
> >> > the size is too large and kernel can not find enough memory it will
> >> > still fail in latter code.
> >> >
> >> > Is below version looks clearer?
> >>
> >> What is the advantage of providing this in a kconfig option rather
> >> than on the kernel command line as we can now?
> >
> > It is not a replacement of the cmdline, this can be a supplement to
> > the crashkernel command line. For a lot of common use cases if we have
> > the auto reservation user just do not need to manually set the cmdline
> > for example on a virtual machine and usual setup (except of the
> > comlicate storage and very large machines). The crashkernel=auto
> > has been used for long time, Red Hat QE tested it on a lot of different
> > lab machines and proved it works well. Kdump usually just works so admin
> > do little work to enable kdump.
> >
> > But the crashkernel=auto implementation has some drawbacks that is it
> > is more like embed policy in the code and it is not flexible like a
> > config option.
>
> Have you considered using the builtin command line aka CONFIG_CMDLINE?
> If as you are reserving a fixed amount of memory as your patch does that
> should be sufficient, and doable without any kernel changes.
Hmm, even in builtin cmdline it is same as a explict used crashkernel=.
If we think from a distribution point of view, it will be hard to
differentiate the builtin provided param and bootloader provided
params. It looks odd to see two crashkernel= when `cat /proc/cmdline`,
it will confuse people and there could cause compatibility problems
because it is explict value visible in kernel cmdline.
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
