Skip to content
This repository was archived by the owner on Sep 30, 2022. It is now read-only.

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented Jun 6, 2016

This commit brings the rgpusm mpool in line with the changes made to
the rcache to make it thread safe. There is no master equivalent of
this commit.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

:bot:assign: @sjeaugey
:bot:milestone:v2.0.0
:bot🏷️bug

@hppritcha This fixes an MTT failure on nvidia.

@ompiteam-bot ompiteam-bot added this to the v2.0.0 milestone Jun 6, 2016
@sjeaugey
Copy link
Member

sjeaugey commented Jun 6, 2016

👎 Does not compile.

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1748/ for details.

@sjeaugey
Copy link
Member

sjeaugey commented Jun 6, 2016

That didn't remove the reviewed flag. Trying again 👎

@sjeaugey
Copy link
Member

sjeaugey commented Jun 6, 2016

You need to replace cachebale by cacheable. Even when fixing that, I get a lot of warnings and errors :

  CC       mpool_rgpusm_module.lo
mpool_rgpusm_module.c: In function 'mca_mpool_rgpusm_find':
mpool_rgpusm_module.c:479:27: warning: variable 'bound' set but not used [-Wunused-but-set-variable]
     unsigned char *base, *bound;
                           ^
mpool_rgpusm_module.c: In function 'iterate_dereg_finalize':
mpool_rgpusm_module.c:606:45: error: 'mca_mpool_rgpusm_module_t' has no member named 'pool'
         opal_list_remove_item (&mpool_rgpusm->pool->lru_list, (opal_list_item_t *) rgpusm_reg);
                                             ^
mpool_rgpusm_module.c: In function 'mca_mpool_rgpusm_finalize':
mpool_rgpusm_module.c:620:41: error: 'RGPUSM_MPOOL_NREGS' undeclared (first use in this function)
     mca_mpool_base_registration_t *regs[RGPUSM_MPOOL_NREGS];
                                         ^
mpool_rgpusm_module.c:620:41: note: each undeclared identifier is reported only once for each function it appears in
mpool_rgpusm_module.c:622:9: warning: unused variable 'rc' [-Wunused-variable]
     int rc;
         ^
mpool_rgpusm_module.c:621:18: warning: unused variable 'i' [-Wunused-variable]
     int reg_cnt, i;
                  ^
mpool_rgpusm_module.c:621:9: warning: unused variable 'reg_cnt' [-Wunused-variable]
     int reg_cnt, i;
         ^
mpool_rgpusm_module.c:620:36: warning: unused variable 'regs' [-Wunused-variable]
     mca_mpool_base_registration_t *regs[RGPUSM_MPOOL_NREGS];
                                    ^
mpool_rgpusm_module.c:619:36: warning: unused variable 'reg' [-Wunused-variable]
     mca_mpool_base_registration_t *reg;
                                    ^

@hjelmn
Copy link
Member Author

hjelmn commented Jun 7, 2016

@sjeaugey Should be fixed now.

@hppritcha
Copy link
Member

Why is there no master equivalent to this commit?

@hjelmn
Copy link
Member Author

hjelmn commented Jun 7, 2016

@hppritcha Because this fixes a bug in the back-port of the rcache threading fixes.

@ibm-ompi
Copy link

ibm-ompi commented Jun 7, 2016

Build Failed with XL compiler! Please review the log, and get in touch if you have questions.

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1749/ for details.

@hjelmn
Copy link
Member Author

hjelmn commented Jun 7, 2016

Mellanox error is a bug in their jenkins script. The IBM one looks like a false failure as well.

@sjeaugey
Copy link
Member

sjeaugey commented Jun 7, 2016

Still need to replace cachebale with cacheable. Other than that, it builds.

@sjeaugey
Copy link
Member

sjeaugey commented Jun 7, 2016

Ok, so if you fix the patch with the cachebale thing, the bug seems to be fixed.

This commit brings the rgpusm mpool in line with the changes made to
the rcache to make it thread safe. There is no master equivalent of
this commit.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@hjelmn
Copy link
Member Author

hjelmn commented Jun 7, 2016

@sjeaugey Ah, yes. Fixed.

@sjeaugey
Copy link
Member

sjeaugey commented Jun 7, 2016

Ok, this one is good. 👍

@sjeaugey
Copy link
Member

sjeaugey commented Jun 7, 2016

@hppritcha you can merge this PR.

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1750/ for details.

@hjelmn
Copy link
Member Author

hjelmn commented Jun 7, 2016

False failure from Mellanox jenkins. rdmacm can not be used if the first QP is not a per-peer QP.

@jsquyres
Copy link
Member

jsquyres commented Jun 7, 2016

bot:retest

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1751/ for details.

@hppritcha
Copy link
Member

bot:retest

@jsquyres
Copy link
Member

jsquyres commented Jun 7, 2016

Per discussion on 2016-06-07 webex, the Mellanox Jenkins fail is on an rdmacm test, and Mellanox is not testing with GPU support -- so that failure cannot be affecting this PR. @sjeaugey confirms that this PR fixes his issue. @hppritcha wants to do some more testing and more thoroughly understand this rdmacm CPC failure before moving forward.

More info on this fail (including stack trace from MPI test): open-mpi/ompi#1758 (comment)

@jsquyres
Copy link
Member

jsquyres commented Jun 7, 2016

@hppritcha This retest failed in the Mellanox Jenkins RDMACM test, like we expected it to.

@mellanox-github
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1752/ for details.

@sjeaugey
Copy link
Member

sjeaugey commented Jun 7, 2016

@hppritcha btw, this PR affects a part of the code that is not even tested by bots since since part did not even compile a couple of weeks ago on v2.x. So, my manual build-and-test is all what we have for this precise PR and bots won't help.

@hppritcha
Copy link
Member

@jsquyres ready to go. I'm convinced the mellanox rdmacm problem is not related to this PR.

@jsquyres jsquyres merged commit fb636c7 into open-mpi:v2.x Jun 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants