UCM/CUDA: cudaFree() hooks #1906

bureddy · 2017-10-11T17:33:27Z

No description provided.

swx-jenkins1 · 2017-10-11T18:04:04Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2791/ for details.

mellanox-github · 2017-10-11T19:44:52Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4796/ for details (Mellanox internal link).

yosefe · 2017-10-13T19:49:52Z

config/m4/cuda.m4

@@ -1 +1,2 @@
 AM_CONDITIONAL([HAVE_CUDA], [true])
+AC_DEFINE([HAVE_CUDA], 1, [cuda enable])


looks like this PR really depends on the PR which adds configuration flags from cuda, right?

yes, will resolve this once config flag is in first

yosefe · 2017-10-13T19:51:08Z

src/ucm/cuda/install.c

+    static int ucm_cudamem_installed = 0;
+    static pthread_mutex_t install_mutex = PTHREAD_MUTEX_INITIALIZER;
+    ucm_reloc_patch_t *patch;
+    ucs_status_t status = UCS_OK;


setting status=UCS_OK should be moved to line 58

yosefe · 2017-10-13T19:52:03Z

src/ucm/cuda/replace.c

+ * the event handler, and if event handler returns error code - calls the original
+ * function.
+ */
+#define UCM_DEFINE_CUDA_FUNC(_name, _rettype, _fail_val, ...) \


isn't it possible to reuse the existing mmap() macros or extend them if needed?
no copy-paste please..

they are almost same, we can extend if we move macros to some common header file. is that ok?

moved macros to commons header

yosefe · 2017-10-13T19:53:02Z

src/ucm/event/event.c

@@ -334,6 +337,25 @@ void *ucm_sbrk(intptr_t increment)
    return event.sbrk.result;
 }

+#if HAVE_CUDA
+cudaError_t ucm_cudaFree(void *addr)


maybe this should be somewhere in src/ucm/cuda/install.c?

I think i have to keep it here, otherwise have to expose ucm_event_enter()/leave()?

yosefe · 2017-10-13T19:54:55Z

can you please also add a unit test for this?

yosefe · 2017-10-13T19:58:50Z

also, the commit title should start with "UCM/CUDA: ..."

swx-jenkins1 · 2017-10-17T19:54:25Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2841/ for details.

mellanox-github · 2017-10-17T20:33:59Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4848/ for details (Mellanox internal link).

yosefe · 2017-10-18T17:55:43Z

src/ucm/util/reloc.h

