On 03/06/2021 01.51, Philippe Mathieu-Daudé wrote:
> Since 00f2cfbbec6 ("glib: bump min required glib library version to
> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
> free an allocated variable when it goes out of scope,
Glad to know this feature.
However per its code, a 'ack' does much more than just free the memory.
not sure g_autoptr have the ability to do the same.
2212 static void ucma_complete_event(struct cma_id_private *id_priv)
2213 {
2214 pthread_mutex_lock(&id_priv->mut);
2215 id_priv->events_completed++;
2216 pthread_cond_signal(&id_priv->cond);
2217 pthread_mutex_unlock(&id_priv->mut);
2218 }
2219
2220 static void ucma_complete_mc_event(struct cma_multicast *mc)
2221 {
2222 pthread_mutex_lock(&mc->id_priv->mut);
2223 mc->events_completed++;
2224 pthread_cond_signal(&mc->cond);
2225 mc->id_priv->events_completed++;
2226 pthread_cond_signal(&mc->id_priv->cond);
2227 pthread_mutex_unlock(&mc->id_priv->mut);
2228 }
2229
2230 int rdma_ack_cm_event(struct rdma_cm_event *event)
2231 {
2232 struct cma_event *evt;
2233
2234 if (!event)
2235 return ERR(EINVAL);
2236
2237 evt = container_of(event, struct cma_event, event);
2238
2239 if (evt->mc)
2240 ucma_complete_mc_event(evt->mc);
2241 else
2242 ucma_complete_event(evt->id_priv);
2243 free(evt);
2244 return 0;
2245 }
Thanks
Zhijian
> removing this
> burden on the developers.
>
> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
>
> "rdma_ack_cm_event() - Free a communication event.
>
> All events which are allocated by rdma_get_cm_event() must be
> released, there should be a one-to-one correspondence between
> successful gets and acks. This call frees the event structure
> and any memory that it references."
>
> Since the 'ack' description doesn't explicit the event is also
> released (free'd), it is safer to use the GLib g_autoptr feature.
> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
> for the type name, so add a type definition to achieve this.
> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
>
> Inspired-by: Li Zhijian <lizhij...@cn.fujitsu.com>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> ---
> RFC: build-tested only
> ---
> migration/rdma.c | 27 ++++++++++-----------------
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index b50ebb9183a..b703bf1b918 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -38,6 +38,9 @@
> #include "qom/object.h"
> #include <poll.h>
>
> +typedef struct rdma_cm_event rdma_cm_event;
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
> +
> /*
> * Print and error on both the Monitor and the Log file.
> */
> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma,
> Error **errp)
> int ret;
> struct rdma_addrinfo *res;
> char port_str[16];
> - struct rdma_cm_event *cm_event;
> + g_autoptr(rdma_cm_event) cm_event = NULL;
> char ip[40] = "unknown";
> struct rdma_addrinfo *e;
>
> @@ -1007,11 +1010,11 @@ route:
> ERROR(errp, "result not equal to event_addr_resolved %s",
> rdma_event_str(cm_event->event));
> perror("rdma_resolve_addr");
> - rdma_ack_cm_event(cm_event);
> ret = -EINVAL;
> goto err_resolve_get_addr;
> }
> rdma_ack_cm_event(cm_event);
> + cm_event = NULL;
>
> /* resolve route */
> ret = rdma_resolve_route(rdma->cm_id, RDMA_RESOLVE_TIMEOUT_MS);
> @@ -1028,11 +1031,9 @@ route:
> if (cm_event->event != RDMA_CM_EVENT_ROUTE_RESOLVED) {
> ERROR(errp, "result not equal to event_route_resolved: %s",
> rdma_event_str(cm_event->event));
> - rdma_ack_cm_event(cm_event);
> ret = -EINVAL;
> goto err_resolve_get_addr;
> }
> - rdma_ack_cm_event(cm_event);
> rdma->verbs = rdma->cm_id->verbs;
> qemu_rdma_dump_id("source_resolve_host", rdma->cm_id->verbs);
> qemu_rdma_dump_gid("source_resolve_host", rdma->cm_id);
> @@ -1501,7 +1502,7 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma,
> uint64_t *wr_id_out,
> */
> static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
> {
> - struct rdma_cm_event *cm_event;
> + g_autoptr(rdma_cm_event) cm_event = NULL;
> int ret = -1;
>
> /*
> @@ -2503,7 +2504,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error
> **errp, bool return_path)
> .private_data = &cap,
> .private_data_len = sizeof(cap),
> };
> - struct rdma_cm_event *cm_event;
> + g_autoptr(rdma_cm_event) cm_event = NULL;
> int ret;
>
> /*
> @@ -2544,7 +2545,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error
> **errp, bool return_path)
> if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
> perror("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect");
> ERROR(errp, "connecting to destination!");
> - rdma_ack_cm_event(cm_event);
> goto err_rdma_source_connect;
> }
> rdma->connected = true;
> @@ -2564,8 +2564,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error
> **errp, bool return_path)
>
> trace_qemu_rdma_connect_pin_all_outcome(rdma->pin_all);
>
> - rdma_ack_cm_event(cm_event);
> -
> rdma->control_ready_expected = 1;
> rdma->nb_sent = 0;
> return 0;
> @@ -3279,7 +3277,7 @@ static void rdma_cm_poll_handler(void *opaque)
> {
> RDMAContext *rdma = opaque;
> int ret;
> - struct rdma_cm_event *cm_event;
> + g_autoptr(rdma_cm_event) cm_event = NULL;
> MigrationIncomingState *mis = migration_incoming_get_current();
>
> ret = rdma_get_cm_event(rdma->channel, &cm_event);
> @@ -3287,7 +3285,6 @@ static void rdma_cm_poll_handler(void *opaque)
> error_report("get_cm_event failed %d", errno);
> return;
> }
> - rdma_ack_cm_event(cm_event);
>
> if (cm_event->event == RDMA_CM_EVENT_DISCONNECTED ||
> cm_event->event == RDMA_CM_EVENT_DEVICE_REMOVAL) {
> @@ -3317,7 +3314,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> .private_data_len = sizeof(cap),
> };
> RDMAContext *rdma_return_path = NULL;
> - struct rdma_cm_event *cm_event;
> + g_autoptr(rdma_cm_event) cm_event = NULL;
> struct ibv_context *verbs;
> int ret = -EINVAL;
> int idx;
> @@ -3328,7 +3325,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> }
>
> if (cm_event->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
> - rdma_ack_cm_event(cm_event);
> goto err_rdma_dest_wait;
> }
>
> @@ -3339,7 +3335,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> if (migrate_postcopy() && !rdma->is_return_path) {
> rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
> if (rdma_return_path == NULL) {
> - rdma_ack_cm_event(cm_event);
> goto err_rdma_dest_wait;
> }
>
> @@ -3353,7 +3348,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> if (cap.version < 1 || cap.version > RDMA_CONTROL_VERSION_CURRENT) {
> error_report("Unknown source RDMA version: %d, bailing...",
> cap.version);
> - rdma_ack_cm_event(cm_event);
> goto err_rdma_dest_wait;
> }
>
> @@ -3374,6 +3368,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
> verbs = cm_event->id->verbs;
>
> rdma_ack_cm_event(cm_event);
> + cm_event = NULL;
>
> trace_qemu_rdma_accept_pin_state(rdma->pin_all);
>
> @@ -3441,11 +3436,9 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>
> if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
> error_report("rdma_accept not event established");
> - rdma_ack_cm_event(cm_event);
> goto err_rdma_dest_wait;
> }
>
> - rdma_ack_cm_event(cm_event);
> rdma->connected = true;
>
> ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
