Skip to content

Conversation

@jsquyres
Copy link
Member

The most important of these is to not show an error if fi_getinfo() returns no data.

Need to get these fixes in to release branches before they go out, or usNIC customers will be asking my why they are getting these warnings. 😦 This affects:

These commits should be added to #4458 and #4459 before those PRs are merged.

No code or logic changes.

Signed-off-by: Jeff Squyres <[email protected]>
No code or logic changes.

Signed-off-by: Jeff Squyres <[email protected]>
The value of ret is negative (e.g., -61), but it is displayed in the
help message as `%zd`, which renders as unsigned (i.e., a giant
positive value).  So make sure to negate the negative value before
rendering it (e.g., so we display "61", not "4294967235").

Signed-off-by: Jeff Squyres <[email protected]>
Before this commit, the presence of usNIC devices -- which will
(currently) return no data when fi_getinfo() is queried for tagged
matching providers -- would cause an error message to be displayed.

Signed-off-by: Jeff Squyres <[email protected]>
gcc 5.2 complains:

```
mtl_ofi_component.c: In function ‘ompi_mtl_ofi_finalize’:
mtl_ofi_component.c:613:5: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
     if (ret = fi_close((fid_t)ompi_mtl_ofi.fabric)) {
     ^
```

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres
Copy link
Member Author

Note: v2.0.x has a similar issue (i.e., shows a warning if fi_getinfo() returns no info), but it's via opal_output_verbose(), and therefore is not displayed by default.

The problem with v2.x and beyond is that it uses opal_show_help() in this case, which does output by default. That's why this fix really only needs to be applied to v2.x and beyond (and we don't need to immediately issue another v2.0.x release right after releasing v2.0.4 -- especially when we wanted that to be the last v2.0.x release!).

@ompiteam-bot
Copy link

All Tests Passed!

@hppritcha
Copy link
Member

checking to see if ompi jenkins chattiness has been disabled if tests pass.
bot:ompi:retest

@ompiteam-bot
Copy link

All Tests Passed!

Copy link
Contributor

@aravindksg aravindksg left a comment

Choose a reason for hiding this comment

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

Will cherry-pick these commits once it's merged.

@jsquyres jsquyres merged commit 6d6f0be into open-mpi:master Nov 12, 2017
@jsquyres jsquyres deleted the pr/ofi-mtl-updates branch November 12, 2017 16:30
@jsquyres
Copy link
Member Author

Thanks @aravindksg

@jsquyres jsquyres mentioned this pull request Nov 12, 2017
&providers); /* Out: List of matching providers */
if (0 != ret) {
if (FI_ENODATA == -ret) {
// It is not an error if no information is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: c++ comment style

Copy link
Member Author

Choose a reason for hiding this comment

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

We allow this in Open MPI (since compiling Open MPI requires a C99 compiler, which officially standardized //-style comments in C).

opal_progress_unregister(ompi_mtl_ofi_progress_no_inline);

/* Close all the OFI objects */
if (ret = fi_close((fid_t)ompi_mtl_ofi.ep)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why the extra parenthesis ?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the commit message on a8686a6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants