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

["Wodkowski, PawelX" <pawelx.wodkowski@intel.com>]

> -----Original Message-----
> From: Zhang, Roy Fan
> Sent: Wednesday, March 21, 2018 5:12 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.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 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!

Can we add this handler in struct vhost_device_ops? There is filed void 
*reserved[2]. We can use first element to define this handler.

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