Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure consistency between memory alloc and free #3073

Merged
merged 1 commit into from
Sep 26, 2015

Conversation

ronghanghu
Copy link
Member

Add a bool flag to record whether a host memory is allocated using malloc or cudaMallocHost, and free correspondingly using this flag, instead of depending on Caffe::mode(), which is mutable during runtime.

This should fix #3053 .

@ronghanghu ronghanghu force-pushed the consistent-malloc-free branch 9 times, most recently from 82ae14b to e8e95da Compare September 16, 2015 20:47
@ronghanghu
Copy link
Member Author

I hope to merge this PR, if no one opposes. This ensures that memories are allocated and freed consistently.

@shelhamer
Copy link
Member

Let's hold off a bit. Although this addresses the immediate issue the root cause is that Nets do not know their own state / mode and adding more state to SyncedMem isn't necessarily how we want to work around that. However I might be convinced this is the way to go for now since it works.

@ronghanghu
Copy link
Member Author

Although this addresses the immediate issue the root cause is that Nets do not know their own state / mode and adding more state to SyncedMem isn't necessarily how we want to work around that.

I agree. In the spirit of #1500 I think it is better to have mode and device in net (or in layers if we plan to have individual mode/device for each layer).

@ronghanghu
Copy link
Member Author

However, I think even the net knows their state/mode/device, it is still not enough, because in order to make SyncMemory and Blob self-contained data structures (not depending on the net holding it), SyncMemory class needs to know things like how memory is allocated, on which device etc.

So personally I still think a bool flag in this PR could be sort of necessary for SyncMemory. But I am open to opinions and suggestions on this issue.

@@ -154,4 +154,3 @@ void SyncedMemory::async_gpu_push(const cudaStream_t& stream) {
#endif

} // namespace caffe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this whitespace change from the commit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

@shelhamer
Copy link
Member

Alright, this is good to merge for solving the immediate issue and we will further plan #1500 for diffusing mode + device through Net, Layer, and Blob.

Add a bool flag to record whether a host memory is allocated using malloc or
cudaMallocHost, and free correspondingly using this flag, instead of depending on Caffe::mode(), which is mutable during runtime.
@ronghanghu ronghanghu force-pushed the consistent-malloc-free branch from e8e95da to bd5f154 Compare September 25, 2015 22:52
@ronghanghu
Copy link
Member Author

This should be good now :)

ronghanghu added a commit that referenced this pull request Sep 26, 2015
Ensure consistency between memory alloc and free
@ronghanghu ronghanghu merged commit ff16f6e into BVLC:master Sep 26, 2015
@ronghanghu ronghanghu deleted the consistent-malloc-free branch September 26, 2015 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with cudaFreeHost(ptr) in syncedmem.hpp:30
3 participants