UCP: Add prot parameter to UCP API. #5309

leibin2014 · 2020-06-21T08:58:45Z

What

Add prot READ/WRITE to prot parameters of UCP API.

Why ?

Currently prot READ|WRITE is hard code in UCX, we need configure them through UCP API.

How ?

Add prot READ/WRITE relative macros in the prot parameter of UCP API.

leibin2014 · 2020-06-30T04:24:35Z

@yosefe Fixed according to our last talk.

leibin2014 · 2020-07-01T02:08:06Z

/azp run

azure-pipelines · 2020-07-01T02:08:10Z

Commenter does not have sufficient privileges for PR 5309 in repo openucx/ucx

src/ucp/api/ucp.h

yosefe · 2020-06-30T08:44:07Z

src/ucp/api/ucp.h

+ * ucp_mem_map() function.
+ */
+enum {
+    UCP_MEM_MAP_WRITE   = UCS_BIT(1), /**< Enable write access. */


UCP_MEM_MAP_PROT_READ = UCS_BIT(0)
UCP_MEM_MAP_PROT_WRITE = UCS_BIT(1)

@yosefe Done

yosefe · 2020-06-30T08:45:04Z

src/uct/api/uct.h

@@ -699,15 +699,21 @@ enum uct_md_mem_flags {
    UCT_MD_MEM_ACCESS_REMOTE_PUT    = UCS_BIT(5), /**< enable remote put access */
    UCT_MD_MEM_ACCESS_REMOTE_GET    = UCS_BIT(6), /**< enable remote get access */
    UCT_MD_MEM_ACCESS_REMOTE_ATOMIC = UCS_BIT(7), /**< enable remote atomic access */
+    UCT_MD_MEM_ACCESS_LOCAL_WRITE   = UCS_BIT(8), /**< enable local write access */


pls remove UCT part from this PR (add in another PR)

yosefe · 2020-07-01T07:07:39Z

/azp run

azure-pipelines · 2020-07-01T07:07:52Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-01T07:15:21Z

/azp run

azure-pipelines · 2020-07-01T07:15:32Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-07-01T09:13:29Z

src/ucp/api/ucp.h

 *
 * The enumeration list describes the memory mapping protections supported by @ref
 * ucp_mem_map() function.
 */
 enum {
-    UCP_MEM_MAP_WRITE   = UCS_BIT(1), /**< Enable write access. */
-    UCP_MEM_MAP_READ    = UCS_BIT(2)  /**< Enable read access. */
+    UCP_MEM_MAP_PROT_READ   = UCS_BIT(0), /**< Enable read  access. */


pls remove space after 'read'

Done. Thanks!

leibin2014 · 2020-07-01T10:58:48Z

/azp run

azure-pipelines · 2020-07-01T10:59:01Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-07-02T12:53:54Z

src/ucp/core/ucp_mm.c


    /* always acquire context lock */
    UCP_THREAD_CS_ENTER(&context->mt_lock);

-    mem_params = *params;
-    status = ucp_mem_map_check_and_adjust_params(&mem_params);


shall we delete ucp_mem_map_check_and_adjust_params() function?

@yosefe Deleted. Thanks!

yosefe · 2020-07-02T12:55:17Z

src/ucp/core/ucp_mm.c

+        goto out;
+    }
+
+    if (((params->field_mask & UCP_MEM_MAP_PARAM_FIELD_FLAGS) &&


unsigned flags;
...
flags = UCP_PARAM_VALUE(MEM_MAP, params, flags, FLAGS, 0);
then, can use 'flags' local var without checking the field every time

@yosefe Fixed.

yosefe · 2020-07-02T12:56:01Z

src/ucp/core/ucp_mm.c

+    if (!(params->field_mask & UCP_MEM_MAP_PARAM_FIELD_ADDRESS)) {
+        address = NULL;
+    } else {
+        address = params->address;
+    }


use UCP_PARAM_VALUE macro

leibin2014 · 2020-07-03T02:04:55Z

retest with bot:pipe:retest

leibin2014 · 2020-07-03T02:07:11Z

bot:pipe:retest

leibin2014 · 2020-07-03T09:41:22Z

bot:pipe:retest

leibin2014 · 2020-07-03T18:55:08Z

bot:pipe:retest

leibin2014 · 2020-07-03T18:56:24Z

bot:pipe:retest

yosefe

@evgeny-leksikov can you pls take a look as well?

yosefe · 2020-07-04T11:53:15Z

src/ucp/core/ucp_mm.c

+ * | result    | err if | alloc/reg | err | reg | err | alloc/reg | err | alloc/reg |
+ * |           | len >0 |           |     |     |     |  (hint)   |     | (fixed)   |
+ * |--------------------------------------------------------------------------------|
+ */


👍 for keeping the documentation

yosefe · 2020-07-06T08:06:23Z

@shamisp @tonycurtis can you pls take a look?

tonycurtis

ucp.h:447 would add "the" after "by"

otherwise fine.

shamisp · 2020-07-06T13:45:11Z

👍

shamisp · 2020-07-06T13:45:39Z

squash please

shamisp · 2020-07-06T13:50:04Z

src/ucp/api/ucp.h

+ * ucp_mem_map() function.
+ */
+enum {
+    UCP_MEM_MAP_PROT_READ   = UCS_BIT(0), /**< Enable read access. */


I would like to withdraw my approval based on UCT patch review :) Let's add LOCAL to both - enum name and description. @yosefe Maybe you want to use this opportunity to add remote read/write ?

UCT patch #5348

@shamisp do you mean the flags we need are: UCP_MAM_MAP_PROT_[LOCAL|REMOTE]_[READ|WRITE] ?

I think so. Even if you want to define only local for now, I would explicitly name it _local_.

Also, to some extend this might be a hint, since some transports may not have permissions for read/write, etc.

@shamisp @yosefe Fixed. One more look pls. Just one more thing to confirm, we are using REMOTE_GET and REMOTE_PUT in UCT, we can use REMOTE_READ and REMOTE_WRITE in UCP. Right?

@yosefe I don't understand what is the value of pre-defined UCP_MEM_MAP_PROT_READ = UCP_MEM_MAP_PROT_LOCAL_READ| UCP_MEM_MAP_PROT_REMOTE_READ

syntax sugar

IMHO, it looks completely out of place in UCP.H. Extra enum to document and manage with no additional value.

@shamisp @yosefe I removed UCP_MEM_MAP_PROT_READ/UCP_MEM_MAP_PROT_WRITE.

leibin2014 · 2020-07-07T03:04:26Z

ucp.h:447 would add "the" after "by"

otherwise fine.

Fixed. Thanks!

azure-pipelines · 2020-07-10T12:51:03Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-11T03:41:32Z

/azp run

azure-pipelines · 2020-07-11T03:41:55Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-11T04:00:04Z

/azp run

azure-pipelines · 2020-07-11T04:00:34Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-11T04:04:26Z

/azp run

azure-pipelines · 2020-07-11T04:04:39Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-11T04:41:04Z

/azp run

azure-pipelines · 2020-07-11T04:41:34Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-07-11T10:25:16Z

@leibin2014 can you pls squash?

leibin2014 · 2020-07-13T02:36:02Z

@leibin2014 can you pls squash?

@yosefe It was squashed before the last /azp run. Thanks!

yosefe · 2020-07-13T08:52:16Z

@yosefe It was squashed before the last /azp run. Thanks!

@leibin2014 there are 6 commits in the PR, it should be 1 only

leibin2014 · 2020-07-13T10:00:48Z

@yosefe It was squashed before the last /azp run. Thanks!

@leibin2014 there are 6 commits in the PR, it should be 1 only

OK, thought it didn't matter having "Merge pull request #8 from openucx/master" this kind of commits. Done now. Thanks!

leibin2014 · 2020-07-14T02:13:51Z

/azp run

azure-pipelines · 2020-07-14T02:14:03Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-14T02:44:31Z

/azp run

azure-pipelines · 2020-07-14T02:44:43Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-14T04:16:11Z

/azp run

azure-pipelines · 2020-07-14T04:16:24Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-07-14T09:18:01Z

bot:pipe:retest

yosefe · 2020-07-14T15:05:55Z

bot:pipe:retest

yosefe reviewed Jul 1, 2020

View reviewed changes

leibin2014 changed the title ~~UCP/UCT: Add prot READ/WRITE to flag parameters of UCP and UCT API.~~ UCP/UCT: Add prot READ/WRITE to parameters of UCP API. Jul 1, 2020

leibin2014 changed the title ~~UCP/UCT: Add prot READ/WRITE to parameters of UCP API.~~ UCP: Add prot READ/WRITE to parameters of UCP API. Jul 1, 2020

yosefe reviewed Jul 1, 2020

View reviewed changes

yosefe reviewed Jul 2, 2020

View reviewed changes

yosefe approved these changes Jul 4, 2020

View reviewed changes

yosefe added the API label Jul 4, 2020

leibin2014 force-pushed the master branch from dc50318 to 15b8601 Compare July 4, 2020 18:49

leibin2014 changed the title ~~UCP: Add prot READ/WRITE to parameters of UCP API.~~ UCP: Add prot parameter to UCP API. Jul 4, 2020

evgeny-leksikov approved these changes Jul 6, 2020

View reviewed changes

tonycurtis suggested changes Jul 6, 2020

View reviewed changes

shamisp requested changes Jul 6, 2020

View reviewed changes

leibin2014 force-pushed the master branch from 788e3ad to f610443 Compare July 13, 2020 09:53

leibin2014 force-pushed the master branch 3 times, most recently from 6a8edc2 to ec8da5b Compare July 14, 2020 04:44

UCP: Add prot READ/WRITE to prot parameter of UCP API.

c354684

leibin2014 force-pushed the master branch from ec8da5b to c354684 Compare July 14, 2020 04:55

yosefe approved these changes Jul 14, 2020

View reviewed changes

yosefe mentioned this pull request Jul 14, 2020

UCP/API: Add flag to pass memory type info in ucp_mem_map() API #5395

Merged

yosefe merged commit 9afdce2 into openucx:master Jul 15, 2020

UCP: Add prot parameter to UCP API. #5309

UCP: Add prot parameter to UCP API. #5309

Conversation

leibin2014 commented Jun 21, 2020 • edited Loading

What

Why ?

How ?

leibin2014 commented Jun 30, 2020

leibin2014 commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

leibin2014 commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibin2014 commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

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

leibin2014 commented Jul 3, 2020

leibin2014 commented Jul 3, 2020

leibin2014 commented Jul 3, 2020

leibin2014 commented Jul 3, 2020

leibin2014 commented Jul 3, 2020

yosefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Jul 6, 2020

tonycurtis left a comment

Choose a reason for hiding this comment

shamisp commented Jul 6, 2020

shamisp commented Jul 6, 2020

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

leibin2014 Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

leibin2014 commented Jul 7, 2020 • edited Loading

azure-pipelines bot commented Jul 10, 2020

leibin2014 commented Jul 11, 2020

azure-pipelines bot commented Jul 11, 2020

leibin2014 commented Jul 11, 2020

azure-pipelines bot commented Jul 11, 2020

leibin2014 commented Jul 11, 2020

azure-pipelines bot commented Jul 11, 2020

leibin2014 commented Jul 11, 2020

azure-pipelines bot commented Jul 11, 2020

yosefe commented Jul 11, 2020

leibin2014 commented Jul 13, 2020

yosefe commented Jul 13, 2020

leibin2014 commented Jul 13, 2020

leibin2014 commented Jul 14, 2020

azure-pipelines bot commented Jul 14, 2020

leibin2014 commented Jul 14, 2020

azure-pipelines bot commented Jul 14, 2020

leibin2014 commented Jul 14, 2020

azure-pipelines bot commented Jul 14, 2020

leibin2014 commented Jul 14, 2020

yosefe commented Jul 14, 2020

leibin2014 commented Jun 21, 2020 •

edited

Loading

leibin2014 Jul 9, 2020 •

edited

Loading

leibin2014 commented Jul 7, 2020 •

edited

Loading