Try to implement rpma send with immediate data #713

yangx-jy · 2020-12-31T06:13:18Z

This change is

codecov-io · 2020-12-31T06:19:11Z

Codecov Report

Merging #713 (fba3749) into master (eb1a0aa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #713   +/-   ##
=======================================
  Coverage   99.82%   99.83%           
=======================================
  Files          15       15           
  Lines        1176     1192   +16     
=======================================
+ Hits         1174     1190   +16     
  Misses          2        2

janekmi

Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 42 unresolved discussions (waiting on @yangx-jy)

a discussion (no related file):
The first PR is obviously not rpma: extend rpma_mr_read() but rpma: extend rpma_mr_send().

examples/run-all-on-SoftRoCE.sh, line 70 at r1 (raw file):

	10-send-with-imm)
		echo "Starting the client ..."
		$DIR/client $IP_ADDRESS $PORT "1st_word" "imm_data=1234"

Can we just assume the 4th argument is an immediate data?

examples/10-send-with-imm/CMakeLists.txt, line 35 at r1 (raw file):

			${LIBRPMA_INCLUDE_DIRS}
			../common)
	target_link_libraries(example-${name} rpma ${LIBPMEM_LIBRARIES})

It seems you do not need libpmem here.

examples/10-send-with-imm/client.c, line 19 at r1 (raw file):

#define USAGE_STR "usage: %s <server_address> <port> <word> " \
		"imm_data=<value>\n"

If we agree to have a plain argument here just <imm>.

examples/10-send-with-imm/client.c, line 25 at r1 (raw file):

{
	/* validate parameters */
	if (argc < 4) {

5?

examples/10-send-with-imm/client.c, line 46 at r1 (raw file):

Quoted 6 lines of code…

	extra_name = strtok(argv[4], "=");
	extra_value = strtok(NULL, "=");
	if (strcmp(extra_name, "imm_data") || !extra_value) {
		fprintf(stderr, USAGE_STR, argv[0]);
		return -1;
	}

I don't see a point in having such complex params parsing. Why not just take the string value from the argv[4]?

examples/10-send-with-imm/client.c, line 54 at r1 (raw file):

	}

	if (data > UINT32_MAX) {

data -> imm?

examples/10-send-with-imm/client.c, line 55 at r1 (raw file):

the provided immediate data is too big (%u > %u)\n

examples/10-send-with-imm/client.c, line 85 at r1 (raw file):

	/* establish a new connection to a server listening at addr:port */
	struct rpma_conn_private_data pdata;
	pdata.ptr = &data;

/* send an immediate value for validation on the server side */

examples/10-send-with-imm/client.c, line 92 at r1 (raw file):

	/* send a message with imm_data to the server */
	fprintf(stdout, "send value %s with imm_data %lu\n", word, data);

%lu is too big. I think %u is enough.

examples/10-send-with-imm/README.md, line 7 at r1 (raw file):

- The client connects to the server and sends a message with imm_data
to the server.
- The server receives a message with imm_data from client.

... with the immediate data from the client. The immediate data is compared with an expected immediate data send by the client as the private data during establishing the connection.

examples/10-send-with-imm/README.md, line 16 at r1 (raw file):

```bash
[user@client]$ ./client $server_address $port $word imm_data=$value

$imm

examples/10-send-with-imm/server.c, line 25 at r1 (raw file):

	if (argc < 3) {
		fprintf(stderr, USAGE_STR, argv[0]);
		exit(-1);

return -1 is enough.

examples/10-send-with-imm/server.c, line 74 at r1 (raw file):

get the expected immediate data value from the connection's private data

examples/10-send-with-imm/server.c, line 79 at r1 (raw file):

	if (ret)
		goto err_conn_disconnect;
	uint32_t *data = pdata.ptr;

Please validate pdata.len.

examples/10-send-with-imm/server.c, line 84 at r1 (raw file):

	ret = rpma_recv(conn, recv_mr, 0, KILOBYTE, NULL);
	if (ret)
		goto err_conn_disconnect;

You have to post a RECV before accepting the connection. Please use rpma_conn_req_recv() in this case.

examples/10-send-with-imm/server.c, line 97 at r1 (raw file):

	if (cmpl.op_status != IBV_WC_SUCCESS) {
		fprintf(stderr,
			"server got the unsuccessful completion of an operation\n");

~~server got~~

examples/10-send-with-imm/server.c, line 97 at r1 (raw file):

	if (cmpl.op_status != IBV_WC_SUCCESS) {
		fprintf(stderr,
			"server got the unsuccessful completion of an operation\n");

"an unexpected completion: %s\n", ibv_wc_status_str(cmpl.op_status)

examples/10-send-with-imm/server.c, line 104 at r1 (raw file):

	if (cmpl.op != RPMA_OP_RECV) {
		fprintf(stderr,
			"server got the unexpected type of an operation\n");

~~server got~~

examples/10-send-with-imm/server.c, line 104 at r1 (raw file):

	if (cmpl.op != RPMA_OP_RECV) {
		fprintf(stderr,
			"server got the unexpected type of an operation\n");

"an unexpected typ of operation (%d != %d)", cmpl.op, RPMA_OP_RECV

examples/10-send-with-imm/server.c, line 111 at r1 (raw file):

	if (!(cmpl.flags & IBV_WC_WITH_IMM)) {
		fprintf(stderr,
			"server didn't get IBV_WC_WITH_IMM flag\n");

~~server didn't get~~

examples/10-send-with-imm/server.c, line 111 at r1 (raw file):

	if (!(cmpl.flags & IBV_WC_WITH_IMM)) {
		fprintf(stderr,
			"server didn't get IBV_WC_WITH_IMM flag\n");

"an unexpected completion flag value (no IBV_WC_WITH_IMM)"

examples/10-send-with-imm/server.c, line 118 at r1 (raw file):

	if (cmpl.imm_data != *data) {
		fprintf(stderr,
			"server got wrong imm_data %u\n", *data);

~~server got~~

examples/10-send-with-imm/server.c, line 118 at r1 (raw file):

	if (cmpl.imm_data != *data) {
		fprintf(stderr,
			"server got wrong imm_data %u\n", *data);

"an unexpected immediate data value (%u != %u)\n", cmpl.imm_data, exp_imm_data

examples/10-send-with-imm/server.c, line 121 at r1 (raw file):

		ret = -1;
	} else {
		printf("receive value %s with imm_data %u\n", recv,

"received a value %s with imm_data %u"

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

	cmpl->byte_len = wc.byte_len;
	cmpl->op_status = wc.status;
	cmpl->flags = wc.wc_flags;

@grom72 I think we have to discuss if it is an optimal way to indicate the end consumer of the completion about available immediate data.

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

	if (cmpl->flags & IBV_WC_WITH_IMM)
		cmpl->imm_data = be32toh(wc.imm_data);

ntohl()?

src/mr.h, line 50 at r1 (raw file):

 * rpma_mr_send() can fail with the following error:
 *
 * - RPMA_E_NOSUPP   - unsupported operation

Please add '... operation value`. At first, I thought you just have written a generic NOSUPP description. ;-)

src/mr.h, line 56 at r1 (raw file):

	const struct rpma_mr_local *src,  size_t offset,
	size_t len, int flags, const void *op_context,
	enum ibv_wr_opcode operation, uint32_t value);

