infohint: add nogpu as a gpudriver_id hint #169

hzhou · 2021-01-14T20:02:12Z

Pull Request Description

The purpose of this PR is add a way to tell yaksa the both in_buf and out_buf is cpu host memory. This is useful when yaksa is configured with GPU support but the application doesn't use gpu memory or the application knows when their buffer is entirely from CPU and thus doesn't need to pay the pointer attribute query overhead.

Expected Impact

Fixes #168

Author Checklist

Reference appropriate issues (with "Fixes" or "See" as appropriate)
Commits are self-contained and do not do two things at once
Commit message is of the form: module: short description and follows good practice
Add comments such that someone without knowledge of the code could understand
Have read and agree to the Yaksa CLA terms (https://github.com/pmodels/yaksa/wiki/Yaksa-Contributor-License-Agreement)

Add a way for application to tell yaksa that both in_buf and out_buf are host memory. Signed-off-by: Hui Zhou <[email protected]>

hzhou · 2021-01-14T22:04:29Z

test:yaksa/gpu

Apply nogpu hint every other test when no device memory is involved. Signed-off-by: Hui Zhou <[email protected]>

The yaksu_atomic_decr returns the value before decrement. Signed-off-by: Hui Zhou <[email protected]>

hzhou · 2021-01-15T03:07:21Z

test:yaksa/gpu

RESULT: 3 hr 28min. 1 failure:

test/pack/pack -datatype int -count 17 -seed 73 -iters 32768 -segments 1 -ordering normal -overlap none -num-threads 4
 Stack Trace

CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): an illegal memory access was encountered
lt-pack: test/pack/pack.c:273: runtest: Assertion `rc == (0)' failed.
CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): an illegal memory access was encountered

hzhou · 2021-01-15T14:44:29Z

@pavanbalaji The PR is ready for your review. There is 1 failure in the newly created gpu testing, see #169 (comment), I am not sure it is related to this PR. Maybe you can help to decide. Meanwhile, I'll re-run the test to confirm.

hzhou · 2021-01-15T14:45:06Z

test:yaksa/gpu ✔️

hzhou · 2021-01-15T21:34:12Z

@pavanbalaji Tests are clean. Ready for your review.

pavanbalaji · 2021-01-17T03:12:59Z

test/pack/pack.c

+            host_only_get_ptr_attr(&unpack_info);
+        } else {
+            pack_get_ptr_attr(tbuf_d, dbuf_d + dobj.DTP_buf_offset, &unpack_info);
+        }


This patch is forcing all host-only testing to use this new info hint. This will cause us to lose all testing for the cases where this info hint is not provided.

It only adds the info hint to half of the cases, similar to the gpu tests. Refer to line 55 above.

pavanbalaji · 2021-01-17T03:22:00Z

test/pack/pack.c

+static void host_only_get_ptr_attr(yaksa_info_t * info)
+{
+    static int count = 0;
+    if ((++count) % 2 == 0) {


Is this thread-safe?

Not really. But the consequence is just less than perfect half by half testing. The same mechanism is used in test/pack/pack-cuda.c. We can make it atomic (do we have atomic library for the testing?) but maybe it isn't worth the trouble. For example, maybe some randomness is even desirable.

No, randomness is not desirable in this case. It's better to make both cases deterministic. If we don't have an atomic library, we could use a lock or decide on the info hint based on the iteration number.

The static counter is not thread-safe. Use iteration number to get more consistent split-testing. Signed-off-by: Hui Zhou <[email protected]>

hzhou · 2021-01-17T14:40:08Z

test:yaksa/gpu

2 failures:

test/pack/pack  -datatype int -count 17 -seed 85 -iters 32768 -segments 64 -ordering  normal -overlap none -num-threads 4        
Stack Trace           
CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): misaligned address 
lt-pack: test/pack/pack.c:272: runtest: Assertion `rc == (0)' failed. 
CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): misaligned address 

test/pack/pack  -datatype int -count 17 -seed 97 -iters 32768 -segments 64 -ordering  reverse -overlap none -num-threads 4        
Stack Trace           
CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): an illegal memory access was encountered 
CUDA Error (yaksuri_cudai_event_query:src/backend/cuda/pup/yaksuri_cudai_event.c,65): an illegal memory access was encountered 
lt-pack: test/pack/pack.c:272: runtest: Assertion `rc == (0)' failed. 
lt-pack: test/pack/pack.c:272: runtest: Assertion `rc == (0)' failed.

hzhou · 2021-01-17T20:10:03Z

test:yaksa/gpu

hzhou · 2021-01-18T00:01:09Z

@pavanbalaji ready for your review again

pavanbalaji · 2021-01-18T00:55:07Z

How did the failures disappear? Did you simply rerun the tests till they disappeared? That's not a good model. It only means that the errors are intermittent and are being ignored.

hzhou · 2021-01-18T00:58:27Z

How did the failures disappear? Did you simply rerun the tests till they disappeared? That's not a good model. It only means that the errors are intermittent and are being ignored.

I did record them down. They seem to be intermittent gpu errors. I won't have time to debug them. I'll have to check them later.

pavanbalaji · 2021-01-18T00:59:54Z

If they are happening with main too, then we should at least have an issue created for it. If it's not happening with main, then it's a problem with this PR and it cannot be merged till that's fixed.

hzhou · 2021-01-18T01:00:39Z

