CUDA VM resource bugfixes #3545

mzient · 2021-11-30T14:07:11Z

Signed-off-by: Michał Zientkiewicz mzient@gmail.com

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

Fixed exception safety bug in cuda_vm_resource.
- bug: when map_storage threw, free_va was already allocated from and blocks were marked as unavailable
- fix: reorganize the code and apply changes to internal structures after potential exception has been thrown
- fix: restore temporarily modified availability mask
Removed a trait that marked cuda_vm_resource as merge-capable.
- bug: async_pool attempted to free a buffer in parts, but cuda_vm_resource enlarge these to meet internal alignment, causing corruption
- fix: remove the trait
Remove debug dump upon every OOM in cuda_vm_resource
Fix stats
- bug: missing stat_add_free in deallocate_impl

Additional information

Affected modules and functionalities:

CUDA VM resource

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2097, DALI-2450

dali-automaton · 2021-11-30T14:16:09Z

CI MESSAGE: [3499776]: BUILD STARTED

dali-automaton · 2021-11-30T15:11:58Z

CI MESSAGE: [3499776]: BUILD FAILED

JanuszL · 2021-11-30T17:53:45Z

include/dali/core/mm/cuda_vm_resource.h

-      print(std::cerr, "Could not allocate physical storage of size ", size, " on device ",
-        device_ordinal_);
-      dbg_dump(std::cerr);
+      free_va_.put(va, size);


I think we still should provide a message like - Attempted to allocate XXX, currently allocated YY, available ZZZ

I don't want to change the original exception type - now I'd have to rethrow.

Maybe you can adjust at least adjust dbg_dump(std::cerr); to print shorter and easier to digest message?

Done for CUDABadAlloc - others are rethrown directly.

JanuszL · 2021-11-30T17:56:44Z

include/dali/core/mm/cuda_vm_resource.h

-    assert(static_cast<int>(free_blocks.size()) == blocks_to_map);
+    free_blocks.resize(blocks_to_map);
+    for (int i = to_unmap.size(); i < blocks_to_map; i++) {
+      // This can throw, but we should leave the object in a consistent state


Suggested change

// This can throw, but we should leave the object in a consistent state

// This can throw, but should leave the object in a consistent state

I meant:
This can throw, but (this) should leave the object in a consistent state ;)

I know that is why we sounds strange here.

It doesn't matter - both are fine and it's just an internal comment - not even doxygen.

JanuszL · 2021-11-30T18:38:58Z

dali/core/mm/cuda_vm_resource_test.cc

+    res.deallocate(p1, size1);
+    auto &region = res.va_regions_[0];
+    EXPECT_EQ(region.available_blocks, 4);
+    EXPECT_EQ(region.mapped.find(false), 6);


Suggested change

EXPECT_EQ(region.mapped.find(false), 6);

EXPECT_EQ(region.mapped.find(true), 0);

EXPECT_EQ(region.mapped.find(false), 6);

JanuszL · 2021-11-30T18:42:16Z

Can you describe in the PR the bugs fixed and the solution?

* Don't mark cuda_vm_resource as merge-capable. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2021-11-30T20:10:14Z

CI MESSAGE: [3501860]: BUILD STARTED

mzient · 2021-11-30T20:13:01Z

Can you describe in the PR the bugs fixed and the solution?

Described in the PR message.

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2021-11-30T20:22:41Z

CI MESSAGE: [3501927]: BUILD STARTED

dali-automaton · 2021-11-30T21:10:06Z

CI MESSAGE: [3501860]: BUILD FAILED

dali-automaton · 2021-11-30T21:19:12Z

CI MESSAGE: [3501927]: BUILD FAILED

dali-automaton · 2021-11-30T23:23:03Z

CI MESSAGE: [3501860]: BUILD PASSED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2021-12-01T08:20:15Z

CI MESSAGE: [3505387]: BUILD STARTED

mzient · 2021-12-01T10:20:22Z

include/dali/core/mm/cuda_vm_resource.h

-          char *ptr = r.block_ptr<char>(block_idx);
-          free_mapped_.get_specific_block(ptr, block_size_);
-          stat_take_free(block_size_);
+          out.push_back({ &r, block_idx });


out is a template - it changed the meaning now.

dali-automaton · 2021-12-01T10:37:13Z

CI MESSAGE: [3505387]: BUILD PASSED