value -> imm?

value conveys no meaning.

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

		break;
	case IBV_WR_SEND_WITH_IMM:
		wr.imm_data = htobe32(value);

I think it can be easier to follow if you will use here htonl().

src/include/librpma.h, line 2204 at r1 (raw file):

 *			const struct rpma_mr_local *src, size_t offset,
 *			size_t len, int flags, const void *op_context,
 *			uint32_t imm_data);

I would like to have op_conext as the last argument in all of the posting operations.
I think such a convention is useful.

src/include/librpma.h, line 2231 at r1 (raw file):

int rpma_send_with_imm(struct rpma_conn *conn,
		const struct rpma_mr_local *src, size_t offset, size_t len,
		int flags, const void *op_context, uint32_t imm_data);

Please put op_context as the last argument.

src/include/librpma.h, line 2331 at r1 (raw file):

	uint32_t byte_len;
	enum ibv_wc_status op_status;
	unsigned flags;

A wc_flags field in the struct ibv_wc is of int type.

src/include/librpma.h, line 2334 at r1 (raw file):

	union {
		uint32_t imm_data;
		uint32_t invalidate_rkey;

I think we do not need the second part of the union derived from struct ibv_wc, do we?

tests/unit/common/mocks-ibverbs.h, line 46 at r1 (raw file):

		__be32	imm_data;
		uint32_t	invalidate_rkey;
	};

Why you need this union?

tests/unit/common/mocks-ibverbs.c, line 255 at r1 (raw file):

	assert_non_null(wr);
	assert_non_null(bad_wr);

/* XXX all wr fields should be validated to avoid posting uninitialized values */

tests/unit/common/mocks-ibverbs.c, line 261 at r1 (raw file):

	assert_int_equal(wr->wr_id, args->wr_id);
	if (args->opcode == IBV_WR_SEND_WITH_IMM)
		assert_int_equal(wr->imm_data, args->imm_data);

I think we should validate imm_data no matter what to avoid posting uninitialized values.
Which reminds me we should validate wr completely. ;-)
Please add a comment as stated above.

tests/unit/common/mocks-rpma-mr.c, line 110 at r1 (raw file):

	const struct rpma_mr_local *src,  size_t offset,
	size_t len, int flags, const void *op_context,
	enum ibv_wr_opcode operation, uint32_t value)

value -> imm

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

#define MOCK_TIMEOUT_MS		5678
#define MOCK_Q_SIZE		123
#define MOCK_IMM_DATA		1234

