From: dongdong.deng@windriver•com (DDD)
To: linux-arm-kernel@lists•infradead.org
Subject: [Kgdb-bugreport] [PATCH] ARM: change definition of cpu_relax() for ARM11MPCore
Date: Thu, 11 Mar 2010 10:47:34 +0800 [thread overview]
Message-ID: <4B9859C6.1020000@windriver.com> (raw)
In-Reply-To: <4B9817CF.2050807@windriver.com>
Jason Wessel wrote:
> Will Deacon wrote:
>
>>> Clearly, kgdb is using atomic_set()/atomic_read() in a way which does not
>>> match this documentation - it's certainly missing the barriers as required
>>> by the above quoted paragraphs.
>>>
>>> Let me repeat: atomic_set() and atomic_read() are NOT atomic. There's
>>> nothing atomic about them. All they do is provide a pair of accessors
>>> to the underlying value in the atomic type. They are no different to
>>> declaring a volatile int and reading/writing it directly.
>
>
> Clearly the docs state this.
>
>> Indeed. I'm not familiar enough with KGDB internals to dive in and look at all the
>> potential barrier conflicts, so I'll submit a patch that addresses the one that's
>> bitten me so far. Maybe it will motivate somebody else to take a closer look!
>>
>
>
> Do you think you can try the patch below?
>
> It seems we might not need to change to using the atomic_add_return(0,...) because using the atomic_inc() and atomic_dec() will end up using the memory barriers.
>
Hi Jason,
Maybe we should initial the atomic_t variable before we using such as
atomic_inc/dec() directly.
Dongdong.
--- a/kernel/kgdb.c
+++ b/kernel/kgdb.c
@@ -227,6 +227,17 @@ kgdb_post_primary_code(struct pt_regs *regs, int
e_vector, int err_code)
return;
}
+static void kgdb_initial_atomic_var()
+{
+ int i;
+ for (i = NR_CPUS-1; i >= 0; i--) {
+ atomic_set(&passive_cpu_wait[i], 0);
+ atomic_set(&cpu_in_kgdb[i], 0);
+ }
+
+ atomic_set(&kgdb_setting_breakpoint, 0);
+}
+
/**
* kgdb_disable_hw_debug - Disable hardware debugging while we in
kgdb.
* @regs: Current &struct pt_regs.
@@ -1619,6 +1630,7 @@ static void kgdb_register_callbacks(void)
{
if (!kgdb_io_module_registered) {
kgdb_io_module_registered = 1;
+ kgdb_initial_atomic_var();
kgdb_arch_init();
#ifdef CONFIG_MAGIC_SYSRQ
register_sysrq_key('g', &sysrq_gdb_op);
> I would certainly rather fix kgdb vs mucking with the internals of cpu_relax().
>
>
> Jason.
>
> --- a/kernel/kgdb.c
> +++ b/kernel/kgdb.c
> @@ -580,14 +580,13 @@ static void kgdb_wait(struct pt_regs *re
> * Make sure the above info reaches the primary CPU before
> * our cpu_in_kgdb[] flag setting does:
> */
> - smp_wmb();
> - atomic_set(&cpu_in_kgdb[cpu], 1);
> + atomic_inc(&cpu_in_kgdb[cpu]);
>
> /* Disable any cpu specific hw breakpoints */
> kgdb_disable_hw_debug(regs);
>
> /* Wait till primary CPU is done with debugging */
> - while (atomic_read(&passive_cpu_wait[cpu]))
> + while (atomic_add_return(0, &passive_cpu_wait[cpu]))
> cpu_relax();
>
> kgdb_info[cpu].debuggerinfo = NULL;
> @@ -598,7 +597,7 @@ static void kgdb_wait(struct pt_regs *re
> arch_kgdb_ops.correct_hw_break();
>
> /* Signal the primary CPU that we are done: */
> - atomic_set(&cpu_in_kgdb[cpu], 0);
> + atomic_dec(&cpu_in_kgdb[cpu]);
> touch_softlockup_watchdog_sync();
> clocksource_touch_watchdog();
> local_irq_restore(flags);
> @@ -1493,7 +1492,7 @@ acquirelock:
> * spin_lock code is good enough as a barrier so we don't
> * need one here:
> */
> - atomic_set(&cpu_in_kgdb[ks->cpu], 1);
> + atomic_inc(&cpu_in_kgdb[ks->cpu]);
>
> #ifdef CONFIG_SMP
> /* Signal the other CPUs to enter kgdb_wait() */
> @@ -1505,7 +1504,7 @@ acquirelock:
> * Wait for the other CPUs to be notified and be waiting for us:
> */
> for_each_online_cpu(i) {
> - while (!atomic_read(&cpu_in_kgdb[i]))
> + while (!atomic_add_return(0, &cpu_in_kgdb[i]))
> cpu_relax();
> }
>
> @@ -1528,7 +1527,7 @@ acquirelock:
>
> kgdb_info[ks->cpu].debuggerinfo = NULL;
> kgdb_info[ks->cpu].task = NULL;
> - atomic_set(&cpu_in_kgdb[ks->cpu], 0);
> + atomic_dec(&cpu_in_kgdb[ks->cpu]);
>
> if (!kgdb_single_step) {
> for (i = NR_CPUS-1; i >= 0; i--)
> @@ -1538,7 +1537,7 @@ acquirelock:
> * from the debugger.
> */
> for_each_online_cpu(i) {
> - while (atomic_read(&cpu_in_kgdb[i]))
> + while (atomic_add_return(0, &cpu_in_kgdb[i]))
> cpu_relax();
> }
> }
> @@ -1742,11 +1741,11 @@ EXPORT_SYMBOL_GPL(kgdb_unregister_io_mod
> */
> void kgdb_breakpoint(void)
> {
> - atomic_set(&kgdb_setting_breakpoint, 1);
> + atomic_inc(&kgdb_setting_breakpoint);
> wmb(); /* Sync point before breakpoint */
> arch_kgdb_breakpoint();
> wmb(); /* Sync point after breakpoint */
> - atomic_set(&kgdb_setting_breakpoint, 0);
> + atomic_dec(&kgdb_setting_breakpoint);
> }
> EXPORT_SYMBOL_GPL(kgdb_breakpoint);
>
>
> ------------------------------------------------------------------------------
> Download Intel® Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgdb-bugreport at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
>
next prev parent reply other threads:[~2010-03-11 2:47 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-09 16:06 [PATCH] ARM: change definition of cpu_relax() for ARM11MPCore Will Deacon
2010-03-09 16:22 ` Russell King - ARM Linux
2010-03-09 16:35 ` Will Deacon
2010-03-09 16:49 ` Russell King - ARM Linux
2010-03-09 17:59 ` Will Deacon
2010-03-10 22:06 ` [Kgdb-bugreport] " Jason Wessel
2010-03-11 2:47 ` DDD [this message]
2010-03-11 13:53 ` Will Deacon
2010-03-11 13:29 ` Will Deacon
2010-03-11 14:51 ` Will Deacon
2010-03-16 17:26 ` Will Deacon
2010-03-16 18:52 ` Jason Wessel
-- strict thread matches above, loose matches on Subject: below --
2010-04-12 17:23 Will Deacon
2010-04-12 17:32 ` Russell King - ARM Linux
2010-04-15 17:36 ` [Kgdb-bugreport] " George G. Davis
2010-04-15 21:03 ` Jamie Lokier
2010-04-16 13:54 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B9859C6.1020000@windriver.com \
--to=dongdong.deng@windriver$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox