ompi/opal: add support for HDR link speeds #3434

dsharma283 · 2017-04-28T13:19:35Z

This patch enables to use adapters with HDR and NDR
speeds.
issue id 3431

Signed-off-by: Devesh Sharma [email protected]

ompiteam-bot · 2017-04-28T13:19:37Z

Can one of the admins verify this patch?

jsquyres · 2017-04-28T14:26:39Z

bot:retest

bwbarrett · 2017-04-28T16:52:19Z

bot:ompi:retest

jsquyres

According to #3431, it looks like you are trying to solve two issues:

Support 25/50Gbps devices.
Fix a segv.

In #3431, the real error in the segv appears to be:

*** Error in `/usr/local/imb/openmpi/dcheck/IMB-MPI1': free(): invalid pointer: 0x00007ff37b2f34d8 ***

However, it's not clear from that output whether that free() is in the Open MPI code or in IMB. I.e., is Open MPI handling the rejected port badly (i.e., not propagating the error properly), or is IMB failing?

If there's a separate fix for fixing a segv in a failure path for the openib BTL when a port is rejected, that would be useful to include in a separate commit on this PR.

jsquyres · 2017-04-29T11:26:34Z

opal/mca/common/verbs/common_verbs_port.c

+        /* HDR: 50Gbps * 64/66, in megabits */
+        *bandwidth = 50000;
+        break;
+    case 128:


Two questions:

According to https://github.com/torvalds/linux/blob/master/include/rdma/ib_verbs.h#L428-L435, I do not see values for 64 and 128 defined in the kernel ABI for the active_speed field when querying IB port attributes. Where are you getting these 64/128 values from?

As a secondary (but related) issue, is there a better way than using hard-coded integer values here in common_verbs_port.c? On the kernel side, we have the nice IB_SPEED_* enums; do public equivalents exist in userspace that we can use here in Open MPI? (ditto for IB_WIDTH_*)

Hi Jeff,

Thanks for your comments and feedback.
Regarding the invalid pointer free(), I did some initial debugging and found that openmpi is raising this stack trace whenever it fails to select any of the usable btl for a given process. for example, with my transport if I select btl_openib_flags in such a way where only PUT support is available (flags value 306). I will open a seprate PR for this issue.

Yes, it is true that currently values 64 and 128 are not defined in the kernel/user IB stack. I will push the needed change to the ib-stack very soon.

To the best of my knowledge, those macros are not there in user-space. I can try pushing those to rdma-core once.

Yes, either a separate PR or another commit on this PR to fix that segv would be great; thanks.

Once you get a patch accepted upstream for the 64/128 values, we can probably take this commit. I just wouldn't want to take a commit here that is dependent upon a vendor-specific OFED stack (which then might vary between vendors). Hope that makes sense.

I think getting the macros would be an extra bonus, and can likely be a separate upstream patch for libibverbs. I.e., depending on the timing, either a separate PR or a separate commit on this PR that converts these naked values to enums would be awesome.

Many thanks.

Just to note that we have been pointed to this PR by Mellanox about OMPI not working with CX5 cards and ROCE.

However, we do not see the backtrace that @dsharma283 reports, just the error:

-------------------------------------------------------------------------- WARNING: There was an error initializing an OpenFabrics device. Local host: spartan-gpgpu008 Local device: mlx5_0 -------------------------------------------------------------------------- -------------------------------------------------------------------------- WARNING: There was an error initializing an OpenFabrics device. Local host: spartan-gpgpu007 Local device: mlx5_0 -------------------------------------------------------------------------- srun: Job step aborted: Waiting up to 32 seconds for job step to finish. srun: error: spartan-gpgpu008: task 1: Exited with exit code 17 srun: error: spartan-gpgpu007: task 0: Killed

dsharma283 · 2017-05-08T14:51:21Z

I have posted a change in upstream linux-rdma community to pull the port-speed and link-width enums. I will supply a rev of this patch once the change is accepted.
http://marc.info/?l=linux-rdma&m=149425108020822&w=2

Just for the reference I am leaving this pull-request open till I comeback with proper fix.

chrissamuel · 2017-11-15T00:19:38Z

I can confirm that the supplied change to opal/mca/common/verbs/common_verbs_port.c makes the verbs interface work with RoCE on Mellanox CX5 cards connected at 50Gb/s .

Before (via TCP btl):

       8 bytes took        53 usec (   0.301 MB/sec)
      16 bytes took        56 usec (   0.569 MB/sec)
[...]

After (via openib btl):

       8 bytes took        15 usec (   1.080 MB/sec)
      16 bytes took         6 usec (   5.096 MB/sec)
[...]

Patched with just:

[samuel@spartan-build openmpi-3.0.0]$ diff -iubw opal/mca/common/verbs/common_verbs_port.c.orig opal/mca/common/verbs/common_verbs_port.c
--- opal/mca/common/verbs/common_verbs_port.c.orig      2017-09-13 07:31:33.000000000 +1000
+++ opal/mca/common/verbs/common_verbs_port.c   2017-11-15 10:45:53.643484102 +1100
@@ -68,6 +68,14 @@
         /* EDR: 25.78125 Gbps * 64/66, in megabits */
         *bandwidth = 25000;
         break;
+    case 64:
+        /* HDR: 50Gbps * 64/66, in megabits */
+        *bandwidth = 50000;
+        break;
+    case 128:
+        /* NDR: 100Gbps * 64/66, in megabits */
+        *bandwidth = 100000;
+        break;
     default:
         /* Who knows? */
         return OPAL_ERR_NOT_FOUND;

jsquyres · 2017-11-15T03:58:10Z

@chrissamuel @dsharma283 Did that patch get accepted upstream?

chrissamuel · 2017-11-15T05:10:03Z

Not from what I see, it was discussed here:

https://patchwork.kernel.org/patch/9716209/

There was talk of rearchitecturing it in a way that might result in some distros being unsupported.

dsharma283 · 2017-11-15T05:26:57Z

This mail also made me think that this patch has been pulled in. This action item is still open its just I am not able to prioritize on it.

…

-Regards Devesh

On Wed, Nov 15, 2017 at 10:40 AM, Chris Samuel ***@***.***> wrote: Not from what I see, it was discussed here: https://patchwork.kernel.org/patch/9716209/ There was talk of rearchitecturing it in a way that might result in some distros being unsupported. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

chrissamuel · 2017-11-15T23:15:37Z

@dsharma283 I see no evidence of any mention of anything using IBV_ in the linux-rdma repos here. https://github.com/linux-rdma

chrissamuel · 2017-11-16T03:08:47Z

@jsquyres looking at your link it looks like the file has changed since then and IB_SPEED_HDR is now defined for ib_port_speed:

https://github.com/torvalds/linux/blob/master/include/rdma/ib_verbs.h#L464-L472

enum ib_port_speed {
	IB_SPEED_SDR	= 1,
	IB_SPEED_DDR	= 2,
	IB_SPEED_QDR	= 4,
	IB_SPEED_FDR10	= 8,
	IB_SPEED_FDR	= 16,
	IB_SPEED_EDR	= 32,
	IB_SPEED_HDR	= 64
};

Could we perhaps merge a reduced version of this PR to just recognise IB_SPEED_HDR devices please?

hppritcha · 2018-03-21T15:41:38Z

@jsquyres could you double check whether this is okay or not now

This patch enables to use adapters with HDR speeds. issue id 3431 Signed-off-by: Devesh Sharma <[email protected]>

jsquyres · 2018-03-22T02:16:54Z

I updated the patch to remove the "128" case (because that's not upstream).

dsharma283 · 2018-03-26T08:28:39Z

Thanks for pulling-in.

…

On Thu, Mar 22, 2018 at 8:27 AM, Jeff Squyres ***@***.***> wrote: Merged #3434 <#3434>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3434 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXVC7phEDjho2wYCIiqixBbcU5A0hV99ks5tgxMVgaJpZM4NLefD> .

jsquyres · 2018-03-26T14:09:51Z

No worries. Note that this cherry pick is slated for the next releases in the v2.1.x, v3.0.x, and v3.1.x series (#4945, #4946, and #4947).

jsquyres mentioned this pull request Apr 29, 2017

Add support for 50G and 100G adapters in openib btl #3431

Open

jsquyres requested changes Apr 29, 2017

View reviewed changes

jsquyres added the pushed-back label Apr 29, 2017

jsquyres force-pushed the pr-3431 branch from 53718b9 to e937d65 Compare March 22, 2018 02:15

ompi/opal: add support for HDR link speeds

90e9b22

This patch enables to use adapters with HDR speeds. issue id 3431 Signed-off-by: Devesh Sharma <[email protected]>

jsquyres force-pushed the pr-3431 branch from e937d65 to 90e9b22 Compare March 22, 2018 02:16

jsquyres removed the pushed-back label Mar 22, 2018

jsquyres approved these changes Mar 22, 2018

View reviewed changes

jsquyres changed the title ~~ompi/opal: add support for HDR and NDR link speeds~~ ompi/opal: add support for HDR link speeds Mar 22, 2018

jsquyres merged commit a15d823 into open-mpi:master Mar 22, 2018

ompi/opal: add support for HDR link speeds #3434

ompi/opal: add support for HDR link speeds #3434

Uh oh!

Conversation

dsharma283 commented Apr 28, 2017

Uh oh!

ompiteam-bot commented Apr 28, 2017

Uh oh!

jsquyres commented Apr 28, 2017

Uh oh!

bwbarrett commented Apr 28, 2017

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres Apr 29, 2017

Choose a reason for hiding this comment

Uh oh!

dsharma283 May 1, 2017

Choose a reason for hiding this comment

Uh oh!

jsquyres May 1, 2017

Choose a reason for hiding this comment

Uh oh!

chrissamuel Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsharma283 commented May 8, 2017

Uh oh!

chrissamuel commented Nov 15, 2017

Uh oh!

jsquyres commented Nov 15, 2017

Uh oh!

chrissamuel commented Nov 15, 2017

Uh oh!

dsharma283 commented Nov 15, 2017 via email

Uh oh!

chrissamuel commented Nov 15, 2017

Uh oh!

chrissamuel commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hppritcha commented Mar 21, 2018

Uh oh!

jsquyres commented Mar 22, 2018

Uh oh!

dsharma283 commented Mar 26, 2018 via email

Uh oh!

jsquyres commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chrissamuel Nov 14, 2017 •

edited

Loading

chrissamuel commented Nov 16, 2017 •

edited

Loading