From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 1B5B14C8B for ; Wed, 3 Oct 2018 10:30:11 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20181003083011euoutp025829c9fa35f642c150f0b582a7bf2330~aC-6ltpCo2998929989euoutp02H for ; Wed, 3 Oct 2018 08:30:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20181003083011euoutp025829c9fa35f642c150f0b582a7bf2330~aC-6ltpCo2998929989euoutp02H DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1538555411; bh=LEw8qW9R0STlxvFlr5cr2B6v2wiGv/XuWQBbczr9nsE=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=ctKTRg7TjOopyAPwKm0XcQd3BFnAMyOR7qcV6IvzxN7ILSarhCkrO7G/eeDr8fyMO CVRglGlNv114SKtwCi7uEQO8Ly41KeclGtkbj3llseMPeNp3UUtjyOLCRuhGss29zb xblxQs4NNYbTYYv+ojSrLXJo5BB2ePMkAaxvGm7o= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20181003083010eucas1p14d8145da533f2e0fb4291648520df97c~aC-6Is-yi3144631446eucas1p1k; Wed, 3 Oct 2018 08:30:10 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id 32.20.04441.21E74BB5; Wed, 3 Oct 2018 09:30:10 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20181003083009eucas1p19f5d8234a3592b9e1753181d858a99aa~aC-5VXlJ51559815598eucas1p1k; Wed, 3 Oct 2018 08:30:09 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20181003083009eusmtrp1882a1e4fa673e214db60b07d1c9bb9d2~aC-5ERc001306413064eusmtrp1U; Wed, 3 Oct 2018 08:30:09 +0000 (GMT) X-AuditID: cbfec7f2-a1ae89c000001159-1d-5bb47e12ef98 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id AF.C2.04284.11E74BB5; Wed, 3 Oct 2018 09:30:09 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20181003083008eusmtip21ac0fb6c87000f9cef1316cfd8c59ca3~aC-4VAAwb1826518265eusmtip2N; Wed, 3 Oct 2018 08:30:08 +0000 (GMT) To: Maxime Coquelin , dev@dpdk.org, tiwei.bie@intel.com, zhihong.wang@intel.com, jfreimann@redhat.com, nicknickolaev@gmail.com, bruce.richardson@intel.com, alejandro.lucero@netronome.com Cc: dgilbert@redhat.com, stable@dpdk.org, "Michael S. Tsirkin" From: Ilya Maximets Date: Wed, 3 Oct 2018 11:32:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrGKsWRmVeSWpSXmKPExsWy7djP87pCdVuiDdYv57U492kZk8WNVfYW 7z5tZ7Lo3XaP3eJK+092i3NrlrJYHOvcw2Lx/9crVovTC6+xWPzr+MNusbXhP5PF5ouTmBx4 PH4tWMrqsXPWXXaPxXteMnlM737I7PF+31U2j74tqxgD2KK4bFJSczLLUov07RK4Mg5vmM5U sEaxou3aRuYGxivSXYycHBICJhI73i9nBrGFBFYwSrx6y9fFyAVkf2GUOHflNxNE4jOjxJt7 +jANV67MZYUoWs4ocefQXTYI5yOjxOxzX4FGcXAIC9hJtD5QBomLCNxhlLiy9BIbSJxZIFBi 08J4kEFsAjoSp1YfYQSxWQRUJBq67oJdISoQIXHkwUKwOK+AoMTJmU9YQGxOoJFTOl6DxZkF xCWavqxkhbDlJZq3zmYG2SUhcI9d4v+6VqjmMoltU18zQVztIrF96glWCFtY4tXxLewQtozE 6ck9LBB2vcT9lpeMEIM6GCWmH/oH1WwvseX1OXaIBzQl1u+ChoSjxI5VDWD/SgjwSdx4Kwhx D5/EpG3TocK8Eh1tQhDVKhK/D0ICWkJASuLmu8/sExiVZiH5chaSz2Yh+WwWwt4FjCyrGMVT S4tz01OLDfNSy/WKE3OLS/PS9ZLzczcxAlPX6X/HP+1g/Hop6RCjAAejEg/vjoWbo4VYE8uK K3MPMUpwMCuJ8PYlAoV4UxIrq1KL8uOLSnNSiw8xSnOwKInzLpu3MVpIID2xJDU7NbUgtQgm y8TBKdXAaPa1doVv3pVmy3fW2x3OmtSfP/vD5UiUknJo1j6+Z3e7asS5V515lCfvYqlrsi9T t9LuouIaqf1Kpp12R6Qzl1w1Lkk8sNZiaz9D//Yn+Q+Tgx2ns6QmWV+IXOMcpeF5/bF83oqU 6rDlTkuP7Iv77+ed7yNk4+vHZ7nTu/xW0CIz+eh9rCuUWIozEg21mIuKEwGb4B8SWQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrNIsWRmVeSWpSXmKPExsVy+t/xe7qCdVuiDZ4uFrU492kZk8WNVfYW 7z5tZ7Lo3XaP3eJK+092i3NrlrJYHOvcw2Lx/9crVovTC6+xWPzr+MNusbXhP5PF5ouTmBx4 PH4tWMrqsXPWXXaPxXteMnlM737I7PF+31U2j74tqxgD2KL0bIryS0tSFTLyi0tslaINLYz0 DC0t9IxMLPUMjc1jrYxMlfTtbFJSczLLUov07RL0Mg5vmM5UsEaxou3aRuYGxivSXYycHBIC JhJXrsxlBbGFBJYySkz/Zw8Rl5L48esCK4QtLPHnWhdbFyMXUM17Rok3nzezdDFycAgL2Em0 PlAGiYsI3GGU2DLrDCNIA7NAoMT+7dfYIRq6WSS2/uxlB0mwCehInFp9BKyIF6h57bI9LCA2 i4CKREPXXWYQW1QgQmL18hesEDWCEidnPgGr4QSqn9LxGmqBusSfeZeYIWxxiaYvK1khbHmJ 5q2zmScwCs1C0j4LScssJC2zkLQsYGRZxSiSWlqcm55bbKhXnJhbXJqXrpecn7uJERix2479 3LyD8dLG4EOMAhyMSjy8OxZujhZiTSwrrsw9xCjBwawkwtuXCBTiTUmsrEotyo8vKs1JLT7E aAr03ERmKdHkfGAyySuJNzQ1NLewNDQ3Njc2s1AS5z1vUBklJJCeWJKanZpakFoE08fEwSnV wOgYtoD9Sr7/Hb8v/aUHvG1bPx97fjih5cPRLYs+vNVkebVHYJrl/vt3vtSa731Z8v9Zv9H9 9TdbRa10pqo0rD9QfiL00cnLjFGaUbZ9NncXHt24IWx/zuaGpiVcHdUr3RYkeLxo23BBamHH Y7Wjcpbnggz9T+hNZsgR9tiV5j+rSO9TAL+u0BIlluKMREMt5qLiRADyhAca7gIAAA== Message-Id: <20181003083009eucas1p19f5d8234a3592b9e1753181d858a99aa~aC-5VXlJ51559815598eucas1p1k@eucas1p1.samsung.com> X-CMS-MailID: 20181003083009eucas1p19f5d8234a3592b9e1753181d858a99aa X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181002093710epcas2p41edcdf02797df82d10457b464a72be77 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181002093710epcas2p41edcdf02797df82d10457b464a72be77 References: <20181002093651.24795-1-maxime.coquelin@redhat.com> <20181002093651.24795-2-maxime.coquelin@redhat.com> <20181002141315eucas1p16c87759329eeb374528bcb70a2d71ee4~Z0CLLGpN92076620766eucas1p1R@eucas1p1.samsung.com> <2a57953d-67c6-26f3-f65f-4e5a1dcf1474@redhat.com> <20181003075530eucas1p1dd7191a728c1129fd5d9dbaed5fa1047~aChpN_6wn1404114041eucas1p1H@eucas1p1.samsung.com> Subject: Re: [dpdk-dev] [PATCH v2 01/17] vhost: fix messages error checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Oct 2018 08:30:12 -0000 On 03.10.2018 11:02, Maxime Coquelin wrote: > > > On 10/03/2018 09:57 AM, Ilya Maximets wrote: >> On 03.10.2018 10:50, Maxime Coquelin wrote: >>> >>> >>> On 10/02/2018 04:15 PM, Ilya Maximets wrote: >>>> On 02.10.2018 12:36, Maxime Coquelin wrote: >>>>> Return of message handling has now changed to an enum that can >>>>> take non-negative value that is not zero in case a reply is >>>>> needed. But the code checking the variable afterwards has not >>>>> been updated, leading to success messages handling being >>>>> treated as errors. >>>>> >>>>> Fixes: 4e601952cae6 ("vhost: message handling implemented as a callback array") >>>>> >>>>> Signed-off-by: Maxime Coquelin >>>>> --- >>>>>    lib/librte_vhost/vhost_user.c | 6 +++--- >>>>>    1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >>>>> index 7ef3fb4a4..060b41893 100644 >>>>> --- a/lib/librte_vhost/vhost_user.c >>>>> +++ b/lib/librte_vhost/vhost_user.c >>>>> @@ -1783,7 +1783,7 @@ vhost_user_msg_handler(int vid, int fd) >>>>>        } >>>>>      skip_to_post_handle: >>>>> -    if (!ret && dev->extern_ops.post_msg_handle) { >>>>> +    if (ret != VH_RESULT_ERR && dev->extern_ops.post_msg_handle) { >>>>>            uint32_t need_reply; >>>>>              ret = (*dev->extern_ops.post_msg_handle)( >>>>> @@ -1800,10 +1800,10 @@ vhost_user_msg_handler(int vid, int fd) >>>>>            vhost_user_unlock_all_queue_pairs(dev); >>>>>          if (msg.flags & VHOST_USER_NEED_REPLY) { >>>> >>>> Maybe we need to reply here only if we didn't reply >>>> already (not VH_RESULT_REPLY) ? Otherwise, we could >>>> reply twice (with payload and with return code). >>> >>> Well, if the master sets this bit, it means it is waiting for >>> a "reply-ack", so not sending it would cause the master to wait >>> forever. >>> >>> It is the master responsibility to not set this bit for requests >>> already expecting a non "reply-ack" reply (as you fixed it for >>> postcopy's set mem table case). >> >> vhost-user docs in QEMU says: >> " >> For the message types that already solicit a reply from the client, the >> presence of VHOST_USER_PROTOCOL_F_REPLY_ACK or need_reply bit being set brings >> no behavioural change. >> " >> i.e. even if QEMU sets the need_reply flag, vhost should not reply twice. >> Am I missing something? > > Oh, right. Thanks for pointing it out. > > So coming back to the DPDK implementation, I just had a look again, and it seems that we don't send a reply twice, as send_vhost_reply takes > care of clearing the VHOST_USER_NEED_REPLY flag. > Do you confirm my understanding is correct? Hmm. Yes, you're right. send_vhost_reply clears the VHOST_USER_NEED_REPLY flag and vhost doesn't send replies twice. Maybe some comment with clarifications needed here, or some more refactoring to make this aspect more clear. So, we have a situation where only one of the message processing stages is able to reply even if it's not the last stage for the message: 1. extern_ops.pre_msg_handle 2. "master" 3. extern_ops.post_msg_handle 4. result i.e. (!!ret) extern_ops API is a bit confusing from this point of view. It has serious restrictions for replies which are not described anywhere. I mean that pre and post processing stages are able to request the reply, but the post processing reply will be just dropped (or "master" reply will be dropped). This is, actually, not an issue until we have only one user of them, which uses only one of the callbacks. But maybe this API should be described more in comments or docs. > >>> >>>>> -        msg.payload.u64 = !!ret; >>>>> +        msg.payload.u64 = ret == VH_RESULT_ERR; >>>>>            msg.size = sizeof(msg.payload.u64); >>>>>            send_vhost_reply(fd, &msg); >>>>> -    } else if (ret) { >>>>> +    } else if (ret == VH_RESULT_ERR) { >>>>>            RTE_LOG(ERR, VHOST_CONFIG, >>>>>                "vhost message handling failed.\n"); >>>>>            return -1; >>>>> >>> >>> > >