Re: [PATCH 4/5] kgdb: Use atomic operators which use barriers

[Linus Torvalds <torvalds@linux-foundation.org>]

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