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

rpma: replace rdma_create_qp() with rdma_create_qp_ex() #2084

Merged
merged 2 commits into from
Nov 28, 2022
Merged

rpma: replace rdma_create_qp() with rdma_create_qp_ex() #2084

merged 2 commits into from
Nov 28, 2022

Conversation

yangx-jy
Copy link
Contributor

@yangx-jy yangx-jy commented Nov 21, 2022 •
edited
Loading

  1. Make rpma_peer_setup_qp() call rdma_create_qp_ex().
  2. Update the related unit tests.
  3. Update suppresions for the rdma_create_qp_ex().

This change is to prepare for new libibverbs flush and atomic write.

Signed-off-by: Xiao Yang yangx.jy@fujitsu.com

This change is Reviewable

ldorau
ldorau reviewed Nov 21, 2022
View reviewed changes
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 all commit messages.
Reviewable status: 0 of 5 files reviewed, all discussions resolved

@codecov
Copy link

codecov bot commented Nov 21, 2022 •
edited
Loading

Codecov Report

Merging #2084 (2c2a1da) into main (34d417c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #2084   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1578      1580    +2     
=========================================
+ Hits          1578      1580    +2

@yangx-jy yangx-jy force-pushed the update branch 2 times, most recently from 4a07874 to eadf278 Compare November 21, 2022 07:12
@yangx-jy yangx-jy mentioned this pull request Nov 21, 2022
Closed
haichangsi
haichangsi reviewed Nov 21, 2022
View reviewed changes
Copy link
Contributor

@haichangsi haichangsi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @ldorau)

grom72
grom72 suggested changes Nov 22, 2022
View reviewed changes
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 3 of 5 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yangx-jy)

src/peer.c line 191 at r4 (raw file):

	qp_init_attr.comp_mask = IBV_QP_INIT_ATTR_PD;
	qp_init_attr.pd = peer->pd;

Shall we explicitly set qp_init_attr.send_ops_flags to 0 and later extend implementation with new capabilities?

Suggestion:

qp_init_attr.send_ops_flags = 0;

@yangx-jy
rpma: replace rdma_create_qp() with rdma_create_qp_ex()
1b25398 
1) Make rpma_peer_setup_qp() call rdma_create_qp_ex().
2) Update the related unit tests.
3) Update suppresions for the rdma_create_qp_ex().

This change is to prepare for new libibverbs flush and atomic write.

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
@yangx-jy
test: update suppressions for rpma_srq_new()
2c2a1da 
rpma_srq_new() with the latest rdma-core threw
the following error on fedora35:
------------------------------------------
Test pmem#261: mtt-srq-rpma_srq_get_rcq_0_drd:
...
{
   <insert_a_suppression_name_here>
   drd:MutexErr
   fun:pthread_mutex_init_intercept
   fun:pthread_mutex_init@*
   fun:ibv_create_srq@@IBVERBS_1.1
   fun:rpma_peer_create_srq
   fun:rpma_srq_new
   fun:prestate_init
   fun:mtt_run
   fun:main
}

Test pmem#256: mtt-srq-rpma_srq_delete_0_memcheck:
Test pmem#260: mtt-srq-rpma_srq_get_rcq_0_memcheck:
Test pmem#264: mtt-srq-rpma_srq_new_0_memcheck:
...
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:ibv_icmd_create_srq
   fun:ibv_cmd_create_srq
   fun:rxe_create_srq
   fun:ibv_create_srq@@IBVERBS_1.1
   fun:rpma_peer_create_srq
   fun:rpma_srq_new
   fun:prestate_init
   fun:mtt_run
   fun:main
}
------------------------------------------

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
@yangx-jy
Copy link
Contributor Author

src/peer.c line 191 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Shall we explicitly set qp_init_attr.send_ops_flags to 0 and later extend implementation with new capabilities?

I prefer to set qp_init_attr.send_ops_flags to 0 when checking and introducing new libibverbs APIs because it is a new member of struct ibv_qp_init_attr_ex. On old libibverbs, setting qp_init_attr.send_ops_flags to 0 will throw the following error:

/rpma/src/peer.c: In function 'rpma_peer_setup_qp':
/rpma/src/peer.c:193:14: error: 'struct ibv_qp_init_attr_ex' has no member named 'send_ops_flags'
  qp_init_attr.send_ops_flags = 0;

qp_init_attr.send_ops_flags was added by the following commit since 2018.
linux-rdma/rdma-core@58ef962

@ldorau ldorau requested a review from grom72 November 22, 2022 14:42
ldorau
ldorau reviewed Nov 22, 2022
View reviewed changes
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.

It builds correctly on all OSes: https://github.com/ldorau/rpma/actions/runs/3523334644

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @grom72)

grom72
grom72 approved these changes Nov 25, 2022
View reviewed changes
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:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

ldorau
ldorau approved these changes Nov 28, 2022
View reviewed changes
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:

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

ldorau
ldorau approved these changes Nov 28, 2022
View reviewed changes
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 5 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

@ldorau ldorau merged commit d6db034 into pmem:main Nov 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@haichangsi haichangsi haichangsi left review comments

@grom72 grom72 grom72 approved these changes

@ldorau ldorau ldorau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yangx-jy @grom72 @ldorau @haichangsi