Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[address@hidden]

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 <lizhijian@cn.fujitsu.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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);