rpma: add rpma_conn_get_qp_num()

[yangx-jy]

Ref #977

---

This change is 

[codecov]

Codecov Report

Merging #1087 (e566279) into master (98c2eca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1087   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          16       16           
  Lines        1334     1339    +5     
=======================================
+ Hits         1332     1337    +5     
  Misses          2        2           

[ldorau]

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

[ldorau]

Reviewed 3 of 9 files at r1.
Reviewable status: 7 of 9 files reviewed, all discussions resolved (waiting on @yangx-jy)

[ldorau]

when is rebased and all builds pass

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

a discussion (no related file):
Please rebase on the current master

---

[yangx-jy]

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @ldorau)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

Please rebase on the current master

Done.

---

[ldorau]

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

[janekmi]

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

a discussion (no related file):
Please rebase.

---

---

doc/manuals_3.txt, line 19 at r2 (raw file):

rpma_conn_get_event_fd.3
rpma_conn_get_private_data.3
rpma_conn_get_qp_num.3

I do not like the idea of providing a qp_num where we do not have a concept of QP on our API.
I do not have a better idea yet too.

---

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

/*
 * rpma_conn_get_qp_num -- get qp_num from the connection object

... of the connection

---

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

/** 3
 * rpma_conn_get_qp_num - get the qp_num from the connection

of the connection?

---

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

 *
 * DESCRIPTION
 * rpma_conn_get_qp_num() obtains the qp_num from the connection.

of the connection.?

---

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

 * RETURN VALUE
 * The rpma_conn_get_qp_num() function returns 0 on success or a negative
 * error code on failure. rpma_conn_get_qp_num() does not set *qp_num

*qp_num*

---

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

 * SEE ALSO
 * rpma_conn_req_new(3), rpma_ep_next_conn_req(3), librpma(7) and
 * https://pmem.io/rpma/

 * SEE ALSO
 * rpma_conn_req_connect(3), librpma(7) and https://pmem.io/rpma/

[yangx-jy]

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

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I do not like the idea of providing a qp_num where we do not have a concept of QP on our API.
I do not have a better idea yet too.

How about conn_num?

---

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

Previously, janekmi (Jan Michalski) wrote…

... of the connection

rpma_conn_get_num -- get the connection's number?

[grom72]

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

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

How about conn_num?

what about rpma_conn_get_uid - unique id

---

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

rpma_conn_get_qp_num

what will be returned after connection disconnect?
do we have a test to cover this?

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

what about rpma_conn_get_uid - unique id

rpma_conn_get_uid or rpma_conn_get_id is ok to me.

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

rpma_conn_get_uid or rpma_conn_get_id is ok to me.

Hi @janekmi @grom72

Which one do you want to use?

[grom72]

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

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @janekmi @grom72

Which one do you want to use?

Let's stay with rpma_conn_get_qp_num.
But documentation should be extended to describe the purpose of this API -> refer to the completion's documentation and vice-versa

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Let's stay with rpma_conn_get_qp_num.
But documentation should be extended to describe the purpose of this API -> refer to the completion's documentation and vice-versa

Hi @grom72

To be honest, I am fine to use rpma_conn_get_uid. One QP always corresponds to one connection in rpma (due to the RC mode).
why do you want to keep rpma_conn_get_qp_num?

By the way, This PR is to track that a wc comes from which connection.

@janekmi
Do you think so?

[grom72]

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

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @grom72

To be honest, I am fine to use rpma_conn_get_uid. One QP always corresponds to one connection in rpma (due to the RC mode).
why do you want to keep rpma_conn_get_qp_num?

By the way, This PR is to track that a wc comes from which connection.

@janekmi
Do you think so?

The problem that I have with ```uid`` is the fact that we introduce a new term to the librpma semantic.

[yangx-jy]

Reviewable status: 0 of 9 files reviewed, 8 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Please rebase.

Done.

---

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

The problem that I have with ```uid`` is the fact that we introduce a new term to the librpma semantic.

Hi @grom72

In my opinion, qp_num is also a new term to the librpma semantic.
Because librpma doesn't have the qp term and use the connection term.

---

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

Previously, yangx-jy (Xiao Yang) wrote…

rpma_conn_get_num -- get the connection's number?

Done.

---

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

Previously, janekmi (Jan Michalski) wrote…

of the connection?

Use the connection's unique ID.

---

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

Previously, janekmi (Jan Michalski) wrote…

of the connection.?

Use the connection's unique ID.

---

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

Previously, janekmi (Jan Michalski) wrote…

*qp_num*

? *qp_num means the value of qp_num pointer.

---

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

Previously, grom72 (Tomasz Gromadzki) wrote…

rpma_conn_get_qp_num

what will be returned after connection disconnect?
do we have a test to cover this?

I added one unit test (get_uid_success_after_disconnect).

---

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

Previously, janekmi (Jan Michalski) wrote…

 * SEE ALSO
 * rpma_conn_req_connect(3), librpma(7) and https://pmem.io/rpma/

Done.

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @grom72

In my opinion, qp_num is also a new term to the librpma semantic.
Because librpma doesn't have the qp term and use the connection term.

@janekmi @ldorau

What do you think?

[grom72]

Reviewed 9 of 9 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @janekmi, @ldorau, and @yangx-jy)

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

@janekmi @ldorau

What do you think?

Yes, but we already planned to use more libibverbs API elements see comments to #979 (enum ibv_wc_opcode) and qp_num is already defined in libibverbs.

---

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

Previously, yangx-jy (Xiao Yang) wrote…

I added one unit test (get_uid_success_after_disconnect).

Is conn->id->qp->qp_num; still a valid pointers path after connection is closed?

[yangx-jy]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @grom72 and @janekmi)

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Yes, but we already planned to use more libibverbs API elements see comments to #979 (enum ibv_wc_opcode) and qp_num is already defined in libibverbs.

Hi @grom72

#979 only mentions that we will use enum ibv_wc_opcode directly. Should we use qp_num directly?
Of course, I will replace conn_uid with qp_num if both you and @janekmi approve it.

---

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

Previously, grom72 (Tomasz Gromadzki) wrote…

Is conn->id->qp->qp_num; still a valid pointers path after connection is closed?

Yes, qp_num is generated by rdma_create_qp instead of rdma_connect/rdma_accept.
qp_num is still valid after rdma_disconnect is called.

[grom72]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi)

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Hi @grom72

#979 only mentions that we will use enum ibv_wc_opcode directly. Should we use qp_num directly?
Of course, I will replace conn_uid with qp_num if both you and @janekmi approve it.

The longer I work with librpma the more I think about making it as light as possible - what means do not introduce any abstraction that:
a) does not add new significant value
b) is already defined in libibverbs

[ldorau]

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

---

doc/manuals_3.txt, line 19 at r2 (raw file):
I agree with @grom72:

Let's stay with rpma_conn_get_qp_num.
But documentation should be extended to describe the purpose of this API -> refer to the completion's documentation and vice-versa

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I agree with @grom72:

Let's stay with rpma_conn_get_qp_num.
But documentation should be extended to describe the purpose of this API -> refer to the completion's documentation and vice-versa

Emmm. qp_num is a hidden concept for RPMA（In other words, connection or connect id is the obvious concept for RPMA)
I also want to make it as light as possible, but the qp_num seems strange for RPMA.
@janekmi do you want to use qp_num as well?

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Emmm. qp_num is a hidden concept for RPMA（In other words, connection or connect id is the obvious concept for RPMA)
I also want to make it as light as possible, but the qp_num seems strange for RPMA.
@janekmi do you want to use qp_num as well?

I want to say that qp or qp_num is a hidden concept for RPMA.

[grom72]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi)

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

I want to say that qp or qp_num is a hidden concept for RPMA.

@yangx-jy we are already in trouble with enum rpma_op as we redefine the semantic that already exists in libibverbs.
I would like to avoid such a situation in the future. If we use qp_num and document this as unique connection identifiers then it should be easy to learn for those who do not know verbs - "just another variable with a funny name"; and for libibverbs users it should also be acceptable.

[yangx-jy]

---

doc/manuals_3.txt, line 19 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

@yangx-jy we are already in trouble with enum rpma_op as we redefine the semantic that already exists in libibverbs.
I would like to avoid such a situation in the future. If we use qp_num and document this as unique connection identifiers then it should be easy to learn for those who do not know verbs - "just another variable with a funny name"; and for libibverbs users it should also be acceptable.

OK，I have updated the PR to use qp_num.

Note: this PR is based on #1214

[grom72]

Reviewed 9 of 9 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi)

[grom72]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi)

[yangx-jy]

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

---

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

Previously, ldorau (Lukasz Dorau) wrote…

assert_int_equal(rpma_conn_disconnect(cstate->conn), MOCK_OK);

rpma_conn_disconnect(cstate->conn) is a part of "run test", so IMHO this way is better and more clear:

	struct conn_test_state *cstate = *cstate_ptr;

	expect_value(rdma_disconnect, id, MOCK_CM_ID);
	will_return(rdma_disconnect, MOCK_OK);

	/* run test */
	int ret = rpma_conn_disconnect(cstate->conn);
	assert_int_equal(ret, MOCK_OK);
	uint32_t qp_num = 0;
	ret = rpma_conn_get_qp_num(cstate->conn, &qp_num);

	/* verify the results */
	assert_int_equal(ret, MOCK_OK);
	assert_int_equal(qp_num, MOCK_QP_NUM);