If they are happening with main too, then we should at least have an issue created for it. If it's not happening with main, then it's a problem with this PR and it cannot be merged till that's fixed.

Let me open a dummy PR to check that

pavanbalaji · 2021-01-18T01:01:37Z

I meant in the nightly tests. You don't need a dummy PR.

hzhou · 2021-01-18T01:03:09Z

I meant in the nightly tests. You don't need a dummy PR.

Sure. Let me create one.

hzhou · 2021-01-18T14:56:58Z

@pavanbalaji The cuda memory issue was reproduced in nightly test (https://jenkins-pmrs.cels.anl.gov/view/yaksa/job/yaksa-nightly-gpu/lastCompletedBuild/testReport/) and tracked in issue #170

hzhou · 2021-01-18T21:01:05Z

@pavanbalaji Is there any remaining issues with this PR?

infohint: add nogpu as a gpudriver_id hint

6d4081c

Add a way for application to tell yaksa that both in_buf and out_buf are host memory. Signed-off-by: Hui Zhou <[email protected]>

hzhou force-pushed the 2101_no_gpu branch 2 times, most recently from e4fbfd2 to b8d3a98 Compare January 14, 2021 21:52

hzhou force-pushed the 2101_no_gpu branch from b8d3a98 to 6704ac5 Compare January 14, 2021 23:18

test: add nogpu hint to pack tests when no device memory

32aa7e7

Apply nogpu hint every other test when no device memory is involved. Signed-off-by: Hui Zhou <[email protected]>

hzhou force-pushed the 2101_no_gpu branch from 6704ac5 to 8c70308 Compare January 15, 2021 03:06

info: fix off-by-1 check of refcount

aafca77

The yaksu_atomic_decr returns the value before decrement. Signed-off-by: Hui Zhou <[email protected]>

hzhou force-pushed the 2101_no_gpu branch from 8c70308 to aafca77 Compare January 15, 2021 03:07

hzhou requested a review from pavanbalaji January 15, 2021 14:45

hzhou mentioned this pull request Jan 15, 2021

typerep/yaksa: always pass info hint for yaksa pack/unpack pmodels/mpich#5005

Merged

10 tasks

pavanbalaji suggested changes Jan 17, 2021

View reviewed changes

pavanbalaji reviewed Jan 17, 2021

View reviewed changes

test/pack: use iteration number to split info hint testing

437e934

The static counter is not thread-safe. Use iteration number to get more consistent split-testing. Signed-off-by: Hui Zhou <[email protected]>

hzhou force-pushed the 2101_no_gpu branch from e7fadce to 437e934 Compare January 17, 2021 03:46

pavanbalaji approved these changes Jan 18, 2021

View reviewed changes

pavanbalaji merged commit 7a4ed0f into pmodels:main Jan 18, 2021

hzhou deleted the 2101_no_gpu branch January 18, 2021 21:25

hzhou mentioned this pull request Mar 25, 2022

add CHANGES file #218

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

infohint: add nogpu as a gpudriver_id hint #169

infohint: add nogpu as a gpudriver_id hint #169

hzhou commented Jan 14, 2021 •

edited

Loading

hzhou commented Jan 14, 2021

hzhou commented Jan 15, 2021 •

edited

Loading

hzhou commented Jan 15, 2021

hzhou commented Jan 15, 2021 •

edited

Loading

hzhou commented Jan 15, 2021

pavanbalaji Jan 17, 2021

hzhou Jan 17, 2021

pavanbalaji Jan 17, 2021

hzhou Jan 17, 2021 •

edited

Loading

pavanbalaji Jan 17, 2021

hzhou Jan 17, 2021

hzhou commented Jan 17, 2021 •

edited

Loading

hzhou commented Jan 17, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021 •

edited

Loading

hzhou commented Jan 18, 2021

hzhou commented Jan 18, 2021

infohint: add nogpu as a gpudriver_id hint #169

infohint: add nogpu as a gpudriver_id hint #169

Conversation

hzhou commented Jan 14, 2021 • edited Loading

Pull Request Description

Expected Impact

Author Checklist

hzhou commented Jan 14, 2021

hzhou commented Jan 15, 2021 • edited Loading

hzhou commented Jan 15, 2021

hzhou commented Jan 15, 2021 • edited Loading

hzhou commented Jan 15, 2021

pavanbalaji Jan 17, 2021

Choose a reason for hiding this comment

hzhou Jan 17, 2021

Choose a reason for hiding this comment

pavanbalaji Jan 17, 2021

Choose a reason for hiding this comment

hzhou Jan 17, 2021 • edited Loading

Choose a reason for hiding this comment

pavanbalaji Jan 17, 2021

Choose a reason for hiding this comment

hzhou Jan 17, 2021

Choose a reason for hiding this comment

hzhou commented Jan 17, 2021 • edited Loading

hzhou commented Jan 17, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021

pavanbalaji commented Jan 18, 2021

hzhou commented Jan 18, 2021 • edited Loading

hzhou commented Jan 18, 2021

hzhou commented Jan 18, 2021

hzhou commented Jan 14, 2021 •

edited

Loading

hzhou commented Jan 15, 2021 •

edited

Loading

hzhou commented Jan 15, 2021 •

edited

Loading

hzhou Jan 17, 2021 •

edited

Loading

hzhou commented Jan 17, 2021 •

edited

Loading

hzhou commented Jan 18, 2021 •

edited

Loading