Fabiano
Thanks for your testing.
On 28/02/2025 21:49, Fabiano Rosas wrote:
> Li Zhijian via <qemu-devel@nongnu.org> writes:
>
>> This qtest requires there is a RDMA(RoCE) link in the host.
>> In order to make the test work smoothly, introduce a
>> scripts/rdma-migration-helper.sh to
>> - setup a new Soft-RoCE(aka RXE) if it's root
>> - detect existing RoCE link
>>
>> Test will be skipped if there is no available RoCE link.
>> # Start of rdma tests
>> # Running /x86_64/migration/precopy/rdma/plain
>> ok 1 /x86_64/migration/precopy/rdma/plain # SKIP
>> There is no available rdma link to run RDMA migration test.
>> To enable the test:
>> (1) Run 'scripts/rdma-migration-helper.sh setup' with root and rerun the
>> test
>
> sudo scripts/rdma-migration-helper.sh setup
> QTEST_QEMU_BINARY=./qemu-system-x86_64 ./tests/qtest/migration-test
> --full -r /x86_64/migration/precopy/rdma/plain
>
> # {
> # "error": {
> # "class": "GenericError",
> # "desc": "RDMA ERROR: rdma migration: error registering 0 control!"
> # }
> # }
>
1333 static int qemu_rdma_reg_control(RDMAContext *rdma, int idx)
1334 {
1335 rdma->wr_data[idx].control_mr = ibv_reg_mr(rdma->pd,
1336 rdma->wr_data[idx].control, RDMA_CONTROL_MAX_BUFFER,
1337 IBV_ACCESS_LOCAL_WRITE | IBV_ACCESS_REMOTE_WRITE); <<<=== It
failed here
1338 if (rdma->wr_data[idx].control_mr) {
1339 rdma->total_registrations++;
1340 return 0;
1341 }
1342 return -1;
1343 }
It appears to have failed at ibv_reg_mr()
This worked on my Ubuntu2204 and Fedora40. I wonder if your distro's security
policy
is preventing MR registration without root privileges...?
>> or
>> (2) Run the test with root privilege
>
> This one works fine.
>
>>
>> # End of rdma tests
>>
>> Reviewed-by: Peter Xu <pet...@redhat.com>
>> Signed-off-by: Li Zhijian <lizhij...@fujitsu.com>
>> ---
>> MAINTAINERS | 1 +
>> scripts/rdma-migration-helper.sh | 41 +++++++++++++++++
>> tests/qtest/migration/precopy-tests.c | 64 +++++++++++++++++++++++++++
>> 3 files changed, 106 insertions(+)
>> create mode 100755 scripts/rdma-migration-helper.sh
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3848d37a38d..15360fcdc4b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3480,6 +3480,7 @@ R: Li Zhijian <lizhij...@fujitsu.com>
>> R: Peter Xu <pet...@redhat.com>
>> S: Odd Fixes
>> F: migration/rdma*
>> +F: scripts/rdma-migration-helper.sh
>>
>> Migration dirty limit and dirty page rate
>> M: Hyman Huang <yong.hu...@smartx.com>
>> diff --git a/scripts/rdma-migration-helper.sh
>> b/scripts/rdma-migration-helper.sh
>> new file mode 100755
>> index 00000000000..66557d9e267
>> --- /dev/null
>> +++ b/scripts/rdma-migration-helper.sh
>> @@ -0,0 +1,41 @@
>> +#!/bin/bash
>> +
>
> I'd prefer a command -v rdma check around here. With the way the script
> pipes commands into one another will cause bash to emit a couple of
> "rdma: command not found" in case rdma command is not present.
>
It sounds good to me.
>> +# Copied from blktests
>> +get_ipv4_addr()
>> +{
>> + ip -4 -o addr show dev "$1" |
>> + sed -n 's/.*[[:blank:]]inet[[:blank:]]*\([^[:blank:]/]*\).*/\1/p' |
>> + tr -d '\n'
>> +}
>> +
>> +has_soft_rdma()
>> +{
>> + rdma link | grep -q " netdev $1[[:blank:]]*\$"
>> +}
>> +
>> +rdma_rxe_setup_detect()
>> +{
>> + (
>> + cd /sys/class/net &&
>> + for i in *; do
>> + [ -e "$i" ] || continue
>> + [ "$i" = "lo" ] && continue
>> + [ "$(<"$i/addr_len")" = 6 ] || continue
>> + [ "$(<"$i/carrier")" = 1 ] || continue
>> +
>> + has_soft_rdma "$i" && break
>> + [ "$operation" = "setup" ] &&
>> + rdma link add "${i}_rxe" type rxe netdev "$i" && break
>> + done
>> + has_soft_rdma "$i" || return
>> + get_ipv4_addr "$i"
>> + )
>> +}
>> +
>> +operation=${1:-setup}
>> +
>> +if [ "$operation" == "setup" ] || [ "$operation" == "detect" ]; then
>> + rdma_rxe_setup_detect
>> +else
>> + echo "Usage: $0 [setup | detect]"
>> +fi
>
> What happened to the cleanup option? I think I missed some discussion on
> this... We can't expect people to know how to clean this up without any
> hint.
Nothing special, one reason could be to keep it as simple as possible in the
beginning.
I'm fine to add it back.
>
>> diff --git a/tests/qtest/migration/precopy-tests.c
>> b/tests/qtest/migration/precopy-tests.c
>> index ba273d10b9a..bf97f4e9325 100644
>> --- a/tests/qtest/migration/precopy-tests.c
>> +++ b/tests/qtest/migration/precopy-tests.c
>> @@ -99,6 +99,66 @@ static void test_precopy_unix_dirty_ring(void)
>> test_precopy_common(&args);
>> }
>>
>> +#ifdef CONFIG_RDMA
>> +
>> +#define RDMA_MIGRATION_HELPER "scripts/rdma-migration-helper.sh"
>> +static int new_rdma_link(char *buffer)
>> +{
>> + const char *argument = (geteuid() == 0) ? "setup" : "detect";
>> + char cmd[1024];
>> +
>> + snprintf(cmd, sizeof(cmd), "%s %s", RDMA_MIGRATION_HELPER, argument);
>> +
>> + FILE *pipe = popen(cmd, "r");
>
> This needs to be silenced, otherwise messages from the script will break
> TAP output. I suggest:
>
> bool verbose = g_getenv("QTEST_LOG");
>
> snprintf(cmd, sizeof(cmd), "%s %s %s", RDMA_MIGRATION_HELPER, argument,
> verbose ? "" : "2>/dev/null");
>
It sound good to me, i will update it.
>> + if (pipe == NULL) {
>> + perror("Failed to run script");
>> + return -1;
>> + }
>> +
>> + int idx = 0;
>> + while (fgets(buffer + idx, 128 - idx, pipe) != NULL) {
>> + idx += strlen(buffer);
>> + }
>> +
>> + int status = pclose(pipe);
>> + if (status == -1) {
>> + perror("Error reported by pclose()");
>> + return -1;
>> + } else if (WIFEXITED(status)) {
>> + return WEXITSTATUS(status);
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +static void test_precopy_rdma_plain(void)
>> +{
>> + char buffer[128] = {};
>> +
>> + if (new_rdma_link(buffer)) {
>> + g_test_skip("\nThere is no available rdma link to run RDMA
>> migration test.\n"
>> + "To enable the test:\n"
>> + "(1) Run \'" RDMA_MIGRATION_HELPER " setup\' with root
>> and rerun the test\n"
>> + "or\n"
>> + "(2) Run the test with root privilege\n");
>
> g_test_skip() needs a one-line message, otherwise it breaks TAP
> output. You can turn this into a g_test_message(), put it under
> QTEST_LOG=1 and add a g_test_skip("no rdma link available") below.
Ditto.
Thanks
Zhijian