Done.

[ldorau]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @janekmi)

[yangx-jy]

a discussion (no related file):

Previously, yangx-jy (Xiao Yang) wrote…

Done.

@janekmi

Could you review it recently? Thanks a lot.

---

[janekmi]

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

---

src/include/librpma.h, line 1704 at r5 (raw file):

int rpma_conn_apply_remote_peer_cfg(struct rpma_conn *conn,
		const struct rpma_peer_cfg *pcfg);

A redundant new line.

---

src/include/librpma.h, line 1719 at r5 (raw file):

 * DESCRIPTION
 * rpma_conn_get_qp_num() obtains the connection's qp_num which is the unique
 * identifier of the connection.

rpma_conn_get_qp_num() obtains the unique identifier of the connection.

---

src/include/librpma.h, line 1732 at r5 (raw file):

 *
 * SEE ALSO
 * rpma_conn_req_new(3), rpma_ep_next_conn_req(3), rpma_conn_req_connect(3),

I have just realized we also need a similar function accepting struct rpma_conn_req *.

---

src/include/librpma.h, line 2661 at r5 (raw file):

	uint32_t byte_len;
	enum ibv_wc_status op_status;
	uint32_t qp_num;

It breaks backward compatibility. I think we have to consider this move jointly with clearing up the enum rpma_op case and make this bold decision as soon as possible.

