[dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure
Maxime Coquelin
maxime.coquelin at redhat.com
Wed Mar 21 14:02:35 CET 2018
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
More information about the dev
mailing list