Re: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure

[Maxime Coquelin]

Hi,

On 03/21/2018 10:10 AM, Zhang, Roy Fan wrote:

Hi Maxime,

Thanks for the review.

I think this include isn't needed looking at the rest of the patch.

I agree. I will remove this line here.

   #define VHOST_USER_VERSION    0x1

+typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg

*msg,

+               int fd);
+
+struct vhost_user_dev_priv {
+       msg_handler vhost_user_msg_handler;
+       char data[0];
+};

   /* vhost_user.c */
   int vhost_user_msg_handler(int vid, int fd);

I think the wording is a bit misleading, I'm fine with having a private_data
pointer, but it should only be used by the external backend.

Maybe what you need here is a new API to be to register a callback for the
external backend to handle specific requests.

That's exactly what I need.
Shall I rework the code like this?

These new API are to be placed in rte_vhost.h, else it won't be
exported.

/* vhost.h */
struct virtio_net {
        ....
        void *extern_data; /*<< private data for external backend */
        
}

Looks good, you may need to add getter and setter APIs as struct
virtio_net isn't part of the API.

/* vhost_user.h */
typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg,
                int fd);

I wonder if the fd is really necessary, as if a reply is to be sent, it
can be done by the vhost lib.

struct vhost_user_dev_extern {
        msg_handler post_vhost_user_msg_handler;
        char data[0];

Why is this filed needed?

};

I would change this to:
struct rte_vhost_user_dev_extern_ops {
        rte_vhost_msg_handler pre_vhost_user_msg_handler;
        rte_vhost_msg_handler post_vhost_user_msg_handler;
};

int
vhost_user_register_call_back(struct virtio_net *dev, msg_handler 
post_msg_handler);

and something like:
rte_vhost_user_register_extern_ops(struct virtio_net *dev, struct
                                rte_vhost_user_dev_extern_ops *ops);

Also, it might be interesting for the external backend to register callbacks for
existing requests. For example .pre_vhost_user_msg_handler
and .post_vhost_user_msg_handler. Doing so, the external backend could
for example catch beforehand any change that could affect resources being
used. Tomasz, Pawel, do you think that could help for the issue you reported?

I think it is definitely a good idea. However there will be a problem. As 
vhost_crypto does not require pre_vhost_user_msg_handler I think it may not 
appropriate to add pre_vhost_user_msg_handler in this patchset.

I see, but please add the pre_ callback directly in this series, as we
know it will be useful (thanks Pawel).
It will avoid breaking the API again when we'll need it.

Thanks,
Maxime

Thanks a million Maxime.

Thanks,
Maxime