gmx_pme_send_coordinates() is moved past the x H2D which will lead to regressions on the default code-path.
This needs to be made conditional and and done only
i) eliminate this in favor of direct CPU->GPU copy.
ii) if there is proof that there is benefit from doing a H2D followed by direct GPU->GPU is beneficial (which may be the case if we have PCIe between CPU<->GPU but NVLink between GPU<->GPU) -- side-note: that's why perhaps we should use nccl.
GPU Coordinate PME/PP Communications
Extends PmePpCommGpu class to provide PP-side support for coordinate
transfers from either GPU or CPU to PME task, and adds new
PmeCoordinateReceiverGpu class to recieve coordinate data directly to
the GPU on the PME task.
Fix conditional on assertion for combining GPU Update and GPU coordinate send
Fixes a problem introduced in 9a3c2ce312c1e04634d636655388a8aaad53c8d1
which unintentionally blocks any use of GPU update with PME-PP
Communication because !stepWork.doNeighborSearch is missing from the
condition on the assertion.
- Status changed from New to In Progress
- Target version changed from 2020 to 2020-beta3
Direct CPU->GPU copy already works in master branch by setting sendCoordinatesFromGpu=true.
With NVLINK on DGX, it is definitely better to use existing mechanism otherwise there are two H2D PCI transfers (to PP and PME) on the same bus. For non-NVLINK, yes it should be better to go direct CPU->GPU. We can choose path based on whether peer access is enabled (and possibly later extend to instead query architecture using NVML which would give more detail). Working on this at the moment.
#7 Updated by Alan Gray about 1 month ago
What is the status of this? I still think this is high prio and we should not leave this unresolved in the release.
It is addressed in
I moved it to master after a request from Paul, but can move it back to release-2020 if Paul has no objections?
It currently still has an issue working correctly when update is on GPU. Under master, I therefore put it on hold until the release branch is merged. But if it goes back to the release branch it is easier to work on it and fix the problem.
#9 Updated by Szilárd Páll about 1 month ago
- Tracker changed from Task to Bug
- Affected version set to 2020-beta3
This is a bug in beta3so I think the 2020-rc1 target is valid -- we agreed to not have the new experimental features introduce regressions, and this is clearly a case of regression.
I agree that the proposed solution goes beyond just fixing the bug.
However, I am not sure there is a net positive value in avoiding the slight risk of destabilizing the experimental features (i.e. PP-PME direct GPU comm) especially when the upside seems significant to the rest of the code: i) fixing the regression ii) simpler and more maintainable code on the long run.
#12 Updated by Paul Bauer 27 days ago
- Target version changed from 2020 to 2020.1
Alan Gray wrote:
It is fixed in https://gerrit.gromacs.org/c/gromacs/+/14238 which I believe is now working correctly.
Yes, but this included feature work that should not have been targeted at 2020.
I retargeted this at 2020.1 for just fixing the regression!