| Message ID | 20240708133348.3592667-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ARM: Use generic interface to simplify crashkernel reservation | expand |
On 07/08/24 at 09:33pm, Jinjie Ruan wrote: > Currently, x86, arm64, riscv and loongarch has been switched to generic > crashkernel reservation. Also use generic interface to simplify crashkernel > reservation for arm32, and fix two bugs by the way. I am not sure if this is a good idea. I added the generic reservation itnerfaces for ARCH which support crashkernel=,high|low and normal crashkernel reservation, with this, the code can be simplified a lot. However, arm32 doesn't support crashkernel=,high, I am not sure if it's worth taking the change, most importantly, if it will cause misunderstanding or misoperation. Thanks Baoquan
On 2024/7/9 17:29, Baoquan He wrote: > On 07/08/24 at 09:33pm, Jinjie Ruan wrote: >> Currently, x86, arm64, riscv and loongarch has been switched to generic >> crashkernel reservation. Also use generic interface to simplify crashkernel >> reservation for arm32, and fix two bugs by the way. > > I am not sure if this is a good idea. I added the generic reservation > itnerfaces for ARCH which support crashkernel=,high|low and normal > crashkernel reservation, with this, the code can be simplified a lot. > However, arm32 doesn't support crashkernel=,high, I am not sure if it's > worth taking the change, most importantly, if it will cause > misunderstanding or misoperation. Yes, arm32 doesn't support crashkernel=,high. However, a little enhancement to the generic code (please see the first patch), the generic reservation interfaces can also be applicable to architectures that do not support "high" such as arm32, and it can also simplify the code (please see the third patch). > > Thanks > Baoquan > >
On 07/09/24 at 05:50pm, Jinjie Ruan wrote: > > > On 2024/7/9 17:29, Baoquan He wrote: > > On 07/08/24 at 09:33pm, Jinjie Ruan wrote: > >> Currently, x86, arm64, riscv and loongarch has been switched to generic > >> crashkernel reservation. Also use generic interface to simplify crashkernel > >> reservation for arm32, and fix two bugs by the way. > > > > I am not sure if this is a good idea. I added the generic reservation > > itnerfaces for ARCH which support crashkernel=,high|low and normal > > crashkernel reservation, with this, the code can be simplified a lot. > > However, arm32 doesn't support crashkernel=,high, I am not sure if it's > > worth taking the change, most importantly, if it will cause > > misunderstanding or misoperation. > > Yes, arm32 doesn't support crashkernel=,high. > > However, a little enhancement to the generic code (please see the first > patch), the generic reservation interfaces can also be applicable to > architectures that do not support "high" such as arm32, and it can also > simplify the code (please see the third patch). Yeah, I can see the code is simplified. When you specified 'crashkernel=xM,high', do you think what should be warn out? Because it's an unsupported syntax on arm32, we should do something to print out appropriate message.
On 2024/7/9 18:39, Baoquan He wrote: > On 07/09/24 at 05:50pm, Jinjie Ruan wrote: >> >> >> On 2024/7/9 17:29, Baoquan He wrote: >>> On 07/08/24 at 09:33pm, Jinjie Ruan wrote: >>>> Currently, x86, arm64, riscv and loongarch has been switched to generic >>>> crashkernel reservation. Also use generic interface to simplify crashkernel >>>> reservation for arm32, and fix two bugs by the way. >>> >>> I am not sure if this is a good idea. I added the generic reservation >>> itnerfaces for ARCH which support crashkernel=,high|low and normal >>> crashkernel reservation, with this, the code can be simplified a lot. >>> However, arm32 doesn't support crashkernel=,high, I am not sure if it's >>> worth taking the change, most importantly, if it will cause >>> misunderstanding or misoperation. >> >> Yes, arm32 doesn't support crashkernel=,high. >> >> However, a little enhancement to the generic code (please see the first >> patch), the generic reservation interfaces can also be applicable to >> architectures that do not support "high" such as arm32, and it can also >> simplify the code (please see the third patch). > > Yeah, I can see the code is simplified. When you specified > 'crashkernel=xM,high', do you think what should be warn out? Because > it's an unsupported syntax on arm32, we should do something to print out > appropriate message. Yes, you are right! In this patch it will print "crashkernel high memory reservation failed." message and out for arm32 if you specify 'crashkernel=xM,high because "CRASH_ADDR_LOW_MAX" and "CRASH_ADDR_HIGH_MAX" is identical for arm32. And it should also warn out for other similar architecture. > >
On 07/09/24 at 07:06pm, Jinjie Ruan wrote: > > > On 2024/7/9 18:39, Baoquan He wrote: > > On 07/09/24 at 05:50pm, Jinjie Ruan wrote: > >> > >> > >> On 2024/7/9 17:29, Baoquan He wrote: > >>> On 07/08/24 at 09:33pm, Jinjie Ruan wrote: > >>>> Currently, x86, arm64, riscv and loongarch has been switched to generic > >>>> crashkernel reservation. Also use generic interface to simplify crashkernel > >>>> reservation for arm32, and fix two bugs by the way. > >>> > >>> I am not sure if this is a good idea. I added the generic reservation > >>> itnerfaces for ARCH which support crashkernel=,high|low and normal > >>> crashkernel reservation, with this, the code can be simplified a lot. > >>> However, arm32 doesn't support crashkernel=,high, I am not sure if it's > >>> worth taking the change, most importantly, if it will cause > >>> misunderstanding or misoperation. > >> > >> Yes, arm32 doesn't support crashkernel=,high. > >> > >> However, a little enhancement to the generic code (please see the first > >> patch), the generic reservation interfaces can also be applicable to > >> architectures that do not support "high" such as arm32, and it can also > >> simplify the code (please see the third patch). > > > > Yeah, I can see the code is simplified. When you specified > > 'crashkernel=xM,high', do you think what should be warn out? Because > > it's an unsupported syntax on arm32, we should do something to print out > > appropriate message. > > Yes, you are right! In this patch it will print "crashkernel high memory > reservation failed." message and out for arm32 if you specify That message may mislead people to believe crashkernel=,high is supported but reservation is failed, then a bug need be filed for this? We may expect a message telling this syntax is not supported on this ARCH. > 'crashkernel=xM,high because "CRASH_ADDR_LOW_MAX" and > "CRASH_ADDR_HIGH_MAX" is identical for arm32. And it should also warn > out for other similar architecture. > > > > > > >
On 2024/7/9 22:06, Baoquan He wrote: > On 07/09/24 at 07:06pm, Jinjie Ruan wrote: >> >> >> On 2024/7/9 18:39, Baoquan He wrote: >>> On 07/09/24 at 05:50pm, Jinjie Ruan wrote: >>>> >>>> >>>> On 2024/7/9 17:29, Baoquan He wrote: >>>>> On 07/08/24 at 09:33pm, Jinjie Ruan wrote: >>>>>> Currently, x86, arm64, riscv and loongarch has been switched to generic >>>>>> crashkernel reservation. Also use generic interface to simplify crashkernel >>>>>> reservation for arm32, and fix two bugs by the way. >>>>> >>>>> I am not sure if this is a good idea. I added the generic reservation >>>>> itnerfaces for ARCH which support crashkernel=,high|low and normal >>>>> crashkernel reservation, with this, the code can be simplified a lot. >>>>> However, arm32 doesn't support crashkernel=,high, I am not sure if it's >>>>> worth taking the change, most importantly, if it will cause >>>>> misunderstanding or misoperation. >>>> >>>> Yes, arm32 doesn't support crashkernel=,high. >>>> >>>> However, a little enhancement to the generic code (please see the first >>>> patch), the generic reservation interfaces can also be applicable to >>>> architectures that do not support "high" such as arm32, and it can also >>>> simplify the code (please see the third patch). >>> >>> Yeah, I can see the code is simplified. When you specified >>> 'crashkernel=xM,high', do you think what should be warn out? Because >>> it's an unsupported syntax on arm32, we should do something to print out >>> appropriate message. >> >> Yes, you are right! In this patch it will print "crashkernel high memory >> reservation failed." message and out for arm32 if you specify > > That message may mislead people to believe crashkernel=,high is > supported but reservation is failed, then a bug need be filed for this? > We may expect a message telling this syntax is not supported on this > ARCH. "CRASH_ADDR_LOW_MAX >= CRASH_ADDR_HIGH_MAX" indicate that the arm32 does not support "crashkernel=,high", I wonder if this is generic for similar architecture. If so, the first patch can print such as "crashkernel=,high is not supported on this ARCH" message. > >> 'crashkernel=xM,high because "CRASH_ADDR_LOW_MAX" and >> "CRASH_ADDR_HIGH_MAX" is identical for arm32. And it should also warn >> out for other similar architecture. >> >> >>> >>> >> > >
On 07/10/24 at 09:52am, Jinjie Ruan wrote: > > > On 2024/7/9 22:06, Baoquan He wrote: > > On 07/09/24 at 07:06pm, Jinjie Ruan wrote: > >> > >> > >> On 2024/7/9 18:39, Baoquan He wrote: > >>> On 07/09/24 at 05:50pm, Jinjie Ruan wrote: > >>>> > >>>> > >>>> On 2024/7/9 17:29, Baoquan He wrote: > >>>>> On 07/08/24 at 09:33pm, Jinjie Ruan wrote: > >>>>>> Currently, x86, arm64, riscv and loongarch has been switched to generic > >>>>>> crashkernel reservation. Also use generic interface to simplify crashkernel > >>>>>> reservation for arm32, and fix two bugs by the way. > >>>>> > >>>>> I am not sure if this is a good idea. I added the generic reservation > >>>>> itnerfaces for ARCH which support crashkernel=,high|low and normal > >>>>> crashkernel reservation, with this, the code can be simplified a lot. > >>>>> However, arm32 doesn't support crashkernel=,high, I am not sure if it's > >>>>> worth taking the change, most importantly, if it will cause > >>>>> misunderstanding or misoperation. > >>>> > >>>> Yes, arm32 doesn't support crashkernel=,high. > >>>> > >>>> However, a little enhancement to the generic code (please see the first > >>>> patch), the generic reservation interfaces can also be applicable to > >>>> architectures that do not support "high" such as arm32, and it can also > >>>> simplify the code (please see the third patch). > >>> > >>> Yeah, I can see the code is simplified. When you specified > >>> 'crashkernel=xM,high', do you think what should be warn out? Because > >>> it's an unsupported syntax on arm32, we should do something to print out > >>> appropriate message. > >> > >> Yes, you are right! In this patch it will print "crashkernel high memory > >> reservation failed." message and out for arm32 if you specify > > > > That message may mislead people to believe crashkernel=,high is > > supported but reservation is failed, then a bug need be filed for this? > > We may expect a message telling this syntax is not supported on this > > ARCH. > > "CRASH_ADDR_LOW_MAX >= CRASH_ADDR_HIGH_MAX" indicate that the arm32 does > not support "crashkernel=,high", I wonder if this is generic for similar Imagine you are a testing engineer or a distros user, how do you know if "CRASH_ADDR_LOW_MAX >= CRASH_ADDR_HIGH_MAX" when you test 'crashkernel=,high' and see the failure message? > architecture. If so, the first patch can print such as > "crashkernel=,high is not supported on this ARCH" message. Please consider conprehensively if this is doable, you can paste draft code here to prove it. > > > > >> 'crashkernel=xM,high because "CRASH_ADDR_LOW_MAX" and > >> "CRASH_ADDR_HIGH_MAX" is identical for arm32. And it should also warn > >> out for other similar architecture. > >> > >> > >>> > >>> > >> > > > > >
On 2024/7/9 17:29, Baoquan He wrote: > On 07/08/24 at 09:33pm, Jinjie Ruan wrote: >> Currently, x86, arm64, riscv and loongarch has been switched to generic >> crashkernel reservation. Also use generic interface to simplify crashkernel >> reservation for arm32, and fix two bugs by the way. > > I am not sure if this is a good idea. I added the generic reservation > itnerfaces for ARCH which support crashkernel=,high|low and normal > crashkernel reservation, with this, the code can be simplified a lot. > However, arm32 doesn't support crashkernel=,high, I am not sure if it's > worth taking the change, most importantly, if it will cause > misunderstanding or misoperation. It seems that x86_32 doesn't support crashkernel=,high and use the generic interface，and also have the first patch bug. I’ll resend the first patch and explain it. > > Thanks > Baoquan > >
Currently, x86, arm64, riscv and loongarch has been switched to generic crashkernel reservation. Also use generic interface to simplify crashkernel reservation for arm32, and fix two bugs by the way. Jinjie Ruan (3): crash: Fix memory reserve dead loop bug in reserve_crashkernel_generic() ARM: Fix crash kenrel data type bug ARM: Use generic interface to simplify crashkernel reservation arch/arm/Kconfig | 3 ++ arch/arm/include/asm/crash_reserve.h | 24 +++++++++++ arch/arm/kernel/setup.c | 63 +++++----------------------- kernel/crash_reserve.c | 8 +++- 4 files changed, 44 insertions(+), 54 deletions(-) create mode 100644 arch/arm/include/asm/crash_reserve.h