Messages in this thread | | | Date | Fri, 2 Apr 2010 12:12:48 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH 4/5] kgdb: Use atomic operators which use barriers |
| |
On Fri, 2 Apr 2010, Jason Wessel wrote: > > A cpu_relax() does not mandate that there is an smp memory barrier. > As a result on the arm smp architecture the kernel debugger can hang > on entry from time to time, as shown by the kgdb regression tests. > > The solution is simply to use the atomic operators which include a > proper smp memory barrier, instead of using atomic_set() and > atomic_read().
Hmm. While I absolutely agree that 'cpu_relax()' does not imply a memory barrier, I disagree that this change should be needed. If ARM has odd semantics where it will never see changes in a busy loop, then ARM is buggy, and that has _nothing_ to do with the Linux notion of memory barriers.
The _whole_ point of "cpu_relax()" is to have busy loops. And the point of busy loops is that they are waiting for something to change. So if this loop:
> for_each_online_cpu(i) { > - while (atomic_read(&cpu_in_kgdb[i])) > + while (atomic_add_return(0, &cpu_in_kgdb[i])) > cpu_relax(); > }
can somehow lock up because "cpu_relax()" doesn't work with an infinite "while (atomic_read(..))" loop, then the ARM implementation of cpu_relax() is buggy.
Here's a simple example of exactly these kinds of busy loops waiting for something to change using cpu_relax() from generic kernel code:
ipc/mqueue.c- while (ewp->state == STATE_PENDING) ipc/mqueue.c: cpu_relax();
ipc/msg.c- while (msg == NULL) { ipc/msg.c: cpu_relax();
kernel/sched.c- while (task_is_waking(p)) kernel/sched.c: cpu_relax();
kernel/smp.c- while (data->flags & CSD_FLAG_LOCK) kernel/smp.c: cpu_relax();
so I'd like to understand what the ARM issue is.
Does ARM have some broken cache coherency model where writes by other CPU's _never_ show up unless the reading CPU does some memory sync thing? If so, then cpu_relax() obviously does need to do that syncing instruction.
And no, that does NOT mean that "cpu_relax()" has any memory barrier semantics. All it means is that cpu_relax() obviously is some architecture-specific way of saying "I'm in a busy loop, waiting for something".
Linus
| |