[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 52/52] migration/rdma: Fix how we show device details on open
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 52/52] migration/rdma: Fix how we show device details on open |
Date: |
Tue, 26 Sep 2023 11:19:12 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
"Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> writes:
> On 18/09/2023 22:42, Markus Armbruster wrote:
>> qemu_rdma_dump_id() dumps RDMA device details to stdout.
>>
>> rdma_start_outgoing_migration() calls it via qemu_rdma_source_init()
>> and qemu_rdma_resolve_host() to show source device details.
>> rdma_start_incoming_migration() arranges its call via
>> rdma_accept_incoming_migration() and qemu_rdma_accept() to show
>> destination device details.
>>
>> Two issues:
>>
>> 1. rdma_start_outgoing_migration() can run in HMP context. The
>> information should arguably go the monitor, not stdout.
>>
>> 2. ibv_query_port() failure is reported as error. Its callers remain
>> unaware of this failure (qemu_rdma_dump_id() can't fail), so
>> reporting this to the user as an error is problematic.
>>
>> Use qemu_printf() instead of printf() and error_report().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> migration/rdma.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 9e9904984e..8c84fbab7a 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -30,6 +30,7 @@
>> #include "qemu/sockets.h"
>> #include "qemu/bitmap.h"
>> #include "qemu/coroutine.h"
>> +#include "qemu/qemu-print.h"
>> #include "exec/memory.h"
>> #include <sys/socket.h>
>> #include <netdb.h>
>> @@ -742,24 +743,25 @@ static void qemu_rdma_dump_id(const char *who, struct
>> ibv_context *verbs)
>> struct ibv_port_attr port;
>>
>> if (ibv_query_port(verbs, 1, &port)) {
>> - error_report("Failed to query port information");
>> + qemu_printf("%s RDMA Device opened, but can't query port
>> information",
>> + who);
>
>
> '\n' newline is missing ?
Yes.
>> return;
>> }
>>
>> - printf("%s RDMA Device opened: kernel name %s "
>> - "uverbs device name %s, "
>> - "infiniband_verbs class device path %s, "
>> - "infiniband class device path %s, "
>> - "transport: (%d) %s\n",
>> + qemu_printf("%s RDMA Device opened: kernel name %s "
>> + "uverbs device name %s, "
>> + "infiniband_verbs class device path %s, "
>> + "infiniband class device path %s, "
>> + "transport: (%d) %s\n",
>> who,
>> verbs->device->name,
>> verbs->device->dev_name,
>> verbs->device->dev_path,
>> verbs->device->ibdev_path,
>> port.link_layer,
>> - (port.link_layer == IBV_LINK_LAYER_INFINIBAND) ?
>> "Infiniband" :
>> - ((port.link_layer == IBV_LINK_LAYER_ETHERNET)
>> - ? "Ethernet" : "Unknown"));
>> + port.link_layer == IBV_LINK_LAYER_INFINIBAND ? "Infiniband"
>> + : port.link_layer == IBV_LINK_LAYER_ETHERNET ? "Ethernet"
>> + : "Unknown");
>
>
> Most of the time, these messages are not needed, so i would prefer to put it
> to the trace instead.
Makes sense.
>> }
>>
>> /*
Thanks!
- [PATCH 22/52] migration/rdma: Fix rdma_getaddrinfo() error checking, (continued)
- [PATCH 22/52] migration/rdma: Fix rdma_getaddrinfo() error checking, Markus Armbruster, 2023/09/18
- [PATCH 45/52] migration/rdma: Silence qemu_rdma_connect(), Markus Armbruster, 2023/09/18
- [PATCH 48/52] migration/rdma: Silence qemu_rdma_block_for_wrid(), Markus Armbruster, 2023/09/18
- [PATCH 49/52] migration/rdma: Silence qemu_rdma_register_and_get_keys(), Markus Armbruster, 2023/09/18
- [PATCH 52/52] migration/rdma: Fix how we show device details on open, Markus Armbruster, 2023/09/18
- Re: [PATCH 00/52] migration/rdma: Error handling fixes, Peter Xu, 2023/09/19
- Re: [PATCH 00/52] migration/rdma: Error handling fixes, Markus Armbruster, 2023/09/28