From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 4AA5C4C90 for ; Mon, 10 Sep 2018 18:43:22 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9329D4555F; Mon, 10 Sep 2018 16:43:21 +0000 (UTC) Received: from [10.36.112.46] (ovpn-112-46.ams2.redhat.com [10.36.112.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A699B2027EA4; Mon, 10 Sep 2018 16:43:19 +0000 (UTC) To: Ilya Maximets , Nikolay Nikolaev , tiwei.bie@intel.com, zhihong.wang@intel.com Cc: dev@dpdk.org References: <153202755842.21481.1772155561595981441.stgit@T460> <153202763503.21481.6074577075024787339.stgit@T460> <1828a711-9db9-4408-c9e4-be49091e8be0@redhat.com> <20180910160804eucas1p22e18ba6128dd3b5592cbca5867784749~TFaI2BMkC1648416484eucas1p2G@eucas1p2.samsung.com> From: Maxime Coquelin Message-ID: Date: Mon, 10 Sep 2018 18:43:17 +0200 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: <20180910160804eucas1p22e18ba6128dd3b5592cbca5867784749~TFaI2BMkC1648416484eucas1p2G@eucas1p2.samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 10 Sep 2018 16:43:21 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 10 Sep 2018 16:43:21 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array 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, 10 Sep 2018 16:43:22 -0000 Hi Ilya, On 09/10/2018 06:09 PM, Ilya Maximets wrote: > Hi Maxime, > Thanks for pointing to this patch set. I missed it somehow. > > This patch could not replace mine [1], because it does not improve > the message handling from the error handling point of view. But > it completely changes the code, so we need to negotiate the order > in which they will be applied or combine them somehow. By complementary I meant both of your patches add similar things, like vhost_user_set_vring_kick() becoming an int (even if Nikolay's version doesn't return an error for now). > > So, what's the plan? What do you think? On one hand Nikolay's series was posted first, and it might be easier to just apply your patch on top. On the other hand, your patch fixes an issue, so it will be easier to backport it if it is applied first. I guess we should go for your patch first to ease backporting. I'd like Tiwei's feedback before applying anything, as he reviewed the first versions of both yours and Nikolay's patches. > Few comments inline. > > [1] http://patchwork.dpdk.org/patch/44168/ > > Best regards, Ilya Maximets. > > On 10.09.2018 18:32, Maxime Coquelin wrote: >> >> >> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote: >>> Introduce vhost_message_handlers, which maps the message request >>> type to the message handler. Then replace the switch construct >>> with a map and call. >>> >>> Signed-off-by: Nikolay Nikolaev >>> --- >>>   lib/librte_vhost/vhost_user.c |  143 +++++++++++++++-------------------------- >>>   1 file changed, 54 insertions(+), 89 deletions(-) >> >> Reviewed-by: Maxime Coquelin >> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >>> index 6b39d1e30..1b164df27 100644 >>> --- a/lib/librte_vhost/vhost_user.c >>> +++ b/lib/librte_vhost/vhost_user.c >>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg) >>>       return VH_RESULT_OK; >>>   } >>>   +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg); >>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = { >>> +    [VHOST_USER_NONE] = NULL, >>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features, >>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features, >>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner, >>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner, >>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table, >>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base, >>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd, >>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num, >>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr, >>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base, >>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base, >>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick, >>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call, >>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err, >>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features, >>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features, >>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num, >>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable, >>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp, >>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu, >>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd, >>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg, >>> +}; >>> + >>> + >>>   /* return bytes# of read on success or negative val on failure. */ >>>   static int >>>   read_vhost_message(int sockfd, VhostUserMsg *msg) >>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd) >>>       int ret; >>>       int unlock_required = 0; >>>       uint32_t skip_master = 0; >>> +    int request; >>>         dev = get_device(vid); >>>       if (dev == NULL) >>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd) >>>               goto skip_to_post_handle; >>>       } >>>   -    switch (msg.request.master) { >>> -    case VHOST_USER_GET_FEATURES: >>> -        ret = vhost_user_get_features(&dev, &msg); >>> -        send_vhost_reply(fd, &msg); >>> -        break; >>> -    case VHOST_USER_SET_FEATURES: >>> -        ret = vhost_user_set_features(&dev, &msg); >>> -        if (ret) >>> -            return -1; > > You can not just remove this. Disconnection was triggered here > because the error is unrecoverable. > See 59fe5e17d930 ("vhost: propagate set features handling error") > for details. Right, maybe adding another enum for unrecoverable errors would do the trick. >>> -        break; >>> - >>> -    case VHOST_USER_GET_PROTOCOL_FEATURES: >>> -        ret = vhost_user_get_protocol_features(&dev, &msg); >>> -        send_vhost_reply(fd, &msg); >>> -        break; >>> -    case VHOST_USER_SET_PROTOCOL_FEATURES: >>> -        ret = vhost_user_set_protocol_features(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_OWNER: >>> -        ret = vhost_user_set_owner(&dev, &msg); >>> -        break; >>> -    case VHOST_USER_RESET_OWNER: >>> -        ret = vhost_user_reset_owner(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_MEM_TABLE: >>> -        ret = vhost_user_set_mem_table(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_LOG_BASE: >>> -        ret = vhost_user_set_log_base(&dev, &msg); >>> -        send_vhost_reply(fd, &msg); >>> -        break; >>> -    case VHOST_USER_SET_LOG_FD: >>> -        ret = vhost_user_set_log_fd(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_VRING_NUM: >>> -        ret = vhost_user_set_vring_num(&dev, &msg); >>> -        break; >>> -    case VHOST_USER_SET_VRING_ADDR: >>> -        ret = vhost_user_set_vring_addr(&dev, &msg); >>> -        break; >>> -    case VHOST_USER_SET_VRING_BASE: >>> -        ret = vhost_user_set_vring_base(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_GET_VRING_BASE: >>> -        ret = vhost_user_get_vring_base(&dev, &msg); >>> -        send_vhost_reply(fd, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_VRING_KICK: >>> -        ret = vhost_user_set_vring_kick(&dev, &msg); >>> -        break; >>> -    case VHOST_USER_SET_VRING_CALL: >>> -        ret = vhost_user_set_vring_call(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_VRING_ERR: >>> -        ret = vhost_user_set_vring_err(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_GET_QUEUE_NUM: >>> -        ret = vhost_user_get_queue_num(&dev, &msg); >>> -        send_vhost_reply(fd, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_VRING_ENABLE: >>> -        ret = vhost_user_set_vring_enable(&dev, &msg); >>> -        break; >>> -    case VHOST_USER_SEND_RARP: >>> -        ret = vhost_user_send_rarp(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_NET_SET_MTU: >>> -        ret = vhost_user_net_set_mtu(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_SET_SLAVE_REQ_FD: >>> -        ret = vhost_user_set_req_fd(&dev, &msg); >>> -        break; >>> - >>> -    case VHOST_USER_IOTLB_MSG: >>> -        ret = vhost_user_iotlb_msg(&dev, &msg); >>> -        break; >>> +    request = msg.request.master; >>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { >>> +        if (!vhost_message_handlers[request]) >>> +            goto skip_to_post_handle; >>> +        ret = vhost_message_handlers[request](&dev, &msg); >>>   -    default: >>> +        switch (ret) { >>> +        case VH_RESULT_ERR: >>> +            RTE_LOG(ERR, VHOST_CONFIG, >>> +                "Processing %s failed.\n", >>> +                vhost_message_str[request]); > > I guess 'break' is missing here. Isn't it? Good catch. > >>> +        case VH_RESULT_OK: >>> +            RTE_LOG(DEBUG, VHOST_CONFIG, >>> +                "Processing %s succeeded.\n", >>> +                vhost_message_str[request]); >>> +            break; >>> +        case VH_RESULT_REPLY: >>> +            RTE_LOG(INFO, VHOST_CONFIG, >>> +                "Processing %s succeeded and needs reply.\n", >>> +                vhost_message_str[request]); >>> +            send_vhost_reply(fd, &msg); >>> +            break; >>> +        } >>> +    } else { >>> +        RTE_LOG(ERR, VHOST_CONFIG, >>> +            "Requested invalid message type %d.\n", request); >>>           ret = -1; >>> -        break; >>>       } >>>     skip_to_post_handle: >>> >> >>