jantonguirao · 2021-12-01T11:32:51Z

include/dali/core/mm/cuda_vm_resource.h

+    }
+    assert(free_blocks.size() == static_cast<size_t>(blocks_to_map));
+
+    stat_allocate_blocks(new_blocks);


move this to line 691?

Nope - if unmapping throws for any reason, the blocks will just go away.

jantonguirao · 2021-12-01T11:37:58Z

dali/core/mm/cuda_vm_resource_test.cc

+    size_t free = 0, total = 0;
+    CUDA_CALL(cudaFree(nullptr));  // initialize the default context
+    CUDA_CALL(cuMemGetInfo(&free, &total));
+    size_t va_size = next_pow2(total * 3 / 2);


maybe you could add a comment here explaining the meaning of those sizes, why it throws or doesn't when we allocate this or that. not a must, just a thought

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2021-12-01T14:29:11Z

CI MESSAGE: [3507145]: BUILD STARTED

dali-automaton · 2021-12-01T15:49:23Z

CI MESSAGE: [3507145]: BUILD PASSED

* Fixed exception safety bug in cuda_vm_resource. bug: when map_storage threw, free_va was already allocated from and blocks were marked as unavailable fix: reorganize the code and apply changes to internal structures after potential exception has been thrown fix: restore temporarily modified availability mask * Removed a trait that marked cuda_vm_resource as merge-capable. bug: async_pool attempted to free a buffer in parts, but cuda_vm_resource enlarge these to meet internal alignment, causing corruption fix: remove the trait * Remove debug dump upon every OOM in cuda_vm_resource * Fix stats bug: missing stat_add_free in deallocate_impl Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

JanuszL self-assigned this Nov 30, 2021

JanuszL reviewed Nov 30, 2021

View reviewed changes

* Fix exception safety bug in cuda_vm_resource.

8a476a5

* Don't mark cuda_vm_resource as merge-capable. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the cuda_vm_resource_fix branch from 9f81351 to 8a476a5 Compare November 30, 2021 20:08

Fix review issues.

8ce45be

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Add more checks in the test.

291c12d

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

JanuszL approved these changes Dec 1, 2021

View reviewed changes

mzient added the important-fix Fixes an important issue in the software or development environment. label Dec 1, 2021

mzient commented Dec 1, 2021

View reviewed changes

jantonguirao self-assigned this Dec 1, 2021

jantonguirao reviewed Dec 1, 2021

View reviewed changes

jantonguirao approved these changes Dec 1, 2021

View reviewed changes

Improve comments.

fe9c7f3

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient merged commit edbc608 into NVIDIA:main Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUDA VM resource bugfixes #3545

CUDA VM resource bugfixes #3545

mzient commented Nov 30, 2021 •

edited

Loading

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

JanuszL Nov 30, 2021

mzient Nov 30, 2021

JanuszL Dec 1, 2021

mzient Dec 1, 2021

JanuszL Nov 30, 2021

mzient Nov 30, 2021

JanuszL Dec 1, 2021

mzient Dec 1, 2021

JanuszL Nov 30, 2021

mzient Dec 1, 2021

JanuszL commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

mzient commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Dec 1, 2021

mzient Dec 1, 2021

dali-automaton commented Dec 1, 2021

jantonguirao Dec 1, 2021

mzient Dec 1, 2021

jantonguirao Dec 1, 2021

dali-automaton commented Dec 1, 2021

dali-automaton commented Dec 1, 2021

	// This can throw, but we should leave the object in a consistent state
	// This can throw, but should leave the object in a consistent state

	EXPECT_EQ(region.mapped.find(false), 6);
	EXPECT_EQ(region.mapped.find(true), 0);
	EXPECT_EQ(region.mapped.find(false), 6);

CUDA VM resource bugfixes #3545

CUDA VM resource bugfixes #3545

Conversation

mzient commented Nov 30, 2021 • edited Loading

Description

What happened in this PR

Additional information

Affected modules and functionalities:

Checklist

Tests

Documentation

DALI team only

Requirements

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

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

JanuszL commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

mzient commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Nov 30, 2021

dali-automaton commented Dec 1, 2021

Choose a reason for hiding this comment

dali-automaton commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Dec 1, 2021

dali-automaton commented Dec 1, 2021

mzient commented Nov 30, 2021 •

edited

Loading