UCX: Adding support for RISC-V architecture; RV64G by jleidel · Pull Request #8246 · openucx/ucx

jleidel · 2022-05-19T17:39:55Z

What

Adding support for the RISC-V architecture to OpenUCX. This includes support for generic RV64G (IMAFDC) micro architectures. Core development by @ct-clmsn and @jleidel

Why ?

Adds support for RISC-V architecture across all subcomponents. This includes support for basic build infrastructure, tooling, ucg, ucm, ucm/bistro, ucp, ucs and uct. Tested using the in-situ test suite on actual RISC-V RV64G hardware in a cluster configuration (see below for configuration details).

How ?

All development and testing was performed using the following parameters:

SiFive HiFive Unmatched boards
Ubuntu 21.04
GCC 10.3.0

shamisp · 2022-05-19T20:59:44Z

@yosefe - @jleidel 's company has signed CLA.

yosefe

as general comment need to update ucx docx (under docs folder) to show riscv support and also see how can we add riscv to CI

yosefe · 2022-05-20T09:09:04Z

src/ucm/bistro/bistro.c

    }

+#if defined(__riscv)
+    __sync_synchronize();


maybe sync on all platforms? or use memory barriers macros?
(to avoid if defined here)

the riscv processor has a fence.i instruction synchronizing both the instruction and data caches; it exists for self modifying code (JITs). would you and @shamisp be open to each architecture providing a macro that is selectively implemented to optionally support this processor feature?

yes; for now it can be empty for all other architectures

@yosefe what would be a good name for the macro? ucm_ifence?

I have it implemented for arm, but we had no reason to expose it outside of arch:

ucx/src/ucs/arch/aarch64/cpu.h

Line 39 in 5761527

#define ucs_aarch64_isb(_op) asm volatile ("isb " #_op ::: "memory")

We have #define ucs_memory_cpu_fence and I think this one can follow similar naming convention #define ucs_instruction_cpu_fence

I'm quire a bit confused why we need I-berrier here. Eventually, when patched applied we invoke ucs_clear_cache which flushes the I-cache.

@ct-clmsn Do you still need the if defined, even after fixing cache flush ?

bumping up the above question @ct-clmsn :)

yosefe · 2022-05-20T09:10:02Z

src/ucm/util/log.c

+               }
+               else if (trace_idx == 0) {


there are few code style issues, pls see https://github.com/openucx/ucx/wiki/Code-style-checking

found and fixed indentation issues. we had errors running the .clang-format script.

yosefe · 2022-05-20T09:10:47Z

src/ucs/arch/riscv/atomic.h

+#define UCS_RISCV64_ATOMIC_H_
+
+#define UCS_DEFINE_ATOMIC_ADD(wordsize, suffix) \
+    static inline void ucs_atomic_add##wordsize(volatile uint##wordsize##_t *ptr, \


do we really want to define these, or could rely on compiler __builtin_XX constructs?

Did a little quick grep work and saw that these files have dependencies on the functions generated by the macros:

src/ucp/wireup/wireup_ep.c

src/ucs/memory/rcache.c

src/ucs/async/async.c

src/ucs/async/thread.c

src/uct/sm/base/sm_ep.c

not sure what the right answer looks like; this implementation attempts to maintain consistency with the other architecture implementations.

see

ucx/src/ucs/arch/atomic.h

Line 17 in 1a9873a

# include "generic/atomic.h"

, maybe we can use generic/atomic.h for riscv as well

yosefe · 2022-05-20T09:11:54Z

src/ucs/arch/riscv/bitops.h

+* See file LICENSE for terms.
+*/
+
+#ifndef UCS_RV64_BITOPS_H_


UCS_RISCV_BITOPS_
let's use RISCV name throughout the code and not RV64

@jleidel @yosefe looking at linux source code I saw that in some places they use RISCV and in other RV64.
It seems line RV64 is an equivalent of aarch64, which actually describes the arch better.
In some other places I see people using riscv64. Anyway, if the code only works for RV64 ISA then maybe it is better to use RV64

fixed these issues. @shamisp; prior to submitting the patch, I chatted with @jleidel about the naming convention rv64 or riscv. we landed on riscv; we have a similar set of concerns and observations. if ya'll want or prefer rv64 that's an easy enough modification.

from my perspective it does not matter as long as it's consistent

@shamisp which is your preference? rv64 or riscv?

@ct-clmsn my approach similar to Yossis' - as long as it is consistent it is all good. if it only works on 64b platforms IMHO rv64 is better name, but this is really up to you.

yosefe · 2022-05-20T09:12:27Z

src/ucs/arch/riscv/cpu.c

+    }
+
+    while (fgets(buf, sizeof(buf), f)) {
+        if (sscanf(buf, "uarch           : %s", value) == 1) {


no need so many spaces before :

fixed these issues.

yosefe · 2022-05-20T09:13:28Z

src/ucs/arch/riscv/cpu.c

+    fclose(f);
+}
+
+void ucs_rv64_cpuid(ucs_rv64_cpuid_t *cpuid)


replace rv64 by riscv in all places

@shamisp 's comment about riscv or rv64 applies here. will need to sort out naming convention.

yosefe · 2022-05-20T09:13:58Z

src/ucs/arch/riscv/cpu.h

+}
+
+
+//double ucs_rv64_init_tsc_freq();


