UCT/UGNI: Start using spinlocks to protect critical structures #1494

MattBBaker · 2017-05-10T20:37:14Z

This PR is deceptively big. A lot of it is shuffling a few big functions and rearranging some data structures to allow a shared thread safe code path. I couldn't find a useful way of splitting it up more.

What it does is add a spin lock to the device, and that is what will get used to do locking in UGNI. This lock is then used to attach the CDM to the device. The cdm device fields are then merged together into a shared struct.

mellanox-github · 2017-05-10T21:19:43Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1607/ for details.

mellanox-github · 2017-05-10T21:59:46Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3528/ for details (Mellanox internal link).

MattBBaker · 2017-05-10T22:13:41Z

@shamisp Could you review?

shamisp · 2017-05-10T23:08:59Z

v1.3, right ?

MattBBaker · 2017-05-11T00:38:53Z

@shamisp Yes, this a bit much of a change for 1.2.

shamisp · 2017-05-11T15:08:04Z

@MattBBaker it is actually up to you. I think these changes can be considered as a thread safety bugfix. If you think it is important for ORNL to have it in v1.2 we can push.
I don't think @yosefe will have major objection

MattBBaker · 2017-05-11T15:13:32Z

@shamisp This is part of a larger set of patches coming down the pipe , with some more rearrangements and potential performance impacts. I think that saying it's 1.3 is safer. Though I think that we should also talk about what kind of release time frame there is for 1.3.

MattBBaker · 2017-05-11T15:18:23Z

@shamisp Also, don't merge yet, I just found a bug.

MattBBaker · 2017-05-11T15:34:01Z

@shamisp Fixed now. I guess gcc doesn't check the parameter list for empty macros.

shamisp · 2017-05-11T15:43:29Z

it does not. Ready for review ?

…

On Thu, May 11, 2017 at 10:34 AM, MattBBaker ***@***.***> wrote: @shamisp <https://github.com/shamisp> Fixed now. I guess gcc doesn't check the parameter list for empty macros. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1494 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACIe2N_8Xbs86PvMCbNkHOL7VFl4Jf1Xks5r4ypqgaJpZM4NXMNw> .

MattBBaker · 2017-05-11T15:56:36Z

Yes.

mellanox-github · 2017-05-11T16:09:48Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1634/ for details.

mellanox-github · 2017-05-11T17:27:27Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3553/ for details (Mellanox internal link).

shamisp · 2017-05-11T18:21:31Z

src/uct/ugni/smsg/ugni_smsg_ep.c

@@ -53,7 +53,7 @@ static ucs_status_t uct_ugni_smsg_mbox_reg(uct_ugni_smsg_iface_t *iface, uct_ugn
    }
    pthread_mutex_lock(&uct_ugni_global_lock);

