[dpdk-dev,v2,01/10] lib/librte_vhost: add vhost user private info structure
Checks
Commit Message
This patch adds a vhost_user_dev_priv structure and a vhost_user
message handler function prototype to vhost_user. This allows
different types of devices to add private information and their
device-specific vhost-user message function handlers to
virtio_net structure. The change to vhost_user_msg_handler is
also added to call the device-specific message handler.
Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
---
lib/librte_vhost/vhost.h | 5 ++++-
lib/librte_vhost/vhost_user.c | 13 ++++++++++++-
lib/librte_vhost/vhost_user.h | 7 +++++++
3 files changed, 23 insertions(+), 2 deletions(-)
Comments
On 02/27/2018 05:29 PM, Fan Zhang wrote:
> This patch adds a vhost_user_dev_priv structure and a vhost_user
> message handler function prototype to vhost_user. This allows
> different types of devices to add private information and their
> device-specific vhost-user message function handlers to
> virtio_net structure. The change to vhost_user_msg_handler is
> also added to call the device-specific message handler.
>
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
> lib/librte_vhost/vhost.h | 5 ++++-
> lib/librte_vhost/vhost_user.c | 13 ++++++++++++-
> lib/librte_vhost/vhost_user.h | 7 +++++++
> 3 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index d947bc9e3..19ee3fd37 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -13,6 +13,7 @@
> #include <linux/virtio_net.h>
> #include <sys/socket.h>
> #include <linux/if.h>
> +#include <stdbool.h>
I think this include isn't needed looking at the rest of the patch.
>
> #include <rte_log.h>
> #include <rte_ether.h>
> @@ -241,8 +242,10 @@ struct virtio_net {
> struct guest_page *guest_pages;
>
> int slave_req_fd;
> -} __rte_cache_aligned;
>
> + /* Private data for different virtio device type */
> + void *private_data;
> +} __rte_cache_aligned;
>
> #define VHOST_LOG_PAGE 4096
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 90ed2112e..6a90d2a96 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1477,7 +1477,18 @@ vhost_user_msg_handler(int vid, int fd)
> break;
>
> default:
> - ret = -1;
> + if (!dev->private_data)
> + ret = -1;
> + else {
> + struct vhost_user_dev_priv *priv = dev->private_data;
> +
> + if (!priv->vhost_user_msg_handler)
> + ret = -1;
> + else {
> + ret = (*priv->vhost_user_msg_handler)(dev,
> + &msg, fd);
> + }
> + }
> break;
>
> }
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index d4bd604b9..354615c8b 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -108,6 +108,13 @@ typedef struct VhostUserMsg {
> /* The version of the protocol we support */
> #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.
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?
Thanks,
Maxime
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?
/* vhost.h */
struct virtio_net {
....
void *extern_data; /*<< private data for external backend */
}
/* vhost_user.h */
typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg,
int fd);
struct vhost_user_dev_extern {
msg_handler post_vhost_user_msg_handler;
char data[0];
};
int
vhost_user_register_call_back(struct virtio_net *dev, msg_handler post_msg_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.
Thanks a million Maxime.
> Thanks,
> Maxime
> int
> vhost_user_register_call_back(struct virtio_net *dev, msg_handler
> post_msg_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?
> >
>
Sorry, I didn't see this patch series somehow. I need to fix my mail filters.
This is definitely good idea to introduce pre and vhost user post message handlers.
>
> 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.
>
> Thanks a million Maxime.
>
> > Thanks,
> > Maxime
Paweł
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
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
> -----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
@@ -13,6 +13,7 @@
#include <linux/virtio_net.h>
#include <sys/socket.h>
#include <linux/if.h>
+#include <stdbool.h>
#include <rte_log.h>
#include <rte_ether.h>
@@ -241,8 +242,10 @@ struct virtio_net {
struct guest_page *guest_pages;
int slave_req_fd;
-} __rte_cache_aligned;
+ /* Private data for different virtio device type */
+ void *private_data;
+} __rte_cache_aligned;
#define VHOST_LOG_PAGE 4096
@@ -1477,7 +1477,18 @@ vhost_user_msg_handler(int vid, int fd)
break;
default:
- ret = -1;
+ if (!dev->private_data)
+ ret = -1;
+ else {
+ struct vhost_user_dev_priv *priv = dev->private_data;
+
+ if (!priv->vhost_user_msg_handler)
+ ret = -1;
+ else {
+ ret = (*priv->vhost_user_msg_handler)(dev,
+ &msg, fd);
+ }
+ }
break;
}
@@ -108,6 +108,13 @@ typedef struct VhostUserMsg {
/* The version of the protocol we support */
#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);