On 04.03.2019 19:02, Maxime Coquelin wrote:
>
>
> On 3/4/19 4:25 PM, Ilya Maximets wrote:
>> 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.maxim...@samsung.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coque...@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;
>> ---
>
> Indeed, it will be part of v1 if Changpeng confirms this RFC is working
> for his usecase.
>
>>
>>
>>> 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.
>
> Actually I think it is necessary, more below.
>
>>
>>> 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.
>
> Agree.
>
>> 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.
>
> I think we need to handle messages out of range, otherwise external
> backend may not implement new requests without patch dpdk first.
>
> Regarding "Requested invalid message type", I think it should just be
> removed.
Agree.
> This version assumes the external backend will implement the
> 'pre' callback for its specific requests, but this is an uneeded
> limitation and could implmeent the 'post' callback only.
Sure. vhost_crypto has only post handler.
>
>>> + 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)
>
> I added 'handled' variable because ret can be
> RTE_VHOST_MSG_RESULT_NOT_HANDLED at this stage but the request has been
> handled.
>
> For example, vhost-user library handles the request and the external
> backend implements post_msg_handle callback. If the external backend
> callback does not handle this psecific request, results will be
> RTE_VHOST_MSG_RESULT_NOT_HANDLED.
>
> So 'handled' is set to true as soon as one of the 3 possible ways to
> handle the request (.pre, vhost-lib, .post) handles it.
Oh. I see. Thanks.
>
>>> + 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
>>>
>
>
