From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f65.google.com (mail-it0-f65.google.com [209.85.214.65]) by dpdk.org (Postfix) with ESMTP id 851A558CB for ; Thu, 20 Sep 2018 08:34:40 +0200 (CEST) Received: by mail-it0-f65.google.com with SMTP id h20-v6so10970973itf.2 for ; Wed, 19 Sep 2018 23:34:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aGhVtsIX9vrI5d0OHIhyj84tV9Bgrknao+NOhp90zg8=; b=mfuN9fBENpDVeeUSf+sb3gxZUx0E9/DkHo9vYOgmh/mTUhiGwyxNtlS1J7jE3vNbEW 4c6rJBS5CClLK9xrBQFsCDnz2Wf/c/8lwuGSGjF0el58/M1ICE6gPaHhlM67f2eeRFV1 Wy8RH8kq+XeTPLeUxpx7qLfF5VmpP2zhdjyP3Eghhx9sHKOhtVeY2nkALpIlwrekbbAT 0IWPqoKtUmeIOgaMG4VGxjTZZauxeLgy2za1zAn+rpUp8LzQPf79mFxcx27y98NIyWwi cf89jPSCsvXzfOUZ/E5NobQ+03NF/9LfpH92guT2Ll5suvo8OKn76QgIrDUc4qO1FNcN FXAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aGhVtsIX9vrI5d0OHIhyj84tV9Bgrknao+NOhp90zg8=; b=O2WVNjOkE+lL2a89Nr8I1LECuRDnepvhDJFFT2B41opaUyNekjPjlubSziUHmiIDy5 5RDdKbQV3ydHPogHnTuDO+1gzsqKzT/L4kB9ad4BtjyZB/e7rXumeYEUA1QDH3nWDLdS 45tnCbl6fOmDu+zI+pJC7z4fK+sFeXyfBuzGx3dWJJTvumKI88PNcWiHF7iAcxiGQevM B1hdD8OmScFXHypOvH7Ollj0/HJnHd2NpsvHzku1ihdqy9UqmRes+y3lI+bR148HaVQQ 1dQ7npoyyQFG/kkS1dtImEH7srag8Jry9adbfN7ZQlKlhChXaptB76n/AACWRCCof0js ny2A== X-Gm-Message-State: APzg51CaEBIxb9ZXak9qJmIcfbeeKp3CSN9wyCIHlSyo65i+s+awmgOn 4+lbtmt4Ma8jiekqIJgCB22rdmUnzwcRGwlMpk0= X-Google-Smtp-Source: ANB0VdbO/6/D2o/cSbZ9u68G/fKYl7/uWh+ywUxfnekmSgtPuDEAXaNFpPvd52rD01zQvC9pV47qxrF3LkaoWde+vts= X-Received: by 2002:a24:6302:: with SMTP id j2-v6mr1006423itc.8.1537425279732; Wed, 19 Sep 2018 23:34:39 -0700 (PDT) MIME-Version: 1.0 References: <153698873346.23049.10966616323159180622.stgit@T460> <153698885466.23049.5627861471035122455.stgit@T460> In-Reply-To: From: Nikolay Nikolaev Date: Thu, 20 Sep 2018 09:34:03 +0300 Message-ID: To: maxime.coquelin@redhat.com Cc: tiwei.bie@intel.com, zhihong.wang@intel.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 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: Thu, 20 Sep 2018 06:34:40 -0000 On Wed, Sep 19, 2018 at 10:37 AM Maxime Coquelin wrote: > > > > On 09/15/2018 07:20 AM, 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. > > > > Failing vhost_user_set_features is fatal and all processing should > > stop immediately and propagate the error to the upper layers. Change > > the code accordingly to reflect that. > > > > Signed-off-by: Nikolay Nikolaev > > --- > > lib/librte_vhost/vhost_user.c | 161 +++++++++++++++++------------------------ > > 1 file changed, 66 insertions(+), 95 deletions(-) > > > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > > index 81c0396de..9004e1ac8 100644 > > --- a/lib/librte_vhost/vhost_user.c > > +++ b/lib/librte_vhost/vhost_user.c > > @@ -73,6 +73,8 @@ static const char *vhost_message_str[VHOST_USER_MAX] = { > > > > /* The possible results of a message handling function */ > > enum vh_result { > > + /* Message handling failed with an unrecoverable error */ > > + VH_RESULT_FATAL = -2, > > /* Message handling failed */ > > VH_RESULT_ERR = -1, > > /* Message handling successful */ > > @@ -206,7 +208,7 @@ vhost_user_set_features(struct virtio_net **pdev, VhostUserMsg *msg) > > RTE_LOG(ERR, VHOST_CONFIG, > > "(%d) received invalid negotiated features.\n", > > dev->vid); > > - return VH_RESULT_ERR; > > + return VH_RESULT_FATAL; > > } > > > > if (dev->flags & VIRTIO_DEV_RUNNING) { > > @@ -222,7 +224,7 @@ vhost_user_set_features(struct virtio_net **pdev, VhostUserMsg *msg) > > RTE_LOG(ERR, VHOST_CONFIG, > > "(%d) features changed while device is running.\n", > > dev->vid); > > - return VH_RESULT_ERR; > > + return VH_RESULT_FATAL; > > } > > > > if (dev->notify_ops->features_changed) > > @@ -1477,6 +1479,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) > > @@ -1629,6 +1659,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) > > @@ -1721,100 +1752,40 @@ 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); > > - 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); > > - if (ret) > > - goto skip_to_reply; > > - /* it needs a reply */ > > - 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); > > - if (ret) > > - goto skip_to_reply; > > - 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: > > - ret = -1; > > - break; > > + switch (ret) { > > + case VH_RESULT_FATAL: > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "Processing %s failed with an unrecoverable error.\n", > > + vhost_message_str[request]); > > + /* Propagate the error to the upper layer */ > > + return -1; > If I'm not mistaken, you are missing to release the lock if it was > taken, which could stuck the PMD threads: > > if (unlock_required) > vhost_user_unlock_all_queue_pairs(dev); > > Also the behavior is changed, as if the message for which handling > failed has the VHOST_USER_NEED_REPLY flag set, error is reported to the > master, and function returns 0: > > skip_to_reply: > if (unlock_required) > vhost_user_unlock_all_queue_pairs(dev); > > if (msg.flags & VHOST_USER_NEED_REPLY) { > msg.payload.u64 = !!ret; > msg.size = sizeof(msg.payload.u64); > send_vhost_reply(fd, &msg); > } else if (ret) { > RTE_LOG(ERR, VHOST_CONFIG, > "vhost message handling failed.\n"); > return -1; > } > Thanks Maxime. I'll revisit this patch and post new version in the next couple of day. Nikolay NIkolaev > > > + case VH_RESULT_ERR: > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "Processing %s failed.\n", > > + vhost_message_str[request]); > > + break; > > + 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 = VH_RESULT_ERR; > > } > > > > skip_to_post_handle: > >