I think it will be nice to have here a value which uses exactly 32-bits.
Just in case we somehow forget about any part of this value.

tests/unit/conn/conn-send.c, line 88 at r1 (raw file):

	expect_value(rpma_mr_send, op_context, MOCK_OP_CONTEXT);
	expect_value(rpma_mr_send, operation, IBV_WR_SEND);
	expect_value(rpma_mr_send, value, 0);

value -> imm

tests/unit/mr/mr-send.c, line 76 at r1 (raw file):

		IBV_WR_SEND_WITH_IMM
	};
	uint32_t values[] = {

imms

tests/unit/mr/mr-send.c, line 92 at r1 (raw file):

		args.wr_id = (uint64_t)MOCK_OP_CONTEXT;
		if (opcodes[i] == IBV_WR_SEND_WITH_IMM)
			args.imm_data = htobe32(MOCK_IMM_DATA);

htonl().

ldorau

Reviewed 4 of 21 files at r1.
Reviewable status: all files reviewed, 42 unresolved discussions (waiting on @yangx-jy)

yangx-jy · 2021-01-12T05:12:47Z

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

The first PR is obviously not rpma: extend rpma_mr_read() but rpma: extend rpma_mr_send().

Sorry for the mistake, I will change it.

yangx-jy · 2021-01-12T05:14:39Z

src/mr.h, line 50 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please add '... operation value`. At first, I thought you just have written a generic NOSUPP description. ;-)

Could you give me a detailed example here?

yangx-jy · 2021-01-12T05:33:19Z

src/include/librpma.h, line 2331 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A wc_flags field in the struct ibv_wc is of int type.

The tye of wc_flags has been changed from int to unigned int since 2017, see the following patch:
linux-rdma/rdma-core@8fe7f12

By the way, the manpage is not updated so I send a patch to do it:
https://www.spinics.net/lists/linux-rdma/msg98897.html

yangx-jy · 2021-01-12T08:14:45Z

examples/10-send-with-imm/server.c, line 84 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You have to post a RECV before accepting the connection. Please use rpma_conn_req_recv() in this case.

For this point, is there any explanation?

yangx-jy · 2021-01-12T08:19:53Z

examples/10-send-with-imm/server.c, line 84 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

For this point, is there any explanation?

I knew, This step guarantees that data from client is not dropped when we have established connection but not created RQ.

yangx-jy

Reviewable status: 3 of 21 files reviewed, 42 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @yangx-jy)

examples/run-all-on-SoftRoCE.sh, line 70 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can we just assume the 4th argument is an immediate data?

Done.

examples/10-send-with-imm/CMakeLists.txt, line 35 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It seems you do not need libpmem here.

Remove libpmem and add libibverbs because client and server use ibv_wc_status_str().

examples/10-send-with-imm/client.c, line 19 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

If we agree to have a plain argument here just <imm>.

Done.

examples/10-send-with-imm/client.c, line 25 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

5?

Done.

examples/10-send-with-imm/client.c, line 46 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

	extra_name = strtok(argv[4], "=");
	extra_value = strtok(NULL, "=");
	if (strcmp(extra_name, "imm_data") || !extra_value) {
		fprintf(stderr, USAGE_STR, argv[0]);
		return -1;
	}
I don't see a point in having such complex params parsing. Why not just take the string value from the argv[4]?

we can use argv[4] directly for now.

examples/10-send-with-imm/client.c, line 54 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

data -> imm?

Done.

examples/10-send-with-imm/client.c, line 55 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

the provided immediate data is too big (%u > %u)\n

Done.

examples/10-send-with-imm/client.c, line 85 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

/* send an immediate value for validation on the server side */

Done.

examples/10-send-with-imm/client.c, line 92 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

%lu is too big. I think %u is enough.

changed data to (uint32_t)imm.

examples/10-send-with-imm/README.md, line 7 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

... with the immediate data from the client. The immediate data is compared with an expected immediate data send by the client as the private data during establishing the connection.

Done.

examples/10-send-with-imm/README.md, line 16 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

$imm

Done.

examples/10-send-with-imm/server.c, line 25 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

return -1 is enough.

Done.

examples/10-send-with-imm/server.c, line 74 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

get the expected immediate data value from the connection's private data

Done.

examples/10-send-with-imm/server.c, line 79 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please validate pdata.len.

Done.

examples/10-send-with-imm/server.c, line 84 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

I knew, This step guarantees that data from client is not dropped when we have established connection but not created RQ.

Done.

examples/10-send-with-imm/server.c, line 97 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

~~server got~~

Done.

examples/10-send-with-imm/server.c, line 97 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

"an unexpected completion: %s\n", ibv_wc_status_str(cmpl.op_status)

Done.

examples/10-send-with-imm/server.c, line 104 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

~~server got~~

Done.

examples/10-send-with-imm/server.c, line 104 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

