[dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

Ilya Maximets i.maximets at samsung.com
Mon Mar 4 17:24:59 CET 2019


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.maximets at samsung.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin at 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
>>>
> 
> 


More information about the dev mailing list