On 04/25/12 01:50, Evan Clinton wrote:
>> - The v5 code doesn't actually make use of the kuser helper barriers
>> in its versions of opal_atomic_cmpset_acq/rel.
> Quote the documentation, __kuser_cmpxchg "already includes memory
> barriers as needed," so the code using it shouldn't need anything
Fair enough - but could you put a comment to this effect into the patch?
> Tweaked the patch, should be a little nicer now.
>> - The patch does not do a runtime test for kernel helper version. While
>> normally not a problem, this could cause tricky to debug issues if
>> running on top of old kernels (as in "executing uninitialized
>> memory" tricky).
> I'm not familiar enough with the codebase to know where it should go,
> and I suspect the obvious check based on the __kuser_helper_version
> will not be available on kernels much older than the ones with the
> features we need anyway. We'd only have a problem on kernels older
> than 2.6.15, which was released in January of 2006, and I'm only
> confident we'd have __kuser_help_version from 2.6.12 onward.
Hmm, true enough...
But I'm still not too happy about even the very unlikely risk of
something executing "random stuff" depending on kernel version.
For now, could you update the comments to that effect that:
"These kernel functions are available on kernel versions 2.6.15 and
greater" + ", and running this software on earlier versions will result
in undefined behaviour."
>> - This patch does not include out-of-line assembly versions
>> (in opal/asm/base) for the atomic operations. However I am not sure
>> if this causes any practical problems.
> As far as I can tell, this isn't an issue, since no actual new
> assembly is added or changed.
It doesn't cause any problems with existing code - so what I'm really
saying is that I don't know that this won't cause issues specifically
for these new targets. Unless Jeff objects, I'm happy.
>> - If 64-bit atomics are desirable, these are actually possible on most
>> ARMv6 platforms (including the Raspberry PI), so a configure test on
>> whether LDREXD assembles without errors could be used to detect this.
> I'd add another case to the architecture detection in the config
> script, but I'm not sure what strings to match on (i.e. uname outputs)
> for detecting presence of LDREXD, and I'm not aware of any particular
> benefit to having the 64bit functions.
What I'm suggesting is not to parse information out of the build host,
but rather using the configured toolchain and options and try to
assemble the 64-bit atomic instructions. This would make it easy to
rebuild a generic ARMv6 package to support 64-bit atomics by just adding
-march=armv6zk to CFLAGS.
One use-case for the 64-bit atomics is in lockless list algorithms where
they let you update two pointers or a pointer and a semaphore
atomically. Whether such code would realistically use Open MPI
primitives, I'm not the right person to say.
But this is not something that needs doing for this patch to be
accepted. If you can add the two comments, I'm happy for this to go in.
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.