"an unexpected typ of operation (%d != %d)", cmpl.op, RPMA_OP_RECV

Done.

examples/10-send-with-imm/server.c, line 111 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

~~server didn't get~~

Done.

examples/10-send-with-imm/server.c, line 111 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

"an unexpected completion flag value (no IBV_WC_WITH_IMM)"

Done.

examples/10-send-with-imm/server.c, line 118 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

~~server got~~

Done.

examples/10-send-with-imm/server.c, line 118 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

"an unexpected immediate data value (%u != %u)\n", cmpl.imm_data, exp_imm_data

Done.

examples/10-send-with-imm/server.c, line 121 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

"received a value %s with imm_data %u"

Done.

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

Previously, janekmi (Jan Michalski) wrote…

@grom72 I think we have to discuss if it is an optimal way to indicate the end consumer of the completion about available immediate data.

Keep it for now.

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

Previously, janekmi (Jan Michalski) wrote…

ntohl()?

Done.

src/mr.h, line 50 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Could you give me a detailed example here?

changed to unsupported operation parament(wr.opcode)

src/mr.h, line 56 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

value -> imm?

value conveys no meaning.

Done.

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

Previously, janekmi (Jan Michalski) wrote…

I think it can be easier to follow if you will use here htonl().

Done.

src/include/librpma.h, line 2204 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I would like to have op_conext as the last argument in all of the posting operations.
I think such a convention is useful.

Done.

src/include/librpma.h, line 2231 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please put op_context as the last argument.

Done.

src/include/librpma.h, line 2331 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

The tye of wc_flags has been changed from int to unigned int since 2017, see the following patch:
linux-rdma/rdma-core@8fe7f12

By the way, the manpage is not updated so I send a patch to do it:
https://www.spinics.net/lists/linux-rdma/msg98897.html

Done.

src/include/librpma.h, line 2334 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we do not need the second part of the union derived from struct ibv_wc, do we?

I remove invalidate_rkey for now

tests/unit/common/mocks-ibverbs.h, line 46 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why you need this union?

Remove it for now

tests/unit/common/mocks-ibverbs.c, line 261 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we should validate imm_data no matter what to avoid posting uninitialized values.
Which reminds me we should validate wr completely. ;-)
Please add a comment as stated above.

Done.

tests/unit/common/mocks-rpma-mr.c, line 110 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

value -> imm

Done.

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

Previously, janekmi (Jan Michalski) wrote…

I think it will be nice to have here a value which uses exactly 32-bits.
Just in case we somehow forget about any part of this value.

How about "#define MOCK_IMM_DATA (uint32_t)1234"?

tests/unit/conn/conn-send.c, line 88 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

value -> imm

Done.

tests/unit/mr/mr-send.c, line 76 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

imms

Done.

tests/unit/mr/mr-send.c, line 92 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

htonl().

Done.

yangx-jy

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

tests/unit/common/mocks-ibverbs.c, line 255 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

/* XXX all wr fields should be validated to avoid posting uninitialized values */

Done.

tests/unit/common/mocks-ibverbs.c, line 261 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Done.

Just add a comment.

ldorau

Reviewed 2 of 18 files at r2.
Reviewable status: 5 of 21 files reviewed, 42 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

ldorau

Reviewed 2 of 21 files at r1, 3 of 18 files at r2.
Reviewable status: 8 of 21 files reviewed, 42 unresolved discussions (waiting on @grom72 and @janekmi)

ldorau

Reviewed 4 of 18 files at r2.
Reviewable status: 12 of 21 files reviewed, 45 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

examples/10-send-with-imm/README.md, line 7 at r2 (raw file):

client.  The

Please move The to the next line.

examples/10-send-with-imm/README.md, line 8 at r2 (raw file):

with an expected

with the expected

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

parament

What do you mean by parament?
Please add a space before the bracket: (

ldorau

Reviewed 3 of 18 files at r2.
Reviewable status: 15 of 21 files reviewed, 45 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

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

Previously, yangx-jy (Xiao Yang) wrote…

How about "#define MOCK_IMM_DATA (uint32_t)1234"?

I agree that a value should require 32 bits to be saved (so it should be just >= 2^31) - (uint32_t)1234 does not do that.

ldorau

@yangx-jy Could you resolve all fixed issues in Reviewable?

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

janekmi

Reviewed 4 of 18 files at r2.
Reviewable status: 16 of 21 files reviewed, 27 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

examples/10-send-with-imm/client.c, line 77 at r2 (raw file):

	/*
	 * establish a new connection to a server and send an immediate
	 * data for valication on the sever side

valication -> validation

examples/10-send-with-imm/server.c, line 73 at r2 (raw file):

		goto err_ep_shutdown;

	/* prepare to receice a message with immediate data from the client */

receice -> receive

janekmi

Reviewed 14 of 18 files at r2.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, and @yangx-jy)

