* [dpdk-dev] [PATCH v2 0/2] vhost: Support external backend only vhost-user requests @ 2019-03-13 15:55 Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 1/2] vhost: add API to set protocol features flags Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend Maxime Coquelin 0 siblings, 2 replies; 6+ messages in thread From: Maxime Coquelin @ 2019-03-13 15:55 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets, dariusz.stojaczyk Cc: Maxime Coquelin The goals of this series is to provide more flexibility to external backends to implement their specific vhost-user request handling without having to patch vhost-user library. First patch implements a new API for external backend to advertize its specific protocol features to vhost-user master. Second patch ensures a request not handled by the vhost-user library but by the external backend only will not be treated as an error or make the vhost lib to crash. Changes in v2: ============== - Remove 'skip_master' param from documentation (Ilya) - Merge pre and post callback in a single one (Ilya) Changes in v1: ============== - Convert vhost-crypto to the API change (Ilya) - Fix build with recent GCC (fall-through) - Check request > VHOST_USER_NONE (Ilya) - Remove 'Requested invalid message type' check (Ilya) Changes in RFC v2: ================== - Fix build & git message - Add new enum for not handled messages & remove skip_master (Ilya) Maxime Coquelin (2): vhost: add API to set protocol features flags vhost: support requests only handled by external backend lib/librte_vhost/rte_vhost.h | 47 ++++++++------- lib/librte_vhost/rte_vhost_version.map | 1 + lib/librte_vhost/socket.c | 14 +++++ lib/librte_vhost/vhost_crypto.c | 10 +++- lib/librte_vhost/vhost_user.c | 82 ++++++++++++++++---------- 5 files changed, 98 insertions(+), 56 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2 1/2] vhost: add API to set protocol features flags 2019-03-13 15:55 [dpdk-dev] [PATCH v2 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin @ 2019-03-13 15:55 ` Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend Maxime Coquelin 1 sibling, 0 replies; 6+ messages in thread From: Maxime Coquelin @ 2019-03-13 15:55 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets, dariusz.stojaczyk Cc: Maxime Coquelin rte_vhost_driver_set_protocol_features API is to be used by external backends to advertize vhost-user protocol features it supports. It has to be called after rte_vhost_driver_register() and before rte_vhost_driver_start(). Example of usage to advertize VHOST_USER_PROTOCOL_F_FOOBAR protocol feature: const char *path = "/tmp/vhost-user"; uint64_t protocol_features; rte_vhost_driver_register(path, 0); rte_vhost_driver_get_protocol_features(path, &protocol_features); protocol_features |= VHOST_USER_PROTOCOL_F_FOOBAR; rte_vhost_driver_set_protocol_features(path, protocol_features); rte_vhost_driver_start(path); Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com> --- lib/librte_vhost/rte_vhost.h | 14 ++++++++++++++ lib/librte_vhost/rte_vhost_version.map | 1 + lib/librte_vhost/socket.c | 14 ++++++++++++++ 3 files changed, 29 insertions(+) diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h index 2753670a2..c9c392975 100644 --- a/lib/librte_vhost/rte_vhost.h +++ b/lib/librte_vhost/rte_vhost.h @@ -405,6 +405,20 @@ int rte_vhost_driver_disable_features(const char *path, uint64_t features); */ int rte_vhost_driver_get_features(const char *path, uint64_t *features); +/** + * Set the protocol feature bits before feature negotiation. + * + * @param path + * The vhost-user socket file path + * @param protocol_features + * Supported protocol features + * @return + * 0 on success, -1 on failure + */ +int __rte_experimental +rte_vhost_driver_set_protocol_features(const char *path, + uint64_t protocol_features); + /** * Get the protocol feature bits before feature negotiation. * diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map index 8a3bc19e0..5f1d4a75c 100644 --- a/lib/librte_vhost/rte_vhost_version.map +++ b/lib/librte_vhost/rte_vhost_version.map @@ -86,4 +86,5 @@ EXPERIMENTAL { rte_vhost_host_notifier_ctrl; rte_vdpa_relay_vring_used; rte_vhost_extern_callback_register; + rte_vhost_driver_set_protocol_features; }; diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c index 9883b0491..3da9de62c 100644 --- a/lib/librte_vhost/socket.c +++ b/lib/librte_vhost/socket.c @@ -707,6 +707,20 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features) return ret; } +int +rte_vhost_driver_set_protocol_features(const char *path, + uint64_t protocol_features) +{ + struct vhost_user_socket *vsocket; + + pthread_mutex_lock(&vhost_user.mutex); + vsocket = find_vhost_user_socket(path); + if (vsocket) + vsocket->protocol_features = protocol_features; + pthread_mutex_unlock(&vhost_user.mutex); + return vsocket ? 0 : -1; +} + int rte_vhost_driver_get_protocol_features(const char *path, uint64_t *protocol_features) -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend 2019-03-13 15:55 [dpdk-dev] [PATCH v2 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 1/2] vhost: add API to set protocol features flags Maxime Coquelin @ 2019-03-13 15:55 ` Maxime Coquelin 2019-03-13 16:09 ` Ilya Maximets 1 sibling, 1 reply; 6+ messages in thread From: Maxime Coquelin @ 2019-03-13 15:55 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets, dariusz.stojaczyk Cc: Maxime Coquelin 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. Vhost-crypto backend is ialso adapted to this API change. Suggested-by: Ilya Maximets <i.maximets@samsung.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> --- lib/librte_vhost/rte_vhost.h | 33 ++++--------- lib/librte_vhost/vhost_crypto.c | 10 +++- lib/librte_vhost/vhost_user.c | 82 +++++++++++++++++++++------------ 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h index c9c392975..8ab4ff299 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, }; /** @@ -131,37 +133,20 @@ enum rte_vhost_msg_result { * vhost device id * @param msg * Message pointer. - * @param skip_master - * 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); - -/** - * Function prototype for the vhost backend to handler specific vhost user - * messages after the master message handling is done - * - * @param vid - * vhost device id - * @param msg - * Message pointer. - * @return - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, - * VH_RESULT_ERR on failure - */ -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, - void *msg); +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg); /** * Optional vhost user message handlers. */ struct rte_vhost_user_extern_ops { - rte_vhost_msg_pre_handle pre_msg_handle; - rte_vhost_msg_post_handle post_msg_handle; + rte_vhost_msg_handle pre_msg_handle; + rte_vhost_msg_handle post_msg_handle; }; /** diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c index 0f437c4a1..9b4b850e8 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) { + case 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; + case 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 555d09ad9..39756fce7 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1910,7 +1910,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; int request; dev = get_device(vid); @@ -1928,27 +1928,30 @@ 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_NONE && request < VHOST_USER_MAX && + vhost_message_str[request]) { + 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(DEBUG, VHOST_CONFIG, "External request %d\n", request); + } ret = vhost_user_check_and_alloc_queue_pair(dev, &msg); if (ret < 0) { @@ -1964,7 +1967,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: @@ -1989,19 +1992,24 @@ vhost_user_msg_handler(int vid, int fd) } + handled = false; 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) + /* Fall-through */ + 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; @@ -2012,40 +2020,54 @@ 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 { - RTE_LOG(ERR, VHOST_CONFIG, - "Requested invalid message type %d.\n", request); - ret = RTE_VHOST_MSG_RESULT_ERR; } 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); + /* Fall-through */ + 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) { + 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 -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend Maxime Coquelin @ 2019-03-13 16:09 ` Ilya Maximets 2019-03-19 10:15 ` Maxime Coquelin 0 siblings, 1 reply; 6+ messages in thread From: Ilya Maximets @ 2019-03-13 16:09 UTC (permalink / raw) To: Maxime Coquelin, dev, changpeng.liu, tiwei.bie, dariusz.stojaczyk On 13.03.2019 18:55, 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. > > Vhost-crypto backend is ialso adapted to this API change. > > Suggested-by: Ilya Maximets <i.maximets@samsung.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> > --- > lib/librte_vhost/rte_vhost.h | 33 ++++--------- > lib/librte_vhost/vhost_crypto.c | 10 +++- > lib/librte_vhost/vhost_user.c | 82 +++++++++++++++++++++------------ > 3 files changed, 69 insertions(+), 56 deletions(-) > > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h > index c9c392975..8ab4ff299 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, > }; > > /** > @@ -131,37 +133,20 @@ enum rte_vhost_msg_result { /** * Function prototype for the vhost backend to handler specific vhost user * messages prior to the master message handling Now you have definition of pre handler for a common handler type. It should be updated. Probably just cropped a bit (and s/handler/handle/): /** * Function prototype for the vhost backend to handle specific vhost user * messages. > * vhost device id > * @param msg > * Message pointer. > - * @param skip_master > - * 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); > - > -/** > - * Function prototype for the vhost backend to handler specific vhost user > - * messages after the master message handling is done > - * > - * @param vid > - * vhost device id > - * @param msg > - * Message pointer. > - * @return > - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, > - * VH_RESULT_ERR on failure > - */ > -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, > - void *msg); > +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg); > > /** > * Optional vhost user message handlers. > */ > struct rte_vhost_user_extern_ops { > - rte_vhost_msg_pre_handle pre_msg_handle; > - rte_vhost_msg_post_handle post_msg_handle; What about some comments here? /* Called prior to the master message handling. */ > + rte_vhost_msg_handle pre_msg_handle; /* Called after the master message handling. */ > + rte_vhost_msg_handle post_msg_handle; > }; > > /** > diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c > index 0f437c4a1..9b4b850e8 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) { > + case 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; > + case 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 555d09ad9..39756fce7 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -1910,7 +1910,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; > int request; > > dev = get_device(vid); > @@ -1928,27 +1928,30 @@ 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_NONE && request < VHOST_USER_MAX && > + vhost_message_str[request]) { > + 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(DEBUG, VHOST_CONFIG, "External request %d\n", request); > + } > > ret = vhost_user_check_and_alloc_queue_pair(dev, &msg); > if (ret < 0) { > @@ -1964,7 +1967,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: > @@ -1989,19 +1992,24 @@ vhost_user_msg_handler(int vid, int fd) > > } > > + handled = false; > 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) > + /* Fall-through */ > + 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; > @@ -2012,40 +2020,54 @@ 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 { > - RTE_LOG(ERR, VHOST_CONFIG, > - "Requested invalid message type %d.\n", request); > - ret = RTE_VHOST_MSG_RESULT_ERR; > } > > 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); > + /* Fall-through */ > + 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) { > + 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend 2019-03-13 16:09 ` Ilya Maximets @ 2019-03-19 10:15 ` Maxime Coquelin 2019-03-19 10:15 ` Maxime Coquelin 0 siblings, 1 reply; 6+ messages in thread From: Maxime Coquelin @ 2019-03-19 10:15 UTC (permalink / raw) To: Ilya Maximets, dev, changpeng.liu, tiwei.bie, dariusz.stojaczyk Hi Ilya, On 3/13/19 5:09 PM, Ilya Maximets wrote: > On 13.03.2019 18:55, 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. >> >> Vhost-crypto backend is ialso adapted to this API change. >> >> Suggested-by: Ilya Maximets<i.maximets@samsung.com> >> Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com> >> Tested-by: Darek Stojaczyk<dariusz.stojaczyk@intel.com> >> --- >> lib/librte_vhost/rte_vhost.h | 33 ++++--------- >> lib/librte_vhost/vhost_crypto.c | 10 +++- >> lib/librte_vhost/vhost_user.c | 82 +++++++++++++++++++++------------ >> 3 files changed, 69 insertions(+), 56 deletions(-) >> >> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h >> index c9c392975..8ab4ff299 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, >> }; >> >> /** >> @@ -131,37 +133,20 @@ enum rte_vhost_msg_result { > /** > * Function prototype for the vhost backend to handler specific vhost user > * messages prior to the master message handling > > Now you have definition of pre handler for a common handler type. > > It should be updated. Probably just cropped a bit (and s/handler/handle/): > > /** > * Function prototype for the vhost backend to handle specific vhost user > * messages. Indeed, I missed to fix that. >> * vhost device id >> * @param msg >> * Message pointer. >> - * @param skip_master >> - * 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); >> - >> -/** >> - * Function prototype for the vhost backend to handler specific vhost user >> - * messages after the master message handling is done >> - * >> - * @param vid >> - * vhost device id >> - * @param msg >> - * Message pointer. >> - * @return >> - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, >> - * VH_RESULT_ERR on failure >> - */ >> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, >> - void *msg); >> +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg); >> >> /** >> * Optional vhost user message handlers. >> */ >> struct rte_vhost_user_extern_ops { >> - rte_vhost_msg_pre_handle pre_msg_handle; >> - rte_vhost_msg_post_handle post_msg_handle; > What about some comments here? > > /* Called prior to the master message handling. */ >> + rte_vhost_msg_handle pre_msg_handle; > /* Called after the master message handling. */ >> + rte_vhost_msg_handle post_msg_handle; >> }; >> That's good suggestions, I'll post a v3 with these changes. Thanks, Maxime ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend 2019-03-19 10:15 ` Maxime Coquelin @ 2019-03-19 10:15 ` Maxime Coquelin 0 siblings, 0 replies; 6+ messages in thread From: Maxime Coquelin @ 2019-03-19 10:15 UTC (permalink / raw) To: Ilya Maximets, dev, changpeng.liu, tiwei.bie, dariusz.stojaczyk Hi Ilya, On 3/13/19 5:09 PM, Ilya Maximets wrote: > On 13.03.2019 18:55, 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. >> >> Vhost-crypto backend is ialso adapted to this API change. >> >> Suggested-by: Ilya Maximets<i.maximets@samsung.com> >> Signed-off-by: Maxime Coquelin<maxime.coquelin@redhat.com> >> Tested-by: Darek Stojaczyk<dariusz.stojaczyk@intel.com> >> --- >> lib/librte_vhost/rte_vhost.h | 33 ++++--------- >> lib/librte_vhost/vhost_crypto.c | 10 +++- >> lib/librte_vhost/vhost_user.c | 82 +++++++++++++++++++++------------ >> 3 files changed, 69 insertions(+), 56 deletions(-) >> >> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h >> index c9c392975..8ab4ff299 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, >> }; >> >> /** >> @@ -131,37 +133,20 @@ enum rte_vhost_msg_result { > /** > * Function prototype for the vhost backend to handler specific vhost user > * messages prior to the master message handling > > Now you have definition of pre handler for a common handler type. > > It should be updated. Probably just cropped a bit (and s/handler/handle/): > > /** > * Function prototype for the vhost backend to handle specific vhost user > * messages. Indeed, I missed to fix that. >> * vhost device id >> * @param msg >> * Message pointer. >> - * @param skip_master >> - * 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); >> - >> -/** >> - * Function prototype for the vhost backend to handler specific vhost user >> - * messages after the master message handling is done >> - * >> - * @param vid >> - * vhost device id >> - * @param msg >> - * Message pointer. >> - * @return >> - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, >> - * VH_RESULT_ERR on failure >> - */ >> -typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, >> - void *msg); >> +typedef enum rte_vhost_msg_result (*rte_vhost_msg_handle)(int vid, void *msg); >> >> /** >> * Optional vhost user message handlers. >> */ >> struct rte_vhost_user_extern_ops { >> - rte_vhost_msg_pre_handle pre_msg_handle; >> - rte_vhost_msg_post_handle post_msg_handle; > What about some comments here? > > /* Called prior to the master message handling. */ >> + rte_vhost_msg_handle pre_msg_handle; > /* Called after the master message handling. */ >> + rte_vhost_msg_handle post_msg_handle; >> }; >> That's good suggestions, I'll post a v3 with these changes. Thanks, Maxime ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-19 10:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-13 15:55 [dpdk-dev] [PATCH v2 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 1/2] vhost: add API to set protocol features flags Maxime Coquelin 2019-03-13 15:55 ` [dpdk-dev] [PATCH v2 2/2] vhost: support requests only handled by external backend Maxime Coquelin 2019-03-13 16:09 ` Ilya Maximets 2019-03-19 10:15 ` Maxime Coquelin 2019-03-19 10:15 ` Maxime Coquelin
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).