removed this one.

yosefe · 2022-05-20T09:14:06Z

src/ucs/arch/riscv/cpu.h

+#define ucs_memory_cpu_load_fence()   ucs_compiler_fence()
+#define ucs_memory_cpu_wc_fence()     asm volatile ("fence w,w" ::: "memory")
+
+static inline double ucs_arch_get_clocks_per_sec()


UCS_F_ALWAYS_INLINE

fixed this one.

yosefe · 2022-05-20T09:14:31Z

src/ucs/arch/riscv/cpu.h

+	 char isa[256];
+	 char mmu[256];
+	 char uarch[256];


how is the cpuid used?

it's used to load data from the operating system as required by ucs; I've seen some debugging messages use this information. I'd like to retain it, removing it would be a fair amount of work.

i don't see any debug message using "uarch" or "mmu" for example, can you pls point to it?

This was implemented because the ARM support had ucs_aarch64_cpuid_t. ucs_aarch64_cpuid_t exists because of an #ifdef __aarch64__ in ./uct/ib/base/ib_device.c; an assumption was made that riscv64 would require similar work.

The type and the code that initializes the type are used in the riscv64 version of ucs_arch_get_cpu_vendor. ucs_arch_get_cpu_vendor is called in several places but the information stored in the type is never actually consumed or used.

This structure and that code have been removed.

yosefe · 2022-05-20T09:15:39Z

src/ucs/arch/riscv/global_opts.c

+ucs_config_field_t ucs_arch_global_opts_table[] = {
+#if ENABLE_BUILTIN_MEMCPY
+  {"BUILTIN_MEMCPY_MIN", "auto",
+   "Minimal threshold of buffer length for using built-in memcpy.",
+   ucs_offsetof(ucs_arch_global_opts_t, builtin_memcpy_min), UCS_CONFIG_TYPE_MEMUNITS},
+
+  {"BUILTIN_MEMCPY_MAX", "auto",
+   "Maximal threshold of buffer length for using built-in memcpy.",
+   ucs_offsetof(ucs_arch_global_opts_t, builtin_memcpy_max), UCS_CONFIG_TYPE_MEMUNITS},
+#endif
+  {NULL}
+};


are we going to have special memcpy for riscv?

A run of ucx_info, ucx_perf, etc without this call causes illegal instruction segmentation faults. I'd like to retain this code.

aarch64 for example does not have it

Quick note, there is still a tremendous amount of experimental code and/or lacking features in portions of the RISC-V system software and tool chains. Went back through the patch, updated some fence.i locations, and was able to remove this successfully.

