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

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

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