UCT/API: remove redundant UCT_CB_FLAG_SYNC #2938

evgeny-leksikov · 2018-10-10T12:49:54Z

What

Remove redundant UCT_CB_FLAG_SYNC

Why ?

Flags UCT_CB_FLAG_SYNC and UCT_CB_FLAG_ASYNC are mutually exclusive

swx-jenkins1 · 2018-10-10T13:49:22Z

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

mellanox-github · 2018-10-10T16:07:39Z

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

shamisp · 2018-10-11T18:27:54Z

@hjelmn FYI

hjelmn · 2018-10-11T18:45:46Z

Yeah, was wondering about that. Seemed odd to have both.

hjelmn · 2018-10-11T19:14:10Z

src/uct/api/uct.h

+                                         UCT_IFACE_FLAG_CB_SYNC flag is set),
+                                         the callback may be invoked only from
+                                         the context that called @ref
+                                         uct_iface_progress). */
 };


Its probably a bad idea to change the UCS_BIT of the async flag. Can break code that is using UCT. Probably better off just blocking out UCS_BIT(1) for awhile and leaving async as UCS_BIT(2).

shamisp · 2018-10-11T19:39:15Z

@hjelmn technically it breaks backward compatibility. But if you are not concerned about this, I'm not supper worried.

hjelmn · 2018-10-11T19:42:38Z

@shamisp I am not too concerned about it. Just concerned at the possibility that UCS_BIT(1) gets redefined at some point to something I don't want. Though I don't use ASYNC right now so I guess just protecting myself by checking for existence of the sync flag in btl/uct is enough. Linking should be protected by a new .so version for libuct.so.

shamisp · 2018-10-12T14:05:31Z

@hjelmn - maybe we should start with UCS_BIT(3) ? @yosefe ?

evgeny-leksikov · 2018-10-12T15:07:56Z

@shamisp why it's better than UCS_BIT(0) which was not used as well? may be rename UCS_BIT(1) and UCS_BIT(2) to UCT_CB_FLAG_DEPRECATED_XXX until lib version is bumped?

hjelmn · 2018-10-12T15:12:56Z

@evgeny-leksikov Just doesn't make as much sense to change the value of the async flag. Then could also block out the old sync flag until the so is bumped. That would make sense to me.

shamisp · 2018-10-12T15:14:31Z

@hjelmn - agreed. This essentially was my intend.

evgeny-leksikov · 2018-10-12T15:21:36Z

So, just to clarify:

UCT_CB_FLAG_ASYNC = UCS_BIT(2) - old value
return UCS_ERR_INVALID_PARAM if UCS_BIT(1) is set or block it by some "deprecated" name?

hjelmn · 2018-10-12T15:38:26Z

@evgeny-leksikov Yes. That would be the safest approach. But do both the block and error return to be safe.

swx-jenkins1 · 2018-10-12T17:17:14Z

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

mellanox-github · 2018-10-12T19:17:39Z

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

yosefe · 2018-10-14T14:11:59Z

bot:mlx:retest

yosefe · 2018-10-14T14:22:24Z

src/uct/api/uct.h

-                                         which only supports sync callback
-                                         (i.e., only the @ref UCT_IFACE_FLAG_CB_SYNC flag is set),
-                                         it will behave exactly like a sync callback.  */
+    UCT_CB_FLAG_DEPRECATED = UCS_BIT(1), /**< Deprecated value. */


UCT_CB_FLAG_RESERVED /* reserved for future use */

yosefe · 2018-10-14T14:23:05Z

src/uct/api/uct.h

