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

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

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