examples/10-send-with-imm/server.c, line 93 at r2 (raw file):

	if (conn_event != RPMA_CONN_ESTABLISHED) {
		fprintf(stderr,
			"rpma_conn_next_event returned an unexptected event\n");

ret = -1;

examples/10-send-with-imm/server.c, line 100 at r2 (raw file):

	struct rpma_conn_private_data pdata;
	ret = rpma_conn_get_private_data(conn, &pdata);
	if (ret || pdata.len < sizeof(uint32_t)) {

You can not validate both conditions at the same time. In this case, if ret != 0 you will print a message about the connection's private data. Where while having an error on this API call you can not expect pdata to have any reasonable data.
You have to do something like:

if (ret) {
    goto err_conn_disconnect;
} else if (pdata.len < sizoef(uint32_t)) {
    fprintf(stderr, /* ... */);
    ret = -1;
    goto err_conn_disconnect;
}

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

			    src, offset, len,
			    flags, IBV_WR_SEND_WITH_IMM,
			    imm, op_context);

Please indent the wrapped lines by two tabs. No spaces.

src/mr.h, line 50 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

changed to unsupported operation parament(wr.opcode)

I think I would go with:

- RPMA_E_NOSUPP - unsupported 'operation' argument

src/include/librpma.h, line 2331 at r1 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Done.

Good job. Thank you for pointing it out.

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

Previously, ldorau (Lukasz Dorau) wrote…

I agree that a value should require 32 bits to be saved (so it should be just >= 2^31) - (uint32_t)1234 does not do that.

e.g. 0xfedcba98

tests/unit/conn/conn-send_with_imm.c, line 98 at r2 (raw file):

				MOCK_RPMA_MR_LOCAL, MOCK_LOCAL_OFFSET,
				MOCK_LEN, MOCK_FLAGS, MOCK_IMM_DATA,
				MOCK_OP_CONTEXT);

Please indent by two tabs.

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

	int ret = rpma_mr_send(MOCK_QP, mrs->local, MOCK_SRC_OFFSET,
				MOCK_LEN, RPMA_F_COMPLETION_ON_ERROR,
				MOCK_UNKNOWN_OP, 0, MOCK_OP_CONTEXT);

Please indent by two tabs.

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

	int ret = rpma_mr_send(MOCK_QP, mrs->local, MOCK_SRC_OFFSET,
				MOCK_LEN, RPMA_F_COMPLETION_ON_ERROR,
				IBV_WR_SEND, 0, MOCK_OP_CONTEXT);

Please indent by two tabs.

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

		int ret = rpma_mr_send(MOCK_QP, mrs->local, MOCK_SRC_OFFSET,
					MOCK_LEN, RPMA_F_COMPLETION_ALWAYS,
					opcodes[i], imms[i], MOCK_OP_CONTEXT);

Please indent by two tabs.

yangx-jy

Sorry for the late reply. I resolve them now.

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

examples/10-send-with-imm/client.c, line 77 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

valication -> validation

Done.

examples/10-send-with-imm/README.md, line 7 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

client.  The
Please move The to the next line.

Done.

examples/10-send-with-imm/README.md, line 8 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

with an expected
with the expected

Done.

examples/10-send-with-imm/server.c, line 73 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

receice -> receive

Done.

examples/10-send-with-imm/server.c, line 93 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

ret = -1;

Done.

examples/10-send-with-imm/server.c, line 100 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You can not validate both conditions at the same time. In this case, if ret != 0 you will print a message about the connection's private data. Where while having an error on this API call you can not expect pdata to have any reasonable data.
You have to do something like:
if (ret) {
    goto err_conn_disconnect;
} else if (pdata.len < sizoef(uint32_t)) {
    fprintf(stderr, /* ... */);
    ret = -1;
    goto err_conn_disconnect;
}

Done.

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

Previously, yangx-jy (Xiao Yang) wrote…

Keep it for now.

Done.

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

Previously, janekmi (Jan Michalski) wrote…

Please indent the wrapped lines by two tabs. No spaces.

Hi @janekmi ,

I actually used tabs instead of spaces. Is it OK?

src/mr.h, line 50 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think I would go with:
- RPMA_E_NOSUPP - unsupported 'operation' argument

Done

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

Previously, ldorau (Lukasz Dorau) wrote…

parament
What do you mean by parament?

