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 4CCE45F20 for ; Wed, 3 Oct 2018 11:04:56 +0200 (CEST) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20181003090455euoutp026f1927bf16498f62f285c3068ca854ff~aDeP5cvX21518815188euoutp02i for ; Wed, 3 Oct 2018 09:04:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20181003090455euoutp026f1927bf16498f62f285c3068ca854ff~aDeP5cvX21518815188euoutp02i DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1538557495; bh=nviQUKsVEBtD/SPVl08zuBY+YiHFnHjc0mPFFLpWKBo=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=Kk1lJ0p3UyVSbvxXsD633j31V6XUx3o2JseXd8fh48dZqoglFkMLkwuQZh2soq7QG n8wkGzJ8vowfTf5eLvzk4vHg3RG6eLmLv8MxqI/g6wGnwChHkFJqZ3KPV9Ph/9HLWy Hsbz9xUnYJ90Y2MzvMKGg0vQAhPXmfNnz2AfvPCU= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20181003090454eucas1p13124dd1f9358584acbd02dc3e6336769~aDePbP-JZ0119601196eucas1p17; Wed, 3 Oct 2018 09:04:54 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id 26.DB.04294.63684BB5; Wed, 3 Oct 2018 10:04:54 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20181003090454eucas1p180d79e1279ca4e1086b2447758686978~aDeOgna1t0120201202eucas1p1W; Wed, 3 Oct 2018 09:04:54 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20181003090453eusmtrp13476ad3afeb313afe2a0f55ede5ec1d5~aDeOPfyy82299222992eusmtrp19; Wed, 3 Oct 2018 09:04:53 +0000 (GMT) X-AuditID: cbfec7f4-835ff700000010c6-dd-5bb48636de3b Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 3A.48.04284.53684BB5; Wed, 3 Oct 2018 10:04:53 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20181003090452eusmtip1899a4292ab09727e4c65461f321a4f45~aDeNe7GPg3245832458eusmtip19; Wed, 3 Oct 2018 09:04:52 +0000 (GMT) From: Ilya Maximets 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" Date: Wed, 3 Oct 2018 12:07:10 +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: <20181003083009eucas1p19f5d8234a3592b9e1753181d858a99aa~aC-5VXlJ51559815598eucas1p1k@eucas1p1.samsung.com> Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrOKsWRmVeSWpSXmKPExsWy7djPc7pmbVuiDe79UbE492kZk8WNVfYW 7z5tZ7Lo3XaP3eJK+092i3NrlrJYHOvcw2Lx/9crVovTC6+xWPzr+MNusbXhP5PF5ouTmBx4 PH4tWMrqsXPWXXaPxXteMnlM737I7PF+31U2j74tqxgD2KK4bFJSczLLUov07RK4Mla1OBb8 V6rY/uIuYwPjf5kuRk4OCQETiQ0PH7F0MXJxCAmsYJTombeIFcL5wigxtfU6M4TzmVFiZsMO dpiW+WcuQFUtZ5RY/qmTHcL5COSsmQY0jINDWMBOovWBMkgDm4COxKnVRxhBakQE7jBKXFl6 iQ2khlkgUGLTwniQGhYBFYnVze+YQGxRgQiJIw8WMoLYvAKCEidnPmEBsTkFyiWaft0Aq2EW EJdo+rKSFcKWl2jeOhvsUgmBe+wSv29+Y4ZoLpNobT7NBnG1i8TRy09ZIGxhiVfHt0B9IyNx enIPVLxe4n7LS0aIQR2MEtMP/WOCSNhLbHl9jh3iaE2J9bv0IcKOEjtWNTCDhCUE+CRuvBWE uIdPYtK26VBhXomONiGIahWJ3weXM0PYUhI3331mn8CoNAvJl7OQfDYLyWezEPYuYGRZxSie Wlqcm55abJSXWq5XnJhbXJqXrpecn7uJEZi8Tv87/mUH464/SYcYBTgYlXh4dyzcHC3EmlhW XJl7iFGCg1lJhLcvESjEm5JYWZValB9fVJqTWnyIUZqDRUmcd9m8jdFCAumJJanZqakFqUUw WSYOTqkGxv6ifw8X56321m8IU2YVXHSKbfLx091KToe1q+ZFPm3YnyoaURw1d0NIFme9bZfe tDcXlzL7P7VXkyh+EL2jM7f6jPyf+ZpT6nLe/gpinCWZ5rHP41bL7JNOp+ZVXc82WS6p1mi9 rOzM9NubHwnOKX3V9CIiYt2bS89r7tw68Myq6MfN3bnzHJVYijMSDbWYi4oTAfGG9TBaAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrFIsWRmVeSWpSXmKPExsVy+t/xu7qmbVuiDfbeUbE492kZk8WNVfYW 7z5tZ7Lo3XaP3eJK+092i3NrlrJYHOvcw2Lx/9crVovTC6+xWPzr+MNusbXhP5PF5ouTmBx4 PH4tWMrqsXPWXXaPxXteMnlM737I7PF+31U2j74tqxgD2KL0bIryS0tSFTLyi0tslaINLYz0 DC0t9IxMLPUMjc1jrYxMlfTtbFJSczLLUov07RL0Mla1OBb8V6rY/uIuYwPjf5kuRk4OCQET iflnLrCC2EICSxkltj+whYhLSfz4BRGXEBCW+HOti62LkQuo5j2jxL9vrUAJDg5hATuJ1gfK IDVsAjoSp1YfYQSpERG4wyixZdYZRpAEs0CgxP7t19ghmn+wSOy//4wFJMEL1Dxx6Q4mEJtF QEVidfM7MFtUIEJi9fIXrBA1ghInZz4Bq+cUKJdo+nWDCWKousSfeZeYIWxxiaYvK1khbHmJ 5q2zmScwCs1C0j4LScssJC2zkLQsYGRZxSiSWlqcm55bbKhXnJhbXJqXrpecn7uJERiv2479 3LyD8dLG4EOMAhyMSjy8OxZujhZiTSwrrsw9xCjBwawkwtuXCBTiTUmsrEotyo8vKs1JLT7E aAr03ERmKdHkfGAqySuJNzQ1NLewNDQ3Njc2s1AS5z1vUBklJJCeWJKanZpakFoE08fEwSnV wOiy4rKO6Z0vvmVN78QWd8w6amTtxaUWc2eB5oSO0s2L4ywWKovHu5zrdMj5avNbfdqX39Or bLuNLedwPDtcX7Kj0ej1UTmn902hD19tFHu2VW/y7OoPRhc1WE9EK1cV1HKnur/ec3dDbakG 97qJhrof5rQKBLmm7Np97MHNZs4/v31WVKkJr1ZiKc5INNRiLipOBAAAWMfU7QIAAA== Message-Id: <20181003090454eucas1p180d79e1279ca4e1086b2447758686978~aDeOgna1t0120201202eucas1p1W@eucas1p1.samsung.com> X-CMS-MailID: 20181003090454eucas1p180d79e1279ca4e1086b2447758686978 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> <20181003083009eucas1p19f5d8234a3592b9e1753181d858a99aa~aC-5VXlJ51559815598eucas1p1k@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 09:04:56 -0000 On 03.10.2018 11:32, Ilya Maximets wrote: > 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. > Sorry. Please, ignore the text below. > 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; >>>>>> >>>> >>>> >> >> > >