* [PATCH] tile: ns2cycles must be called with preempt disabled.
@ 2013-03-22 17:37 Henrik Austad
2013-03-22 19:37 ` Chris Metcalf
0 siblings, 1 reply; 6+ messages in thread
From: Henrik Austad @ 2013-03-22 17:37 UTC (permalink / raw)
To: Chris Metcalf; +Cc: LKML, Henrik Austad
ns2cycles use per_cpu variables, and will, eventually, find its way into
smp_processord_id(). The latter must be called with either preempt
disabled, irq's off, from a thread locked to a single core or early in
the bootprocess (according to debug_smp_processor_id())
BUG: using smp_processor_id() in preemptible [00000000] code: systemd-modules/367
caller is ns2cycles+0x40/0xb8
Starting stack dump of tid 367, pid 367 (systemd-modules) on cpu 2 at cycle 20969956421
frame 0: 0xfffffff70004b860 dump_stack+0x0/0x20 (sp 0xfffffe407993fa90)
frame 1: 0xfffffff7006abc28 debug_smp_processor_id+0x1a8/0x1e0 (sp 0xfffffe407993fa90)
frame 2: 0xfffffff7004d7b40 ns2cycles+0x40/0xb8 (sp 0xfffffe407993fab8)
frame 3: 0xfffffff7004dc578 __ndelay+0x38/0x80 (sp 0xfffffe407993fae0)
Note: wrapping the -entire- delay-loop in a preempt-off section can be a
bad idea as long delays would case kernel lockups without being able to
preempt the thread at all.
Signed-off-by: Henrik Austad <haustad@cisco.com>
---
arch/tile/lib/delay.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/tile/lib/delay.c b/arch/tile/lib/delay.c
index cdacdd1..a53cdcd 100644
--- a/arch/tile/lib/delay.c
+++ b/arch/tile/lib/delay.c
@@ -30,7 +30,9 @@ EXPORT_SYMBOL(__udelay);
void __ndelay(unsigned long nsecs)
{
cycles_t target = get_cycles();
+ preempt_disable();
target += ns2cycles(nsecs);
+ preempt_enable();
while (get_cycles() < target)
cpu_relax();
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tile: ns2cycles must be called with preempt disabled.
2013-03-22 17:37 [PATCH] tile: ns2cycles must be called with preempt disabled Henrik Austad
@ 2013-03-22 19:37 ` Chris Metcalf
2013-03-23 7:23 ` [PATCH] tile: ns2cycles should use __raw_get_cpu_var Henrik Austad
0 siblings, 1 reply; 6+ messages in thread
From: Chris Metcalf @ 2013-03-22 19:37 UTC (permalink / raw)
To: Henrik Austad; +Cc: LKML
On 3/22/2013 1:37 PM, Henrik Austad wrote:
> ns2cycles use per_cpu variables, and will, eventually, find its way into
> smp_processord_id(). The latter must be called with either preempt
> disabled, irq's off, from a thread locked to a single core or early in
> the bootprocess (according to debug_smp_processor_id())
A better fix would be to use __raw_get_cpu_var() in ns2cycles(), along with a comment saying that the frequency of the tile timer is the same on all cores, so it doesn't matter which copy of the per-cpu structure we look at.
I don't object to the performance hit of the preemption calls so much as the confusing aspect of why it appears we need to disable preemption to compute the value, but not to use it :-)
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] tile: ns2cycles should use __raw_get_cpu_var
2013-03-22 19:37 ` Chris Metcalf
@ 2013-03-23 7:23 ` Henrik Austad
2013-03-23 14:13 ` Chris Metcalf
0 siblings, 1 reply; 6+ messages in thread
From: Henrik Austad @ 2013-03-23 7:23 UTC (permalink / raw)
To: Chris Metcalf; +Cc: LKML, Henrik Austad
ns2cycles use per_cpu variables, and will, eventually, find its way into
smp_processord_id(). This is not safe in a preemptible kernel,
preemption should ideally be disabled.
BUG: using smp_processor_id() in preemptible [00000000] code:
systemd-modules/367
caller is ns2cycles+0x40/0xb8
Starting stack dump of tid 367, pid 367 (systemd-modules) on cpu 2 at
cycle 20969956421
frame 0: 0xfffffff70004b860 dump_stack+0x0/0x20 (sp 0xfffffe407993fa90)
frame 1: 0xfffffff7006abc28 debug_smp_processor_id+0x1a8/0x1e0 (sp
0xfffffe407993fa90)
frame 2: 0xfffffff7004d7b40 ns2cycles+0x40/0xb8 (sp 0xfffffe407993fab8)
frame 3: 0xfffffff7004dc578 __ndelay+0x38/0x80 (sp 0xfffffe407993fae0)
However, in this case:
- the frequency is the same accross all cores
- we use the data read-only
- we do not scale the frequency
Which means that we can use the __raw_get_cpu_var instead.
Signed-off-by: Henrik Austad <haustad@cisco.com>
---
arch/tile/kernel/time.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c
index 42297f6..4e87cb9 100644
--- a/arch/tile/kernel/time.c
+++ b/arch/tile/kernel/time.c
@@ -234,7 +234,9 @@ int setup_profiling_timer(unsigned int multiplier)
*/
cycles_t ns2cycles(unsigned long nsecs)
{
- struct clock_event_device *dev = &__get_cpu_var(tile_timer);
+ /* we don't have to disable preemptions here as each core has the same
+ * clock-frequency */
+ struct clock_event_device *dev = &__raw_get_cpu_var(tile_timer);
return ((u64)nsecs * dev->mult) >> dev->shift;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tile: ns2cycles should use __raw_get_cpu_var
2013-03-23 7:23 ` [PATCH] tile: ns2cycles should use __raw_get_cpu_var Henrik Austad
@ 2013-03-23 14:13 ` Chris Metcalf
2013-03-26 7:16 ` Henrik Austad
0 siblings, 1 reply; 6+ messages in thread
From: Chris Metcalf @ 2013-03-23 14:13 UTC (permalink / raw)
To: Henrik Austad; +Cc: LKML
On 3/23/2013 3:23 AM, Henrik Austad wrote:
> + /* we don't have to disable preemptions here as each core has the same
> + * clock-frequency */
Looks OK except the comment is in the wrong style. Kernel multiline comments
/*
* Should look
* like this.
*/
and use full sentences with punctuation. (Also, don't hyphenate clock frequency here.) Thanks!
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] tile: ns2cycles should use __raw_get_cpu_var
2013-03-23 14:13 ` Chris Metcalf
@ 2013-03-26 7:16 ` Henrik Austad
2013-03-26 13:14 ` Chris Metcalf
0 siblings, 1 reply; 6+ messages in thread
From: Henrik Austad @ 2013-03-26 7:16 UTC (permalink / raw)
To: Chris Metcalf; +Cc: LKML, Henrik Austad
ns2cycles use per_cpu variables, and will, eventually, find its way into
smp_processord_id(). This is not safe in a preemptible kernel,
preemption should ideally be disabled.
BUG: using smp_processor_id() in preemptible [00000000] code:
systemd-modules/367
caller is ns2cycles+0x40/0xb8
Starting stack dump of tid 367, pid 367 (systemd-modules) on cpu 2 at
cycle 20969956421
frame 0: 0xfffffff70004b860 dump_stack+0x0/0x20 (sp 0xfffffe407993fa90)
frame 1: 0xfffffff7006abc28 debug_smp_processor_id+0x1a8/0x1e0 (sp
0xfffffe407993fa90)
frame 2: 0xfffffff7004d7b40 ns2cycles+0x40/0xb8 (sp 0xfffffe407993fab8)
frame 3: 0xfffffff7004dc578 __ndelay+0x38/0x80 (sp 0xfffffe407993fae0)
However, in this case:
- the frequency is the same accross all cores
- we use the data read-only
- we do not scale the frequency
Which means that we can use the __raw_get_cpu_var instead.
Signed-off-by: Henrik Austad <haustad@cisco.com>
---
arch/tile/kernel/time.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/arch/tile/kernel/time.c b/arch/tile/kernel/time.c
index 42297f6..1dc1d61 100644
--- a/arch/tile/kernel/time.c
+++ b/arch/tile/kernel/time.c
@@ -234,7 +234,11 @@ int setup_profiling_timer(unsigned int multiplier)
*/
cycles_t ns2cycles(unsigned long nsecs)
{
- struct clock_event_device *dev = &__get_cpu_var(tile_timer);
+ /*
+ * We do not have to disable preemptions here as each core has the same
+ * clock frequency.
+ */
+ struct clock_event_device *dev = &__raw_get_cpu_var(tile_timer);
return ((u64)nsecs * dev->mult) >> dev->shift;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] tile: ns2cycles should use __raw_get_cpu_var
2013-03-26 7:16 ` Henrik Austad
@ 2013-03-26 13:14 ` Chris Metcalf
0 siblings, 0 replies; 6+ messages in thread
From: Chris Metcalf @ 2013-03-26 13:14 UTC (permalink / raw)
To: Henrik Austad; +Cc: LKML
On 3/26/2013 3:16 AM, Henrik Austad wrote:
> ns2cycles use per_cpu variables, and will, eventually, find its way into
> smp_processord_id(). This is not safe in a preemptible kernel,
> preemption should ideally be disabled.
>
> BUG: using smp_processor_id() in preemptible [00000000] code:
> systemd-modules/367
> caller is ns2cycles+0x40/0xb8
>
> Starting stack dump of tid 367, pid 367 (systemd-modules) on cpu 2 at
> cycle 20969956421
> frame 0: 0xfffffff70004b860 dump_stack+0x0/0x20 (sp 0xfffffe407993fa90)
> frame 1: 0xfffffff7006abc28 debug_smp_processor_id+0x1a8/0x1e0 (sp
> 0xfffffe407993fa90)
> frame 2: 0xfffffff7004d7b40 ns2cycles+0x40/0xb8 (sp 0xfffffe407993fab8)
> frame 3: 0xfffffff7004dc578 __ndelay+0x38/0x80 (sp 0xfffffe407993fae0)
>
> However, in this case:
>
> - the frequency is the same accross all cores
> - we use the data read-only
> - we do not scale the frequency
>
> Which means that we can use the __raw_get_cpu_var instead.
>
> Signed-off-by: Henrik Austad <haustad@cisco.com>
> ---
> arch/tile/kernel/time.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
Thanks - taken into the tile tree!
--
Chris Metcalf, Tilera Corp.
http://www.tilera.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-26 13:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-22 17:37 [PATCH] tile: ns2cycles must be called with preempt disabled Henrik Austad
2013-03-22 19:37 ` Chris Metcalf
2013-03-23 7:23 ` [PATCH] tile: ns2cycles should use __raw_get_cpu_var Henrik Austad
2013-03-23 14:13 ` Chris Metcalf
2013-03-26 7:16 ` Henrik Austad
2013-03-26 13:14 ` Chris Metcalf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox