Implement rpma write with immediate data

[yangx-jy]

This change is 

[yangx-jy]

Hi @grom72 @ldorau @janekmi

1. From IB spec, rdma's atomic operation doesn't support imm_data.
2. As @grom72 metioned, rpma will enhance write atomic by introducing rdma's atomic operation.

According to above reasons, is it wrong to support imm_data for current rpma_write_atomic()?
Perhaps, I should remove the related code which make rpma_write_atomic() support imm_data.

Best Regards,
Xiao Yang

Reviewable status: 0 of 22 files reviewed, all discussions resolved

[janekmi]

Reviewed 3 of 22 files at r1.
Reviewable status: 3 of 22 files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

a discussion (no related file):
I have discussed this topic with @grom72. The native atomic write, when it will happen, won't have a variant with immediate. So we cannot have it in our API. @yangx-jy please remove the rpma_write_atomic_with_imm().

---

[janekmi]

Reviewed 8 of 22 files at r1.
Reviewable status: 7 of 21 files reviewed, 4 unresolved discussions (waiting on @janekmi and @yangx-jy)

---

src/conn.c, line 343 at r1 (raw file):

{
	if (conn == NULL || dst == NULL || src == NULL || flags == 0)
	return RPMA_E_INVAL;

Indentation.

---

src/conn.c, line 601 at r1 (raw file):

	 */
	if ((cmpl->op == RPMA_OP_RECV) ||
		(cmpl->op == RPMA_OP_RECV_RDMA_WITH_IMM)) {

I think with one more tab it looks better. Please consider.

---

src/conn.c, line 602 at r1 (raw file):

	if ((cmpl->op == RPMA_OP_RECV) ||
		(cmpl->op == RPMA_OP_RECV_RDMA_WITH_IMM)) {
		if (cmpl->flags & IBV_WC_WITH_IMM)

You are sure it is expected for both IBV_WC_RECV and IBV_WC_RECV_RDMA_WITH_IMM?

[janekmi]

Reviewed 3 of 22 files at r1, 4 of 9 files at r2.
Reviewable status: 14 of 21 files reviewed, 4 unresolved discussions (waiting on @yangx-jy)

[yangx-jy]

Reviewable status: 14 of 21 files reviewed, 4 unresolved discussions (waiting on @janekmi and @yangx-jy)

---

src/conn.c, line 602 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You are sure it is expected for both IBV_WC_RECV and IBV_WC_RECV_RDMA_WITH_IMM?

Hi @janekmi

For send with imm_data, IBV_WC_RECV is expected.
For rdma write with imm_data, IBV_WC_RECV_RDMA_WITH_IMM is expected.
Do you think the condition is not correct here?

Best Regards,
Xiao Yang

[janekmi]

Reviewed 1 of 22 files at r1, 1 of 9 files at r2.
Reviewable status: 16 of 21 files reviewed, 5 unresolved discussions (waiting on @janekmi and @yangx-jy)

---

src/conn.c, line 564 at r2 (raw file):

		RPMA_LOG_ERROR("unsupported wc.opcode == %d", wc.opcode);
		return RPMA_E_NOSUPP;
	}

@grom72 @ldorau @osalyk This switch-case looks dumb. We have to reconsider its existence.

[janekmi]

Reviewed 1 of 22 files at r1, 2 of 9 files at r2.
Reviewable status: 19 of 21 files reviewed, 6 unresolved discussions (waiting on @janekmi and @yangx-jy)

---

examples/11-write-with-imm/README.md, line 9 at r2 (raw file):

- The server receives a message with immediate data from client.
The immediate data is compared with the expected immediate data sent
by the client as the private data during establishing the connection.

Why not just transfer the same data via write and immediate?
It would be a nice twist compared to what we have done for send with immediate.

[janekmi]

Reviewed 1 of 22 files at r1, 1 of 9 files at r2.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @janekmi and @yangx-jy)

---

examples/11-write-with-imm/client.c, line 2 at r2 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright (c) 2020-2021 Fujitsu */

2021 only.

---

examples/11-write-with-imm/client.c, line 72 at r2 (raw file):

	/* register the memory */
	ret = rpma_mr_reg(peer, src, KILOBYTE, RPMA_MR_USAGE_SEND, &src_mr);

RPMA_MR_USAFE_WRITE_SRC?

---

examples/11-write-with-imm/client.c, line 80 at r2 (raw file):

	 * data for validation on the sever side
	 */
	struct rpma_conn_private_data src_pdata;

Why not just pdata as for example no 10?

---

examples/11-write-with-imm/client.c, line 88 at r2 (raw file):

	/* obtain the remote memory description */
	struct rpma_conn_private_data dst_pdata;

We can reuse the structure. No need for having another one.

---

examples/11-write-with-imm/client.c, line 136 at r2 (raw file):

		fprintf(stderr,
			"an unexpected type of operation (%d != %d)\n",
			cmpl.op, RPMA_OP_SEND);

RPMA_OP_WRITE

---

examples/11-write-with-imm/server.c, line 2 at r2 (raw file):

// SPDX-License-Identifier: BSD-3-Clause
/* Copyright (c) 2020-2021 Fujitsu */

2021?

---

examples/11-write-with-imm/server.c, line 91 at r2 (raw file):

	/* prepare to receive a message with immediate data from the client */
	ret = rpma_conn_req_recv(req, dst_mr, 0, KILOBYTE, NULL);

dst_mr is the write destination, not a receive. You have to post a Receive Request using another buffer. It can be very small since it does not will carry any data in this example.

---

examples/11-write-with-imm/server.c, line 170 at r2 (raw file):

		ret = -1;
	} else {
		printf("receive a value %s with immediate data %u\n", dst,

a value %s was written with immediate data %u

[janekmi]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @yangx-jy)

---

src/conn.c, line 602 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @janekmi

For send with imm_data, IBV_WC_RECV is expected.
For rdma write with imm_data, IBV_WC_RECV_RDMA_WITH_IMM is expected.
Do you think the condition is not correct here?

Best Regards,
Xiao Yang

I was thinking for a while because having IBV_WC_RECV_RDMA_WITH_IMM indicates the same what IBV_WC_WITH_IMM so the letter one seems redundant in this case. But the documentation seems to support your implementation.

I have to do a hands-on. ;-)

