Re: [Qemu-devel] [PATCH v4 11/12] migration: poll the cm event while wait RDMA work request completion

[Aviad Yehezkel <aviadye@dev.mellanox.co.il>]

+Gal

Gal, please comment with our findings.

Thanks!


On 5/31/2018 10:36 AM, 858585 jemmy wrote:
> On Thu, May 31, 2018 at 1:33 AM, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
>> * Lidong Chen (jemmy858585@gmail.com) wrote:
>>> If the peer qemu is crashed, the qemu_rdma_wait_comp_channel function
>>> maybe loop forever. so we should also poll the cm event fd, and when
>>> receive any cm event, we consider some error happened.
>>>
>>> Signed-off-by: Lidong Chen <lidongchen@tencent.com>
>> I don't understand enough about the way the infiniband fd's work to
>> fully review this; so I'd appreciate if some one who does could
>> comment/add their review.
> Hi Avaid:
>      we need your help. I also not find any document about the cq
> channel event fd and
> cm channel event f.
>      Should we set the events to G_IO_IN | G_IO_HUP | G_IO_ERR? or
> G_IO_IN is enough?
>      pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
>      pfds[1].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
>      Thanks.
>
>>> ---
>>>   migration/rdma.c | 35 ++++++++++++++++++++++++-----------
>>>   1 file changed, 24 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index 1b9e261..d611a06 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -1489,6 +1489,9 @@ 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;
>>> +    int ret = -1;
>>> +
>>>       /*
>>>        * Coroutine doesn't start until migration_fd_process_incoming()
>>>        * so don't yield unless we know we're running inside of a coroutine.
>>> @@ -1504,25 +1507,35 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
>>>            * But we need to be able to handle 'cancel' or an error
>>>            * without hanging forever.
>>>            */
>>> -        while (!rdma->error_state  && !rdma->received_error) {
>>> -            GPollFD pfds[1];
>>> +        while (!rdma->error_state && !rdma->received_error) {
>>> +            GPollFD pfds[2];
>>>               pfds[0].fd = rdma->comp_channel->fd;
>>>               pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
>>> +            pfds[0].revents = 0;
>>> +
>>> +            pfds[1].fd = rdma->channel->fd;
>>> +            pfds[1].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
>>> +            pfds[1].revents = 0;
>>> +
>>>               /* 0.1s timeout, should be fine for a 'cancel' */
>>> -            switch (qemu_poll_ns(pfds, 1, 100 * 1000 * 1000)) {
>>> -            case 1: /* fd active */
>>> -                return 0;
>>> +            qemu_poll_ns(pfds, 2, 100 * 1000 * 1000);
>> Shouldn't we still check the return value of this; if it's negative
>> something has gone wrong.
> I will fix this.
> Thanks.
>
>> Dave
>>
>>> -            case 0: /* Timeout, go around again */
>>> -                break;
>>> +            if (pfds[1].revents) {
>>> +                ret = rdma_get_cm_event(rdma->channel, &cm_event);
>>> +                if (!ret) {
>>> +                    rdma_ack_cm_event(cm_event);
>>> +                }
>>> +                error_report("receive cm event while wait comp channel,"
>>> +                             "cm event is %d", cm_event->event);
>>>
>>> -            default: /* Error of some type -
>>> -                      * I don't trust errno from qemu_poll_ns
>>> -                     */
>>> -                error_report("%s: poll failed", __func__);
>>> +                /* consider any rdma communication event as an error */
>>>                   return -EPIPE;
>>>               }
>>>
>>> +            if (pfds[0].revents) {
>>> +                return 0;
>>> +            }
>>> +
>>>               if (migrate_get_current()->state == MIGRATION_STATUS_CANCELLING) {
>>>                   /* Bail out and let the cancellation happen */
>>>                   return -EPIPE;
>>> --
>>> 1.8.3.1
>>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK