From: Ilya Maximets <i.maximets@samsung.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
dev@dpdk.org, changpeng.liu@intel.com, tiwei.bie@intel.com
Subject: Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend
Date: Mon, 4 Mar 2019 18:25:18 +0300 [thread overview]
Message-ID: <37a4d589-8094-9c03-244e-cf2710bfee4c@samsung.com> (raw)
In-Reply-To: <20190228153134.31865-3-maxime.coquelin@redhat.com>
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 <i.maximets@samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> 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;
---
> 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.
> 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.
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.
> + 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)
> + 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
>
next prev parent reply other threads:[~2019-03-04 15:25 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-28 15:31 [dpdk-dev] [RFC v2 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin
2019-02-28 15:31 ` [dpdk-dev] [RFC v2 1/2] vhost: add API to set protocol features flags Maxime Coquelin
2019-03-08 9:13 ` Stojaczyk, Dariusz
2019-02-28 15:31 ` [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend Maxime Coquelin
2019-02-28 16:56 ` Maxime Coquelin
2019-03-04 15:25 ` Ilya Maximets [this message]
2019-03-04 16:02 ` Maxime Coquelin
2019-03-04 16:24 ` Ilya Maximets
2019-03-08 9:18 ` Stojaczyk, Dariusz
2019-03-08 10:01 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=37a4d589-8094-9c03-244e-cf2710bfee4c@samsung.com \
--to=i.maximets@samsung.com \
--cc=changpeng.liu@intel.com \
--cc=dev@dpdk.org \
--cc=maxime.coquelin@redhat.com \
--cc=tiwei.bie@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).