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 183094C94 for ; Mon, 4 Mar 2019 17:25:03 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20190304162502euoutp022efc1fe88ff4b2fca3a64c15ab9f1ffe~Izh6XGrft0627206272euoutp02D for ; Mon, 4 Mar 2019 16:25:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20190304162502euoutp022efc1fe88ff4b2fca3a64c15ab9f1ffe~Izh6XGrft0627206272euoutp02D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1551716702; bh=4/+sA7fD9H1A8wLsyI/NcX9Vx6DgVwvHqQMJz31+uK0=; h=Subject:To:From:Date:In-Reply-To:References:From; b=gtCQAP6s4QYup3xauMxDvSxwXwk+VSZJKSkE0KU8BRe64adAD1htzMQTsZaEq3e/4 2Nz7q6UInf+ViFBd3a97QieOR92aINm5PYv5605msvzWaEm1iqEe11RWiI8L+EiPrI F1biEBsXPC2iiQRg0cJZCZtXog4E5ecixIRHGa80= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20190304162501eucas1p2a1d586e4d9b4d7b45bd5d00c3f329a2b~Izh5yjA5P2173621736eucas1p2J; Mon, 4 Mar 2019 16:25:01 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id F1.D5.04806.D515D7C5; Mon, 4 Mar 2019 16:25:01 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20190304162501eucas1p2f064542b682bbcec14e546517f888933~Izh5FFTXG2174621746eucas1p2G; Mon, 4 Mar 2019 16:25:01 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190304162500eusmtrp289c6abb5948233e3f33412292c9da5f0~Izh42McxL2062220622eusmtrp2e; Mon, 4 Mar 2019 16:25:00 +0000 (GMT) X-AuditID: cbfec7f5-367ff700000012c6-1b-5c7d515d6be2 Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 7F.23.04284.C515D7C5; Mon, 4 Mar 2019 16:25:00 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20190304162500eusmtip119c71fcbda9e9832133dd0e938fabd97~Izh4czAqv2783927839eusmtip1N; Mon, 4 Mar 2019 16:25:00 +0000 (GMT) To: Maxime Coquelin , dev@dpdk.org, changpeng.liu@intel.com, tiwei.bie@intel.com From: Ilya Maximets Message-ID: <17baeb16-35b0-192e-01e9-d8711cdcc4c0@samsung.com> Date: Mon, 4 Mar 2019 19:24:59 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprDKsWRmVeSWpSXmKPExsWy7djP87qxgbUxBmfnKlhc/PmV3eLdp+1M Flfaf7JbHOvcw2KxteE/kwOrx68FS1k9Fu95yeTxft9VNo++LasYA1iiuGxSUnMyy1KL9O0S uDLOzDvCVvAmu2LPoo/MDYxLY7oYOTkkBEwk7j2YxtrFyMUhJLCCUeL7ix42COcLo8SyeX+Y IJzPjBIdu44xw7Q8vviOBSKxnFFiZXcrO0hCSOAjo0TX/joQW1ggRGLT4sOMILaIQJ7E3FOn mEBsNgEdiVOrj4DFeQXsJNa/uA9mswioSBx7vgTMFhWIkHj/dDcLRI2gxMmZT8BsTqD66Zf2 gtUwC4hLNH1ZyQphy0s0b53NDHKQhEA3u8Ty699ZIC51kTj+oJMdwhaWeHV8C5QtI/F/53wm CLte4n7LS0aI5g5GiemH/kEl7CW2vD4H1MABtEFTYv0ufYiwo8T9JQ9YQMISAnwSN94KQtzA JzFp23RmiDCvREebEES1isTvg8uh4SYlcfPdZ6gLPCSO/J/FPIFRcRaSL2ch+WwWks9mIdyw gJFlFaN4amlxbnpqsXFearlecWJucWleul5yfu4mRmCaOf3v+NcdjPv+JB1iFOBgVOLhzTCr jRFiTSwrrsw9xCjBwawkwmvmABTiTUmsrEotyo8vKs1JLT7EKM3BoiTOW83wIFpIID2xJDU7 NbUgtQgmy8TBKdXAaC1o5nYzblmE8LXIumzLjG91RquXrplz12/L23l6KxfoOTd9Obokubn0 3ofPobdvrmP9eUiXV+nbO5/7pXpPT25/zL414i37p9Uri9LmTZLjc4+Z97KqcO+nHaJRU8Tl agVtnypIlRZ116s/nr9p07t9f2IU9uiYzaieevHmTMdP/menTZjrGaHEUpyRaKjFXFScCAAi yNbDLwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMIsWRmVeSWpSXmKPExsVy+t/xu7oxgbUxBsuu61pc/PmV3eLdp+1M Flfaf7JbHOvcw2KxteE/kwOrx68FS1k9Fu95yeTxft9VNo++LasYA1ii9GyK8ktLUhUy8otL bJWiDS2M9AwtLfSMTCz1DI3NY62MTJX07WxSUnMyy1KL9O0S9DLOzDvCVvAmu2LPoo/MDYxL Y7oYOTkkBEwkHl98x9LFyMUhJLCUUaL7yQtmiISUxI9fF1ghbGGJP9e62CCK3jNK/Lq/FSwh LBAisWnxYUYQW0QgT+LY7SdgzUICm5gkJv5VALHZBHQkTq0+AlbDK2Ansf7FfTCbRUBF4tjz JWC2qECExN2LL1ggagQlTs58AmZzAtVPv7QXrIZZQF3iz7xLzBC2uETTl5WsELa8RPPW2cwT GAVnIWmfhaRlFpKWWUhaFjCyrGIUSS0tzk3PLTbUK07MLS7NS9dLzs/dxAiMoG3Hfm7ewXhp Y/AhRgEORiUe3gyz2hgh1sSy4srcQ4wSHMxKIrxmDkAh3pTEyqrUovz4otKc1OJDjKZAz01k lhJNzgdGd15JvKGpobmFpaG5sbmxmYWSOO95g8ooIYH0xJLU7NTUgtQimD4mDk6pBsapr4/e eODgfeeDW4hzhuIR5YR+6U6p18HuiVValx8dPvE/VH9iYqplda3uh76C9gsFMxw2r1/nkR5s 4DL95ZPYD7xxCYEBrTO43wUzdslrd22a9vPayoeTjpy3KGg73fRixZqNLT90Pup8+BWxM+bp 5AVTi6Ymsai+19ax12y5e1zRwc5z5yElluKMREMt5qLiRABftLUVtgIAAA== X-CMS-MailID: 20190304162501eucas1p2f064542b682bbcec14e546517f888933 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20190228153153epcas4p166039186da1d9cf15a34da5d3296ba0d X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20190228153153epcas4p166039186da1d9cf15a34da5d3296ba0d References: <20190228153134.31865-1-maxime.coquelin@redhat.com> <20190228153134.31865-3-maxime.coquelin@redhat.com> <37a4d589-8094-9c03-244e-cf2710bfee4c@samsung.com> Subject: Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend 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: Mon, 04 Mar 2019 16:25:04 -0000 On 04.03.2019 19:02, Maxime Coquelin wrote: > > > On 3/4/19 4:25 PM, Ilya Maximets wrote: >> On 28.02.2019 18:31, Maxime Coquelin wrote: >>> External backends may have specific requests to handle, and so >>> we don't want the vhost-user lib to handle these requests as >>> errors. >>> >>> This patch also changes the experimental API by introducing >>> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib >>> can report an error if a message is handled neither by >>> the vhost-user library nor by the external backend. >>> >>> The logic changes a bit so that if the callback returns >>> with ERR, OK or REPLY, it is considered the message >>> is handled by the external backend so it won't be >>> handled by the vhost-user library. >>> It is still possible for an external backend to listen >>> to requests that have to be handled by the vhost-user >>> library like SET_MEM_TABLE, but the callback have to >>> return NOT_HANDLED in that case. >>> >>> Suggested-by: Ilya Maximets >>> Signed-off-by: Maxime Coquelin >>> --- >>>   lib/librte_vhost/rte_vhost.h  | 16 +++++--- >>>   lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------ >>>   2 files changed, 60 insertions(+), 31 deletions(-) >>> >>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h >>> index c9c392975..b1c5a0908 100644 >>> --- a/lib/librte_vhost/rte_vhost.h >>> +++ b/lib/librte_vhost/rte_vhost.h >>> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result { >>>       RTE_VHOST_MSG_RESULT_OK =  0, >>>       /* Message handling successful and reply prepared */ >>>       RTE_VHOST_MSG_RESULT_REPLY =  1, >>> +    /* Message not handled */ >>> +    RTE_VHOST_MSG_RESULT_NOT_HANDLED, >>>   }; >>>     /** >>> @@ -135,11 +137,13 @@ enum rte_vhost_msg_result { >>>    *  If the handler requires skipping the master message handling, this variable >>>    *  shall be written 1, otherwise 0. >>>    * @return >>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, >>> - *  VH_RESULT_ERR on failure >>> + *  RTE_VHOST_MSG_RESULT_OK on success, >>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply, >>> + *  RTE_VHOST_MSG_RESULT_ERR on failure, >>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled. >>>    */ >>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid, >>> -        void *msg, uint32_t *skip_master); >>> +        void *msg); >>>     /** >>>    * Function prototype for the vhost backend to handler specific vhost user >>> @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid, >>>    * @param msg >>>    *  Message pointer. >>>    * @return >>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, >>> - *  VH_RESULT_ERR on failure >>> + *  RTE_VHOST_MSG_RESULT_OK on success, >>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply, >>> + *  RTE_VHOST_MSG_RESULT_ERR on failure, >>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled. >>>    */ >>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, >>>           void *msg); >> >> According to above definition, we should make corresponding change in vhost_crypto. >> Something like this: >> --- >> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c >> index 0f437c4a1..f0eedd422 100644 >> --- a/lib/librte_vhost/vhost_crypto.c >> +++ b/lib/librte_vhost/vhost_crypto.c >> @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg) >>           return RTE_VHOST_MSG_RESULT_ERR; >>       } >>   -    if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) { >> +    switch (vmsg->request.master) { >> +    VHOST_USER_CRYPTO_CREATE_SESS: >>           vhost_crypto_create_sess(vcrypto, >>                   &vmsg->payload.crypto_session); >>           vmsg->fd_num = 0; >>           ret = RTE_VHOST_MSG_RESULT_REPLY; >> -    } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) { >> +        break; >> +    VHOST_USER_CRYPTO_CLOSE_SESS: >>           if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64)) >>               ret = RTE_VHOST_MSG_RESULT_ERR; >> +        break; >> +    default: >> +        ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED; >> +        break; >>       } >>         return ret; >> --- > > Indeed, it will be part of v1 if Changpeng confirms this RFC is working > for his usecase. > >> >> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >>> index 36c0c676d..ca9167f1d 100644 >>> --- a/lib/librte_vhost/vhost_user.c >>> +++ b/lib/librte_vhost/vhost_user.c >>> @@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd) >>>       int did = -1; >>>       int ret; >>>       int unlock_required = 0; >>> -    uint32_t skip_master = 0; >>> +    bool handled; >> >> In below code 'handled' equals to 'false' only if 'ret' equals to >> 'RTE_VHOST_MSG_RESULT_NOT_HANDLED'. Looks like we don't need this >> variable. > > Actually I think it is necessary, more below. > >> >>>       int request; >>>         dev = get_device(vid); >>> @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd) >>>       } >>>         ret = read_vhost_message(fd, &msg); >>> -    if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) { >>> +    if (ret <= 0) { >>>           if (ret < 0) >>>               RTE_LOG(ERR, VHOST_CONFIG, >>>                   "vhost read message failed\n"); >>> -        else if (ret == 0) >>> +        else >>>               RTE_LOG(INFO, VHOST_CONFIG, >>>                   "vhost peer closed\n"); >>> -        else >>> -            RTE_LOG(ERR, VHOST_CONFIG, >>> -                "vhost read incorrect message\n"); >>>             return -1; >>>       } >>>         ret = 0; >>> -    if (msg.request.master != VHOST_USER_IOTLB_MSG) >>> -        RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", >>> -            vhost_message_str[msg.request.master]); >>> -    else >>> -        RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", >>> -            vhost_message_str[msg.request.master]); >>> +    request = msg.request.master; >>> +    if (request < VHOST_USER_MAX && vhost_message_str[request]) { >> >> We probably need to check for 'request > VHOST_USER_NONE' because it >> has signed type. > > Agree. > >> BTW, do we heed to allow requests out of (VHOST_USER_NONE, VHOST_USER_MAX) >> range? This 'if' statement reports them as 'External' requests. >> However, the 'master' 'if' statement will treat them as error, printing >> "Requested invalid message type". >> >> If we don't need to handle messages out of our range, we could check the >> range once at the top of this function and never check again. > > I think we need to handle messages out of range, otherwise external > backend may not implement new requests without patch dpdk first. > > Regarding "Requested invalid message type", I think it should just be > removed. Agree. > This version assumes the external backend will implement the > 'pre' callback for its specific requests, but this is an uneeded > limitation and could implmeent the 'post' callback only. Sure. vhost_crypto has only post handler. > >>> +        if (request != VHOST_USER_IOTLB_MSG) >>> +            RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", >>> +                vhost_message_str[request]); >>> +        else >>> +            RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", >>> +                vhost_message_str[request]); >>> +    } else { >>> +        RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request); >>> +    } >>>         ret = vhost_user_check_and_alloc_queue_pair(dev, &msg); >>>       if (ret < 0) { >>> @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd) >>>        * inactive, so it is safe. Otherwise taking the access_lock >>>        * would cause a dead lock. >>>        */ >>> -    switch (msg.request.master) { >>> +    switch (request) { >>>       case VHOST_USER_SET_FEATURES: >>>       case VHOST_USER_SET_PROTOCOL_FEATURES: >>>       case VHOST_USER_SET_OWNER: >>> @@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd) >>>         } >>>   +    handled = false; >> >> 'ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED' instead. >> >>>       if (dev->extern_ops.pre_msg_handle) { >>>           ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, >>> -                (void *)&msg, &skip_master); >>> -        if (ret == RTE_VHOST_MSG_RESULT_ERR) >>> -            goto skip_to_reply; >>> -        else if (ret == RTE_VHOST_MSG_RESULT_REPLY) >>> +                (void *)&msg); >>> +        switch (ret) { >>> +        case RTE_VHOST_MSG_RESULT_REPLY: >>>               send_vhost_reply(fd, &msg); >>> - >>> -        if (skip_master) >>> +        case RTE_VHOST_MSG_RESULT_ERR: >>> +        case RTE_VHOST_MSG_RESULT_OK: >>> +            handled = true; >>>               goto skip_to_post_handle; >>> +        case RTE_VHOST_MSG_RESULT_NOT_HANDLED: >>> +        default: >>> +            break; >>> +        } >>>       } >>>   -    request = msg.request.master; >>>       if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { >>>           if (!vhost_message_handlers[request]) >>>               goto skip_to_post_handle; >>> @@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd) >>>               RTE_LOG(ERR, VHOST_CONFIG, >>>                   "Processing %s failed.\n", >>>                   vhost_message_str[request]); >>> +            handled = true; >>>               break; >>>           case RTE_VHOST_MSG_RESULT_OK: >>>               RTE_LOG(DEBUG, VHOST_CONFIG, >>>                   "Processing %s succeeded.\n", >>>                   vhost_message_str[request]); >>> +            handled = true; >>>               break; >>>           case RTE_VHOST_MSG_RESULT_REPLY: >>>               RTE_LOG(DEBUG, VHOST_CONFIG, >>>                   "Processing %s succeeded and needs reply.\n", >>>                   vhost_message_str[request]); >>>               send_vhost_reply(fd, &msg); >>> +            handled = true; >>> +            break; >>> +        default: >>>               break; >>>           } >>>       } else { >>> @@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd) >>>   skip_to_post_handle: >>>       if (ret != RTE_VHOST_MSG_RESULT_ERR && >>>               dev->extern_ops.post_msg_handle) { >>> -        ret = (*dev->extern_ops.post_msg_handle)( >>> -                dev->vid, (void *)&msg); >>> -        if (ret == RTE_VHOST_MSG_RESULT_ERR) >>> -            goto skip_to_reply; >>> -        else if (ret == RTE_VHOST_MSG_RESULT_REPLY) >>> +        ret = (*dev->extern_ops.post_msg_handle)(dev->vid, >>> +                (void *)&msg); >>> +        switch (ret) { >>> +        case RTE_VHOST_MSG_RESULT_REPLY: >>>               send_vhost_reply(fd, &msg); >>> +        case RTE_VHOST_MSG_RESULT_ERR: >>> +        case RTE_VHOST_MSG_RESULT_OK: >>> +            handled = true; >>> +        case RTE_VHOST_MSG_RESULT_NOT_HANDLED: >>> +        default: >>> +            break; >>> +        } >>>       } >>>   -skip_to_reply: >>>       if (unlock_required) >>>           vhost_user_unlock_all_queue_pairs(dev); >>>   +    /* If message was not handled at this stage, treat it as an error */ >>> +    if (!handled) { >> >> if (ret == RTE_VHOST_MSG_RESULT_NOT_HANDLED) > > I added 'handled' variable because ret can be > RTE_VHOST_MSG_RESULT_NOT_HANDLED at this stage but the request has been > handled. > > For example, vhost-user library handles the request and the external > backend implements post_msg_handle callback. If the external backend > callback does not handle this psecific request, results will be > RTE_VHOST_MSG_RESULT_NOT_HANDLED. > > So 'handled' is set to true as soon as one of the 3 possible ways to > handle the request (.pre, vhost-lib, .post) handles it. Oh. I see. Thanks. > >>> +        RTE_LOG(ERR, VHOST_CONFIG, >>> +            "vhost message (req: %d) was not handled.\n", request); >>> +        ret = RTE_VHOST_MSG_RESULT_ERR; >>> +    } >>> + >>>       /* >>>        * If the request required a reply that was already sent, >>>        * this optional reply-ack won't be sent as the >>> > >