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

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Maxime,

Thanks a lot for the fast reply. 

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, March 21, 2018 1:03 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org; Kulasek,
> TomaszX <tomaszx.kulasek@intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski@intel.com>
> Cc: jianjay.zhou@huawei.com; yliu@fridaylinux.org; Tan, Jianfeng
> <jianfeng.tan@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user
> private info structure
> 
> 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.

Same as the messages VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_GET_VRING_BASE etc, the external backend such as vhost crypto will require to send an reply independently. 
In case of vhost crypto, the message create_session will require the backend to send a session id back to the frontend. 

I tried to set VHOST_USER_NEED_REPLY bit in the vhost crypto message handler but it will cause problem, qemu returns " qemu-system-x86_64: Received bad msg size."

Another solution is to pass a variable back from the message handler to indicate sending the reply instead.
So the function prototype can be 
typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg, uint32_t *require_reply);


> > struct vhost_user_dev_extern {
> > 	msg_handler post_vhost_user_msg_handler;
> > 	char data[0];
> Why is this filed needed?

Either this way, or using a pointer. The idea is to allocate contiguous memory when creating new vhost_user_dev_extern structure. The former way is slightly more performant to avoid another memory read, but I can live with the pointer instead :-)

> > };
> 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);

Great idea!

> >>
> >> 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.

Will do.

> 
> Thanks,
> Maxime
> > Thanks a million Maxime.
> >
> >> Thanks,
> >> Maxime

Thanks,
Fan