Re: [dpdk-dev] [PATCH v2 5/5] vhost: message handling implemented as a callback array

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Ilya,

On 09/10/2018 06:09 PM, Ilya Maximets wrote:
> Hi Maxime,
> Thanks for pointing to this patch set. I missed it somehow.
> 
> This patch could not replace mine [1], because it does not improve
> the message handling from the error handling point of view. But
> it completely changes the code, so we need to negotiate the order
> in which they will be applied or combine them somehow.

By complementary I meant both of your patches add similar things, like
vhost_user_set_vring_kick() becoming an int (even if Nikolay's version
doesn't return an error for now).

> 
> So, what's the plan? What do you think?

On one hand Nikolay's series was posted first, and it might be easier to
just apply your patch on top. On the other hand, your patch fixes an
issue, so it will be easier to backport it if it is applied first.

I guess we should go for your patch first to ease backporting.

I'd like Tiwei's feedback before applying anything, as he reviewed the
first versions of both yours and Nikolay's patches.

> Few comments inline.
> 
> [1] http://patchwork.dpdk.org/patch/44168/
> 
> Best regards, Ilya Maximets.
> 
> On 10.09.2018 18:32, Maxime Coquelin wrote:
>>
>>
>> On 07/19/2018 09:13 PM, Nikolay Nikolaev wrote:
>>> Introduce vhost_message_handlers, which maps the message request
>>> type to the message handler. Then replace the switch construct
>>> with a map and call.
>>>
>>> Signed-off-by: Nikolay Nikolaev <nicknickolaev@gmail.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c |  143 +++++++++++++++--------------------------
>>>    1 file changed, 54 insertions(+), 89 deletions(-)
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 6b39d1e30..1b164df27 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1466,6 +1466,34 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, VhostUserMsg *msg)
>>>        return VH_RESULT_OK;
>>>    }
>>>    +typedef int (*vhost_message_handler_t)(struct virtio_net **pdev, VhostUserMsg * msg);
>>> +static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
>>> +    [VHOST_USER_NONE] = NULL,
>>> +    [VHOST_USER_GET_FEATURES] = vhost_user_get_features,
>>> +    [VHOST_USER_SET_FEATURES] = vhost_user_set_features,
>>> +    [VHOST_USER_SET_OWNER] = vhost_user_set_owner,
>>> +    [VHOST_USER_RESET_OWNER] = vhost_user_reset_owner,
>>> +    [VHOST_USER_SET_MEM_TABLE] = vhost_user_set_mem_table,
>>> +    [VHOST_USER_SET_LOG_BASE] = vhost_user_set_log_base,
>>> +    [VHOST_USER_SET_LOG_FD] = vhost_user_set_log_fd,
>>> +    [VHOST_USER_SET_VRING_NUM] = vhost_user_set_vring_num,
>>> +    [VHOST_USER_SET_VRING_ADDR] = vhost_user_set_vring_addr,
>>> +    [VHOST_USER_SET_VRING_BASE] = vhost_user_set_vring_base,
>>> +    [VHOST_USER_GET_VRING_BASE] = vhost_user_get_vring_base,
>>> +    [VHOST_USER_SET_VRING_KICK] = vhost_user_set_vring_kick,
>>> +    [VHOST_USER_SET_VRING_CALL] = vhost_user_set_vring_call,
>>> +    [VHOST_USER_SET_VRING_ERR] = vhost_user_set_vring_err,
>>> +    [VHOST_USER_GET_PROTOCOL_FEATURES] = vhost_user_get_protocol_features,
>>> +    [VHOST_USER_SET_PROTOCOL_FEATURES] = vhost_user_set_protocol_features,
>>> +    [VHOST_USER_GET_QUEUE_NUM] = vhost_user_get_queue_num,
>>> +    [VHOST_USER_SET_VRING_ENABLE] = vhost_user_set_vring_enable,
>>> +    [VHOST_USER_SEND_RARP] = vhost_user_send_rarp,
>>> +    [VHOST_USER_NET_SET_MTU] = vhost_user_net_set_mtu,
>>> +    [VHOST_USER_SET_SLAVE_REQ_FD] = vhost_user_set_req_fd,
>>> +    [VHOST_USER_IOTLB_MSG] = vhost_user_iotlb_msg,
>>> +};
>>> +
>>> +
>>>    /* return bytes# of read on success or negative val on failure. */
>>>    static int
>>>    read_vhost_message(int sockfd, VhostUserMsg *msg)
>>> @@ -1618,6 +1646,7 @@ vhost_user_msg_handler(int vid, int fd)
>>>        int ret;
>>>        int unlock_required = 0;
>>>        uint32_t skip_master = 0;
>>> +    int request;
>>>          dev = get_device(vid);
>>>        if (dev == NULL)
>>> @@ -1710,97 +1739,33 @@ vhost_user_msg_handler(int vid, int fd)
>>>                goto skip_to_post_handle;
>>>        }
>>>    -    switch (msg.request.master) {
>>> -    case VHOST_USER_GET_FEATURES:
>>> -        ret = vhost_user_get_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_FEATURES:
>>> -        ret = vhost_user_set_features(&dev, &msg);
>>> -        if (ret)
>>> -            return -1;
> 
> You can not just remove this. Disconnection was triggered here
> because the error is unrecoverable.
> See 59fe5e17d930 ("vhost: propagate set features handling error")
> for details.

Right, maybe adding another enum for unrecoverable errors would do the
trick.

>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_get_protocol_features(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_PROTOCOL_FEATURES:
>>> -        ret = vhost_user_set_protocol_features(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_OWNER:
>>> -        ret = vhost_user_set_owner(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_RESET_OWNER:
>>> -        ret = vhost_user_reset_owner(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_MEM_TABLE:
>>> -        ret = vhost_user_set_mem_table(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_LOG_BASE:
>>> -        ret = vhost_user_set_log_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_LOG_FD:
>>> -        ret = vhost_user_set_log_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_NUM:
>>> -        ret = vhost_user_set_vring_num(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_ADDR:
>>> -        ret = vhost_user_set_vring_addr(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_BASE:
>>> -        ret = vhost_user_set_vring_base(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_VRING_BASE:
>>> -        ret = vhost_user_get_vring_base(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_KICK:
>>> -        ret = vhost_user_set_vring_kick(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SET_VRING_CALL:
>>> -        ret = vhost_user_set_vring_call(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ERR:
>>> -        ret = vhost_user_set_vring_err(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_GET_QUEUE_NUM:
>>> -        ret = vhost_user_get_queue_num(&dev, &msg);
>>> -        send_vhost_reply(fd, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_VRING_ENABLE:
>>> -        ret = vhost_user_set_vring_enable(&dev, &msg);
>>> -        break;
>>> -    case VHOST_USER_SEND_RARP:
>>> -        ret = vhost_user_send_rarp(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_NET_SET_MTU:
>>> -        ret = vhost_user_net_set_mtu(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_SET_SLAVE_REQ_FD:
>>> -        ret = vhost_user_set_req_fd(&dev, &msg);
>>> -        break;
>>> -
>>> -    case VHOST_USER_IOTLB_MSG:
>>> -        ret = vhost_user_iotlb_msg(&dev, &msg);
>>> -        break;
>>> +    request = msg.request.master;
>>> +    if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>> +        if (!vhost_message_handlers[request])
>>> +            goto skip_to_post_handle;
>>> +        ret = vhost_message_handlers[request](&dev, &msg);
>>>    -    default:
>>> +        switch (ret) {
>>> +        case VH_RESULT_ERR:
>>> +            RTE_LOG(ERR, VHOST_CONFIG,
>>> +                "Processing %s failed.\n",
>>> +                vhost_message_str[request]);
> 
> I guess 'break' is missing here. Isn't it?

Good catch.

> 
>>> +        case VH_RESULT_OK:
>>> +            RTE_LOG(DEBUG, VHOST_CONFIG,
>>> +                "Processing %s succeeded.\n",
>>> +                vhost_message_str[request]);
>>> +            break;
>>> +        case VH_RESULT_REPLY:
>>> +            RTE_LOG(INFO, VHOST_CONFIG,
>>> +                "Processing %s succeeded and needs reply.\n",
>>> +                vhost_message_str[request]);
>>> +            send_vhost_reply(fd, &msg);
>>> +            break;
>>> +        }
>>> +    } else {
>>> +        RTE_LOG(ERR, VHOST_CONFIG,
>>> +            "Requested invalid message type %d.\n", request);
>>>            ret = -1;
>>> -        break;
>>>        }
>>>      skip_to_post_handle:
>>>
>>
>>