* [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG @ 2021-04-23 8:14 ` zhouchuangao 0 siblings, 0 replies; 6+ messages in thread From: zhouchuangao @ 2021-04-23 8:14 UTC (permalink / raw) To: Wei Xu, Russell King, linux-arm-kernel, linux-kernel; +Cc: zhouchuangao BUG_ON uses unlikely in if(). Through disassembly, we can see that brk #0x800 is compiled to the end of the function. As you can see below: ...... ffffff8008660bec: d65f03c0 ret ffffff8008660bf0: d4210000 brk #0x800 Usually, the condition in if () is not satisfied. For the multi-stage pipeline, we do not need to perform fetch decode and excute operation on brk instruction. In my opinion, this can improve the efficiency of the multi-stage pipeline. Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> --- arch/arm/mach-hisi/hotplug.c | 3 +-- arch/arm/mach-hisi/platmcpm.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c index c517941..b9ced60 100644 --- a/arch/arm/mach-hisi/hotplug.c +++ b/arch/arm/mach-hisi/hotplug.c @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) u32 val = 0; if (!ctrl_base) - if (!hix5hd2_hotplug_init()) - BUG(); + BUG_ON(!hix5hd2_hotplug_init()); if (enable) { /* power on cpu1 */ diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c index 96a4840..6c90039 100644 --- a/arch/arm/mach-hisi/platmcpm.c +++ b/arch/arm/mach-hisi/platmcpm.c @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) { unsigned long data; - if (!fabric) - BUG(); + BUG_ON(!fabric); + data = readl_relaxed(fabric + FAB_SF_MODE); if (on) data |= 1 << cluster; -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG @ 2021-04-23 8:14 ` zhouchuangao 0 siblings, 0 replies; 6+ messages in thread From: zhouchuangao @ 2021-04-23 8:14 UTC (permalink / raw) To: Wei Xu, Russell King, linux-arm-kernel, linux-kernel; +Cc: zhouchuangao BUG_ON uses unlikely in if(). Through disassembly, we can see that brk #0x800 is compiled to the end of the function. As you can see below: ...... ffffff8008660bec: d65f03c0 ret ffffff8008660bf0: d4210000 brk #0x800 Usually, the condition in if () is not satisfied. For the multi-stage pipeline, we do not need to perform fetch decode and excute operation on brk instruction. In my opinion, this can improve the efficiency of the multi-stage pipeline. Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> --- arch/arm/mach-hisi/hotplug.c | 3 +-- arch/arm/mach-hisi/platmcpm.c | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c index c517941..b9ced60 100644 --- a/arch/arm/mach-hisi/hotplug.c +++ b/arch/arm/mach-hisi/hotplug.c @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) u32 val = 0; if (!ctrl_base) - if (!hix5hd2_hotplug_init()) - BUG(); + BUG_ON(!hix5hd2_hotplug_init()); if (enable) { /* power on cpu1 */ diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c index 96a4840..6c90039 100644 --- a/arch/arm/mach-hisi/platmcpm.c +++ b/arch/arm/mach-hisi/platmcpm.c @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) { unsigned long data; - if (!fabric) - BUG(); + BUG_ON(!fabric); + data = readl_relaxed(fabric + FAB_SF_MODE); if (on) data |= 1 << cluster; -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG 2021-04-23 8:14 ` zhouchuangao @ 2021-04-23 12:16 ` Robin Murphy -1 siblings, 0 replies; 6+ messages in thread From: Robin Murphy @ 2021-04-23 12:16 UTC (permalink / raw) To: zhouchuangao, Wei Xu, Russell King, linux-arm-kernel, linux-kernel On 2021-04-23 09:14, zhouchuangao wrote: > BUG_ON uses unlikely in if(). Through disassembly, we can see that > brk #0x800 is compiled to the end of the function. > As you can see below: > ...... > ffffff8008660bec: d65f03c0 ret > ffffff8008660bf0: d4210000 brk #0x800 > > Usually, the condition in if () is not satisfied. For the > multi-stage pipeline, we do not need to perform fetch decode > and excute operation on brk instruction. 32-bit Arm does not have "ret" and "brk" instructions, and either way the relevant BUG() instruction(s) aren't executed unless the condition is met, so this really makes very little sense. > In my opinion, this can improve the efficiency of the > multi-stage pipeline. It has very little to do with the pipeline - modern cores are considerably more sophisticated than the 3-stage Acorn RISC Machine of 1985, and are not usually limited by frontend throughput. The point of unlikely() is to avoid having a normally-taken forward branch to skip over in-line code, and instead make sure the only thing in the normal execution path is a normally-not-taken branch to handle the condition out-of-line. Yes, the impact of branches - and thus why it can be desirable to avoid them - is indeed *related* to pipelining, but that's rather tangential. Even then, it's only worth considering things at this level in frequently-executed and/or performance-critical code. Saving a couple of CPU cycles in something that is effectively a one-time operation is utterly immaterial. The realistic justification for these patches is that that BUG_ON() exists for implementing conditional BUG()s, so we may as well use it if it makes the source code more readable. > Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> > --- > arch/arm/mach-hisi/hotplug.c | 3 +-- > arch/arm/mach-hisi/platmcpm.c | 4 ++-- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c > index c517941..b9ced60 100644 > --- a/arch/arm/mach-hisi/hotplug.c > +++ b/arch/arm/mach-hisi/hotplug.c > @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) > u32 val = 0; > > if (!ctrl_base) > - if (!hix5hd2_hotplug_init()) > - BUG(); > + BUG_ON(!hix5hd2_hotplug_init()); Whatever tool you're using to detect these patterns, consider improving it, or at least giving a bit more thought to the results beyond blindly applying one single rule - "if(x) BUG_ON(y);" arguably makes even less sense since it's now neither one thing nor the other. Robin. > if (enable) { > /* power on cpu1 */ > diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c > index 96a4840..6c90039 100644 > --- a/arch/arm/mach-hisi/platmcpm.c > +++ b/arch/arm/mach-hisi/platmcpm.c > @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) > { > unsigned long data; > > - if (!fabric) > - BUG(); > + BUG_ON(!fabric); > + > data = readl_relaxed(fabric + FAB_SF_MODE); > if (on) > data |= 1 << cluster; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG @ 2021-04-23 12:16 ` Robin Murphy 0 siblings, 0 replies; 6+ messages in thread From: Robin Murphy @ 2021-04-23 12:16 UTC (permalink / raw) To: zhouchuangao, Wei Xu, Russell King, linux-arm-kernel, linux-kernel On 2021-04-23 09:14, zhouchuangao wrote: > BUG_ON uses unlikely in if(). Through disassembly, we can see that > brk #0x800 is compiled to the end of the function. > As you can see below: > ...... > ffffff8008660bec: d65f03c0 ret > ffffff8008660bf0: d4210000 brk #0x800 > > Usually, the condition in if () is not satisfied. For the > multi-stage pipeline, we do not need to perform fetch decode > and excute operation on brk instruction. 32-bit Arm does not have "ret" and "brk" instructions, and either way the relevant BUG() instruction(s) aren't executed unless the condition is met, so this really makes very little sense. > In my opinion, this can improve the efficiency of the > multi-stage pipeline. It has very little to do with the pipeline - modern cores are considerably more sophisticated than the 3-stage Acorn RISC Machine of 1985, and are not usually limited by frontend throughput. The point of unlikely() is to avoid having a normally-taken forward branch to skip over in-line code, and instead make sure the only thing in the normal execution path is a normally-not-taken branch to handle the condition out-of-line. Yes, the impact of branches - and thus why it can be desirable to avoid them - is indeed *related* to pipelining, but that's rather tangential. Even then, it's only worth considering things at this level in frequently-executed and/or performance-critical code. Saving a couple of CPU cycles in something that is effectively a one-time operation is utterly immaterial. The realistic justification for these patches is that that BUG_ON() exists for implementing conditional BUG()s, so we may as well use it if it makes the source code more readable. > Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> > --- > arch/arm/mach-hisi/hotplug.c | 3 +-- > arch/arm/mach-hisi/platmcpm.c | 4 ++-- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c > index c517941..b9ced60 100644 > --- a/arch/arm/mach-hisi/hotplug.c > +++ b/arch/arm/mach-hisi/hotplug.c > @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) > u32 val = 0; > > if (!ctrl_base) > - if (!hix5hd2_hotplug_init()) > - BUG(); > + BUG_ON(!hix5hd2_hotplug_init()); Whatever tool you're using to detect these patterns, consider improving it, or at least giving a bit more thought to the results beyond blindly applying one single rule - "if(x) BUG_ON(y);" arguably makes even less sense since it's now neither one thing nor the other. Robin. > if (enable) { > /* power on cpu1 */ > diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c > index 96a4840..6c90039 100644 > --- a/arch/arm/mach-hisi/platmcpm.c > +++ b/arch/arm/mach-hisi/platmcpm.c > @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) > { > unsigned long data; > > - if (!fabric) > - BUG(); > + BUG_ON(!fabric); > + > data = readl_relaxed(fabric + FAB_SF_MODE); > if (on) > data |= 1 << cluster; > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG 2021-04-23 12:16 ` Robin Murphy @ 2021-04-23 13:10 ` 周传高 -1 siblings, 0 replies; 6+ messages in thread From: 周传高 @ 2021-04-23 13:10 UTC (permalink / raw) To: Robin Murphy; +Cc: Wei Xu, Russell King, linux-arm-kernel, linux-kernel >On 2021-04-23 09:14, zhouchuangao wrote: >> BUG_ON uses unlikely in if(). Through disassembly, we can see that >> brk #0x800 is compiled to the end of the function. >> As you can see below: >> ...... >> ffffff8008660bec: d65f03c0 ret >> ffffff8008660bf0: d4210000 brk #0x800 >> >> Usually, the condition in if () is not satisfied. For the >> multi-stage pipeline, we do not need to perform fetch decode >> and excute operation on brk instruction. > >32-bit Arm does not have "ret" and "brk" instructions, and either way >the relevant BUG() instruction(s) aren't executed unless the condition >is met, so this really makes very little sense. > Sorry, this is just an analysis based on ARM64. >> In my opinion, this can improve the efficiency of the >> multi-stage pipeline. > >It has very little to do with the pipeline - modern cores are >considerably more sophisticated than the 3-stage Acorn RISC Machine of >1985, and are not usually limited by frontend throughput. The point of >unlikely() is to avoid having a normally-taken forward branch to skip >over in-line code, and instead make sure the only thing in the normal >execution path is a normally-not-taken branch to handle the condition >out-of-line. Yes, the impact of branches - and thus why it can be >desirable to avoid them - is indeed *related* to pipelining, but that's >rather tangential. > >Even then, it's only worth considering things at this level in >frequently-executed and/or performance-critical code. Saving a couple of >CPU cycles in something that is effectively a one-time operation is >utterly immaterial. > >The realistic justification for these patches is that that BUG_ON() >exists for implementing conditional BUG()s, so we may as well use it if >it makes the source code more readable. > Thank you for your excellent analysis, Indeed, only in the case of Frequently Executed and/or Performance-Critical Code, the patch is of great value. Hmm...Perhaps the best reason is to make the code more readable. BR, zhouchuangao >> Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> >> --- >> arch/arm/mach-hisi/hotplug.c | 3 +-- >> arch/arm/mach-hisi/platmcpm.c | 4 ++-- >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c >> index c517941..b9ced60 100644 >> --- a/arch/arm/mach-hisi/hotplug.c >> +++ b/arch/arm/mach-hisi/hotplug.c >> @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) >> u32 val = 0; >> >> if (!ctrl_base) >> - if (!hix5hd2_hotplug_init()) >> - BUG(); >> + BUG_ON(!hix5hd2_hotplug_init()); > >Whatever tool you're using to detect these patterns, consider improving >it, or at least giving a bit more thought to the results beyond blindly >applying one single rule - "if(x) BUG_ON(y);" arguably makes even less >sense since it's now neither one thing nor the other. > >Robin. > >> if (enable) { >> /* power on cpu1 */ >> diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c >> index 96a4840..6c90039 100644 >> --- a/arch/arm/mach-hisi/platmcpm.c >> +++ b/arch/arm/mach-hisi/platmcpm.c >> @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) >> { >> unsigned long data; >> >> - if (!fabric) >> - BUG(); >> + BUG_ON(!fabric); >> + >> data = readl_relaxed(fabric + FAB_SF_MODE); >> if (on) >> data |= 1 << cluster; >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG @ 2021-04-23 13:10 ` 周传高 0 siblings, 0 replies; 6+ messages in thread From: 周传高 @ 2021-04-23 13:10 UTC (permalink / raw) To: Robin Murphy; +Cc: Wei Xu, Russell King, linux-arm-kernel, linux-kernel >On 2021-04-23 09:14, zhouchuangao wrote: >> BUG_ON uses unlikely in if(). Through disassembly, we can see that >> brk #0x800 is compiled to the end of the function. >> As you can see below: >> ...... >> ffffff8008660bec: d65f03c0 ret >> ffffff8008660bf0: d4210000 brk #0x800 >> >> Usually, the condition in if () is not satisfied. For the >> multi-stage pipeline, we do not need to perform fetch decode >> and excute operation on brk instruction. > >32-bit Arm does not have "ret" and "brk" instructions, and either way >the relevant BUG() instruction(s) aren't executed unless the condition >is met, so this really makes very little sense. > Sorry, this is just an analysis based on ARM64. >> In my opinion, this can improve the efficiency of the >> multi-stage pipeline. > >It has very little to do with the pipeline - modern cores are >considerably more sophisticated than the 3-stage Acorn RISC Machine of >1985, and are not usually limited by frontend throughput. The point of >unlikely() is to avoid having a normally-taken forward branch to skip >over in-line code, and instead make sure the only thing in the normal >execution path is a normally-not-taken branch to handle the condition >out-of-line. Yes, the impact of branches - and thus why it can be >desirable to avoid them - is indeed *related* to pipelining, but that's >rather tangential. > >Even then, it's only worth considering things at this level in >frequently-executed and/or performance-critical code. Saving a couple of >CPU cycles in something that is effectively a one-time operation is >utterly immaterial. > >The realistic justification for these patches is that that BUG_ON() >exists for implementing conditional BUG()s, so we may as well use it if >it makes the source code more readable. > Thank you for your excellent analysis, Indeed, only in the case of Frequently Executed and/or Performance-Critical Code, the patch is of great value. Hmm...Perhaps the best reason is to make the code more readable. BR, zhouchuangao >> Signed-off-by: zhouchuangao <zhouchuangao@vivo.com> >> --- >> arch/arm/mach-hisi/hotplug.c | 3 +-- >> arch/arm/mach-hisi/platmcpm.c | 4 ++-- >> 2 files changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c >> index c517941..b9ced60 100644 >> --- a/arch/arm/mach-hisi/hotplug.c >> +++ b/arch/arm/mach-hisi/hotplug.c >> @@ -193,8 +193,7 @@ void hix5hd2_set_cpu(int cpu, bool enable) >> u32 val = 0; >> >> if (!ctrl_base) >> - if (!hix5hd2_hotplug_init()) >> - BUG(); >> + BUG_ON(!hix5hd2_hotplug_init()); > >Whatever tool you're using to detect these patterns, consider improving >it, or at least giving a bit more thought to the results beyond blindly >applying one single rule - "if(x) BUG_ON(y);" arguably makes even less >sense since it's now neither one thing nor the other. > >Robin. > >> if (enable) { >> /* power on cpu1 */ >> diff --git a/arch/arm/mach-hisi/platmcpm.c b/arch/arm/mach-hisi/platmcpm.c >> index 96a4840..6c90039 100644 >> --- a/arch/arm/mach-hisi/platmcpm.c >> +++ b/arch/arm/mach-hisi/platmcpm.c >> @@ -82,8 +82,8 @@ static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on) >> { >> unsigned long data; >> >> - if (!fabric) >> - BUG(); >> + BUG_ON(!fabric); >> + >> data = readl_relaxed(fabric + FAB_SF_MODE); >> if (on) >> data |= 1 << cluster; >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-23 13:13 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-23 8:14 [PATCH] arm/mach-hisi: Use BUG_ON instead of if condition followed by BUG zhouchuangao 2021-04-23 8:14 ` zhouchuangao 2021-04-23 12:16 ` Robin Murphy 2021-04-23 12:16 ` Robin Murphy 2021-04-23 13:10 ` 周传高 2021-04-23 13:10 ` 周传高
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.