Repository navigation
Verbs hang fix - #12361
Verbs hang fix#12361
Conversation
…ce operations, with a call to the async function with a callback function. This is done in order to fix a bug that occurs while using verbs, and causes the program to hang.
|
Can one of the admins verify this patch? |
|
Thanks for the fix. Is there any reason you did not get rid of CopyCPUTensorToGPUSync in verbs_util.h? |
|
CopyCPUTensorToGPUSync is still being used by RdmaRemoteRendezvous here |
|
Nice work! I think I am using the same sync wrappers (yes all of us are lazy) in my GDR patch. I might as well take a closer look on this before someone raises another issue :) |
|
@yanivbl6 |
|
Sure, I am on it. |
|
I only have access to two boxes with fairly old OS (RHEL6.5) and CUDA 7.5, which limited me to TF1.1. I modify the patch to fit TF1.1, but got the following error during benchmark test. |
|
Very good work @yanivbl6 , patch looks good. |
|
I ran successful tf_cnn_benchmarks with r1.3 ( 4 hosts (worker + ps), 8X4 GPUs, Imagenet data, Resnet-50 model). |
|
Hi @poxvoculi @junshi15, Can we merge this ? |
| // "val" is on a GPU. No longer uses GPUUtil to fill the proto, use | ||
| // aync instead | ||
| GPUUtil::SetProtoFromGPU( | ||
| in, src_dev, send_args.device_context, &proto, is_dead, |
There was a problem hiding this comment.
Someday you should return an error status instead of check failing, especially if there's a chance this is a transient error. (Applies to the CHECK a couple lines below.)
There was a problem hiding this comment.
Agreed. The error handling can be improved. Maybe we should send error status back to the caller (like here) and let the caller decide what to do, either re-try or abort.
However, for this patch, let's not address it as returning the error status to the caller/requester may require an extra signal path.
This patch looks good to me otherwise.
|
@poxvoculi What's the policy for patching a branch? Given the fix for verbs on master will take time, it will be useful to patch TF 1.3 (and maybe 1.2). |
|
I don't know about branch patching. @jhseu might know. |
|
Jenkins, test this please |
|
We're not making anymore changes to v1.3. This will appear in TF 1.4, though. |
|
Thanks @jhseu |
9 participants
Replaces the sync wrappers for device to device operations in verbs_util, with a call to the async function with a callback function. Resolves Issue #11725
As mentioned in the issue:
I think the problem rises because the Sync deviceToDevice operation blocks the thread, preventing the earlier Async Device to Device operation from finishing- which, for some reason, blocks the later operation.
I've tested this fix in origin/r1.3 since origin/master is currently broken.
2nd commit only removes the unused functions from verbs_utils. It is not required for validity.
@junshi15 , @shamoya , @byronyi , can you please go over this?