shamisp · 2022-05-25T03:23:51Z

src/ucs/arch/cpu.h

 #elif defined(__aarch64__)
 #  include "aarch64/cpu.h"
+#elif defined(__riscv)
+#  ifdef HAVE___CLEAR_CACHE


What is the problem with built-in clear cache ?

It doesn't necessarily clear the cache in the manner expected. The particular implementation of RISC-V that we're testing on predates the ratification of the CMOBase set of extensions (Zicbom, Zicbop, Zicboz) that implement explicit cache flush operations. Given that the community was well aware of the forthcoming CMOBase infrastructure (ratified in Nov 2021), the toolchain and system libs have somewhat lagged their support of particular builtins. They're "implemented", but not fully implemented. This is quite literally a situation where we're running on rather early hardware/software.

shamisp · 2022-05-25T03:25:51Z

src/ucs/arch/riscv/cpu.h

+    return UCS_ERR_UNSUPPORTED;
+}
+
+#if !HAVE___CLEAR_CACHE


In the above code you undefined HAVE___CLEAR_CACHE , so it is never defined.

Thank you for pointing this out - this issue has been fixed.

shamisp · 2022-05-25T03:27:22Z

src/ucs/arch/riscv/cpu.h

+#if !HAVE___CLEAR_CACHE
+static inline void ucs_arch_clear_cache(void *start, void *end)
+{
+    __riscv_flush_icache(start, end, 0);


I suspect clear cache maps to the same call. IMHO something really broken here and this is why you had to introduce fence.i in multiple places.

@jleidel's response clarifies things a bit. gcc has 2 intrinsics for barriers on both caches. __sync_synchronize for data cache and __riscv_flush_icache for instruction cache. It was a little unclear which cache is flushed by this particular ucs function; the naming conventions used in the macros for memory barriers are really nice.

Additionally, I've done some runtime gdb disassembly of both (__sync_synchronize , __riscv_flush_icache ) intrinsics; the disassembly for both intrinsics have no reference to the fence.i instruction. The disassembly looks like an attempt to perform both memory barriers using software (ie: loop until the cache clears) as opposed to using the hardware instruction.

This is the reference I mentioned on the call:

ucx/src/ucs/arch/aarch64/cpu.h

Line 157 in 5761527

static inline void ucs_arch_clear_cache(void *start, void *end)

ct-clmsn · 2022-06-08T15:27:54Z

@shamisp @yosefe quick update: several modifications have been made and pushed into this PR.

yosefe · 2022-06-08T15:37:02Z

@ct-clmsn can you pls fix the commit title according to code style https://github.com/openucx/ucx/wiki/Guidance-for-contributors item 3?
(would need to force-push to resolve)

ct-clmsn · 2022-06-08T16:20:42Z

@yosefe change the automated commit message for the merge (commit # a3e2624)?

yosefe · 2022-06-08T16:26:03Z

Not the merge commit; the other ones.

ct-clmsn · 2022-06-08T20:37:40Z

@yosefe apologies for the 2 rebases - found an issue with a single message. I believe the commit titles/messages are now updated to meet the specification.

yosefe · 2022-06-12T08:52:11Z

src/ucm/mmap/install.c

+     do {                                                                      \
+         int exp_events = (_event) & (_mask);                                  \
+         (_data)->fired_events = 0;                                            \
+         ucs_rv64_isb();                                                       \


seems the only difference here is ucs_rv64_isb()
so better define a memory barrier macro, empty in all architectures except rv64, and use it.

well, ISB is actually and an instruction on Arm.

Why do you think instruction barrier is needed here - from the code is not as obvious.

The RV64 manual provides RV64 mappings to fence instructions offered on other architectures (ARM, PPC, x86, etc). I implemented the ARM flavors of fences (as per the RV64 mapping) to provide something that looks familiar to ya'll. RV64's data cache flush support is not implemented in the available hardware; the ISA extension for data cache flush was ratified after the available hardware was released. The hardware vendor has implemented their own, non-ISA, instruction cache 'discard'. The non-ISA 'discard' instruction does not perform a 'write back' to memory when invoked; the cache flushes and memory is not touched. 'discard' is not a data cache flush to use in bistro because the instruction is vendor specific and does not 'write back' to memory. The official ISA instruction cache flush is implemented in the available hardware. Segmentation faults happen when fences and the icache flush are not placed before the first time a function modified by bistro is invoked.

yosefe · 2022-06-12T08:53:46Z

src/ucm/mmap/install.c

+#if defined(__riscv)
+        ucs_arch_clear_cache(p, p+ucm_get_page_size());
+#endif


why clear cache is needed here? and why only on rv64?
(trying to remove #if from the code as much as possible to generic arch macros)

Segmentation faults happen when fences and the icache flush are not placed before the first time a bistro modified function is invoked.

keep the cache flush. but make it unconditional, i.e do it for all architectures

would a macro like the following suffice?

UCM_INVOKE(mmap, ...);

On RV64, cache flushes could be added by the macro prior to mmap(...) getting invoked. On the non-RV64 platform, the cacheflushes would not occur.

@ct-clmsn should be something like:

if (events & (UCM_EVENT_SHMAT|UCM_EVENT_SHMDT|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) { ucs_arch_sync_inst_cache(p, p + ucm_get_page_size()); shmid = shmget(IPC_PRIVATE, ucm_get_page_size(), IPC_CREAT | SHM_R | SHM_W); ... }

And ucs_arch_sync_inst_cache() is defined to ucs_arch_clear_cache on rv4, empty on other archs.

However, looking at the code more, why is this needed only before shmget and not in other places?

yosefe · 2022-06-12T08:54:32Z

src/ucm/util/replace.h

 #define UCM_DEFINE_REPLACE_FUNC(_name, _rettype, _fail_val, ...) \
    _UCM_DEFINE_REPLACE_FUNC(ucm_override_##_name, ucm_##_name, _rettype, _fail_val, __VA_ARGS__)

+#if defined(__riscv)


is the difference only ucm_trace? why needed to change?

Found an issue with ucm_trace on RV64, there is a ~~memcpy~~/strlen in ucm_trace that is causing segmentation faults. Removing the ucm_trace fixed the issue. ~~This is a side effect of the lack of a data cache flush implementation in the hardware.~~/This strlen issue is why ucm_trace is removed in src/ucm/mmap/install.c.

which memcpy was it?

apologies, the issue wasn't memcpy, the segmentation fault happens in ucm_log_vsnprintf; which is called by ucm_trace under the hood. There is a situation on the RV64 platform where the strlen on the line linked to above causes a segfault. I have a custom RV64 patch for ucm_log_vsnprintf to fix the strlen issue, not sure if ya'll would want to provide a custom architecture fix for that function; at the time it seemed less controversial to provide an architecture specific macro that dropped ucm_trace.

@ct-clmsn what is the reason strlen fails on rv64 and not on other archs?
what is the fix?
IMO better try to fix the underlying issue rather and working around it

I agree with @yosefe suggestion. Let's fix the actual problem and not hack around it.

yosefe · 2022-06-12T08:56:10Z

src/ucs/arch/rv64/atomic.h

+* See file LICENSE for terms.
+*/
+
+#ifndef UCS_RISCV64_ATOMIC_H_


seems this file is not needed anymore

yosefe · 2022-06-12T08:56:46Z

src/ucs/arch/rv64/bitops.h

+    union input {
+        uint64_t value;
+        struct pvalue {
+            uint32_t rhs;
+            uint32_t lhs;
+        } pvalue;
+    } n_sto = { .value = n };
+
+    const int lhs = __builtin_ctz(n_sto.pvalue.lhs);
+    const int rhs = __builtin_ctz(n_sto.pvalue.rhs);
+
+    int val = rhs;
+    if(rhs < 0) {
+        val = 32 + lhs;
+    }


pls fix code style according to clang-format

yosefe · 2022-06-12T08:58:23Z

src/ucs/arch/rv64/cpu.h

+    return (double) freq;
+}
+
+static inline ucs_cpu_model_t ucs_arch_get_cpu_model()


one more space line before

yosefe · 2022-06-12T08:58:27Z

src/ucs/arch/rv64/cpu.h

+    return UCS_CPU_FLAG_UNKNOWN;
+}
+
+static inline void ucs_cpu_init()


one more space line before
use 2 space lines in H file (in multiple other places as well)

yosefe · 2022-06-12T08:58:57Z

src/ucs/arch/rv64/cpu.h

+
+#define ucs_arch_wait_mem ucs_arch_generic_wait_mem
+
+static inline void ucs_arch_clear_cache(void * start, void * end) {


yosefe · 2022-06-12T09:03:16Z

src/ucs/arch/rv64/global_opts.h

+#define UCS_ARCH_GLOBAL_OPTS_INITALIZER {   \
+    .builtin_memcpy_min = UCS_MEMUNITS_AUTO, \
+    .builtin_memcpy_max = UCS_MEMUNITS_AUTO  \
+}
+
+/* built-in memcpy config */
+typedef struct ucs_arch_global_opts {
+    size_t builtin_memcpy_min;
+    size_t builtin_memcpy_max;
+} ucs_arch_global_opts_t;


can remove builtin memcpy options for rv64, it's not used anyway
for example aarch64 does not have it

thought this was removed previously, looks like the last push didn't remove it (merge artifact). will fix promptly.

yosefe · 2022-06-12T09:03:56Z

test/gtest/uct/ib/test_ud.cc

 UCT_INSTANTIATE_UD_TEST_CASE(test_ud)

+
+class test_ud_peer_failure : public ud_base_test {


is this a merge artifact? if not why needed to change UD tests?

merge artifact; will fix.

yosefe · 2022-06-17T13:21:54Z

src/ucm/mmap/install.c

+        ucs_arch_clear_cache(p, p+ucm_get_page_size());
+#endif
        shmid = shmget(IPC_PRIVATE, ucm_get_page_size(), IPC_CREAT | SHM_R | SHM_W);
+


remove the spaceline

yosefe · 2022-06-17T13:23:03Z

src/ucm/mmap/install.c

+#if defined(__riscv)
+        ucs_arch_clear_cache(p, p+ucm_get_page_size());
+#endif


@ct-clmsn should be something like:

if (events & (UCM_EVENT_SHMAT|UCM_EVENT_SHMDT|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) { ucs_arch_sync_inst_cache(p, p + ucm_get_page_size()); shmid = shmget(IPC_PRIVATE, ucm_get_page_size(), IPC_CREAT | SHM_R | SHM_W); ... }

And ucs_arch_sync_inst_cache() is defined to ucs_arch_clear_cache on rv4, empty on other archs.

However, looking at the code more, why is this needed only before shmget and not in other places?

shamisp · 2022-08-04T20:38:29Z

src/ucm/util/replace.h

+#define _UCM_DEFINE_REPLACE_FUNC(_over_name, _ucm_name, _rettype, _fail_val, ...) \
+    \
+    /* Define a symbol which goes to the replacement - in case we are loaded first */ \
+    _rettype _over_name(UCM_FUNC_DEFINE_ARGS(__VA_ARGS__)) \


it seems like only the difference here is ucm_trace call ? What is the problem with the trace call ?

Sorry I missed the above discussion

shamisp · 2022-08-04T21:03:55Z

src/ucm/mmap/install.c

+     do {                                                                      \
+         int exp_events = (_event) & (_mask);                                  \
+         (_data)->fired_events = 0;                                            \
+         ucs_rv64_isb();                                                       \


Hmm, why ISB is needed here ? I don't see any obvious reason.
If there is good reason probably it is required on all architectures.

More I read it and more I don't understand it. The ISB if followed by system call that switches to privilege mode. In theory it should include ISB and memory barrier. Out curiosity, have you tried to replace it with memory write barrier ?

ct-clmsn · 2022-08-09T21:30:09Z

@shamisp @yosefe did some testing today. It turns out riscv64's implementation of brk is not equivalent to syscall(SYS_brk, ...). This is the source of several issues and work arounds you have seen in the patch. I'm in the process of implementing an architecture specific solution in the function ucm_brk_syscall in src/util/sys.c around line 378. ucm_brk_syscall already has architecture specific code for calling syscall(SYS_brk, ...) for x86; comments in the function state architecture specific code should be used in the implementation. This should resolve all of the issues above, with exception to any outstanding character-spacing issues.

ct-clmsn · 2022-08-11T18:58:51Z

@yosefe @shamisp ucm_log_vsnprintf (in the file src/ucm/util/log.c) accepts an argument va_list ap. A line of code value.s = va_arg(ap, char *); expects va_arg to return NULL or 0 when it currently does not behave like this on RV64. Working on a solution.

jleidel · 2022-08-18T18:03:48Z

At this point, I believe we have exhausted all possible options for removing architecture-specific I$ flushes in the RV64 implementation. Given that this is currently the "most" functional platform we have available for UCX prototyping, we limited in our options with respect to removing architecture specific code from non-architecture specific sections. Our current platform lacks certain support for hardware-driven cache flushes (RV-CMOBASE extension), compat headers (for libfabrics support; kernel 5.11.0-1028) and certain ELF extensions (DT_JMPREL is missing in binutils). We've tried all combinations of architecture-agnostic user and system calls to arrive at a solution. Many of which would potentially induce performance issues in future platform variants. Given the "experimental" nature of RISC-V platforms in HPC, we would like to request that we allow this PR to merge as is. This would allow everyone in the RISC-V community to make forward progress on other HPC runtime/comm layers. However, we will also sign up to amend the code once we have a more functional/stable baseline platform to use for tests and experiments. The RISC-V "platforms" and "profiles" specs will define specific extension requirements that will ease the pain of porting complex runtimes such as OpenUCX to future RISC-V variants.

shamisp · 2022-08-19T13:14:27Z

At this point, I believe we have exhausted all possible options for removing architecture-specific I$ flushes in the RV64 implementation. Given that this is currently the "most" functional platform we have available for UCX prototyping, we limited in our options with respect to removing architecture specific code from non-architecture specific sections. Our current platform lacks certain support for hardware-driven cache flushes (RV-CMOBASE extension), compat headers (for libfabrics support; kernel 5.11.0-1028) and certain ELF extensions (DT_JMPREL is missing in binutils). We've tried all combinations of architecture-agnostic user and system calls to arrive at a solution. Many of which would potentially induce performance issues in future platform variants. Given the "experimental" nature of RISC-V platforms in HPC, we would like to request that we allow this PR to merge as is. This would allow everyone in the RISC-V community to make forward progress on other HPC runtime/comm layers. However, we will also sign up to amend the code once we have a more functional/stable baseline platform to use for tests and experiments. The RISC-V "platforms" and "profiles" specs will define specific extension requirements that will ease the pain of porting complex runtimes such as OpenUCX to future RISC-V variants.

@yosefe @ct-clmsn @jleidel Since the u-arch/arch issues cannot be resolved what about moving parts of the abstraction that got ifdef-ed (UCM_FIRE_EVENT, ucs_arch_clear_cache, _UCM_DEFINE_REPLACE_FUNC) to arch files in bistro. This way we can have one generic implementation and RV64 will have an option to overwrite those without ifdefs in the main code. @yosefe ?

ct-clmsn · 2023-07-13T15:16:00Z

@shamisp @yosefe I've made some updates to the bistro support and placed compile guards around some of the openmp code.

The icache issues have been resolved.

The modifications have been tested against a sifive unmatched and a pine64/star64 device. The code runs successfully on both devices.

Also, I've merged the implementation against master so, it is ready to go.

yosefe · 2023-07-26T13:26:47Z

@ct-clmsn can you pls fix the commit title? would need force-push

ct-clmsn · 2023-07-26T13:35:44Z

@yosefe apologies, to clarify this commit? 06a1c3f

rbradford · 2023-07-26T13:42:48Z

src/ucm/bistro/bistro_rv64.c

+ /**
+  * @brief JALR - Add 12 bit immediate to source register, save to destination register, jump and link from destination register
+  *
+  * @param[in] _reg  register number (0-31), @param[out] _reg register number (0-31), @param[imm] 12 bit immmediate value


I fixed this in f9083eb

These are not out variables in the sense that doxygen would consider it. Also there is no "imm" type.

This limit is no longer needed after moving protov1 to ucp rcache, and replacing datatype registration with ucp_memh pointer.

…m alltoall

Need to always pass mr_id on rc_verbs transport, even if flush_remote is disabled. On machines with relaxed ordering and DevX md, we would use the atomic-KSM key for put operations after a fence. So need to use the right offset.

The active message lane may be used only for control messages, and not send data during rendezvous protocol. Signed-off-by: Yossi Itigin <yosefe@nvidia.com>

Low byte of flush mkey (aka atomic mr_id) can be 0. When unified mode is used, UCP uses local iface query to know the length of ep address. If one of the peers atomic mr_id is 0 while another is not, UCP uses worng ep address length and incorrectly unpacks UCP address.

Use simplified versions of mem_reg/dereg/pack, that do not rely on uct_ib_md_ops_t callbacks. Update some unit tests that called IB functions directly.

Signed-off-by: Liu, Changcheng <jerrliu@nvidia.com>

Need to always pass mr_id on rc_verbs transport, even if flush_remote is disabled. On machines with relaxed ordering and DevX md, we would use the atomic-KSM key for put operations after a fence. So need to use the right offset.

Low byte of flush mkey (aka atomic mr_id) can be 0. When unified mode is used, UCP uses local iface query to know the length of ep address. If one of the peers atomic mr_id is 0 while another is not, UCP uses worng ep address length and incorrectly unpacks UCP address.

Use simplified versions of mem_reg/dereg/pack, that do not rely on uct_ib_md_ops_t callbacks. Update some unit tests that called IB functions directly.

ct-clmsn · 2023-07-28T18:04:24Z

@yosefe @jleidel @shamisp UCX has changed significantly since this PR was proposed. This PR is OBE (overcome by events) and I'd encourage merging #9168 as it's a more up to date proposal.

yosefe reviewed May 20, 2022

View reviewed changes

shamisp reviewed May 25, 2022

View reviewed changes

ct-clmsn force-pushed the rv64-support branch 2 times, most recently from 9bbfaed to b84d18d Compare June 8, 2022 20:36

yosefe reviewed Jun 12, 2022

View reviewed changes

yosefe reviewed Jun 17, 2022

View reviewed changes

shamisp reviewed Aug 4, 2022

View reviewed changes

ct-clmsn mentioned this pull request Apr 19, 2023

riscv (rv64g) support tactcomplabs/libfabric#1

Merged

ct-clmsn force-pushed the rv64-support branch 2 times, most recently from f2d9c2b to 06a1c3f Compare July 19, 2023 14:49

ct-clmsn mentioned this pull request Jul 26, 2023

UCX: Add support for RISC-V #9168

Merged

rbradford reviewed Jul 26, 2023

View reviewed changes

ct-clmsn force-pushed the rv64-support branch 2 times, most recently from cffb8a6 to 942f468 Compare July 26, 2023 13:55

Alexey-Rivkin and others added 26 commits July 26, 2023 12:59

INFRA: Log zombie kill events

920954a

UCT/CUDA/CUDA_IPC: Minor code improvements.

db4c72e

UCP/EAGER: Remove redundant declaration

8499b1c

UCP/WIREUP/TEST: Remove UCP_MAX_OP_MDS limit

a18c146

This limit is no longer needed after moving protov1 to ucp rcache, and replacing datatype registration with ucp_memh pointer.

UCP/PROTO: Assert in case of invalid user header size.

be48140

CONTRIB/MODULE: Provide development modulefile

62e6c5d

UCP/PROTO: Adapt FIFO size and RTS message to help at high parallelis…

8f32804

…m alltoall

GTEST/UCP: Skipped AM datatype duplicate tests.

1ae6e34

GTEST/UCP: Skipped AM datatype test for ud_v and ud_x.

8d1475e

UCT/IB: Fix remote access error with rc_verbs transport

63b6c24

Need to always pass mr_id on rc_verbs transport, even if flush_remote is disabled. On machines with relaxed ordering and DevX md, we would use the atomic-KSM key for put operations after a fence. So need to use the right offset.

CONFIG/M4: Separated library flags for CUDA, CUDA RT, and NVML.

d5dd71a

TEST/TAG: Fix multi_rail_max.max_lanes test

a201b09

The active message lane may be used only for control messages, and not send data during rendezvous protocol. Signed-off-by: Yossi Itigin <yosefe@nvidia.com>

CONTRIB: Removed extra arguments from running gtest on valgrind.

aa0e402

UCP/CORE: Check length of user header in ucp_am_send_nbx.

4ab94cb

UCP/TAG: Reduce trace level of tag_probe

851b8d6

UCT/IB/MLX5: Refactor verbs and mlx5dv memory domains operations

8b791d8

Use simplified versions of mem_reg/dereg/pack, that do not rely on uct_ib_md_ops_t callbacks. Update some unit tests that called IB functions directly.

UCT/IB: add flow_label info in ah_attr output string

be5f395

Signed-off-by: Liu, Changcheng <jerrliu@nvidia.com>

UCP: Improve ep config info

b493422

JUCX: Add methods to query local and remote address from endpoint

afda21d

UCT/CUDA/GDR_COPY: Cleaned up configure and makefile.

8a5da0b

UCP/PROTO: Assert in case of invalid user header size.

0186828

UCT/IB: Fix remote access error with rc_verbs transport

7b9b2e9

Need to always pass mr_id on rc_verbs transport, even if flush_remote is disabled. On machines with relaxed ordering and DevX md, we would use the atomic-KSM key for put operations after a fence. So need to use the right offset.

UCP/CORE: Check length of user header in ucp_am_send_nbx.

538fb45

UCT/IB/MLX5: Refactor verbs and mlx5dv memory domains operations

a1f060c

Use simplified versions of mem_reg/dereg/pack, that do not rely on uct_ib_md_ops_t callbacks. Update some unit tests that called IB functions directly.

ct-clmsn force-pushed the rv64-support branch from 2ee91c5 to a1f060c Compare July 26, 2023 17:02

Merge branch 'master' into rv64-support

540f8fb

shamisp closed this Jul 29, 2023


		#define ucs_arch_wait_mem ucs_arch_generic_wait_mem

		static inline void ucs_arch_clear_cache(void * start, void * end) {

		UCT_INSTANTIATE_UD_TEST_CASE(test_ud)


		class test_ud_peer_failure : public ud_base_test {

		}


		//double ucs_rv64_init_tsc_freq();

Comments

Conversation

jleidel commented May 19, 2022

What

Why ?

How ?

Uh oh!

shamisp commented May 19, 2022

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ct-clmsn May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ct-clmsn May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ct-clmsn May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ct-clmsn May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ct-clmsn May 24, 2022 •

edited

Loading

ct-clmsn May 24, 2022 •

edited

Loading

ct-clmsn May 20, 2022 •

edited

Loading

ct-clmsn May 24, 2022 •

edited

Loading

ct-clmsn May 25, 2022 •

edited

Loading