[codecov-io]

Codecov Report

Merging #856 (d66cf3c) into master (d021126) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #856   +/-   ##
=======================================
  Coverage   99.83%   99.83%           
=======================================
  Files          15       15           
  Lines        1197     1215   +18     
=======================================
+ Hits         1195     1213   +18     
  Misses          2        2           

[yangx-jy]

Reviewable status: 15 of 21 files reviewed, 11 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I have discussed this topic with @grom72. The native atomic write, when it will happen, won't have a variant with immediate. So we cannot have it in our API. @yangx-jy please remove the rpma_write_atomic_with_imm().

Done.

---

---

examples/11-write-with-imm/client.c, line 2 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

2021 only.

Done.

---

examples/11-write-with-imm/client.c, line 72 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

RPMA_MR_USAFE_WRITE_SRC?

Done.

---

examples/11-write-with-imm/client.c, line 80 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why not just pdata as for example no 10?

Done.

---

examples/11-write-with-imm/client.c, line 88 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We can reuse the structure. No need for having another one.

Done.

---

examples/11-write-with-imm/client.c, line 136 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

RPMA_OP_WRITE

Done.

---

examples/11-write-with-imm/server.c, line 2 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

2021?

Done.

---

examples/11-write-with-imm/server.c, line 91 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

dst_mr is the write destination, not a receive. You have to post a Receive Request using another buffer. It can be very small since it does not will carry any data in this example.

Done.

---

examples/11-write-with-imm/server.c, line 170 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

a value %s was written with immediate data %u

Done.

---

src/conn.c, line 343 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Indentation.

Done.

---

src/conn.c, line 564 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

@grom72 @ldorau @osalyk This switch-case looks dumb. We have to reconsider its existence.

Is there any detailed change?

[janekmi]

Reviewed 1 of 6 files at r3.
Reviewable status: 16 of 21 files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @osalyk)

---

src/conn.c, line 564 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Is there any detailed change?

Not for now. It is potentially a big change. I'm not sure we want to do this. @yangx-jy ignore for now.

[yangx-jy]

Reviewable status: 16 of 21 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)

---

src/conn.c, line 564 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not for now. It is potentially a big change. I'm not sure we want to do this. @yangx-jy ignore for now.

OK

[osalyk]

Reviewed 5 of 6 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

---

