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

[Jason Wessel]

On 04/02/2010 02:12 PM, Linus Torvalds wrote:
>
>   
> 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();
>>  		}
>>     
>
>   

So this part might be overkill, but I don't actually have the hardware,
schematics or reference manuals to ascertain what is going on.  The
other changes in this patch should be correct because we really do want
memory barriers which come along with the inc and dec operators.

> 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.
>
>   

Will originally proposed a patch for cpu_relax:
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011076.html
--- The patch ---

+#if __LINUX_ARM_ARCH__ == 6
+#define cpu_relax()			smp_mb()
+#else
 #define cpu_relax()			barrier()
+#endif

---

Russell had this thread: 
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/75717

--- clip from url ---

cpu_relax() is also defined to be a compiler barrier so that the compiler
reloads the variable on every iteration.

> This patch changes the definition of cpu_relax() to smp_mb() for ARMv6 cores,
> forcing a flushing of the write buffer on SMP systems. If the Kernel is not
> compiled for SMP support, this will expand to a barrier() as before.

I don't think this is correct.  You're making a macro do something on ARM
which no other platform, apart from blackfin (which I believe is wrong)
makes it do.

---

> 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.
>
>   

Given your statements, I can just keep the atomic reads as they were
previously, but keep the inc and dec parts.  And we can wait for a
further response from either Will or Russell.

Thanks,
Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/