-                                         (i.e., only the @ref UCT_IFACE_FLAG_CB_SYNC flag is set),
-                                         it will behave exactly like a sync callback.  */
+    UCT_CB_FLAG_DEPRECATED = UCS_BIT(1), /**< Deprecated value. */
+    UCT_CB_FLAG_ASYNC = UCS_BIT(2)  /**< Callback may be invoked from any


pls indent on column

yosefe · 2018-10-14T14:24:03Z

src/uct/base/uct_iface.c

    status = uct_iface_query(tl_iface, &attr);
    if (status != UCS_OK) {
        return status;
    }

+    if (flags & UCT_CB_FLAG_DEPRECATED) {
+        ucs_error("Deprecated flags value");


"Unsupported callback flag 0x%x"

yosefe · 2018-10-14T14:26:04Z

src/uct/base/uct_iface.c

@@ -412,6 +412,10 @@ UCS_CLASS_INIT_FUNC(uct_base_iface_t, uct_iface_ops_t *ops, uct_md_h md,

    UCS_CLASS_CALL_SUPER_INIT(uct_iface_t, ops);

+    if (params->err_handler_flags & UCT_CB_FLAG_DEPRECATED) {
+        return UCS_ERR_INVALID_PARAM;


"Unsupported callback flag 0x%x"
maybe wrap this in a macro to avoid code duplication with am_handler flags check

mellanox-github · 2018-10-14T15:45:17Z

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

mellanox-github · 2018-10-15T07:46:58Z

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

swx-jenkins1 · 2018-10-15T07:49:50Z

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

yosefe · 2018-10-15T07:57:58Z

@shamisp @gmegan @hjelmn pls take a look

mellanox-github · 2018-10-15T09:16:00Z

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

swx-jenkins1 · 2018-10-15T09:31:29Z

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

swx-jenkins1 · 2018-10-15T10:38:18Z

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

mellanox-github · 2018-10-15T12:38:19Z

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

shamisp · 2018-10-15T14:53:53Z

👍

evgeny-leksikov · 2018-10-15T18:54:10Z

Not related failures should be fixed by #2958
bot:mlx:retest

mellanox-github · 2018-10-15T22:20:44Z

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

evgeny-leksikov · 2018-10-15T22:49:03Z

timed out
bot:mlx:retest

mellanox-github · 2018-10-16T03:25:19Z

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

evgeny-leksikov · 2018-10-16T04:33:09Z

#2958
bot:mlx:retest

mellanox-github · 2018-10-16T07:32:20Z

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

evgeny-leksikov · 2018-10-16T08:01:18Z

bot:mlx:retest

mellanox-github · 2018-10-16T11:13:26Z

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

evgeny-leksikov · 2018-10-16T12:41:19Z

bot:mlx:retest

mellanox-github · 2018-10-16T16:08:54Z

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

evgeny-leksikov · 2018-10-16T20:20:48Z

bot:mlx:retest

mellanox-github · 2018-10-16T23:35:10Z

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

evgeny-leksikov · 2018-10-17T04:22:18Z

good to go?

UCT/API: remove redundant UCT_CB_FLAG_SYNC

09f5940

evgeny-leksikov added API Cleanup labels Oct 10, 2018

brminich approved these changes Oct 11, 2018

View reviewed changes

evgeny-leksikov requested a review from yosefe October 11, 2018 08:24

hjelmn reviewed Oct 11, 2018

View reviewed changes

UCT/API: uct_cb_flags, apply CR comments

85311af

yosefe reviewed Oct 14, 2018

View reviewed changes

UCT/API: uct_cb_flags, apply CR comments (2)

95aed6c

yosefe approved these changes Oct 15, 2018

View reviewed changes

yosefe added the Ready for Review label Oct 15, 2018

GTEST: fix uct_iface_params in test_tag

a86036f

evgeny-leksikov force-pushed the uct_am_cb_refact branch from caf4e68 to a86036f Compare October 15, 2018 09:16

yosefe merged commit a90c731 into openucx:master Oct 17, 2018

UCT/API: remove redundant UCT_CB_FLAG_SYNC #2938

UCT/API: remove redundant UCT_CB_FLAG_SYNC #2938

Conversation

evgeny-leksikov commented Oct 10, 2018

What

Why ?

swx-jenkins1 commented Oct 10, 2018

mellanox-github commented Oct 10, 2018

shamisp commented Oct 11, 2018

hjelmn commented Oct 11, 2018

hjelmn Oct 11, 2018

Choose a reason for hiding this comment

shamisp commented Oct 11, 2018

hjelmn commented Oct 11, 2018 • edited Loading

shamisp commented Oct 12, 2018

evgeny-leksikov commented Oct 12, 2018

hjelmn commented Oct 12, 2018

shamisp commented Oct 12, 2018

evgeny-leksikov commented Oct 12, 2018

hjelmn commented Oct 12, 2018

swx-jenkins1 commented Oct 12, 2018

mellanox-github commented Oct 12, 2018

yosefe commented Oct 14, 2018

yosefe Oct 14, 2018

Choose a reason for hiding this comment

yosefe Oct 14, 2018

Choose a reason for hiding this comment

yosefe Oct 14, 2018

Choose a reason for hiding this comment

yosefe Oct 14, 2018

Choose a reason for hiding this comment

mellanox-github commented Oct 14, 2018

mellanox-github commented Oct 15, 2018

swx-jenkins1 commented Oct 15, 2018

yosefe commented Oct 15, 2018

mellanox-github commented Oct 15, 2018

swx-jenkins1 commented Oct 15, 2018

swx-jenkins1 commented Oct 15, 2018

mellanox-github commented Oct 15, 2018

shamisp commented Oct 15, 2018

evgeny-leksikov commented Oct 15, 2018

mellanox-github commented Oct 15, 2018

evgeny-leksikov commented Oct 15, 2018

mellanox-github commented Oct 16, 2018

evgeny-leksikov commented Oct 16, 2018

mellanox-github commented Oct 16, 2018

evgeny-leksikov commented Oct 16, 2018

mellanox-github commented Oct 16, 2018

evgeny-leksikov commented Oct 16, 2018

mellanox-github commented Oct 16, 2018

evgeny-leksikov commented Oct 16, 2018

mellanox-github commented Oct 16, 2018

evgeny-leksikov commented Oct 17, 2018

hjelmn commented Oct 11, 2018 •

edited

Loading