[dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure
Zhang, Roy Fan
roy.fan.zhang at intel.com
Wed Mar 21 17:11:31 CET 2018
Hi Maxime,
Thanks a lot for the fast reply.
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> Sent: Wednesday, March 21, 2018 1:03 PM
> To: Zhang, Roy Fan <roy.fan.zhang at intel.com>; dev at dpdk.org; Kulasek,
> TomaszX <tomaszx.kulasek at intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski at intel.com>
> Cc: jianjay.zhou at huawei.com; yliu at fridaylinux.org; Tan, Jianfeng
> <jianfeng.tan at intel.com>; Bie, Tiwei <tiwei.bie at 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
More information about the dev
mailing list