@grom72 I think we should discuss this ASAP and consider all possible effects.

---

src/include/librpma.h, line 2875 at r5 (raw file):

 * rpma_cq_get_fd(3), rpma_flush(3), rpma_read(3), rpma_recv(3),
 * rpma_send(3), rpma_send_with_imm(3), rpma_write(3), rpma_write_with_imm(3),
 * rpma_write_atomic(3), librpma(7) and https://pmem.io/rpma/

Please add the rpma_conn_get_qp_num(3) reference.

[janekmi]

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

---

src/include/librpma.h, line 2661 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It breaks backward compatibility. I think we have to consider this move jointly with clearing up the enum rpma_op case and make this bold decision as soon as possible.

@grom72 I think we should discuss this ASAP and consider all possible effects.

Issue

How to introduce new fields to struct rpma_completion without breaking binary compatibility with all existing software without an urgent need of releasing librpma 1.0.

From the top of my head I think we can handle this situation in a few steps:

1. Introduce a struct rpma_completion_ex

• has to start exactly the same as already existing struct rpma_completion
• at the end has a few new fields e.g. qp_num, opcode (anything more?)

Along with the new structure we have to introduce a new function:

int rpma_cq_get_completion_ex(struct rpma_cq *, struct rpma_completion_ex *);

2. Mark fields we want to remove as deprecated to promote not using them

• enum rpma_op op;
• anything else?

3. Adjust all existing software

4. librpma 1.0

Note: By definition software compiled for 0.* releases is not required to work with 1.0 release.

• Just before releasing 1.0 all deprecated functions and fields are to be removed!
• struct rpma_completion_ex becomes the new `struct rpma_completion_ex
• the 1.0 release takes place
• all existing software is updated after the release simply by removing the _ex part.

Question

Do the existing software takes into account that bumping up the major breaks backwards compatibility?

Discussion

What do you think?

[yangx-jy]

---

src/include/librpma.h, line 2661 at r5 (raw file):

It breaks backward compatibility.
@janekmi

Could you tell why a new member will break backward compatibility?

[grom72]

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

[yangx-jy]

Reviewable status: 6 of 9 files reviewed, 5 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)

---

src/include/librpma.h, line 1704 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A redundant new line.

Done.

---

src/include/librpma.h, line 1719 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

rpma_conn_get_qp_num() obtains the unique identifier of the connection.

Done.

---

src/include/librpma.h, line 1732 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I have just realized we also need a similar function accepting struct rpma_conn_req *.

Is it necessary? qp_num is introduced to track which connection a completion comes from.
I think we don't need to track the connection request for now.

---

src/include/librpma.h, line 2661 at r5 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

It breaks backward compatibility.
@janekmi

Could you tell why a new member will break backward compatibility?

If we just introduce a new member of the structure (e.g. like this PR), I think old executable file can be run on new library.

---

src/include/librpma.h, line 2875 at r5 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please add the rpma_conn_get_qp_num(3) reference.

Done.

[ldorau]

Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 2 of 3 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

[ldorau]

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

src/include/librpma.h, line 2661 at r5 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

If we just introduce a new member of the structure (e.g. like this PR), I think old executable file can be run on new library.

The new structure is bigger than the old one, so the exceeding part of the structure will overwrite a random place in the memory and corrupt it, see: #976 (comment)

[grom72]

Reviewed 2 of 3 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

[yangx-jy]

---

src/include/librpma.h, line 2661 at r5 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

The new structure is bigger than the old one, so the exceeding part of the structure will overwrite a random place in the memory and corrupt it, see: #976 (comment)

@ldorau Thanks for your explanation.

I removed the code related to struct rpma_completion on this PR so that it can be merged.
We can focus on removing struct rpma_completion on #979 .

[grom72]

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

[ldorau]

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

[janekmi]

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

---

src/include/librpma.h, line 1732 at r5 (raw file):

Previously, yangx-jy (Xiao Yang) wrote…

Is it necessary? qp_num is introduced to track which connection a completion comes from.
I think we don't need to track the connection request for now.

Maybe it is really unnecessary. We will come back to this when we will start using it.

[janekmi]

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

[ldorau]

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