Re: [PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
On 2017/7/6 22:35, Mark Rutland wrote: > On Thu, Jul 06, 2017 at 08:49:33PM +0800, Ding Tianhong wrote: >> When enable preempt and ftrace, and perform the following steps, the system >> will hang: >> mount -t debugfs nodev /sys/kernel/debug/ >> cd /sys/kernel/debug/tracing/ >> echo function_graph > current_tracer >> >> This is because the preempt_disable and preempt_enable calls would cause >> infinite >> recursion for some chips which needs the timer erratum workaround and the >> system >> will hang, so use the preempt_disable/enable_notrace calls to prevent >> recursion. > > Nit: Please wrap commit messages at 72 characters, unless you have > something that wrapping would destroy. > OK > That aside, this looks sane, but a better expanation would be > worthwhile. What exactly causes the recursion? Does ftrace instrument > preempt_{disable,enable} ? > > Can you give an example of the recursion? > As the 96b3d28bf4(sched/clock: Prevent tracing recursion in sched_clock_cpu()) said, the preempt_disable/enable() are traced and this cause trace_clock() users to get the local timer again, it will cause the infinite recursion. > Thanks, > Mark. > >> >> Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to >> only affect a subset of CPUs") >> Signed-off-by: Ding Tianhong>> --- >> arch/arm64/include/asm/arch_timer.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/include/asm/arch_timer.h >> b/arch/arm64/include/asm/arch_timer.h >> index 74d08e4..67bb7a4 100644 >> --- a/arch/arm64/include/asm/arch_timer.h >> +++ b/arch/arm64/include/asm/arch_timer.h >> @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { >> u64 _val; \ >> if (needs_unstable_timer_counter_workaround()) {\ >> const struct arch_timer_erratum_workaround *wa; \ >> -preempt_disable(); \ >> +preempt_disable_notrace(); \ >> wa = __this_cpu_read(timer_unstable_counter_workaround); \ >> if (wa && wa->read_##reg) \ >> _val = wa->read_##reg();\ >> else\ >> _val = read_sysreg(reg);\ >> -preempt_enable(); \ >> +preempt_enable_notrace(); \ >> } else {\ >> _val = read_sysreg(reg);\ >> } \ >> -- >> 1.9.0 >> >> > > . >
Re: [PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
On 2017/7/6 22:35, Mark Rutland wrote: > On Thu, Jul 06, 2017 at 08:49:33PM +0800, Ding Tianhong wrote: >> When enable preempt and ftrace, and perform the following steps, the system >> will hang: >> mount -t debugfs nodev /sys/kernel/debug/ >> cd /sys/kernel/debug/tracing/ >> echo function_graph > current_tracer >> >> This is because the preempt_disable and preempt_enable calls would cause >> infinite >> recursion for some chips which needs the timer erratum workaround and the >> system >> will hang, so use the preempt_disable/enable_notrace calls to prevent >> recursion. > > Nit: Please wrap commit messages at 72 characters, unless you have > something that wrapping would destroy. > OK > That aside, this looks sane, but a better expanation would be > worthwhile. What exactly causes the recursion? Does ftrace instrument > preempt_{disable,enable} ? > > Can you give an example of the recursion? > As the 96b3d28bf4(sched/clock: Prevent tracing recursion in sched_clock_cpu()) said, the preempt_disable/enable() are traced and this cause trace_clock() users to get the local timer again, it will cause the infinite recursion. > Thanks, > Mark. > >> >> Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to >> only affect a subset of CPUs") >> Signed-off-by: Ding Tianhong >> --- >> arch/arm64/include/asm/arch_timer.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/include/asm/arch_timer.h >> b/arch/arm64/include/asm/arch_timer.h >> index 74d08e4..67bb7a4 100644 >> --- a/arch/arm64/include/asm/arch_timer.h >> +++ b/arch/arm64/include/asm/arch_timer.h >> @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { >> u64 _val; \ >> if (needs_unstable_timer_counter_workaround()) {\ >> const struct arch_timer_erratum_workaround *wa; \ >> -preempt_disable(); \ >> +preempt_disable_notrace(); \ >> wa = __this_cpu_read(timer_unstable_counter_workaround); \ >> if (wa && wa->read_##reg) \ >> _val = wa->read_##reg();\ >> else\ >> _val = read_sysreg(reg);\ >> -preempt_enable(); \ >> +preempt_enable_notrace(); \ >> } else {\ >> _val = read_sysreg(reg);\ >> } \ >> -- >> 1.9.0 >> >> > > . >
Re: [PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
On Thu, Jul 06, 2017 at 08:49:33PM +0800, Ding Tianhong wrote: > When enable preempt and ftrace, and perform the following steps, the system > will hang: > mount -t debugfs nodev /sys/kernel/debug/ > cd /sys/kernel/debug/tracing/ > echo function_graph > current_tracer > > This is because the preempt_disable and preempt_enable calls would cause > infinite > recursion for some chips which needs the timer erratum workaround and the > system > will hang, so use the preempt_disable/enable_notrace calls to prevent > recursion. Nit: Please wrap commit messages at 72 characters, unless you have something that wrapping would destroy. That aside, this looks sane, but a better expanation would be worthwhile. What exactly causes the recursion? Does ftrace instrument preempt_{disable,enable} ? Can you give an example of the recursion? Thanks, Mark. > > Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to > only affect a subset of CPUs") > Signed-off-by: Ding Tianhong> --- > arch/arm64/include/asm/arch_timer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h > b/arch/arm64/include/asm/arch_timer.h > index 74d08e4..67bb7a4 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { > u64 _val; \ > if (needs_unstable_timer_counter_workaround()) {\ > const struct arch_timer_erratum_workaround *wa; \ > - preempt_disable(); \ > + preempt_disable_notrace(); \ > wa = __this_cpu_read(timer_unstable_counter_workaround); \ > if (wa && wa->read_##reg) \ > _val = wa->read_##reg();\ > else\ > _val = read_sysreg(reg);\ > - preempt_enable(); \ > + preempt_enable_notrace(); \ > } else {\ > _val = read_sysreg(reg);\ > } \ > -- > 1.9.0 > >
Re: [PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
On Thu, Jul 06, 2017 at 08:49:33PM +0800, Ding Tianhong wrote: > When enable preempt and ftrace, and perform the following steps, the system > will hang: > mount -t debugfs nodev /sys/kernel/debug/ > cd /sys/kernel/debug/tracing/ > echo function_graph > current_tracer > > This is because the preempt_disable and preempt_enable calls would cause > infinite > recursion for some chips which needs the timer erratum workaround and the > system > will hang, so use the preempt_disable/enable_notrace calls to prevent > recursion. Nit: Please wrap commit messages at 72 characters, unless you have something that wrapping would destroy. That aside, this looks sane, but a better expanation would be worthwhile. What exactly causes the recursion? Does ftrace instrument preempt_{disable,enable} ? Can you give an example of the recursion? Thanks, Mark. > > Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to > only affect a subset of CPUs") > Signed-off-by: Ding Tianhong > --- > arch/arm64/include/asm/arch_timer.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h > b/arch/arm64/include/asm/arch_timer.h > index 74d08e4..67bb7a4 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { > u64 _val; \ > if (needs_unstable_timer_counter_workaround()) {\ > const struct arch_timer_erratum_workaround *wa; \ > - preempt_disable(); \ > + preempt_disable_notrace(); \ > wa = __this_cpu_read(timer_unstable_counter_workaround); \ > if (wa && wa->read_##reg) \ > _val = wa->read_##reg();\ > else\ > _val = read_sysreg(reg);\ > - preempt_enable(); \ > + preempt_enable_notrace(); \ > } else {\ > _val = read_sysreg(reg);\ > } \ > -- > 1.9.0 > >
[PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
When enable preempt and ftrace, and perform the following steps, the system will hang: mount -t debugfs nodev /sys/kernel/debug/ cd /sys/kernel/debug/tracing/ echo function_graph > current_tracer This is because the preempt_disable and preempt_enable calls would cause infinite recursion for some chips which needs the timer erratum workaround and the system will hang, so use the preempt_disable/enable_notrace calls to prevent recursion. Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs") Signed-off-by: Ding Tianhong--- arch/arm64/include/asm/arch_timer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h index 74d08e4..67bb7a4 100644 --- a/arch/arm64/include/asm/arch_timer.h +++ b/arch/arm64/include/asm/arch_timer.h @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { u64 _val; \ if (needs_unstable_timer_counter_workaround()) {\ const struct arch_timer_erratum_workaround *wa; \ - preempt_disable(); \ + preempt_disable_notrace(); \ wa = __this_cpu_read(timer_unstable_counter_workaround); \ if (wa && wa->read_##reg) \ _val = wa->read_##reg();\ else\ _val = read_sysreg(reg);\ - preempt_enable(); \ + preempt_enable_notrace(); \ } else {\ _val = read_sysreg(reg);\ } \ -- 1.9.0
[PATCH] arm64: arch_timer: fix the infinite recursion when enable ftrace and erratum workaround
When enable preempt and ftrace, and perform the following steps, the system will hang: mount -t debugfs nodev /sys/kernel/debug/ cd /sys/kernel/debug/tracing/ echo function_graph > current_tracer This is because the preempt_disable and preempt_enable calls would cause infinite recursion for some chips which needs the timer erratum workaround and the system will hang, so use the preempt_disable/enable_notrace calls to prevent recursion. Fixes: 6acc71ccac71 ("arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs") Signed-off-by: Ding Tianhong --- arch/arm64/include/asm/arch_timer.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h index 74d08e4..67bb7a4 100644 --- a/arch/arm64/include/asm/arch_timer.h +++ b/arch/arm64/include/asm/arch_timer.h @@ -65,13 +65,13 @@ struct arch_timer_erratum_workaround { u64 _val; \ if (needs_unstable_timer_counter_workaround()) {\ const struct arch_timer_erratum_workaround *wa; \ - preempt_disable(); \ + preempt_disable_notrace(); \ wa = __this_cpu_read(timer_unstable_counter_workaround); \ if (wa && wa->read_##reg) \ _val = wa->read_##reg();\ else\ _val = read_sysreg(reg);\ - preempt_enable(); \ + preempt_enable_notrace(); \ } else {\ _val = read_sysreg(reg);\ } \ -- 1.9.0