* [dpdk-dev] [RFC 0/2] vhost: Support external backend only vhost-user requests @ 2019-02-27 10:02 Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 1/2] vhost: add API to set protocol features flags Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend Maxime Coquelin 0 siblings, 2 replies; 5+ messages in thread From: Maxime Coquelin @ 2019-02-27 10:02 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets; +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. Maxime Coquelin (2): vhost: add API to set protocol features flags vhost: support vhost-user request only handled by external backend lib/librte_vhost/rte_vhost.h | 14 +++++++++++++ lib/librte_vhost/rte_vhost_version.map | 1 + lib/librte_vhost/socket.c | 15 ++++++++++++++ lib/librte_vhost/vhost_user.c | 28 ++++++++++++++------------ 4 files changed, 45 insertions(+), 13 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [dpdk-dev] [RFC 1/2] vhost: add API to set protocol features flags 2019-02-27 10:02 [dpdk-dev] [RFC 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin @ 2019-02-27 10:02 ` Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend Maxime Coquelin 1 sibling, 0 replies; 5+ messages in thread From: Maxime Coquelin @ 2019-02-27 10:02 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets; +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> --- lib/librte_vhost/rte_vhost.h | 14 ++++++++++++++ lib/librte_vhost/rte_vhost_version.map | 1 + lib/librte_vhost/socket.c | 15 +++++++++++++++ 3 files changed, 30 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..129a047f6 100644 --- a/lib/librte_vhost/socket.c +++ b/lib/librte_vhost/socket.c @@ -707,6 +707,21 @@ 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; + int ret = 0; + + 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] 5+ messages in thread
* [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend 2019-02-27 10:02 [dpdk-dev] [RFC 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 1/2] vhost: add API to set protocol features flags Maxime Coquelin @ 2019-02-27 10:02 ` Maxime Coquelin 2019-02-27 13:15 ` Ilya Maximets 1 sibling, 1 reply; 5+ messages in thread From: Maxime Coquelin @ 2019-02-27 10:02 UTC (permalink / raw) To: dev, changpeng.liu, tiwei.bie, i.maximets; +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 catch the case where a request is neither handled by the external backend nor by the vhost library. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost_user.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 36c0c676d..bae5ef1cc 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -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[req]) { + if (request != VHOST_USER_IOTLB_MSG) + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", + vhost_message_str[request]); + else if ( + 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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd) } + ret = RTE_VHOST_MSG_RESULT_ERR; if (dev->extern_ops.pre_msg_handle) { ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, (void *)&msg, &skip_master); @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd) goto skip_to_post_handle; } - request = msg.request.master; if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { if (!vhost_message_handlers[request]) goto skip_to_post_handle; -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend 2019-02-27 10:02 ` [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend Maxime Coquelin @ 2019-02-27 13:15 ` Ilya Maximets 2019-02-28 8:46 ` Maxime Coquelin 0 siblings, 1 reply; 5+ messages in thread From: Ilya Maximets @ 2019-02-27 13:15 UTC (permalink / raw) To: Maxime Coquelin, dev, changpeng.liu, tiwei.bie On 27.02.2019 13:02, 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 catch the case where a request is neither handled > by the external backend nor by the vhost library. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/librte_vhost/vhost_user.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index 36c0c676d..bae5ef1cc 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -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[req]) { > + if (request != VHOST_USER_IOTLB_MSG) > + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", > + vhost_message_str[request]); > + else if ( > + RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", > + vhost_message_str[request]); There is no need for the 'if' without the body. > + } else { > + RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request); External requests could be annoying. Maybe we'll need to move them under DBG ? I'm not sure. > + } > > 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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd) > > } > > + ret = RTE_VHOST_MSG_RESULT_ERR; This will break the 'vhost_crypto', because it has no 'pre_msg_handler' and master will skip to 'post_msg_handler', but it will not be called because current status is ERR. Maybe it's easier to introduce RTE_VHOST_MSG_RESULT_NOT_HANDLED and convert it to ERR before the reply ? This will require changing the pre_msg_handlers to return RTE_VHOST_MSG_RESULT_NOT_HANDLED if message wasn't recognized. And we'll possibly be able to drop the 'skip_master' in this case. > if (dev->extern_ops.pre_msg_handle) { > ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, > (void *)&msg, &skip_master); > @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd) > goto skip_to_post_handle; > } > > - request = msg.request.master; > if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { > if (!vhost_message_handlers[request]) > goto skip_to_post_handle; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend 2019-02-27 13:15 ` Ilya Maximets @ 2019-02-28 8:46 ` Maxime Coquelin 0 siblings, 0 replies; 5+ messages in thread From: Maxime Coquelin @ 2019-02-28 8:46 UTC (permalink / raw) To: Ilya Maximets, dev, changpeng.liu, tiwei.bie On 2/27/19 2:15 PM, Ilya Maximets wrote: > On 27.02.2019 13:02, 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 catch the case where a request is neither handled >> by the external backend nor by the vhost library. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/librte_vhost/vhost_user.c | 28 +++++++++++++++------------- >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index 36c0c676d..bae5ef1cc 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -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[req]) { >> + if (request != VHOST_USER_IOTLB_MSG) >> + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", >> + vhost_message_str[request]); >> + else if ( >> + RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", >> + vhost_message_str[request]); > > There is no need for the 'if' without the body. Oops, indeed. I was pretty sure I did compile test it, but looking at the history I didn't. >> + } else { >> + RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request); > > External requests could be annoying. Maybe we'll need to move them under DBG ? > I'm not sure. Fair point. I'll change to DBG. >> + } >> >> 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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd) >> >> } >> >> + ret = RTE_VHOST_MSG_RESULT_ERR; > > This will break the 'vhost_crypto', because it has no 'pre_msg_handler' > and master will skip to 'post_msg_handler', but it will not be called > because current status is ERR. Thanks for catching it. > Maybe it's easier to introduce RTE_VHOST_MSG_RESULT_NOT_HANDLED and convert > it to ERR before the reply ? > This will require changing the pre_msg_handlers to return > RTE_VHOST_MSG_RESULT_NOT_HANDLED if message wasn't recognized. > And we'll possibly be able to drop the 'skip_master' in this case. Ok, that means breaking the API, but it is still experimental so not a blocker. I like the idea because it would also make it possible to add some debug prints. I'll post new iteration this morning. Thanks, Maxime >> if (dev->extern_ops.pre_msg_handle) { >> ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, >> (void *)&msg, &skip_master); >> @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd) >> goto skip_to_post_handle; >> } >> >> - request = msg.request.master; >> if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { >> if (!vhost_message_handlers[request]) >> goto skip_to_post_handle; >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-28 8:46 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-27 10:02 [dpdk-dev] [RFC 0/2] vhost: Support external backend only vhost-user requests Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 1/2] vhost: add API to set protocol features flags Maxime Coquelin 2019-02-27 10:02 ` [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend Maxime Coquelin 2019-02-27 13:15 ` Ilya Maximets 2019-02-28 8:46 ` 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).