| Message ID | 20220227030717.1464-2-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support reserving crashkernel above 4G on arm64 kdump | expand |
On 02/27/22 at 11:07am, Zhen Lei wrote: > The crashkernel=Y,low is an optional command-line option. When it doesn't > exist, kernel will try to allocate minimum required memory below 4G > automatically. Give it a unique error code to distinguish it from other > error scenarios. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > kernel/crash_core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 256cf6db573cd09..4d57c03714f4e13 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, > *crash_base = 0; > > ck_cmdline = get_last_crashkernel(cmdline, name, suffix); > - > if (!ck_cmdline) > - return -EINVAL; > + return -ENOENT; Firstly, I am not sure if '-ENOENT' is a right value to return. From the code comment of ENOENT, it's used for file or dir? #define ENOENT 2 /* No such file or directory */ Secondly, we ever discussed the case including - no crashkernel=,low is provided; - messy code is provied, e.g crashkernel=aaaaaabbbb,low The 2nd one is not handled in this patchset. How about taking the handling into another round of patches. This patchset just adds crashkernel=,high purely. > > ck_cmdline += strlen(name); > > -- > 2.25.1 >
On 03/15/22 at 07:57pm, Baoquan He wrote: > On 02/27/22 at 11:07am, Zhen Lei wrote: > > The crashkernel=Y,low is an optional command-line option. When it doesn't > > exist, kernel will try to allocate minimum required memory below 4G > > automatically. Give it a unique error code to distinguish it from other > > error scenarios. > > > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > > --- > > kernel/crash_core.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > > index 256cf6db573cd09..4d57c03714f4e13 100644 > > --- a/kernel/crash_core.c > > +++ b/kernel/crash_core.c > > @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, > > *crash_base = 0; > > > > ck_cmdline = get_last_crashkernel(cmdline, name, suffix); > > - > > if (!ck_cmdline) > > - return -EINVAL; > > + return -ENOENT; > > Firstly, I am not sure if '-ENOENT' is a right value to return. From the > code comment of ENOENT, it's used for file or dir? > #define ENOENT 2 /* No such file or directory */ > > Secondly, we ever discussed the case including > - no crashkernel=,low is provided; > - messy code is provied, e.g crashkernel=aaaaaabbbb,low Checking the 3rd pach, this is handled. Take back my below words, continue reviewing. > > The 2nd one is not handled in this patchset. How about taking the > handling into another round of patches. This patchset just adds > crashkernel=,high purely. > > > > > ck_cmdline += strlen(name); > > > > -- > > 2.25.1 > > >
On 2022/3/15 20:21, Baoquan He wrote: > On 03/15/22 at 07:57pm, Baoquan He wrote: >> On 02/27/22 at 11:07am, Zhen Lei wrote: >>> The crashkernel=Y,low is an optional command-line option. When it doesn't >>> exist, kernel will try to allocate minimum required memory below 4G >>> automatically. Give it a unique error code to distinguish it from other >>> error scenarios. >>> >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> --- >>> kernel/crash_core.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >>> index 256cf6db573cd09..4d57c03714f4e13 100644 >>> --- a/kernel/crash_core.c >>> +++ b/kernel/crash_core.c >>> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, >>> *crash_base = 0; >>> >>> ck_cmdline = get_last_crashkernel(cmdline, name, suffix); >>> - >>> if (!ck_cmdline) >>> - return -EINVAL; >>> + return -ENOENT; >> >> Firstly, I am not sure if '-ENOENT' is a right value to return. From the >> code comment of ENOENT, it's used for file or dir? >> #define ENOENT 2 /* No such file or directory */ This error code does not return to user mode, so there is no problem. There are a lot of places in the kernel that are used this way. For example: int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg) { if (!cpu_stop_queue_work(cpu, &work)) return -ENOENT; >> >> Secondly, we ever discussed the case including >> - no crashkernel=,low is provided; >> - messy code is provied, e.g crashkernel=aaaaaabbbb,low > > Checking the 3rd pach, this is handled. Take back my below words, > continue reviewing. Yes. > >> >> The 2nd one is not handled in this patchset. How about taking the >> handling into another round of patches. This patchset just adds >> crashkernel=,high purely. >> >>> >>> ck_cmdline += strlen(name); >>> >>> -- >>> 2.25.1 >>> >> > > . >
On 03/15/22 at 09:32pm, Leizhen (ThunderTown) wrote: > > > On 2022/3/15 20:21, Baoquan He wrote: > > On 03/15/22 at 07:57pm, Baoquan He wrote: > >> On 02/27/22 at 11:07am, Zhen Lei wrote: > >>> The crashkernel=Y,low is an optional command-line option. When it doesn't > >>> exist, kernel will try to allocate minimum required memory below 4G > >>> automatically. Give it a unique error code to distinguish it from other > >>> error scenarios. > >>> > >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >>> --- > >>> kernel/crash_core.c | 3 +-- > >>> 1 file changed, 1 insertion(+), 2 deletions(-) > >>> > >>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c > >>> index 256cf6db573cd09..4d57c03714f4e13 100644 > >>> --- a/kernel/crash_core.c > >>> +++ b/kernel/crash_core.c > >>> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, > >>> *crash_base = 0; > >>> > >>> ck_cmdline = get_last_crashkernel(cmdline, name, suffix); > >>> - > >>> if (!ck_cmdline) > >>> - return -EINVAL; > >>> + return -ENOENT; > >> > >> Firstly, I am not sure if '-ENOENT' is a right value to return. From the > >> code comment of ENOENT, it's used for file or dir? > >> #define ENOENT 2 /* No such file or directory */ > > This error code does not return to user mode, so there is no problem. > There are a lot of places in the kernel that are used this way. For example: > > int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg) > { > if (!cpu_stop_queue_work(cpu, &work)) > return -ENOENT; OK, it's fine to me. Thanks for the investigation.
On 02/27/22 at 11:07am, Zhen Lei wrote: > The crashkernel=Y,low is an optional command-line option. When it doesn't > exist, kernel will try to allocate minimum required memory below 4G > automatically. Give it a unique error code to distinguish it from other > error scenarios. This log is a little confusing. __parse_crashkernel() has three callers. - parse_crashkernel() - parse_crashkernel_high() - parse_crashkernel_low() How about tuning the git log as below: ================== According to the current crashkernel=Y,low support in other ARCHes, it's an optional command-line option. When it doesn't exist, kernel will try to allocate minimum required memory below 4G automatically. However, __parse_crashkernel() returns '-EINVAL' for all error cases. It can't distinguish the nonexistent option from invalid option. Change __parse_crashkernel() to return '-ENOENT' for the nonexistent option case. With this change, crashkernel,low memory will take the default value if crashkernel=,low is not specified; while crashkernel reservation will fail and bail out if an invalid option is specified. ================== > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > kernel/crash_core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > kernel/crash_core.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 256cf6db573cd09..4d57c03714f4e13 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, > *crash_base = 0; > > ck_cmdline = get_last_crashkernel(cmdline, name, suffix); > - > if (!ck_cmdline) > - return -EINVAL; > + return -ENOENT; > > ck_cmdline += strlen(name); > > -- > 2.25.1 >
On 2022/3/16 13:39, Baoquan He wrote: > On 02/27/22 at 11:07am, Zhen Lei wrote: >> The crashkernel=Y,low is an optional command-line option. When it doesn't >> exist, kernel will try to allocate minimum required memory below 4G >> automatically. Give it a unique error code to distinguish it from other >> error scenarios. > > This log is a little confusing. __parse_crashkernel() has three callers. > - parse_crashkernel() > - parse_crashkernel_high() > - parse_crashkernel_low() > > How about tuning the git log as below: Sure. Your description is much clearer than mine. > > ================== > According to the current crashkernel=Y,low support in other ARCHes, it's > an optional command-line option. When it doesn't exist, kernel will try > to allocate minimum required memory below 4G automatically. > > However, __parse_crashkernel() returns '-EINVAL' for all error cases. It > can't distinguish the nonexistent option from invalid option. > > Change __parse_crashkernel() to return '-ENOENT' for the nonexistent option > case. With this change, crashkernel,low memory will take the default > value if crashkernel=,low is not specified; while crashkernel reservation > will fail and bail out if an invalid option is specified. > ================== > >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> kernel/crash_core.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> kernel/crash_core.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/kernel/crash_core.c b/kernel/crash_core.c >> index 256cf6db573cd09..4d57c03714f4e13 100644 >> --- a/kernel/crash_core.c >> +++ b/kernel/crash_core.c >> @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, >> *crash_base = 0; >> >> ck_cmdline = get_last_crashkernel(cmdline, name, suffix); >> - >> if (!ck_cmdline) >> - return -EINVAL; >> + return -ENOENT; >> >> ck_cmdline += strlen(name); >> >> -- >> 2.25.1 >> > > . >
diff --git a/kernel/crash_core.c b/kernel/crash_core.c index 256cf6db573cd09..4d57c03714f4e13 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -243,9 +243,8 @@ static int __init __parse_crashkernel(char *cmdline, *crash_base = 0; ck_cmdline = get_last_crashkernel(cmdline, name, suffix); - if (!ck_cmdline) - return -EINVAL; + return -ENOENT; ck_cmdline += strlen(name);
The crashkernel=Y,low is an optional command-line option. When it doesn't exist, kernel will try to allocate minimum required memory below 4G automatically. Give it a unique error code to distinguish it from other error scenarios. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- kernel/crash_core.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)