Please add a space before the bracket: (

Change to "- RPMA_E_NOSUPP - unsupported 'operation' argument" as Jan suggested.

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

Previously, janekmi (Jan Michalski) wrote…

e.g. 0xfedcba98

Take use of 0xfffffffe

tests/unit/conn/conn-send_with_imm.c, line 98 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please indent by two tabs.

Done.

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

Previously, janekmi (Jan Michalski) wrote…

Please indent by two tabs.

Done.

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

Previously, janekmi (Jan Michalski) wrote…

Please indent by two tabs.

Done.

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

Previously, janekmi (Jan Michalski) wrote…

Please indent by two tabs.

Done.

ldorau

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

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

Previously, yangx-jy (Xiao Yang) wrote…

Take use of 0xfffffffe

Maybe 0xDeadBeaf or 0x12345678 ?

yangx-jy · 2021-01-25T08:28:28Z

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

Previously, ldorau (Lukasz Dorau) wrote…

Maybe 0xDeadBeaf or 0x12345678 ?

Hi @ldorau ,

I perfer to enable the highest bit, so either 0xDeadBeaf or 0x87654321 is OK for me.
By the way, I can change it to 0x87654321 if you want.

ldorau

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

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

Previously, yangx-jy (Xiao Yang) wrote…

Hi @ldorau ,

I perfer to enable the highest bit, so either 0xDeadBeaf or 0x87654321 is OK for me.
By the way, I can change it 0x87654321 if you want

0x87654321 is OK for me

yangx-jy

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

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

Previously, ldorau (Lukasz Dorau) wrote…

0x87654321 is OK for me

Done.

ldorau

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

ldorau

Reviewed 1 of 9 files at r3.
Reviewable status: 17 of 21 files reviewed, 12 unresolved discussions (waiting on @grom72 and @janekmi)

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

Previously, yangx-jy (Xiao Yang) wrote…

Hi @janekmi ,

I actually used tabs instead of spaces. Is it OK?

There are 4 spaces in each of three above lines, but there should be no spaces at all.

yangx-jy · 2021-01-25T09:11:54Z

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

Previously, ldorau (Lukasz Dorau) wrote…

There are 4 spaces in each of three above lines, but there should be no spaces at all.

Sorry, I read code in the wrong place.

Done.

ldorau

Reviewed 1 of 1 files at r5.
Reviewable status: 17 of 21 files reviewed, 12 unresolved discussions (waiting on @grom72 and @janekmi)

ldorau

Reviewed 2 of 18 files at r2.
Reviewable status: 17 of 21 files reviewed, 13 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

tests/unit/conn/conn-completion_get.c, line 181 at r4 (raw file):

Quoted 21 lines of code…

	enum ibv_wc_opcode opcodes[] = {
			IBV_WC_RDMA_READ,
			IBV_WC_RDMA_WRITE,
			IBV_WC_SEND,
			IBV_WC_RECV,
			IBV_WC_RECV
	};
	enum rpma_op ops[] = {
			RPMA_OP_READ,
			RPMA_OP_WRITE,
			RPMA_OP_SEND,
			RPMA_OP_RECV,
			RPMA_OP_RECV
	};
	unsigned flags[] = {
		0,
		0,
		0,
		0,
		IBV_WC_WITH_IMM
	};

Maybe I do not understand something, but shouldn't it be like that?

	enum ibv_wc_opcode opcodes[] = {
			IBV_WC_RDMA_READ,
			IBV_WC_RDMA_WRITE,
			IBV_WC_SEND,
			IBV_WC_SEND,
			IBV_WC_RECV
	};
	enum rpma_op ops[] = {
			RPMA_OP_READ,
			RPMA_OP_WRITE,
			RPMA_OP_SEND,
			RPMA_OP_SEND,
			RPMA_OP_RECV
	};
	unsigned flags[] = {
		0,
		0,
		0,
		IBV_WC_WITH_IMM,
		0
	};

e.g. the second send op with IBV_WC_WITH_IMM ?

ldorau

Reviewed 2 of 9 files at r3.
Reviewable status: 19 of 21 files reviewed, 13 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

ldorau

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

examples/run-all-on-SoftRoCE.sh, line 102 at r5 (raw file):

$VLD_SCMD

$VLD_CCMD

ldorau

Reviewed 1 of 1 files at r6.
Reviewable status: 19 of 21 files reviewed, 13 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)

yangx-jy

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

examples/run-all-on-SoftRoCE.sh, line 102 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

$VLD_SCMD
$VLD_CCMD

Done.

tests/unit/conn/conn-completion_get.c, line 181 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

	enum ibv_wc_opcode opcodes[] = {
			IBV_WC_RDMA_READ,
			IBV_WC_RDMA_WRITE,
			IBV_WC_SEND,
			IBV_WC_RECV,
			IBV_WC_RECV
	};
	enum rpma_op ops[] = {
			RPMA_OP_READ,
			RPMA_OP_WRITE,
			RPMA_OP_SEND,
			RPMA_OP_RECV,
			RPMA_OP_RECV
	};
	unsigned flags[] = {
		0,
		0,
		0,
		0,
		IBV_WC_WITH_IMM
	};

