Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

rpma: Atomic write with IBV_SEND_FENCE flag #603

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Conversation

ewanglong
Copy link

@ewanglong ewanglong commented Nov 23, 2020

This change is Reviewable

Copy link

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ewanglong)


src/mr.c, line 171 at r1 (raw file):

	wr.send_flags = (flags & RPMA_F_COMPLETION_ON_SUCCESS) ?
		IBV_SEND_SIGNALED : 0;
	wr.send_flags |= (flags & RPMA_F_COMPLETION_ON_ERROR) ?

I do not understand the condition here. Can you elaborate?

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @ewanglong and @janekmi)


src/mr.h, line 41 at r2 (raw file):

int fence

why is this not combined with other flags?

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 9 files at r2.
Reviewable status: 3 of 9 files reviewed, 2 unresolved discussions (waiting on @ewanglong)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 8 of 9 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ewanglong)


src/mr.h, line 41 at r2 (raw file):

int

bool is enough


src/mr.c, line 171 at r1 (raw file):

flags & RPMA_F_COMPLETION_ON_ERROR

fence & RPMA_F_SEND_FENCE


src/include/librpma.h, line 426 at r2 (raw file):

#define RPMA_F_SEND_NOFENCE		0
#define RPMA_F_SEND_FENCE		1

Shall it be in mr.h? as it is not a part of public API yet


tests/unit/common/test-common.h, line 33 at r2 (raw file):

#define MOCK_NOFENCE		0
#define MOCK_FENCE			1

shall we use RPMA_F_SEND_NOFENCE and RPMA_F_SEND_FENCE instead of MOCK_(NO)FENCE
or just True/False if we use bool instead of int?


tests/unit/mr/mr-write.c, line 23 at r2 (raw file):

 * with RPMA_E_PROVIDER when send_flags == RPMA_F_COMPLETION_ON_SUCCESS
 */
static void

tests for FENCE are missing

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 8 files reviewed, 7 unresolved discussions (waiting on @ewanglong, @grom72, @janekmi, and @ldorau)


src/mr.h, line 41 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…
int fence

why is this not combined with other flags?

Why is bool fence not merged with int flags?


src/mr.c, line 171 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not understand the condition here. Can you elaborate?

I do not understand it either, wr.send_flags can have only IBV_* values

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 6 files at r3.
Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @ewanglong, @grom72, and @janekmi)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ewanglong, @janekmi, and @ldorau)


src/mr.h, line 41 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Why is bool fence not merged with int flags?

because flags is related to public API while fence is an internal feature

Copy link

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 9 files at r2, 5 of 6 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ewanglong, @grom72, and @ldorau)


src/mr.c, line 172 at r3 (raw file):

		IBV_SEND_SIGNALED : 0;
	wr.send_flags |= (fence == true) ?
		IBV_SEND_FENCE : 0;

I think this can fit into the previous line.


tests/unit/common/test-common.h, line 33 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…
#define MOCK_NOFENCE		0
#define MOCK_FENCE			1

shall we use RPMA_F_SEND_NOFENCE and RPMA_F_SEND_FENCE instead of MOCK_(NO)FENCE
or just True/False if we use bool instead of int?

I like it the way it is.

Copy link

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ewanglong, @grom72, and @ldorau)

a discussion (no related file):
Please use common: prefix for your commit.


Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ewanglong)

Copy link

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ewanglong)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ewanglong)


src/mr.c, line 171 at r3 (raw file):

fence == true

fence instead of fence == true

and the rest of operator ? in the same line

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @ewanglong, @grom72, @janekmi, and @ldorau)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ewanglong)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 9 files at r2, 2 of 6 files at r3, 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ewanglong)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ewanglong)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants