| Message ID | 20211228132612.1860-3-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | support reserving crashkernel above 4G on arm64 kdump | expand |
On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: > Use parse_crashkernel_high_low() to bring the parsing of > "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they > are strongly dependent, make code logic clear and more readable. > > Suggested-by: Borislav Petkov <bp@alien8.de> Yeah, doesn't look like something I suggested... > @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) > /* crashkernel=XM */ > ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > if (ret != 0 || crash_size <= 0) { > - /* crashkernel=X,high */ > - ret = parse_crashkernel_high(boot_command_line, total_mem, > - &crash_size, &crash_base); > - if (ret != 0 || crash_size <= 0) > + /* crashkernel=X,high and possible crashkernel=Y,low */ > + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); So this calls parse_crashkernel() and when that one fails, it calls this new weird parse high/low helper you added. But then all three end up in the same __parse_crashkernel() worker function which seems to do the actual parsing. What I suggested and what would be real clean is if the arches would simply call a *single* parse_crashkernel() function and when that one returns, *all* crashkernel= options would have been parsed properly, low, high, middle crashkernel, whatever... and the caller would know what crash kernel needs to be allocated. Then each arch can do its memory allocations and checks based on that parsed data and decide to allocate or bail. So it is getting there but it needs more surgery... Thx.
On 2021/12/29 0:13, Borislav Petkov wrote: > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: >> Use parse_crashkernel_high_low() to bring the parsing of >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they >> are strongly dependent, make code logic clear and more readable. >> >> Suggested-by: Borislav Petkov <bp@alien8.de> > > Yeah, doesn't look like something I suggested... > >> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) >> /* crashkernel=XM */ >> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); >> if (ret != 0 || crash_size <= 0) { >> - /* crashkernel=X,high */ >> - ret = parse_crashkernel_high(boot_command_line, total_mem, >> - &crash_size, &crash_base); >> - if (ret != 0 || crash_size <= 0) >> + /* crashkernel=X,high and possible crashkernel=Y,low */ >> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); > > So this calls parse_crashkernel() and when that one fails, it calls this > new weird parse high/low helper you added. > > But then all three end up in the same __parse_crashkernel() worker > function which seems to do the actual parsing. > > What I suggested and what would be real clean is if the arches would > simply call a *single* > > parse_crashkernel() > > function and when that one returns, *all* crashkernel= options would > have been parsed properly, low, high, middle crashkernel, whatever... > and the caller would know what crash kernel needs to be allocated. > > Then each arch can do its memory allocations and checks based on that > parsed data and decide to allocate or bail. However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64 will also support it. It is not supported on other architectures. So changing parse_crashkernel() is not appropriate unless a new function is introduced. But naming this new function isn't easy, and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good. Of course, we can also consider changing parse_crashkernel() to another name, then use parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause other architectures to change as well. > > So it is getting there but it needs more surgery... > > Thx. >
On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote: > > > On 2021/12/29 0:13, Borislav Petkov wrote: > > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: > >> Use parse_crashkernel_high_low() to bring the parsing of > >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they > >> are strongly dependent, make code logic clear and more readable. > >> > >> Suggested-by: Borislav Petkov <bp@alien8.de> > > > > Yeah, doesn't look like something I suggested... > > > >> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) > >> /* crashkernel=XM */ > >> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > >> if (ret != 0 || crash_size <= 0) { > >> - /* crashkernel=X,high */ > >> - ret = parse_crashkernel_high(boot_command_line, total_mem, > >> - &crash_size, &crash_base); > >> - if (ret != 0 || crash_size <= 0) > >> + /* crashkernel=X,high and possible crashkernel=Y,low */ > >> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); > > > > So this calls parse_crashkernel() and when that one fails, it calls this > > new weird parse high/low helper you added. > > > > But then all three end up in the same __parse_crashkernel() worker > > function which seems to do the actual parsing. > > > > What I suggested and what would be real clean is if the arches would > > simply call a *single* > > > > parse_crashkernel() > > > > function and when that one returns, *all* crashkernel= options would > > have been parsed properly, low, high, middle crashkernel, whatever... > > and the caller would know what crash kernel needs to be allocated. > > > > Then each arch can do its memory allocations and checks based on that > > parsed data and decide to allocate or bail. > > However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64 > will also support it. It is not supported on other architectures. So changing parse_crashkernel() > is not appropriate unless a new function is introduced. But naming this new function isn't easy, > and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good. > Of course, we can also consider changing parse_crashkernel() to another name, then use > parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause > other architectures to change as well. Hi, I did not follow up all discussions, but if the only difference is about the low -> high fallback, I think you can add another argument in parse_crashkernel(..., *fallback_high), and doing some changes in __parse_crashkernel() before it returns. But since there are two many arguments, you could need a wrapper struct for crashkernel_param if needed. If you do not want to touch other arches, another function maybe something like parse_crashkernel_fallback() for x86 and arm64 to use. But I may not get all the context, please ignore if this is not the case. I agree that calling parse_crash_kernel* in the reserve_crashkernel funtions looks not good though. OTOH there are bunch of other logics in param parsing code, eg. determin the final size and offset etc. To split the logic out more things need to be done, eg. firstly parsing function just get all the inputs from cmdline string eg. an array of struct crashkernel_param with mem_range, expected size, expected offset, high, or low, and do not do any other things. Then pass these parsed inputs to another function to determine the final crash_size and crash_base, that part can be arch specific somehow. So I think you can unify the parse_crashkernel* in x86 first with just one function. And leave the further improvements to later work. But let's see how Boris think about this. > > > > > So it is getting there but it needs more surgery... > > > > Thx. > > > > -- > Regards, > Zhen Lei > Thanks Dave
On 12/29/21 at 03:27pm, Dave Young wrote: > On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote: > > > > > > On 2021/12/29 0:13, Borislav Petkov wrote: > > > On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: > > >> Use parse_crashkernel_high_low() to bring the parsing of > > >> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they > > >> are strongly dependent, make code logic clear and more readable. > > >> > > >> Suggested-by: Borislav Petkov <bp@alien8.de> > > > > > > Yeah, doesn't look like something I suggested... > > > > > >> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) > > >> /* crashkernel=XM */ > > >> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > > >> if (ret != 0 || crash_size <= 0) { > > >> - /* crashkernel=X,high */ > > >> - ret = parse_crashkernel_high(boot_command_line, total_mem, > > >> - &crash_size, &crash_base); > > >> - if (ret != 0 || crash_size <= 0) > > >> + /* crashkernel=X,high and possible crashkernel=Y,low */ > > >> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); > > > > > > So this calls parse_crashkernel() and when that one fails, it calls this > > > new weird parse high/low helper you added. > > > > > > But then all three end up in the same __parse_crashkernel() worker > > > function which seems to do the actual parsing. > > > > > > What I suggested and what would be real clean is if the arches would > > > simply call a *single* > > > > > > parse_crashkernel() > > > > > > function and when that one returns, *all* crashkernel= options would > > > have been parsed properly, low, high, middle crashkernel, whatever... > > > and the caller would know what crash kernel needs to be allocated. > > > > > > Then each arch can do its memory allocations and checks based on that > > > parsed data and decide to allocate or bail. > > > > However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64 > > will also support it. It is not supported on other architectures. So changing parse_crashkernel() > > is not appropriate unless a new function is introduced. But naming this new function isn't easy, > > and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good. > > Of course, we can also consider changing parse_crashkernel() to another name, then use > > parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause > > other architectures to change as well. > > Hi, I did not follow up all discussions, but if the only difference is > about the low -> high fallback, I think you can add another argument in > parse_crashkernel(..., *fallback_high), and doing some changes in > __parse_crashkernel() before it returns. But since there are two > many arguments, you could need a wrapper struct for crashkernel_param if > needed. > > If you do not want to touch other arches, another function maybe > something like parse_crashkernel_fallback() for x86 and arm64 to use. > > But I may not get all the context, please ignore if this is not the > case. I agree that calling parse_crash_kernel* in the > reserve_crashkernel funtions looks not good though. > > OTOH there are bunch of other logics in param parsing code, > eg. determin the final size and offset etc. To split the logic out more > things need to be done, eg. firstly parsing function just get all the > inputs from cmdline string eg. an array of struct crashkernel_param with > mem_range, expected size, expected offset, high, or low, and do not do > any other things. Then pass these parsed inputs to another function to > determine the final crash_size and crash_base, that part can be arch > specific somehow. > > So I think you can unify the parse_crashkernel* in x86 first with just > one function. And leave the further improvements to later work. But > let's see how Boris think about this. > BTW, I would suggest to wait for reviewers to response (eg. one week at least, or more due to the holidays) before updating another version Do not worry to miss the 5.17. I would say take it easy if it will miss then let's just leave with it and continue to work on the future improvements. I think one reason this issue takes too long time is that it was discussed some time but no followup and later people need to warm up again. Just keep it warm and continue to engage in the improvements, do not hurry for the specific mainline release. Thanks Dave
On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote: > So I think you can unify the parse_crashkernel* in x86 first with just > one function. And leave the further improvements to later work. But > let's see how Boris think about this. Well, I think this all unnecessary work. Why? If the goal is to support crashkernel...high,low on arm64, then you should simply *copy* the functionality on arm64 and be done with it. Unification is done by looking at code which is duplicated across architectures and which has been untouched for a while now, i.e., no new or arch-specific changes are going to it so a unification can be as simple as trivially switching the architectures to call a generic function. What this does is carve out the "generic" parts and then try not to break existing usage. Which is a total waste of energy and resources. And it is casting that functionality in stone so that when x86 wants to change something there, it should do it in a way not to break arm64. And I fail to see the advantage of all that. Code sharing ain't it. So what it should do is simply copy the necessary code to arm64. Unifications can always be done later, when the dust settles. IMNSVHO.
On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote: > BTW, I would suggest to wait for reviewers to response (eg. one week at > least, or more due to the holidays) before updating another version > > Do not worry to miss the 5.17. I would say take it easy if it will > miss then let's just leave with it and continue to work on the future > improvements. I think one reason this issue takes too long time is that it was > discussed some time but no followup and later people need to warm up > again. Just keep it warm and continue to engage in the improvements, do > not hurry for the specific mainline release. Can you tell this to *all* patch submitters please? I can't count the times where people simply hurry to send the new revision just to get it in the next kernel, and make silly mistakes while doing so. Or not think things straight and misdesign it all. And what this causes is the opposite of what they wanna achieve - pissed maintainers and ignored threads. And they all *know* that the next kernel is around the corner. So why the hell does it even matter when? What most submitters fail to realize is, the moment your code hits upstream, it becomes the maintainers' problem and submitters can relax. But maintainers get to deal with this code forever. So after a while maintainers learn that they either accept ready code and it all just works or they make the mistake to take half-baked crap in and then they themselves get to clean it up and fix it. So maintainers learn quickly to push back. But it is annoying and it would help immensely if submitters would consider this and stop hurrying the code in but try to do a *good* job first, design-wise and code-wise by thinking hard about what they're trying to do. Yeah, things could be a lot simpler and easier - it only takes a little bit of effort...
On 12/29/21 at 11:11am, Borislav Petkov wrote: > On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote: > > BTW, I would suggest to wait for reviewers to response (eg. one week at > > least, or more due to the holidays) before updating another version > > > > Do not worry to miss the 5.17. I would say take it easy if it will > > miss then let's just leave with it and continue to work on the future > > improvements. I think one reason this issue takes too long time is that it was > > discussed some time but no followup and later people need to warm up > > again. Just keep it warm and continue to engage in the improvements, do > > not hurry for the specific mainline release. > > Can you tell this to *all* patch submitters please? I appreciate you further explanation below to describe the situation. I do not see how can I tell this to *all* submitters, but I am and I will try to do this as far as I can. Maintainers and patch submitters, it would help for both parties show sympathy with each other, some soft reminders will help people to understand each other, especially for new comers. > > I can't count the times where people simply hurry to send the new > revision just to get it in the next kernel, and make silly mistakes > while doing so. Or not think things straight and misdesign it all. > > And what this causes is the opposite of what they wanna achieve - pissed > maintainers and ignored threads. > > And they all *know* that the next kernel is around the corner. So why > the hell does it even matter when? > > What most submitters fail to realize is, the moment your code hits > upstream, it becomes the maintainers' problem and submitters can relax. > > But maintainers get to deal with this code forever. So after a while > maintainers learn that they either accept ready code and it all just > works or they make the mistake to take half-baked crap in and then they > themselves get to clean it up and fix it. > > So maintainers learn quickly to push back. > > But it is annoying and it would help immensely if submitters would > consider this and stop hurrying the code in but try to do a *good* job > first, design-wise and code-wise by thinking hard about what they're > trying to do. > > Yeah, things could be a lot simpler and easier - it only takes a little > bit of effort... > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette > Thanks Dave
On 12/29/21 at 11:03am, Borislav Petkov wrote: > On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote: > > So I think you can unify the parse_crashkernel* in x86 first with just > > one function. And leave the further improvements to later work. But > > let's see how Boris think about this. > > Well, I think this all unnecessary work. Why? > > If the goal is to support crashkernel...high,low on arm64, then you > should simply *copy* the functionality on arm64 and be done with it. > > Unification is done by looking at code which is duplicated across > architectures and which has been untouched for a while now, i.e., no > new or arch-specific changes are going to it so a unification can be > as simple as trivially switching the architectures to call a generic > function. > > What this does is carve out the "generic" parts and then try not to > break existing usage. > > Which is a total waste of energy and resources. And it is casting that > functionality in stone so that when x86 wants to change something there, > it should do it in a way not to break arm64. And I fail to see the > advantage of all that. Code sharing ain't it. > > So what it should do is simply copy the necessary code to arm64. > Unifications can always be done later, when the dust settles. I think I agree with you about the better way is to doing some improvements so that arches can logically doing things better. I can leave with the way I suggested although it is not the best. But I think Leizhen needs a clear direction about how to do it. It is very clear now. See how he will handle this. > > IMNSVHO. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette >
On Wed, Dec 29, 2021 at 06:38:43PM +0800, Dave Young wrote: > I appreciate you further explanation below to describe the situation. > I do not see how can I tell this to *all* submitters, You don't have to - that was hypothetical. :-) I'm typing this on a public mailing list with the hope that people will see it. > but I am and I will try to do this as far as I can. Much appreciated. > Maintainers and patch submitters, it would help for both > parties show sympathy with each other, some soft reminders will help > people to understand each other, especially for new comers. Yap, that's why we keep repeating it. Thx.
On 2021/12/29 15:27, Dave Young wrote: > On 12/29/21 at 10:27am, Leizhen (ThunderTown) wrote: >> >> >> On 2021/12/29 0:13, Borislav Petkov wrote: >>> On Tue, Dec 28, 2021 at 09:26:01PM +0800, Zhen Lei wrote: >>>> Use parse_crashkernel_high_low() to bring the parsing of >>>> "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they >>>> are strongly dependent, make code logic clear and more readable. >>>> >>>> Suggested-by: Borislav Petkov <bp@alien8.de> >>> >>> Yeah, doesn't look like something I suggested... >>> >>>> @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) >>>> /* crashkernel=XM */ >>>> ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); >>>> if (ret != 0 || crash_size <= 0) { >>>> - /* crashkernel=X,high */ >>>> - ret = parse_crashkernel_high(boot_command_line, total_mem, >>>> - &crash_size, &crash_base); >>>> - if (ret != 0 || crash_size <= 0) >>>> + /* crashkernel=X,high and possible crashkernel=Y,low */ >>>> + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); >>> >>> So this calls parse_crashkernel() and when that one fails, it calls this >>> new weird parse high/low helper you added. >>> >>> But then all three end up in the same __parse_crashkernel() worker >>> function which seems to do the actual parsing. >>> >>> What I suggested and what would be real clean is if the arches would >>> simply call a *single* >>> >>> parse_crashkernel() >>> >>> function and when that one returns, *all* crashkernel= options would >>> have been parsed properly, low, high, middle crashkernel, whatever... >>> and the caller would know what crash kernel needs to be allocated. >>> >>> Then each arch can do its memory allocations and checks based on that >>> parsed data and decide to allocate or bail. >> >> However, only x86 currently supports "crashkernel=X,high" and "crashkernel=Y,low", and arm64 >> will also support it. It is not supported on other architectures. So changing parse_crashkernel() >> is not appropriate unless a new function is introduced. But naming this new function isn't easy, >> and the name parse_crashkernel_in_order() that I've named before doesn't seem to be good. >> Of course, we can also consider changing parse_crashkernel() to another name, then use >> parse_crashkernel() to parse all possible "crashkernel=" options in order, but this will cause >> other architectures to change as well. > > Hi, I did not follow up all discussions, but if the only difference is > about the low -> high fallback, I think you can add another argument in > parse_crashkernel(..., *fallback_high), and doing some changes in > __parse_crashkernel() before it returns. But since there are two > many arguments, you could need a wrapper struct for crashkernel_param if > needed. A wrapper struct is needed. Because at least two new output parameters need to be added: 1. high, to indicate whether "crashkernel=X,high" is specified 2. low_size, to save the memory size specified by "crashkernel=Y,low" > > If you do not want to touch other arches, another function maybe > something like parse_crashkernel_fallback() for x86 and arm64 to use. > > But I may not get all the context, please ignore if this is not the > case. I agree that calling parse_crash_kernel* in the > reserve_crashkernel funtions looks not good though. > > OTOH there are bunch of other logics in param parsing code, > eg. determin the final size and offset etc. To split the logic out more > things need to be done, eg. firstly parsing function just get all the > inputs from cmdline string eg. an array of struct crashkernel_param with > mem_range, expected size, expected offset, high, or low, and do not do > any other things. Then pass these parsed inputs to another function to > determine the final crash_size and crash_base, that part can be arch > specific somehow. Yes, this way makes the code logic clear. But there's a bit of trouble with "crashkernel=range1:size1[,range2:size2,...][@offset]", the array size is dynamic. > > So I think you can unify the parse_crashkernel* in x86 first with just > one function. And leave the further improvements to later work. But > let's see how Boris think about this. > >> >>> >>> So it is getting there but it needs more surgery... >>> >>> Thx. >>> >> >> -- >> Regards, >> Zhen Lei >> > > Thanks > Dave > > . >
On 2021/12/29 18:38, Dave Young wrote: > On 12/29/21 at 11:11am, Borislav Petkov wrote: >> On Wed, Dec 29, 2021 at 03:45:12PM +0800, Dave Young wrote: >>> BTW, I would suggest to wait for reviewers to response (eg. one week at >>> least, or more due to the holidays) before updating another version >>> >>> Do not worry to miss the 5.17. I would say take it easy if it will >>> miss then let's just leave with it and continue to work on the future >>> improvements. I think one reason this issue takes too long time is that it was >>> discussed some time but no followup and later people need to warm up >>> again. Just keep it warm and continue to engage in the improvements, do >>> not hurry for the specific mainline release. >> >> Can you tell this to *all* patch submitters please? > > I appreciate you further explanation below to describe the situation. I do not > see how can I tell this to *all* submitters, but I am and I will try to do this > as far as I can. Maintainers and patch submitters, it would help for both > parties show sympathy with each other, some soft reminders will help > people to understand each other, especially for new comers. > >> >> I can't count the times where people simply hurry to send the new >> revision just to get it in the next kernel, and make silly mistakes >> while doing so. Or not think things straight and misdesign it all. >> >> And what this causes is the opposite of what they wanna achieve - pissed >> maintainers and ignored threads. I just hope the first 4 patches can be merged into v5.17. It seems to me that it is quite clear. Although the goal of the final stage is to modify function parse_crashkernel() according to the current opinion. But that's not a lightweight change after all. The final parse_crashkernel() change may take one version or two. In this process, it maybe OK to do a part of cleanup first. It's like someone who wants to buy a luxury car to commute to work six months later. He buys a cheap used car and sells it six months later. It sounds right to me, don't you think? >> >> And they all *know* that the next kernel is around the corner. So why >> the hell does it even matter when? Because all programmers should have confidence in the code they write. I have a new idea, and I'm free these days, so I updated v19. I can't rely on people telling me to take a step forward, then take a step forward. Otherwise, stand still. >> >> What most submitters fail to realize is, the moment your code hits >> upstream, it becomes the maintainers' problem and submitters can relax. Sorry, I'll make sure all the comments are collected and then send the next edition. >> >> But maintainers get to deal with this code forever. So after a while >> maintainers learn that they either accept ready code and it all just >> works or they make the mistake to take half-baked crap in and then they >> themselves get to clean it up and fix it. >> >> So maintainers learn quickly to push back. >> >> But it is annoying and it would help immensely if submitters would >> consider this and stop hurrying the code in but try to do a *good* job >> first, design-wise and code-wise by thinking hard about what they're >> trying to do. >> >> Yeah, things could be a lot simpler and easier - it only takes a little >> bit of effort... >> >> -- >> Regards/Gruss, >> Boris. >> >> https://people.kernel.org/tglx/notes-about-netiquette >> > > Thanks > Dave > > . >
On 2021/12/29 18:46, Dave Young wrote: > On 12/29/21 at 11:03am, Borislav Petkov wrote: >> On Wed, Dec 29, 2021 at 03:27:48PM +0800, Dave Young wrote: >>> So I think you can unify the parse_crashkernel* in x86 first with just >>> one function. And leave the further improvements to later work. But >>> let's see how Boris think about this. >> >> Well, I think this all unnecessary work. Why? >> >> If the goal is to support crashkernel...high,low on arm64, then you >> should simply *copy* the functionality on arm64 and be done with it. >> >> Unification is done by looking at code which is duplicated across >> architectures and which has been untouched for a while now, i.e., no >> new or arch-specific changes are going to it so a unification can be >> as simple as trivially switching the architectures to call a generic >> function. >> >> What this does is carve out the "generic" parts and then try not to >> break existing usage. >> >> Which is a total waste of energy and resources. And it is casting that >> functionality in stone so that when x86 wants to change something there, >> it should do it in a way not to break arm64. And I fail to see the >> advantage of all that. Code sharing ain't it. It's just a worry, there's uncertainty about whether it's going to be. I think the only thing that might change is the default value of "low_size". Of course, the alignment size and start address may also change, but most of them can be controlled by macros. Chen Zhou and I tried to share the code because of a suggestion. After so many attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured out a good solution yet. >> >> So what it should do is simply copy the necessary code to arm64. >> Unifications can always be done later, when the dust settles. > > I think I agree with you about the better way is to doing some > improvements so that arches can logically doing things better. I can > leave with the way I suggested although it is not the best. But I think > Leizhen needs a clear direction about how to do it. It is very clear > now. See how he will handle this. Surviving, then pursuing ideals. I will put the patches that make arm64 support crashkernel...high,low to the front, then the parse_crashkernel() unification patches. Even if the second half of the patches is not ready for v5.18, the first half of the patches is ready. > >> >> IMNSVHO. >> >> -- >> Regards/Gruss, >> Boris. >> >> https://people.kernel.org/tglx/notes-about-netiquette >> > > . >
On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote: > Chen Zhou and I tried to share the code because of a suggestion. After so many > attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured > out a good solution yet. Well, you learned a very important lesson and the many attempts are not in vain: code sharing does not make sense in every case. > I will put the patches that make arm64 support crashkernel...high,low to > the front, then the parse_crashkernel() unification patches. Even if the > second half of the patches is not ready for v5.18, the first half of the > patches is ready. I think you should concentrate on the arm64 side which is, AFAICT, what you're trying to achieve. The "parse_crashkernel() unification" needs more thought because, as I said already, that doesn't make a whole lot of sense to me. If you want to enforce the fact that "low" makes sense only when "high" is supplied, parse_crashkernel_high_low() is not the right thing to do. You need to have a *single* function which does all the parsing where you can decide what to do: "if high, parse low", "if no high supplied, ignore low" and so on. And if those are supported on certain architectures only, you can do ifdeffery... But I think I already stated that I don't like such unifications which introduce unnecessary dependencies between architectures. Therefore, I won't accept them into x86 unless there's a strong compelling reason. Which I don't see ATM. Thx.
On 2021/12/30 0:51, Borislav Petkov wrote: > On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote: >> Chen Zhou and I tried to share the code because of a suggestion. After so many >> attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured >> out a good solution yet. > > Well, you learned a very important lesson and the many attempts are not > in vain: code sharing does not make sense in every case. > >> I will put the patches that make arm64 support crashkernel...high,low to >> the front, then the parse_crashkernel() unification patches. Even if the >> second half of the patches is not ready for v5.18, the first half of the >> patches is ready. > > I think you should concentrate on the arm64 side which is, AFAICT, what > you're trying to achieve. Right, a patchset should focus on just one thing. > > The "parse_crashkernel() unification" needs more thought because, as I > said already, that doesn't make a whole lot of sense to me. Yes, because it's not a functional improvement, it's not a performance optimization, it's also not a fix for a known bug, it's just a programmer's artistic pursuit. > > If you want to enforce the fact that "low" makes sense only when "high" > is supplied, parse_crashkernel_high_low() is not the right thing to do. > You need to have a *single* function which does all the parsing where > you can decide what to do: "if high, parse low", "if no high supplied, > ignore low" and so on. I understand your proposal, but parse_crashkernel_high_low() is a cost-effective and profitable change, it makes the current code a little clearer, and avoid passing unnecessary parameters "system_ram" and "crash_base" when other architectures use parse_crashkernel_{high|low}(). I actually followed your advice in the beginning to do "parse_crashkernel() and parse_crashkernel_{high|low}() unification". But I found it's difficult and the end result may not be as good as expected. So I introduced parse_crashkernel_high_low(). The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by "crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low() is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification" complicates things. For example, the parameter "crash_size" means "low or high" memory size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high". So we'd better give it two names with union. > > And if those are supported on certain architectures only, you can do > ifdeffery... I don't think so. These __init functions are small and architecture-independent, and do not affect compilation of other architectures. There may be other architectures that use it in the future, such as the current arm64. > > But I think I already stated that I don't like such unifications which > introduce unnecessary dependencies between architectures. Therefore, I > won't accept them into x86 unless there's a strong compelling reason. > Which I don't see ATM. OK. > > Thx. >
On 2021/12/30 10:39, Leizhen (ThunderTown) wrote: > > > On 2021/12/30 0:51, Borislav Petkov wrote: >> On Wed, Dec 29, 2021 at 11:04:21PM +0800, Leizhen (ThunderTown) wrote: >>> Chen Zhou and I tried to share the code because of a suggestion. After so many >>> attempts, it doesn't seem to fit to make generic. Or maybe I haven't figured >>> out a good solution yet. >> >> Well, you learned a very important lesson and the many attempts are not >> in vain: code sharing does not make sense in every case. >> >>> I will put the patches that make arm64 support crashkernel...high,low to >>> the front, then the parse_crashkernel() unification patches. Even if the >>> second half of the patches is not ready for v5.18, the first half of the >>> patches is ready. >> >> I think you should concentrate on the arm64 side which is, AFAICT, what >> you're trying to achieve. > > Right, a patchset should focus on just one thing. > >> >> The "parse_crashkernel() unification" needs more thought because, as I >> said already, that doesn't make a whole lot of sense to me. > > Yes, because it's not a functional improvement, it's not a performance optimization, > it's also not a fix for a known bug, it's just a programmer's artistic pursuit. > >> >> If you want to enforce the fact that "low" makes sense only when "high" >> is supplied, parse_crashkernel_high_low() is not the right thing to do. >> You need to have a *single* function which does all the parsing where >> you can decide what to do: "if high, parse low", "if no high supplied, >> ignore low" and so on. In fact, this is how my current function parse_crashkernel_high_low() is implemented. + /* crashkernel=X,high */ + ret = parse_crashkernel_high(cmdline, 0, high_size, &base); + if (ret) //crashkernel=X,high is not specified + return ret; + + if (*high_size <= 0) //crashkernel=X,high is specified but the value is invalid + return -EINVAL; //Sorry, the type of high_size is "unsigned long long *", so less than zero is impossible + + /* crashkernel=Y,low */ + ret = parse_crashkernel_low(cmdline, 0, low_size, &base); //If crashkernel=Y,low is specified, the parsed value is stored in *low_size + if (ret) + *low_size = -1; //crashkernel=Y,low is not specified > > I understand your proposal, but parse_crashkernel_high_low() is a cost-effective > and profitable change, it makes the current code a little clearer, and avoid passing > unnecessary parameters "system_ram" and "crash_base" when other architectures use > parse_crashkernel_{high|low}(). > > I actually followed your advice in the beginning to do "parse_crashkernel() and > parse_crashkernel_{high|low}() unification". But I found it's difficult and the > end result may not be as good as expected. So I introduced parse_crashkernel_high_low(). > > The parameter "system_ram" and "crash_base" of parse_crashkernel() is not need by > "crashkernel=X,[high,low]". And parameter "low_size" of parse_crashkernel_high_low() > is not need by "crashkernel=X[@offset]". The "parse_crashkernel() unification" > complicates things. For example, the parameter "crash_size" means "low or high" memory > size for "crashkernel=X[@offset]", but only means "high" memory size for "crashkernel=X,high". > So we'd better give it two names with union. > >> >> And if those are supported on certain architectures only, you can do >> ifdeffery... > > I don't think so. These __init functions are small and architecture-independent, and do not > affect compilation of other architectures. There may be other architectures that use > it in the future, such as the current arm64. > >> >> But I think I already stated that I don't like such unifications which >> introduce unnecessary dependencies between architectures. Therefore, I >> won't accept them into x86 unless there's a strong compelling reason. >> Which I don't see ATM. > > OK. > >> >> Thx. >> >
On 12/28/21 7:26 AM, Zhen Lei wrote: > Use parse_crashkernel_high_low() to bring the parsing of > "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they > are strongly dependent, make code logic clear and more readable. > > Suggested-by: Borislav Petkov <bp@alien8.de> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > Acked-by: John Donnelly <john.p.donnelly@oracle.com> > --- > arch/x86/kernel/setup.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c > index 6a190c7f4d71b05..93d78aae1937db3 100644 > --- a/arch/x86/kernel/setup.c > +++ b/arch/x86/kernel/setup.c > @@ -416,18 +416,16 @@ static void __init memblock_x86_reserve_range_setup_data(void) > # define CRASH_ADDR_HIGH_MAX SZ_64T > #endif > > -static int __init reserve_crashkernel_low(void) > +static int __init reserve_crashkernel_low(unsigned long long low_size) > { > #ifdef CONFIG_X86_64 > - unsigned long long base, low_base = 0, low_size = 0; > + unsigned long long low_base = 0; > unsigned long low_mem_limit; > - int ret; > > low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX); > > - /* crashkernel=Y,low */ > - ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base); > - if (ret) { > + /* crashkernel=Y,low is not specified */ > + if ((long)low_size < 0) { > /* > * two parts from kernel/dma/swiotlb.c: > * -swiotlb size: user-specified with swiotlb= or default. > @@ -465,7 +463,7 @@ static int __init reserve_crashkernel_low(void) > > static void __init reserve_crashkernel(void) > { > - unsigned long long crash_size, crash_base, total_mem; > + unsigned long long crash_size, crash_base, total_mem, low_size; > bool high = false; > int ret; > > @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) > /* crashkernel=XM */ > ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); > if (ret != 0 || crash_size <= 0) { > - /* crashkernel=X,high */ > - ret = parse_crashkernel_high(boot_command_line, total_mem, > - &crash_size, &crash_base); > - if (ret != 0 || crash_size <= 0) > + /* crashkernel=X,high and possible crashkernel=Y,low */ > + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); > + if (ret) > return; > high = true; > } > @@ -520,7 +517,7 @@ static void __init reserve_crashkernel(void) > } > } > > - if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) { > + if (crash_base >= (1ULL << 32) && reserve_crashkernel_low(low_size)) { > memblock_phys_free(crash_base, crash_size); > return; > }
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 6a190c7f4d71b05..93d78aae1937db3 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -416,18 +416,16 @@ static void __init memblock_x86_reserve_range_setup_data(void) # define CRASH_ADDR_HIGH_MAX SZ_64T #endif -static int __init reserve_crashkernel_low(void) +static int __init reserve_crashkernel_low(unsigned long long low_size) { #ifdef CONFIG_X86_64 - unsigned long long base, low_base = 0, low_size = 0; + unsigned long long low_base = 0; unsigned long low_mem_limit; - int ret; low_mem_limit = min(memblock_phys_mem_size(), CRASH_ADDR_LOW_MAX); - /* crashkernel=Y,low */ - ret = parse_crashkernel_low(boot_command_line, low_mem_limit, &low_size, &base); - if (ret) { + /* crashkernel=Y,low is not specified */ + if ((long)low_size < 0) { /* * two parts from kernel/dma/swiotlb.c: * -swiotlb size: user-specified with swiotlb= or default. @@ -465,7 +463,7 @@ static int __init reserve_crashkernel_low(void) static void __init reserve_crashkernel(void) { - unsigned long long crash_size, crash_base, total_mem; + unsigned long long crash_size, crash_base, total_mem, low_size; bool high = false; int ret; @@ -474,10 +472,9 @@ static void __init reserve_crashkernel(void) /* crashkernel=XM */ ret = parse_crashkernel(boot_command_line, total_mem, &crash_size, &crash_base); if (ret != 0 || crash_size <= 0) { - /* crashkernel=X,high */ - ret = parse_crashkernel_high(boot_command_line, total_mem, - &crash_size, &crash_base); - if (ret != 0 || crash_size <= 0) + /* crashkernel=X,high and possible crashkernel=Y,low */ + ret = parse_crashkernel_high_low(boot_command_line, &crash_size, &low_size); + if (ret) return; high = true; } @@ -520,7 +517,7 @@ static void __init reserve_crashkernel(void) } } - if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) { + if (crash_base >= (1ULL << 32) && reserve_crashkernel_low(low_size)) { memblock_phys_free(crash_base, crash_size); return; }
Use parse_crashkernel_high_low() to bring the parsing of "crashkernel=X,high" and the parsing of "crashkernel=Y,low" together, they are strongly dependent, make code logic clear and more readable. Suggested-by: Borislav Petkov <bp@alien8.de> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/x86/kernel/setup.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)