examples/11-write-with-imm/client.c, line 77 at r3 (raw file):

	/*
	 * establish a new connection to a server

This comment fits on one line.

[yangx-jy]

Reviewable status: 15 of 21 files reviewed, 3 unresolved discussions (waiting on @grom72, @janekmi, and @osalyk)

---

examples/11-write-with-imm/client.c, line 77 at r3 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

This comment fits on one line.

Done.

[osalyk]

Reviewed 6 of 6 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)

[janekmi]

Reviewed 1 of 6 files at r3, 2 of 6 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)

[janekmi]

Reviewed 1 of 6 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

---

examples/11-write-with-imm/README.md, line 7 at r5 (raw file):

- The client connects to the server and writes a message with immediate
data to the server.
- The server receives an immediate data from client.

the client

---

examples/11-write-with-imm/README.md, line 8 at r5 (raw file):

data to the server.
- The server receives an immediate data from client.
The immediate data has the same value as the message written by client

the client

[janekmi]

Reviewed 2 of 6 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72 and @yangx-jy)

---

examples/11-write-with-imm/client.c, line 103 at r5 (raw file):

	memcpy(src, (uint32_t *)&imm, sizeof(uint32_t));
	fprintf(stdout, "write a value %u with immediate data %u\n",
		(uint32_t)imm, (uint32_t)imm);

I think you mean:

	fprintf(stdout, "write a value %u with immediate data %u\n",
		src, (uint32_t)imm);

[yangx-jy]

Reviewable status: 20 of 21 files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, @osalyk, and @yangx-jy)

---

examples/11-write-with-imm/client.c, line 103 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think you mean:

	fprintf(stdout, "write a value %u with immediate data %u\n",
		src, (uint32_t)imm);

Done.

---

examples/11-write-with-imm/README.md, line 7 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

the client

Hi @janekmi

The first letter is capitalized in many examples(e.g. example03-11):

git grep '\- The'
03-read-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
03-read-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
04-write-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
04-write-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
05-flush-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
05-flush-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
...

Should we update the first letter for all example?
By the way, it is ok for me to capitalize the first letter.

Best Regards,
Xiao Yang

[osalyk]

Reviewable status: 20 of 21 files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, @osalyk, and @yangx-jy)

---

examples/11-write-with-imm/README.md, line 7 at r5 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @janekmi

The first letter is capitalized in many examples(e.g. example03-11):

git grep '\- The'
03-read-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
03-read-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
04-write-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
04-write-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
05-flush-to-persistent/README.md:- The server, if provided (and capable of), prepares a local persistent memory
05-flush-to-persistent/README.md:- The client, if provided (and capable of), prepares a local persistent memory
...

Should we update the first letter for all example?
By the way, it is ok for me to capitalize the first letter.

Best Regards,
Xiao Yang

I think @janekmi is referring to the last word.

[yangx-jy]

Reviewable status: 19 of 21 files reviewed, 4 unresolved discussions (waiting on @grom72, @janekmi, and @osalyk)

---

examples/11-write-with-imm/README.md, line 7 at r5 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

I think @janekmi is referring to the last word.

Sorry, I didn't read it carefully. Done.

---

examples/11-write-with-imm/README.md, line 8 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

the client

Done.

[janekmi]

Reviewed 1 of 6 files at r5, 2 of 2 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @yangx-jy)

---

examples/11-write-with-imm/server.c, line 164 at r6 (raw file):

	uint32_t exp_imm;
	memcpy(&exp_imm, dst, sizeof(uint32_t));

uint32_t *exp_imm = (uint32_t)dst;

No need to make a copy.

---

examples/11-write-with-imm/server.c, line 176 at r6 (raw file):

err_conn_disconnect:
	common_wait_for_conn_close_and_disconnect(&conn);

void

---

examples/11-write-with-imm/server.c, line 179 at r6 (raw file):

err_ep_shutdown:
	rpma_ep_shutdown(&ep);

void

---

examples/11-write-with-imm/server.c, line 184 at r6 (raw file):

	/* deregister the memory regions */
	rpma_mr_dereg(&recv_mr);
	rpma_mr_dereg(&dst_mr);

Because here you deliberately ignore the return value I recommend:

	(void) rpma_mr_dereg(&recv_mr);
	(void) rpma_mr_dereg(&dst_mr);

---

examples/11-write-with-imm/server.c, line 188 at r6 (raw file):

err_peer_delete:
	/* delete the peer object */
	rpma_peer_delete(&peer);

Because here you deliberately ignore the return value I recommend:

(void) rpma_peer_delete(&peer);

[janekmi]

Reviewed 2 of 2 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

[janekmi]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

[osalyk]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

[janekmi]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)