-
Notifications
You must be signed in to change notification settings - Fork 56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 19 files at r1.
Reviewable status: 2 of 19 files reviewed, 3 unresolved discussions (waiting on @yangx-jy)
examples/05-flush-to-persistent/server.c
line 72 at r1 (raw file):
goto err_free; ret = rpma_utils_ibv_context_is_flush_capable(ibv_ctx, &is_native_flush_supported);
flush selection should be transparent for the application.
it is internal librpma decision what type of flush shall be used
Code quote:
ret = rpma_utils_ibv_context_is_flush_capable(ibv_ctx, &is_native_flush_supported);
if (ret)
goto err_free;
src/conn.c
line 568 at r1 (raw file):
#endif return rpma_flush_apm(conn->id->qp, conn->flush, dst, dst_offset,
We should not change the logic here, we should provide a new flush
with _execute
and _delete
functions.
See existing APM solution.
Code quote:
return rpma_flush_apm(conn->id->qp, conn->flush, dst, dst_offset,
len, type, flags, op_context, conn->direct_write_to_pmem);
src/peer.c
line 67 at r1 (raw file):
access |= IBV_ACCESS_FLUSH_PERSISTENT; goto next_step;
Please avoid goto
except in an error situation.
Suggestion:
} else {
if (usage & (RPMA_MR_USAGE_FLUSH_TYPE_VISIBILITY | RPMA_MR_USAGE_FLUSH_TYPE_PERSISTENT))
access |= IBV_ACCESS_REMOTE_READ;
}
#else
if (usage & (RPMA_MR_USAGE_FLUSH_TYPE_VISIBILITY | RPMA_MR_USAGE_FLUSH_TYPE_PERSISTENT))
access |= IBV_ACCESS_REMOTE_READ;
#endif
Previously, grom72 (Tomasz Gromadzki) wrote…
In my opinion, native flush can ensure that the data is written into PMEM as GPSPM flush does so that it doesn't need to check |
cfce2f7
to
170781a
Compare
There was a problem hiding this 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 21 files reviewed, 3 unresolved discussions (waiting on @grom72 and @yangx-jy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 21 files at r2.
Reviewable status: 4 of 21 files reviewed, 3 unresolved discussions (waiting on @grom72 and @yangx-jy)
Please add the temporary commit ldorau@616070d to disable building the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 21 files reviewed, 5 unresolved discussions (waiting on @grom72 and @yangx-jy)
src/include/librpma.h
line 1763 at r2 (raw file):
/** 3 * rpma_conn_get_direct_write_to_pmem - check if connection supports direct write to PMEM
We use PMem
here - please replace all occurrences.
Code quote:
PMEM
src/include/librpma.h
line 1773 at r2 (raw file):
* * DESCRIPTION * rpma_conn_get_direct_write_to_pmem() checks the support of the direct write to PMEM
Maybe * rpma_conn_get_direct_write_to_pmem() checks if the connection supports the direct write to PMem.
?
@grom72 ?
Code quote:
* rpma_conn_get_direct_write_to_pmem() checks the support of the direct write to PMEM
* on connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 21 files at r2.
Reviewable status: 5 of 21 files reviewed, 5 unresolved discussions (waiting on @grom72 and @yangx-jy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 21 files reviewed, 5 unresolved discussions (waiting on @ldorau and @yangx-jy)
examples/05-flush-to-persistent/server.c
line 72 at r1 (raw file):
Previously, yangx-jy (Xiao Yang) wrote…
In my opinion, native flush can ensure that the data is written into PMEM as GPSPM flush does so that it doesn't need to check
pcfg->direct_write_to_pmem
. I try to make server know if librpma supports native flush byrpma_utils_ibv_context_is_flush_capable()
and then avoid settingargv[4]
when librpma supports native flush.
Do you have any suggestions about it?
Native flush support only informs about local RNIC capabilities.
It does not know anything about remote capabilities, which should be delivered (if needed) in a similar way as direct_write_to_pmem
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 21 files at r2.
Reviewable status: 12 of 21 files reviewed, 5 unresolved discussions (waiting on @yangx-jy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 12 of 21 files reviewed, 6 unresolved discussions (waiting on @grom72 and @yangx-jy)
a discussion (no related file):
Please add the temporary commit ldorau/rpma@616070d to disable building the tests.
4053861
to
31045ef
Compare
Codecov Report
@@ Coverage Diff @@
## main #2104 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 1611 1670 +59
=========================================
+ Hits 1611 1670 +59 |
b8b2a33
to
9cfbfe9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 9 of 23 files reviewed, 6 unresolved discussions (waiting on @grom72 and @ldorau)
a discussion (no related file):
Previously, ldorau (Lukasz Dorau) wrote…
Please add the temporary commit ldorau/rpma@616070d to disable building the tests.
Done. Disable building the tests for both github action and circleci.
src/conn.c
line 568 at r1 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
We should not change the logic here, we should provide a new
flush
with_execute
and_delete
functions.
See existing APM solution.
Done. Provided rpma_native_flush_new()
and rpma_native_flush_execute()
.
src/peer.c
line 67 at r1 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Please avoid
goto
except in an error situation.
Done.
src/include/librpma.h
line 1763 at r2 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
We use
PMem
here - please replace all occurrences.
Done.
src/include/librpma.h
line 1773 at r2 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
Maybe
* rpma_conn_get_direct_write_to_pmem() checks if the connection supports the direct write to PMem.
?
@grom72 ?
Done.
examples/05-flush-to-persistent/server.c
line 72 at r1 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Native flush support only informs about local RNIC capabilities.
It does not know anything about remote capabilities, which should be delivered (if needed) in a similar way asdirect_write_to_pmem
is.
Please review the following logic of my implementation.
conn->direct_write_to_pmem
will be set to true by default when native flush is supported.- You can query the value of
conn->direct_write_to_pmem
byrpma_conn_get_direct_write_to_pmem()
. pcfg->direct_write_to_pmem
on 05-flush-to-persistent/server can be passed to 05-flush-to-persistent/client during connection establishment but it will be ignored ifconn->direct_write_to_pmem
has been true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 6 files at r4, 1 of 1 files at r5, 1 of 2 files at r6, all commit messages.
Reviewable status: 13 of 23 files reviewed, 3 unresolved discussions (waiting on @grom72)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 6 files at r4.
Reviewable status: 14 of 23 files reviewed, 3 unresolved discussions (waiting on @grom72)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 21 files at r2, 1 of 1 files at r5.
Reviewable status: 15 of 23 files reviewed, 2 unresolved discussions (waiting on @ldorau and @yangx-jy)
src/conn.c
line 98 at r7 (raw file):
conn->flush = flush; /* set direct_write_to_pmem to ture by default when native flush is supported */ conn->direct_write_to_pmem = is_native_flush_supported;
I can not agree with this assumption.
qpx->wr_flush
says only that the local RDMA HW Interface supports native flush operation (provide a handler for that), but it does not say anything about the remote side.
Please take a look at rpma_conn_apply_remote_peer_cfg
.
When having native flush support, the only thing that change is calling flush
instead of read
, but the rest of the logic stays the same.
It is related to how the flush can be implemented on RDMA HW with existing PCIe specs where there is no PCIe.Flush read defined yet, but PCIe.Read is used for flushing the PCIe bus's buffers.
direct_write_to_pmem
is a target platform configuration-driven parameter and can not be easily detected by the RDMA HW itself..
Code quote:
conn->direct_write_to_pmem = is_native_flush_supported;
examples/05-flush-to-persistent/server.c
line 72 at r1 (raw file):
Previously, yangx-jy (Xiao Yang) wrote…
Please review the following logic of my implementation.
conn->direct_write_to_pmem
will be set to true by default when native flush is supported.- You can query the value of
conn->direct_write_to_pmem
byrpma_conn_get_direct_write_to_pmem()
.pcfg->direct_write_to_pmem
on 05-flush-to-persistent/server can be passed to 05-flush-to-persistent/client during connection establishment but it will be ignored ifconn->direct_write_to_pmem
has been true.
We can not assume that when RNIC supports a flush, it also supports flush to perisstency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 23 files reviewed, 3 unresolved discussions (waiting on @ldorau and @yangx-jy)
examples/05-flush-to-persistent/client.c
line 125 at r2 (raw file):
goto err_mr_dereg; /* ignore the remote peer configuration when connection supports direct write to PMEM. */
We can not ignore remote configuration.
Code quote:
ignore the remote peer configuration when connection supports direct write to PMEM
Previously, grom72 (Tomasz Gromadzki) wrote…
Right, it's not a good solution. But I think we should get the support of How about the following logic:
|
Previously, grom72 (Tomasz Gromadzki) wrote…
If RNIC supports native flush, it means that kernel on remote side will call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 15 of 23 files reviewed, 6 unresolved discussions (waiting on @grom72 and @yangx-jy)
utils/docker/run-build.sh
line 153 at r7 (raw file):
CC=$CC \ $CMAKE .. -DCMAKE_BUILD_TYPE=Debug \ -DBUILD_TESTS=OFF \
Remove it before merging (just to not forget)
Code quote:
-DBUILD_TESTS=OFF \
utils/docker/run-build.sh
line 175 at r7 (raw file):
CC=$CC \ $CMAKE .. -DCMAKE_BUILD_TYPE=Debug \ -DBUILD_TESTS=OFF \
Remove it before merging (just to not forget)
Code quote:
-DBUILD_TESTS=OFF \
utils/docker/run-build.sh
line 208 at r7 (raw file):
CC=$CC \ $CMAKE .. -DCMAKE_BUILD_TYPE=Release \ -DBUILD_TESTS=OFF \
Remove it before merging (just to not forget)
Code quote:
-DBUILD_TESTS=OFF \
9cfbfe9
to
1182b48
Compare
There was a problem hiding this 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, 4 unresolved discussions (waiting on @grom72 and @yangx-jy)
a discussion (no related file):
Previously, ldorau (Lukasz Dorau) wrote…
There is still an error: https://github.com/ldorau/rpma/actions/runs/4405938979/jobs/7717494188
@yangx-jy NOTICE that this error occurs on master, not on this PR.
0208220
to
48f30bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 43 of 45 files reviewed, 3 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)
CHANGELOG.md
line 13 at r21 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
OK
Done.
tests/unit/mr/mr-flush.c
line 95 at r21 (raw file):
Previously, janekmi (Jan Michalski) wrote…
These two cases are basically the same:
- flush__COMPL_ON_ERROR_failed_E_PROVIDER
- flush__COMPL_ON_SUCCESS_failed_E_PROVIDER
Can we cover both of them by introducing another for-loop? 🤔
Done.
tests/unit/mr/mr-flush.c
line 139 at r21 (raw file):
Previously, janekmi (Jan Michalski) wrote…
The same question for these two cases:
- flush__COMPLETION_ON_ERROR_success
- flush__COMPLETION_ALWAYS_success
Done.
ed5e86d
to
eaf7fb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 21 files at r13, 3 of 4 files at r23.
Reviewable status: 44 of 46 files reviewed, 10 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)
a discussion (no related file):
Previously, yangx-jy (Xiao Yang) wrote…
Please see #2122
OK. #2122 and #2125 should be merged before this PR
CHANGELOG.md
line 16 at r23 (raw file):
and native flush) on Ubuntu-latest and Fedora-latest CIs - internal APIs: - rpma_utils_ibv_context_is_flush_capable() - checks if kernel supports native flush
the native flush
Code quote:
native flush
CHANGELOG.md
line 17 at r23 (raw file):
- internal APIs: - rpma_utils_ibv_context_is_flush_capable() - checks if kernel supports native flush - a option for cmake to disable support for native flush in libibverbs
an option
Code quote:
a option
CHANGELOG.md
line 17 at r23 (raw file):
- internal APIs: - rpma_utils_ibv_context_is_flush_capable() - checks if kernel supports native flush - a option for cmake to disable support for native flush in libibverbs
the native flush
Code quote:
native flush
CHANGELOG.md
line 32 at r23 (raw file):
required for Rocky Linux 8 and 9 and verify if the installation succeeded - rpma_peer_new() to check the native flush support in kernel - rpma_peer_setup_qp() to enable native flush if both kernel and libibverbs supported it
the native flush
Code quote:
native flush
CHANGELOG.md
line 32 at r23 (raw file):
required for Rocky Linux 8 and 9 and verify if the installation succeeded - rpma_peer_new() to check the native flush support in kernel - rpma_peer_setup_qp() to enable native flush if both kernel and libibverbs supported it
support
Code quote:
supported
CHANGELOG.md
line 33 at r23 (raw file):
- rpma_peer_new() to check the native flush support in kernel - rpma_peer_setup_qp() to enable native flush if both kernel and libibverbs supported it - rpma_peer_usage2access() to return native access flags if both kernel and libibverbs supported native flush
support the native flush
Code quote:
supported native flush
CHANGELOG.md
line 34 at r23 (raw file):
- rpma_peer_setup_qp() to enable native flush if both kernel and libibverbs supported it - rpma_peer_usage2access() to return native access flags if both kernel and libibverbs supported native flush - rpma_flush() to use native flush if the created QP supported it
the native flush
Code quote:
native flush
CHANGELOG.md
line 34 at r23 (raw file):
- rpma_peer_setup_qp() to enable native flush if both kernel and libibverbs supported it - rpma_peer_usage2access() to return native access flags if both kernel and libibverbs supported native flush - rpma_flush() to use native flush if the created QP supported it
supports
Code quote:
supported
There was a problem hiding this 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: 44 of 46 files reviewed, 10 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 44 of 46 files reviewed, 10 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)
a discussion (no related file):
Rebase please.
Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
This internal function is to check if kernel supports two native flush types (Global visibility and Persistence). Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
Check if kernel supports both of native flush types in rpma_peer_new() nd then save the result into the is_native_flush_supported member of struct rpma_peer. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
rpma_peer_setup_qp() enables the native flush if both kernel and libibverbs support it. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
rpma_peer_setup_mr_reg() registers a memory region with IBV_ACCESS_FLUSH_GLOBAL or IBV_ACCESS_FLUSH_PERSISTENT if both kernel and libibverbs support the native flush. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
rpma_flush() uses the native flush when the created QP supports it. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 44 of 46 files reviewed, 11 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)
utils/docker/run-build.sh
line 165 at r23 (raw file):
rm -rf $WORKDIR/build echo
Please move it here: https://github.com/pmem/rpma/blob/main/utils/docker/run-build.sh#L235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r23.
Reviewable status: 45 of 46 files reviewed, 11 unresolved discussions (waiting on @grom72, @janekmi, and @yangx-jy)
eaf7fb5
to
e885482
Compare
1) Add a BUILD_FORCE_NATIVE_FLUSH_NOT_SUPPORTED option. 2) Add a relevant building test. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
e885482
to
2c98624
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 38 of 46 files reviewed, 11 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)
a discussion (no related file):
Done.
CHANGELOG.md
line 16 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
the native flush
Done.
CHANGELOG.md
line 17 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
an option
Done. Use the BUILD_FORCE_NATIVE_FLUSH_NOT_SUPPORTED CMake option
.
CHANGELOG.md
line 17 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
the native flush
Done.
CHANGELOG.md
line 32 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
the native flush
Done.
CHANGELOG.md
line 32 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
support
Done.
CHANGELOG.md
line 33 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
support the native flush
Done.
CHANGELOG.md
line 34 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
the native flush
Done.
CHANGELOG.md
line 34 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
supports
Done.
utils/docker/run-build.sh
line 165 at r23 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
Please move it here: https://github.com/pmem/rpma/blob/main/utils/docker/run-build.sh#L235
Done.
There was a problem hiding this 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 r19, 7 of 7 files at r24, 3 of 3 files at r25, all commit messages.
Reviewable status: 45 of 46 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 19 files at r12, 2 of 21 files at r13.
Reviewable status: 45 of 46 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 45 of 46 files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)
tests/unit/mr/mr-flush.c
line 139 at r21 (raw file):
Previously, yangx-jy (Xiao Yang) wrote…
Done.
@janekmi ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 11 files at r8, 1 of 21 files at r13, 1 of 2 files at r21, 1 of 2 files at r22, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r22, 1 of 3 files at r25, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 19 files at r12, 8 of 21 files at r13, 1 of 2 files at r14, 1 of 4 files at r15, 2 of 4 files at r17, 1 of 3 files at r18, 1 of 2 files at r21, 1 of 2 files at r22.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)
The whole libibverbs flush patch sets have been merged into kernel and rdma-core repo. I think it is time to discuss this PR. ^_^
kernel: https://lore.kernel.org/linux-rdma/20221206130201.30986-1-lizhijian@fujitsu.com/
rdma-core: linux-rdma/rdma-core#1181
linux-rdma/rdma-core#1307 (This is the PR that fixes a bug)
@grom72 @ldorau
This PR can be built with BUILD_TESTS=OFF because I have not written the related unit tests. In this case, I hope we can discuss the logic about this PR first.
Best Regards,
Xiao Yang
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)