-    ugni_rc = GNI_MemRegister(iface->super.nic_handle, (uint64_t)address,
+    ugni_rc = GNI_MemRegister(uct_ugni_iface_nic_handle(&iface->super), (uint64_t)address,


I'm curious - what is the motivation behind uct_ugni_iface_nic_handle() ? type more :)

It's most useful in UDT, where changes to the structure (like when I was deciding where that field should land in the final structure layout) required changes in a lot of place. This way, change it once and everything compiles fine. I can condense the name. :)

okay... you are the one that will be typing this

shamisp · 2017-05-11T18:23:54Z

src/uct/ugni/udt/ugni_udt_iface.c

@@ -479,8 +466,6 @@ static UCS_CLASS_INIT_FUNC(uct_ugni_udt_iface_t, uct_md_h md, uct_worker_h worke
        ucs_warn("GNI_EpDestroy failed, Error status: %s %d",
                 gni_err_str[ugni_rc], ugni_rc);
    }
- clean_iface_activate:
-    ugni_deactivate_iface(&self->super);


Where did the cleanup go ?

In a different commit that apparently I failed to include. Oops, will rectify that.

Overall I noticed that the clean up flow disappeared from a few places.

Story is that there are multiple related commits, together they are massive so I broke them up. The clean up path was fixed in the next PR, but I need to fix it for this one.

shamisp · 2017-05-11T18:25:41Z

src/uct/ugni/base/ugni_iface.h

+static inline int uct_ugni_check_device_type(uct_ugni_iface_t *iface, gni_nic_device_t type)
+{
+    uct_ugni_device_t *dev = uct_ugni_iface_device(iface);
+    return dev->type == type;


bool instead of int ?
Also I can be a single line.

shamisp · 2017-05-11T18:26:55Z

src/uct/ugni/base/ugni_iface.c

+                           GNI_CQ_NOBLOCK,
+                           NULL, NULL, &self->local_cq);
+    if (GNI_RC_SUCCESS != ugni_rc) {
+        ucs_error("GNI_CqCreate failed, Error status: %s %d",


Somebody has to clean CDM ?

mellanox-github · 2017-05-11T20:41:43Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1638/ for details.

mellanox-github · 2017-05-11T21:40:00Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1640/ for details.

mellanox-github · 2017-05-11T22:00:15Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3557/ for details (Mellanox internal link).

shamisp · 2017-05-11T22:47:35Z

src/uct/ugni/udt/ugni_udt_iface.h

@@ -101,4 +101,8 @@ static inline int uct_ugni_udt_ep_any_post(uct_ugni_udt_iface_t *iface)
    return UCS_OK;
 }

+static inline gni_nic_handle_t uct_ugni_udt_iface_nic_handle(uct_ugni_udt_iface_t *iface)


Looks like a macro to me..

Looks like an inline function. The one just pushed one looks like a macro.

shamisp · 2017-05-11T22:48:04Z

@hppritcha if you have cycles, please take a look.

mellanox-github · 2017-05-11T23:19:56Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3559/ for details (Mellanox internal link).

mellanox-github · 2017-05-12T15:09:25Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1651/ for details.

MattBBaker · 2017-05-12T16:24:12Z

ORNL jenkins failure is an mlx failure that is already known. Not relevant to this PR.

mellanox-github · 2017-05-12T16:32:57Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3569/ for details (Mellanox internal link).

MattBBaker · 2017-05-15T01:05:55Z

bot:ornl:retest

shamisp · 2017-05-15T13:23:41Z

src/uct/ugni/base/ugni_device.c

+    if (GNI_RC_SUCCESS != ugni_rc) {
+        ucs_error("GNI_CdmAttach failed (domain id %d, %d), Error status: %s %d",
+                  cdm->domain_id, ugni_domain_counter, gni_err_str[ugni_rc], ugni_rc);
+        GNI_CdmDestroy(cdm->cdm_handle);


No status check for the return code

MattBBaker · 2017-05-18T20:41:44Z

@shamisp Updated. Should be good to merge now yes?

shamisp

Please review the rest of the code for similar issues

shamisp · 2017-05-18T21:09:53Z

src/uct/ugni/base/ugni_iface.c

    }
    return status;
+clean_cq:
+    GNI_CqDestroy(self->local_cq);


Log error code

shamisp · 2017-05-18T21:11:14Z

src/uct/ugni/smsg/ugni_smsg_iface.c

    ucs_mpool_cleanup(&self->free_desc, 1);
    ucs_mpool_cleanup(&self->free_mbox, 1);
+    GNI_CqDestroy(self->remote_cq);


please log the error

mellanox-github · 2017-05-18T21:15:04Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/1693/ for details.

shamisp · 2017-05-18T21:43:42Z

src/uct/ugni/base/ugni_iface.c

+{
+    ucs_arbiter_cleanup(&iface->arbiter);
+    ucs_mpool_cleanup(&iface->flush_pool, 1);
+    GNI_CqDestroy(iface->local_cq);


@shamisp Thing is, the next PR pulls out all of the GNI_Cq* calls and has common spin locked code paths with error logging. I'd like to fix these problems in the next PR instead.

shamisp · 2017-05-18T21:46:05Z

src/uct/ugni/smsg/ugni_smsg_ep.c

@@ -180,7 +180,7 @@ ucs_status_t uct_ugni_smsg_ep_connect_to_ep(uct_ep_h tl_ep,
    }

    ep_hash = (uint32_t)iface_addr->ep_hash;
-    gni_rc = GNI_EpSetEventData(ep->super.ep, iface->domain_id, ep_hash);
+    gni_rc = GNI_EpSetEventData(ep->super.ep, iface->cdm.domain_id, ep_hash);


seems in some places we use gni_rc and another ugni_rc. IMHO should be consistent. (separate PR)

Sounds like a plan.

mellanox-github · 2017-05-18T22:02:14Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/3623/ for details (Mellanox internal link).

shamisp · 2017-05-19T18:45:08Z

Opened Issue #1529

MattBBaker added this to the v1.3 milestone May 11, 2017

UCT/UGNI: Start using spinlocks to protect critical structures

e88b6c6

MattBBaker force-pushed the topic/uct-ugni-thread-safe-cdm branch from 55f8146 to e88b6c6 Compare May 11, 2017 15:32

shamisp requested changes May 11, 2017

View reviewed changes

Add back in missing cdm cleanup paths

6dc47d9

shamisp reviewed May 11, 2017

View reviewed changes

shamisp added the Waiting for Author Response label May 12, 2017

UCT/UGNI: Change small inline functions into macros

6a82d5c

MattBBaker force-pushed the topic/uct-ugni-thread-safe-cdm branch from 38e5ea0 to 6a82d5c Compare May 12, 2017 14:25

MattBBaker added Enhancement Ready for Review and removed Waiting for Author Response labels May 12, 2017

shamisp reviewed May 17, 2017

View reviewed changes

Log an error message of tearing down the CDM fails.

aa2b76c

shamisp reviewed May 18, 2017

View reviewed changes

shamisp approved these changes May 19, 2017

View reviewed changes

shamisp merged commit 6b29f42 into openucx:master May 19, 2017

MattBBaker deleted the topic/uct-ugni-thread-safe-cdm branch May 24, 2017 19:24

UCT/UGNI: Start using spinlocks to protect critical structures #1494

UCT/UGNI: Start using spinlocks to protect critical structures #1494

Conversation

MattBBaker commented May 10, 2017

mellanox-github commented May 10, 2017

mellanox-github commented May 10, 2017

MattBBaker commented May 10, 2017

shamisp commented May 10, 2017

MattBBaker commented May 11, 2017

shamisp commented May 11, 2017

MattBBaker commented May 11, 2017

MattBBaker commented May 11, 2017

MattBBaker commented May 11, 2017

shamisp commented May 11, 2017 via email

MattBBaker commented May 11, 2017

mellanox-github commented May 11, 2017

mellanox-github commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellanox-github commented May 11, 2017

mellanox-github commented May 11, 2017

mellanox-github commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamisp commented May 11, 2017

mellanox-github commented May 11, 2017

mellanox-github commented May 12, 2017

MattBBaker commented May 12, 2017

mellanox-github commented May 12, 2017

MattBBaker commented May 15, 2017

Choose a reason for hiding this comment

MattBBaker commented May 18, 2017

shamisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellanox-github commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellanox-github commented May 18, 2017

shamisp commented May 19, 2017