Re: [PATCH v3] arm64: kdump: simplify the reservation behaviour of crashkernel=,high
On 2023/3/3 11:01, Baoquan He wrote:
> On 03/02/23 at 11:32am, Leizhen (ThunderTown) wrote:
> ..
>>> @@ -166,31 +169,51 @@ static void __init reserve_crashkernel(void)
>>> /* User specifies base address explicitly. */
>>> if (crash_base) {
>>> fixed_base = true;
>>> + search_base = crash_base;
>>> crash_max = crash_base + crash_size;
>>> }
>>>
>>> retry:
>>> crash_base = memblock_phys_alloc_range(crash_size, CRASH_ALIGN,
>>> - crash_base, crash_max);
>>> + search_base, crash_max);
>>> if (!crash_base) {
>>> /*
>>> -* If the first attempt was for low memory, fall back to
>>> -* high memory, the minimum required low memory will be
>>> -* reserved later.
>>> +* For crashkernel=size[KMG]@offset[KMG], print out failure
>>> +* message if can't reserve the specified region.
>>> */
>>> - if (!fixed_base && (crash_max == CRASH_ADDR_LOW_MAX)) {
>>> + if (fixed_base) {
>>> + pr_info("crashkernel reservation failed - memory is in
>>> use.\n");
>>
>> How about changing pr_info to pr_warn?
>>
>>> + return;
>>> + }
>>> +
>>> + /*
>>> +* For crashkernel=size[KMG], if the first attempt was for
>>> +* low memory, fall back to high memory, the minimum required
>>> +* low memory will be reserved later.
>>> +*/
>>> + if (!high && crash_max == CRASH_ADDR_LOW_MAX) {
>>> crash_max = CRASH_ADDR_HIGH_MAX;
>>> + search_base = CRASH_ADDR_LOW_MAX;
>>> crash_low_size = DEFAULT_CRASH_KERNEL_LOW_SIZE;
>>> goto retry;
>>> }
>>>
>>> + /*
>>> +* For crashkernel=size[KMG],high, if the first attempt was
>>> +* for high memory, fall back to low memory.
>>> +*/
>>> + if (high && crash_max == CRASH_ADDR_HIGH_MAX) {
>>
>> Adding unlikely to indicate that it is rare would be better.
>>
>> if (unlikely(high && crash_max == CRASH_ADDR_HIGH_MAX))
>
> Rethink about this and checked code in kernel, seems likely|unlikely are
> mostly used in highly frequent execution branch optimize code path, while
> crashkernel resevatoin is one time execution during boot, we may not
> need to bother to add unlikely. What do you think?
OK.
>
>
>>
>>> + crash_max = CRASH_ADDR_LOW_MAX;
>>> + search_base = 0;
>>> + goto retry;
>>> + }
>>> pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
>>> crash_size);
>>> return;
>>> }
>>>
>>> - if ((crash_base > CRASH_ADDR_LOW_MAX - crash_low_size) &&
>>> -crash_low_size && reserve_crashkernel_low(crash_low_size)) {
>>> + if ((crash_base >= CRASH_ADDR_LOW_MAX) && crash_low_size &&
>>> +reserve_crashkernel_low(crash_low_size)) {
>>> memblock_phys_free(crash_base, crash_size);
>>> return;
>>> }
>>>
>>
>> --
>> Regards,
>> Zhen Lei
>>
>
> .
>
--
Regards,
Zhen Lei
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/2] kexec: Default __NR_kexec_file_load is set to undefined
Hi Baoquan, On Tue, 28 Feb 2023 21:32:26 +0800 Baoquan He wrote: > Hi Philipp, > > On 02/27/23 at 04:19pm, Philipp Rudo wrote: > .. > > > diff --git a/kexec/kexec-syscall.h b/kexec/kexec-syscall.h > > > index be6ccd5..ea77936 100644 > > > --- a/kexec/kexec-syscall.h > > > +++ b/kexec/kexec-syscall.h > > > @@ -59,9 +59,7 @@ > > > #endif > > > #endif /*ifndef __NR_kexec_load*/ > > > > > > -#ifdef __arm__ > > > #undef __NR_kexec_file_load > > > -#endif > > > > > > #ifndef __NR_kexec_file_load > > > > I don't think this will work as intended. > > > > On the top of the file sys/syscall.h gets included. In there > > architectures that support kexec_file_load define __NR_kexec_file_load. > > This also means that if an architecture doesn't support kexec_file_load > > __NR_kexec_file_load shouldn't be defined in the first place. Thus I > > suggest that you find out why sys/syscall.h defines > > __NR_kexec_file_load for LoongArch even when the system call is not > > supported and fix it there. > > Checking whether LoongArch has defined __NR_kexec_file_load sounds a > good suggestion. Wondering why we still need add __NR_kexec_file_load > definition in kexec-tools. E.g below s390 kexec_file support you added. > sometime won't be found or __NR_kexec_file_load could be > not defined yet? > > commit d4a948c268272cf37c71be820fb02bf40e56292b > Author: Philipp Rudo > Date: Wed May 16 14:27:18 2018 +0200 > > kexec/s390: Add support for kexec_file_load > To be honest I don't remember why I have added it back then. Most likely I simply copied what others have done before. The benefit in having the extra definition I see is that you don't need to rebuild glibc when you implement the syscall and don't use the generic unistd.h. But that is something only few people should ever encounter. Thanks Philipp ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 1/2] kexec: Default __NR_kexec_file_load is set to undefined
Hi Youling,
On Tue, 28 Feb 2023 10:02:16 +0800
Youling Tang wrote:
> Hi, Philipp
>
> On 02/27/2023 11:19 PM, Philipp Rudo wrote:
> > Hi Youling,
> >
> > On Fri, 24 Feb 2023 17:51:07 +0800
> > Youling Tang wrote:
> >
> >> The initial reason is that after the merger of 29fe5067ed07
> >> ("kexec: make -a the default"), kexec cannot be used on LoongArch,
> >> MIPS .etc architectures. We need to add "-c" for normal use. The
> >> current kexec_file_load system call is not implemented in
> >> architectures such as LoongArch, so it needs to pass kexec_load.
> >> So we need to set __NR_kexec_file_load to undefined in unsupported
> >> architectures. This will return EFALLBACK via
> >> is_kexec_file_load_implemented,
> >> and then via kexec_load.
> >>
> >> Signed-off-by: Youling Tang
> >> ---
> >> kexec/kexec-syscall.h | 2 --
> >> 1 file changed, 2 deletions(-)
> >>
> >> diff --git a/kexec/kexec-syscall.h b/kexec/kexec-syscall.h
> >> index be6ccd5..ea77936 100644
> >> --- a/kexec/kexec-syscall.h
> >> +++ b/kexec/kexec-syscall.h
> >> @@ -59,9 +59,7 @@
> >> #endif
> >> #endif /*ifndef __NR_kexec_load*/
> >>
> >> -#ifdef __arm__
> >> #undef __NR_kexec_file_load
> >> -#endif
> >>
> >> #ifndef __NR_kexec_file_load
> >
> > I don't think this will work as intended.
>
> Works fine in LoongArch after applying this patch.
I believe you. The problem is that it changes the behavior on other
architectures.
> >
> > On the top of the file sys/syscall.h gets included. In there
> > architectures that support kexec_file_load define __NR_kexec_file_load.
> > This also means that if an architecture doesn't support kexec_file_load
> > __NR_kexec_file_load shouldn't be defined in the first place. Thus I
> > suggest that you find out why sys/syscall.h defines
> > __NR_kexec_file_load for LoongArch even when the system call is not
> > supported and fix it there.
> Yes, in the kernel, LoongArch uses the generic syscall number (in
> include/uapi/asm-generic/unistd.h), so __NR_kexec_file_load is
> defined as 294.
Ok, I see. I have expected that it is wrapped in some ifdef
CONFIG_KEXEC_FILE. But apparently that is not the case. Thanks for the
clarification.
> I think the simpler way is to modify it in kexec-tools as follows,
> #ifdef __loongarch__
> #undef __NR_kexec_file_load
> #endif
Yeah, looks like this is the best solution. Only thing you could do is
to merge it with the arm special handling. I.e. so it reads
#if defined(__arm__) || defined(__longarch__)
#undef __NR_kexec_file_load
#endif
But that is more a matter of taste. So you and Simon need to agree what
you like more.
Thanks
Philipp
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] ppc64: Add elf-ppc64 file types/options and an arch specific flag to man page
On Wed, Mar 01, 2023 at 03:58:19AM -0500, Gautam Menghani wrote: > Document the elf-ppc64 file options and the "--dt-no-old-root" arch > specific flag in the man page. > > Signed-off-by: Gautam Menghani Thanks, applied. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC][nvdimm][crash] pmem memmap dump support
On 03/03/23 at 02:27am, lizhij...@fujitsu.com wrote: > > > On 01/03/2023 16:17, Baoquan He wrote: > > On 03/01/23 at 06:27am, lizhij...@fujitsu.com wrote: > > .. > >> Hi Baoquan > >> > >> Greatly appreciate your feedback. > >> > >> > >>> 1) In kernel side, export info of pmem meta data; > >>> 2) in makedumpfile size, add an option to specify if we want to dump > >>> pmem meta data; An option or in dump level? > >> > >> Yes, I'm working on these 2 step. > >> > >>> 3) In glue script, detect and warn if pmem data is in pmem and wanted, > >>> and dump target is the same pmem. > >>> > >> > >> The 'glue script' means the scirpt like '/usr/bin/kdump.sh' in 2nd kernel? > >> That would be an option, > >> Shall we abort this dump if "pmem data is in pmem and wanted, and dump > >> target is the same pmem" ? > > > > Guess you are saying scripts in RHEL/centos/fedora, and yes if I guess > > righ. Other distros could have different scripts. For kdump, we need > > load kdump kernel/initramfs in advance, then wait to capture any crash. > > When we load, we can detect and check whether the environment and > > setup is expected. If not, we can warn or error out message to users. > > > IIUC, take fedora for example, > T1: in 1st kernel, kdump.service(/usr/bin/kdumpctl) will do a sanity check > before loading kernel and initramfs. > In this moment, as you said "we can detect and check whether the > environment and setup is expected. If not, > we can warn or error out message to users." > I think we should abort the kdump service if "pmem data is in pmem and > wanted, and dump target is the same pmem". > For OS administrators, they could either change the dump target or > disable the pmem metadadata dump to make > kdump.service work again. > > But kdump.service is distros independent, some OS administrators will use > `kexec` command directly instead of service/script helpers. Yeah, we can add document in kernel or somewhere else that dumping to pmem is dangerous, especially when we want to dump pmem meta. People who dare use kexec command directly, should handle it by her/his own. > > > We don't need to do the checking until crash is triggered, then decide > > to abort the dump or not. > > T2: in 2nd kernel, since 1st kernel's glue scripts vary by distribution, we > have to do the sanity check again to decide > to abort the dump or not. Hmm, we may not need to worry about that. kernel just need to do its own business, not touching pmem data during kdump jumping and booting, and provide way to allow makedumpfile to read out pmem meta. Anything else should be taken care of by user or distros.
