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

yoda: avoid useless memcpy in mca_spml_yoda_get when the btl is get capa... #270

Conversation

ggouaillardet
Copy link
Contributor

...ble

@ggouaillardet
Copy link
Contributor Author

@miked-mellanox @hjelmn could you please review this ?

by setting getreq->p_dst to NULL, we avoid a useless memcpy (src==dst !)
in mca_spml_yoda_get_completion.
oshrun -np 2 --mca btl vader,self examples/oshmem_max_reduction
can be used to test with knem loaded.

@mellanox-github
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
http://bgate.mellanox.com/jenkins/job/gh-ompi-master-pr/41/
Test PASSed.

@mike-dubman
Copy link
Member

@alex-mikheev - please comment.

@ggouaillardet
Copy link
Contributor Author

@alex-mikheev at first glance that does make sense.

now when i run the oshmem_max_reduction test, the destination is not a symmetric object (it is a local variable in the stack) and :

  1. registration is set to NULL since l_mkey is NULL
  2. mca_spml_yoda_get invokes vader_prepare_dst, that will KNEM_CMD_CREATE_REGION for the local variable (in the case of vader with knem, this is useless, in a more general case, this might just be suboptimal, @hjelmn could you please comment ?)
  3. so your proposed fix will not set getreq->p_dst to NULL so memcpy will be invoked with src==dst

@alex-mikheev
Copy link
Contributor

Is there a way to know that btl can do get() to non registered memory or put() from non registered memory without requiring bounce buffers ?

@hjelmn
Copy link
Member

hjelmn commented Nov 13, 2014

@ggouaillardet The new btl interface will resolve issue #2. If you register a region for local write only vader will not create a knem cookie for the region (this is not implemented in the code yet-- will probably be implemented this weekend).

@alex-mikheev There is no way to tell ATM if a region can be used for BTL rdma without a registration key. I will make sure this can be determined in the new interface.

@jsquyres
Copy link
Member

jsquyres commented Dec 1, 2014

So this PR is awaiting #278.

@jsquyres
Copy link
Member

@alex-mikheev @ggouaillardet @miked-mellanox Is this PR still relevant? If so, is it ready to go?

@hppritcha
Copy link
Member

Can one of the admins verify this patch?

@hppritcha
Copy link
Member

Where are we with this PR now? Given this is more than 6 months old seems to be time to decide whether to close or merge.

@lanl-ompi
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_distcheck/8/
Test PASSed.

@lanl-ompi
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
http://jenkins.open-mpi.org/job/ompi_master_pr_cle5.2up02/97/
Test PASSed.

@jsquyres
Copy link
Member

Last activity on this PR was in 2014. I'm guessing it isn't relevant any more.

Feel free to re-open if that's and incorrect assumption.

@jsquyres jsquyres closed this Aug 15, 2015
jsquyres pushed a commit to jsquyres/ompi that referenced this pull request Sep 21, 2016
dong0321 pushed a commit to dong0321/ompi that referenced this pull request Feb 17, 2020
Few missing prrte prefixes and fix -hostfile/-host
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants