From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id AC8A44C90 for ; Mon, 10 Sep 2018 18:08:06 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20180910160805euoutp01487d1a0d01dbbbaf4a3eb228b88ca5bc~TFaKFPFZE0680706807euoutp01Q for ; Mon, 10 Sep 2018 16:08:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20180910160805euoutp01487d1a0d01dbbbaf4a3eb228b88ca5bc~TFaKFPFZE0680706807euoutp01Q DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1536595685; bh=LhEA1b0WH8uKwCl0vXa1vlV7lhj5KTVCCBp5RNBTjhc=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=Ly1bPNYF566hX/W+qbsUP+Zr80Fs2QWDT1jyg/UUnNPYXJ/Eg5zzX2l96ujhN615y vtJfJySJV5WGuDcGESIvCXYhBMGF5q2U6RgIBv55z0BGzXY5J1GjuTMjLVYGrIHneR Tl9YBLbREyLWqZlT0cbOaZ3FdyI/fkPfEb4TaJ4A= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20180910160805eucas1p1dfc18c5b8c7b7b8752e2849f3bb68b60~TFaJmuKmC3157531575eucas1p1H; Mon, 10 Sep 2018 16:08:05 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 55.DC.04806.4E6969B5; Mon, 10 Sep 2018 17:08:04 +0100 (BST) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20180910160804eucas1p22e18ba6128dd3b5592cbca5867784749~TFaI2BMkC1648416484eucas1p2G; Mon, 10 Sep 2018 16:08:04 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20180910160804eusmtrp1a4792f9cc2a68a38a7d039f96619bfc6~TFaIlMrBk1253412534eusmtrp1Y; Mon, 10 Sep 2018 16:08:04 +0000 (GMT) X-AuditID: cbfec7f5-34dff700000012c6-22-5b9696e430be Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 2D.4F.04128.3E6969B5; Mon, 10 Sep 2018 17:08:03 +0100 (BST) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20180910160803eusmtip26b53d4c3f533fba5c69d17d391212949~TFaIKUYop3070130701eusmtip2b; Mon, 10 Sep 2018 16:08:03 +0000 (GMT) To: Maxime Coquelin , Nikolay Nikolaev , tiwei.bie@intel.com, zhihong.wang@intel.com Cc: dev@dpdk.org From: Ilya Maximets Date: Mon, 10 Sep 2018 19:09:45 +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: <1828a711-9db9-4408-c9e4-be49091e8be0@redhat.com> Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrOKsWRmVeSWpSXmKPExsWy7djP87pPpk2LNli4ycji3aftTBZX2n+y Wxzr3MNicXrhNRaLrQ3/mSw2X5zE5MDm8WvBUlaPnbPusnss3vOSyeP9vqtsHn1bVjEGsEZx 2aSk5mSWpRbp2yVwZfQ3nGQp6HWpmPrlDWsD42zrLkZODgkBE4mmhg0sXYxcHEICKxgltn2Y yw7hfGGU2Np5HSrzmVHi8dUTrDAtm2d+YIJILGeUOD7/M1TLR0aJX/+OsYBUCQuESnR+3wfW LiLQzSjxYPcpsASzgIDE43vPwEaxCehInFp9hBHEZhFQlbi4ahIziC0qECFx5MFCsDivgKDE yZlPwHo5BewkHtyZAjVHXKLpy0pWCFteonnrbGaI81axS0z/IQzRWyaxaNFFNoi4i8SmCZ9Z IGxhiVfHt7BD2DISpyf3QMXrJe63vGQEOVpCoINRYvqhf0wQCXuJLa/PATVwAC3TlFi/Sx8i 7CixsW8NWFhCgE/ixltBiHP4JCZtm84MEeaV6GgTgqhWkfh9cDnUlVISN999Zp/AqDQLyZOz kDw2C8ljsxD2LmBkWcUonlpanJueWmycl1quV5yYW1yal66XnJ+7iRGYfE7/O/51B+O+P0mH GAU4GJV4eC9kTYsWYk0sK67MPcQowcGsJMK7SwcoxJuSWFmVWpQfX1Sak1p8iFGag0VJnJdP Ky1aSCA9sSQ1OzW1ILUIJsvEwSnVwKhs8FjhSaKw7ZVHzHYVhS8Lvqxy9VUK4jd8Nyec4UKU 88uzh5YXcK7LnG80Kar0Nd+BHYeC3k4+lSB6+PCcgtzewlbRVL97BqJ+u/hFma+eM26W+u6e KjNdaZu74qG9qQFVu2XZZx46bHNCZ9Vk8777ryoz1qz90Lzef+HWzmzXo78lWFzzdyuxFGck GmoxFxUnAgBxvobcOgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrFIsWRmVeSWpSXmKPExsVy+t/xe7qPp02LNji1y9ri3aftTBZX2n+y Wxzr3MNicXrhNRaLrQ3/mSw2X5zE5MDm8WvBUlaPnbPusnss3vOSyeP9vqtsHn1bVjEGsEbp 2RTll5akKmTkF5fYKkUbWhjpGVpa6BmZWOoZGpvHWhmZKunb2aSk5mSWpRbp2yXoZfQ3nGQp 6HWpmPrlDWsD42zrLkZODgkBE4nNMz8wdTFycQgJLGWU+Lt7BitEQkrix68LULawxJ9rXWwQ Re8ZJe5d+sUMkhAWCJXYfmUNE4gtItDLKNG6JRbEZhYQkHh87xkrRMNfRomZZw6ygSTYBHQk Tq0+wghi8wrYScxpWQ5mswioSlxcNQlsqKhAhMTq5S9YIWoEJU7OfMICYnMC1T+4M4UFYoG6 xJ95l5ghbHGJpi8rWSFseYnmrbOZJzAKzULSPgtJyywkLbOQtCxgZFnFKJJaWpybnltspFec mFtcmpeul5yfu4kRGG/bjv3csoOx613wIUYBDkYlHt4LWdOihVgTy4orcw8xSnAwK4nw7tIB CvGmJFZWpRblxxeV5qQWH2I0BXpuIrOUaHI+MBXklcQbmhqaW1gamhubG5tZKInznjeojBIS SE8sSc1OTS1ILYLpY+LglGpgPNduYq97SDzzaIerc4hNyu8XYv9+dnzZ+rXvwbMe41Oef59P krRS2nm4V897SZ+Spanoz5Ub7/Q37Vh8buKj2Q8dJ7aVZd10uP3a+3dM1nuGLzfmSXqZHfu0 LOTVp9XbjG4fu2kgafJRbee/lzZ6kutOOShnxC3Y+Fz9xOYrRUmhJYGrnTVLtZVYijMSDbWY i4oTAU9wsX/NAgAA Message-Id: <20180910160804eucas1p22e18ba6128dd3b5592cbca5867784749~TFaI2BMkC1648416484eucas1p2G@eucas1p2.samsung.com> X-CMS-MailID: 20180910160804eucas1p22e18ba6128dd3b5592cbca5867784749 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20180910153230epcas5p28b552089ca3472806f4442e47a464543 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180910153230epcas5p28b552089ca3472806f4442e47a464543 References: <153202755842.21481.1772155561595981441.stgit@T460> <153202763503.21481.6074577075024787339.stgit@T460> <1828a711-9db9-4408-c9e4-be49091e8be0@redhat.com> 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:08:06 -0000 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. So, what's the plan? What do you think? 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. >> -        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? >> +        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: >> > >