Maybe I do not understand something, but shouldn't it be like that?

	enum ibv_wc_opcode opcodes[] = {
			IBV_WC_RDMA_READ,
			IBV_WC_RDMA_WRITE,
			IBV_WC_SEND,
			IBV_WC_SEND,
			IBV_WC_RECV
	};
	enum rpma_op ops[] = {
			RPMA_OP_READ,
			RPMA_OP_WRITE,
			RPMA_OP_SEND,
			RPMA_OP_SEND,
			RPMA_OP_RECV
	};
	unsigned flags[] = {
		0,
		0,
		0,
		IBV_WC_WITH_IMM,
		0
	};

e.g. the second send op with IBV_WC_WITH_IMM ?

Hi @ldorau

For ibv_post_send() with IBV_WR_SEND_WITH_IMM, ibv_poll_cq() only gets the value of wc_flags(i.e. IBV_WC_WITH_IMM) but cannot get the vaule of imm_data.
For ibv_post_recv(), ibv_poll_cq() can get the value of wc_flags(i.e. IBV_WC_WITH_IMM) and the value of imm_data.
According to above behaviors, I test wc_flags and imm_data by recv opeartion.
I am not sure if it is an expected behavior, do you know this rule or behavior?
By the way, I will try to look into the kernel code tomorrow.

ldorau

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

tests/unit/conn/conn-completion_get.c, line 181 at r4 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @ldorau

For ibv_post_send() with IBV_WR_SEND_WITH_IMM, ibv_poll_cq() only gets the value of wc_flags(i.e. IBV_WC_WITH_IMM) but cannot get the vaule of imm_data.
For ibv_post_recv(), ibv_poll_cq() can get the value of wc_flags(i.e. IBV_WC_WITH_IMM) and the value of imm_data.
According to above behaviors, I test wc_flags and imm_data by recv opeartion.
I am not sure if it is an expected behavior, do you know this rule or behavior?
By the way, I will try to look into the kernel code tomorrow.

Ah, yes, of course, you are verifying the completion of the RECV operation. I see now. All is OK.

ldorau

Reviewed 1 of 1 files at r7.
Reviewable status: 19 of 21 files reviewed, 12 unresolved discussions (waiting on @grom72 and @janekmi)

yangx-jy · 2021-01-29T07:30:29Z

tests/unit/conn/conn-completion_get.c, line 181 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Ah, yes, of course, you are verifying the completion of the RECV operation. I see now. All is OK.

Hi @ldorau

According to the IB Architecture Specification, the value of imm_data can only be placed in the receive Completion Queue Element :

5.2.11 IMMEDIATE DATA EXTENDED TRANSPORT HEADER (ImmDt) - 4 BYTES
"Immediate Data (ImmDt) contains data that is placed in the receive
 Completion Queue Element (CQE). The ImmDt is only allowed in SEND or
 RDMA WRITE packets with Immediate Data."

I also confirm that kernel follows the rule, so I will add another check in rpma_conn_completion_get(), like this:

/*
 * The value of imm_data can only be placed in the receive Completion Queue Element.
 */
if ((cmpl->op == RPMA_OP_RECV) && (cmpl->flags & IBV_WC_WITH_IMM))
        cmpl->imm = ntohl(wc.imm_data);

yangx-jy

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

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

Previously, yangx-jy (Xiao Yang) wrote…

Sorry, I read code in the wrong place.

Done.

Done.

1) Extend rpma_mr_send() to support different send operations (e.g. IBV_WR_SEND_WITH_IMM), because they will be implemented in future. 2) Also update the related unit tests. Signed-off-by: Xiao Yang <[email protected]>

Signed-off-by: Xiao Yang <[email protected]>

ldorau

Reviewed 1 of 1 files at r8.
Reviewable status: 19 of 21 files reviewed, 12 unresolved discussions (waiting on @grom72 and @janekmi)

yangx-jy

Hi all,

I have resolved all the comments. Could you review the patchset again?
By the way, I have written the new rpma_{write,write_atomic}_with_imm() and will send them after the patchset is merged.

Best Regards,
Xiao Yang

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

ldorau · 2021-02-02T08:19:46Z

I have resolved all the comments. Could you review the patchset again?

Hi @yangx-jy, we have a lot of work to do until the Friday before the release of https://github.com/pmem/fio, so we probably won't be able to do it until Friday or Monday.

yangx-jy

@ldorau

Got it, thanks a lot for your explanation.
Don't worry, I will wait for you.

Best Regards,
Xiao Yang

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

janekmi

Reviewed 7 of 9 files at r3, 1 of 1 files at r4, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)

yangx-jy · 2021-02-04T01:41:28Z

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

Previously, yangx-jy (Xiao Yang) wrote…

Done.

Hi @janekmi

Is there any detailed suggestion to improve the code?
Could we improve the code as a separate patch?

Best Regards,
Xiao Yang

janekmi

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

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

Previously, yangx-jy (Xiao Yang) wrote…

Hi @janekmi

Is there any detailed suggestion to improve the code?

Could we improve the code as a separate patch?

Best Regards,
Xiao Yang