+        ucm_trace("%s()", __FUNCTION__); \
+        \
+        if (ucs_unlikely(orig_func_ptr == NULL)) { \
+            pthread_mutex_lock(&ucm_##_category##_get_orig_lock); \


do we really need a separate lock for cuda?

not really. but not sure if I can unify lock for both mmap and cuda

i think that if the lock will not be split, there would not be a need to define separate macros for cuda and mmap

unified lock and used same macro for cuda and mmap

yosefe · 2017-10-18T17:56:12Z

src/ucm/cuda/install.c

+
+
+static ucm_reloc_patch_t ucm_cudamem_symbol_patches[] = {
+    {"cudaFree",   ucm_override_cudaFree},


why need an array? we have only one function to patch

just to able to add new funcs in future if needed

then we can add the array in the future when it would be needed

swx-jenkins1 · 2017-10-19T02:07:47Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2848/ for details.

mellanox-github · 2017-10-19T03:00:05Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4857/ for details (Mellanox internal link).

yosefe · 2017-10-20T12:29:57Z

bot:mlx:retest

mellanox-github · 2017-10-20T14:06:35Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4879/ for details (Mellanox internal link).

bureddy · 2017-10-20T15:42:42Z

bot:mlx:retest

mellanox-github · 2017-10-20T17:07:57Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4880/ for details (Mellanox internal link).

yosefe · 2017-10-21T11:09:32Z

src/ucm/event/event.c

@@ -334,6 +337,25 @@ void *ucm_sbrk(intptr_t increment)
    return event.sbrk.result;
 }

+#if HAVE_CUDA
+cudaError_t ucm_cudaFree(void *addr)


yosefe · 2017-10-21T11:11:41Z

src/ucm/util/reloc.h

@@ -46,4 +47,74 @@ ucs_status_t ucm_reloc_modify(ucm_reloc_patch_t* patch);
 void* ucm_reloc_get_orig(const char *symbol, void *replacement);


+extern pthread_mutex_t ucm_reloc_get_orig_lock;


need to create new file: replace,h, replace.c
"reloc" is handling changing program relocation tables
these are macros to generate replacement function

moved headers to new file utils/replace.h.
Also unified {cuda,mmap}/replace.c to util/replace.c. is this ok?

swx-jenkins1 · 2017-10-23T20:43:45Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2872/ for details.

swx-jenkins1 · 2017-10-23T22:29:11Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2873/ for details.

swx-jenkins1 · 2017-10-23T22:32:11Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2874/ for details.

mellanox-github · 2017-10-24T05:41:24Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4897/ for details (Mellanox internal link).

mellanox-github · 2017-10-24T06:58:02Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4899/ for details (Mellanox internal link).

mellanox-github · 2017-10-24T07:52:24Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4900/ for details (Mellanox internal link).

yosefe · 2017-10-26T12:55:40Z

bot:mlx:retest

mellanox-github · 2017-10-26T14:40:15Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4935/ for details (Mellanox internal link).

yosefe · 2017-10-29T17:48:42Z

@bureddy code looks good, however need to add a unit test for this. see #1906 (comment)

swx-jenkins1 · 2017-10-31T00:15:20Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2929/ for details.

swx-jenkins1 · 2017-10-31T00:46:56Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2930/ for details.

mellanox-github · 2017-10-31T01:41:17Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4974/ for details (Mellanox internal link).

swx-jenkins1 · 2017-10-31T02:07:23Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2931/ for details.

mellanox-github · 2017-10-31T02:13:18Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4975/ for details (Mellanox internal link).

swx-jenkins1 · 2017-10-31T03:01:44Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2932/ for details.

mellanox-github · 2017-10-31T03:36:16Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4976/ for details (Mellanox internal link).

bureddy · 2017-10-31T05:33:09Z

bot:mlx:retest

mellanox-github · 2017-10-31T05:36:20Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4977/ for details (Mellanox internal link).

swx-jenkins1 · 2017-10-31T06:13:51Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2933/ for details.

yosefe · 2017-10-31T07:14:51Z

contrib/test_jenkins.sh

@@ -518,6 +518,14 @@ run_coverity() {
 # Run the test suite (gtest)
 #
 run_gtest() {
+
+    #load cuda modules if available


indentation (use tabs)

yosefe · 2017-10-31T07:17:04Z

contrib/test_jenkins.sh

+
+    #load cuda modules if available
+    if module_load dev/cuda; then
+        if module_load dev/gdrcopy; then


what's the purpose of using 'if' ? can just do module_load dev/cuda || true

yosefe · 2017-10-31T07:18:09Z

test/gtest/ucm/cuda_hooks.cc

+    }
+
+    /* Install memory hooks */
+    result = ucm_set_event_handler(UCM_EVENT_VM_UNMAPPED,0, cuda_mem_event_callback,


space after UCM_EVENT_VM_UNMAPPED,

yosefe · 2017-10-31T07:18:51Z

test/gtest/ucm/cuda_hooks.cc

+
+    ret = cudaFree(ptr);
+    EXPECT_EQ(ret, cudaSuccess);
+    EXPECT_EQ(ptr, free_ptr);


need to reset free_ptr to NULL before every cudaMalloc

mellanox-github · 2017-10-31T08:27:16Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4978/ for details (Mellanox internal link).

swx-jenkins1 · 2017-10-31T10:30:40Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2937/ for details.

mellanox-github · 2017-10-31T11:12:38Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4982/ for details (Mellanox internal link).

bureddy · 2017-10-31T15:01:53Z

@yosefe updated pr with review fixes

yosefe · 2017-10-31T16:49:05Z

test/gtest/ucm/cuda_hooks.cc

+    EXPECT_EQ(ret, cudaSuccess);
+    EXPECT_EQ(ptr1, free_ptr);
+
+    ret = cudaFree(ptr);


you need to set free_ptr = NULL here as well..

Will add it . But i think it should be fine becuase free_ptr = ptr1 here, which is different from ptr.

mellanox-github · 2017-10-31T19:46:00Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4985/ for details (Mellanox internal link).

swx-jenkins1 · 2017-10-31T20:00:41Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2940/ for details.

bureddy · 2017-10-31T20:03:33Z

bot:bgate:retest

swx-jenkins1 · 2017-10-31T20:41:58Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2943/ for details.

yosefe reviewed Oct 13, 2017

View reviewed changes

yosefe added the Feature New feature label Oct 13, 2017

bureddy force-pushed the cuda-mem-hooks branch from 2cfddd3 to 1d991c2 Compare October 17, 2017 19:15

yosefe requested changes Oct 18, 2017

View reviewed changes

bureddy force-pushed the cuda-mem-hooks branch from 1d991c2 to 0064411 Compare October 19, 2017 01:29

yosefe reviewed Oct 21, 2017

View reviewed changes

bureddy force-pushed the cuda-mem-hooks branch 2 times, most recently from 957e2e3 to 3061074 Compare October 23, 2017 21:50

UCM/CUDA: cudaFree() hooks

2851e4e

bureddy force-pushed the cuda-mem-hooks branch from 06e634d to 7939dd3 Compare October 31, 2017 02:05

bureddy force-pushed the cuda-mem-hooks branch from 7939dd3 to ea631cc Compare October 31, 2017 02:26

bureddy force-pushed the cuda-mem-hooks branch from ea631cc to e7263d0 Compare October 31, 2017 05:37

yosefe reviewed Oct 31, 2017

View reviewed changes

bureddy force-pushed the cuda-mem-hooks branch from e7263d0 to f9daad5 Compare October 31, 2017 09:53

yosefe reviewed Oct 31, 2017

View reviewed changes

bureddy added 2 commits October 31, 2017 20:17

TEST/UCM/CUDA: test cudaFree hooks

ffabef6

JENKINS: load cuda modules for gtest build

8b20824

bureddy force-pushed the cuda-mem-hooks branch from f9daad5 to 8b20824 Compare October 31, 2017 18:18

yosefe approved these changes Nov 1, 2017

View reviewed changes

yosefe merged commit 173338c into openucx:master Nov 1, 2017

bureddy deleted the cuda-mem-hooks branch November 3, 2017 20:31

		@@ -1 +1,2 @@
		AM_CONDITIONAL([HAVE_CUDA], [true])
		AC_DEFINE([HAVE_CUDA], 1, [cuda enable])



		static ucm_reloc_patch_t ucm_cudamem_symbol_patches[] = {
		{"cudaFree", ucm_override_cudaFree},

		@@ -46,4 +47,74 @@ ucs_status_t ucm_reloc_modify(ucm_reloc_patch_t* patch);
		void* ucm_reloc_get_orig(const char symbol, void replacement);


		extern pthread_mutex_t ucm_reloc_get_orig_lock;

UCM/CUDA: cudaFree() hooks #1906

UCM/CUDA: cudaFree() hooks #1906

Conversation

bureddy commented Oct 11, 2017

swx-jenkins1 commented Oct 11, 2017

mellanox-github commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Oct 13, 2017

yosefe commented Oct 13, 2017

swx-jenkins1 commented Oct 17, 2017

mellanox-github commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swx-jenkins1 commented Oct 19, 2017

mellanox-github commented Oct 19, 2017

yosefe commented Oct 20, 2017

mellanox-github commented Oct 20, 2017

bureddy commented Oct 20, 2017

mellanox-github commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swx-jenkins1 commented Oct 23, 2017

swx-jenkins1 commented Oct 23, 2017

swx-jenkins1 commented Oct 23, 2017

mellanox-github commented Oct 24, 2017

mellanox-github commented Oct 24, 2017

mellanox-github commented Oct 24, 2017

yosefe commented Oct 26, 2017

mellanox-github commented Oct 26, 2017

yosefe commented Oct 29, 2017

swx-jenkins1 commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

mellanox-github commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

mellanox-github commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

mellanox-github commented Oct 31, 2017

bureddy commented Oct 31, 2017

mellanox-github commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellanox-github commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

mellanox-github commented Oct 31, 2017

bureddy commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellanox-github commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017

bureddy commented Oct 31, 2017

swx-jenkins1 commented Oct 31, 2017