Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

Subject: Re: [OMPI devel] [PATCH] Open MPI on ARMv5
From: Evan Clinton (evan.m.clinton_at_[hidden])
Date: 2012-04-24 20:53:31


Hi Leif, thanks for the comments.

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

> - The line
> +#if (OMPI_GCC_INLINE_ASSEMBLY || (OPAL_ASM_ARM_VERSION < 6))
> is correct, but does my head in. Could another define like
> OPAL_ARM_KUSER_BARRIERS or similar be added by the barrier definition
> and used here instead, to improve readability?
> - Could you change the line
> +#if (OPAL_ASM_ARM_VERSION >= 6 && OMPI_GCC_INLINE_ASSEMBLY)
> to
> +#if (OMPI_GCC_INLINE_ASSEMBLY && (OPAL_ASM_ARM_VERSION >= 6))
> again for readability?
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.

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

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

New patch against current trunk (r26333) attached.

(sorry Leif and possibly Jeffrey if I double-posted, messed up some
address stuff)