Sadly @grom72 (who is our architect) cannot discuss this topic with us currently.
Luckily, there is not much to discuss. I expect maybe a discussion about passing the whole wc_flags outside vs passing only information about the presence of the immediate data e.g. bool imm_present. So, not much of a difference implementation-wise.

@grom72 and I agreed to merge it as is and discuss it and optionally adjust it, hopefully, next week.

ldorau

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

ldorau

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

yangx-jy · 2021-02-05T03:01:25Z

Hi @ldorau @janekmi @grom72

Thanks a lot for reviewing the patchset.
In addtion, thank @ldorau for fixing some compatible issues.

Best Regards,
Xiao Yang

janekmi suggested changes Jan 8, 2021

View reviewed changes

ldorau reviewed Jan 11, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from 70830af to fba3749 Compare January 12, 2021 13:44

yangx-jy commented Jan 12, 2021

View reviewed changes

yangx-jy requested a review from janekmi January 14, 2021 02:08

ldorau reviewed Jan 22, 2021

View reviewed changes

ldorau suggested changes Jan 22, 2021

View reviewed changes

ldorau reviewed Jan 22, 2021

View reviewed changes

janekmi suggested changes Jan 22, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from fba3749 to 713ea93 Compare January 25, 2021 06:46

yangx-jy commented Jan 25, 2021

View reviewed changes

ldorau reviewed Jan 25, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from 713ea93 to f31a183 Compare January 25, 2021 08:39

yangx-jy commented Jan 25, 2021

View reviewed changes

ldorau reviewed Jan 25, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from f31a183 to b42c7e0 Compare January 25, 2021 09:09

ldorau reviewed Jan 25, 2021

View reviewed changes

ldorau suggested changes Jan 25, 2021

View reviewed changes

ldorau reviewed Jan 25, 2021

View reviewed changes

ldorau suggested changes Jan 25, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from b42c7e0 to 367a65f Compare January 25, 2021 11:47

ldorau reviewed Jan 25, 2021

View reviewed changes

yangx-jy commented Jan 25, 2021

View reviewed changes

ldorau reviewed Jan 25, 2021

View reviewed changes

yangx-jy force-pushed the rpma_send_with_imm branch from 367a65f to e4e31ae Compare January 27, 2021 07:19

ldorau reviewed Jan 27, 2021

View reviewed changes

yangx-jy commented Jan 29, 2021

View reviewed changes

yangx-jy added 4 commits January 29, 2021 15:44

rpma: extend rpma_mr_send()

f816a44

1) Extend rpma_mr_send() to support different send operations (e.g. IBV_WR_SEND_WITH_IMM), because they will be implemented in future. 2) Also update the related unit tests. Signed-off-by: Xiao Yang <[email protected]>

rpma: implement rpma_send_with_imm()

0f071c1

Signed-off-by: Xiao Yang <[email protected]>

test: add unit test for rpma_send_with_imm()

43fcc5e

Signed-off-by: Xiao Yang <[email protected]>

examples: add 10-send-with-imm

7ebb687

Signed-off-by: Xiao Yang <[email protected]>

yangx-jy force-pushed the rpma_send_with_imm branch from e4e31ae to 7ebb687 Compare January 29, 2021 08:01

ldorau reviewed Feb 1, 2021

View reviewed changes

yangx-jy requested a review from janekmi February 2, 2021 07:44

yangx-jy commented Feb 2, 2021

View reviewed changes

janekmi reviewed Feb 3, 2021

View reviewed changes

janekmi approved these changes Feb 4, 2021

View reviewed changes

ldorau mentioned this pull request Feb 4, 2021

rpma: fix the implicit conversion error #851

Merged

ldorau approved these changes Feb 4, 2021

View reviewed changes

ldorau merged commit d3f3af8 into pmem:master Feb 4, 2021

Try to implement rpma send with immediate data #713

Try to implement rpma send with immediate data #713

Conversation

yangx-jy commented Dec 31, 2020 • edited by janekmi Loading

codecov-io commented Dec 31, 2020 • edited Loading

Codecov Report

janekmi left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Jan 12, 2021

yangx-jy commented Jan 12, 2021

yangx-jy commented Jan 12, 2021

yangx-jy commented Jan 12, 2021

yangx-jy commented Jan 12, 2021

yangx-jy left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Jan 25, 2021 • edited Loading

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Jan 25, 2021

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Jan 29, 2021

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy left a comment

Choose a reason for hiding this comment

ldorau commented Feb 2, 2021

yangx-jy left a comment

Choose a reason for hiding this comment

janekmi left a comment

Choose a reason for hiding this comment

yangx-jy commented Feb 4, 2021

janekmi left a comment

Choose a reason for hiding this comment

ldorau left a comment

Choose a reason for hiding this comment

yangx-jy commented Dec 31, 2020 •

edited by janekmi

Loading

codecov-io commented Dec 31, 2020 •

edited

Loading

yangx-jy commented Jan 25, 2021 •

edited

Loading