Dave Young <dyo...@redhat.com> writes:
> Hi Eric,
> On 05/23/18 at 10:53am, Eric W. Biederman wrote:
>> Dave Young <dyo...@redhat.com> 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.
Eric
