From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure Date: Tue, 13 Mar 2018 10:08:10 +0100 Message-ID: <0f21227c-5b5f-a3f7-707a-60476826158e@redhat.com> References: <20180227162917.35125-1-roy.fan.zhang@intel.com> <20180227162917.35125-2-roy.fan.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: jianjay.zhou@huawei.com, yliu@fridaylinux.org To: Fan Zhang , dev@dpdk.org, Tomasz Kulasek , Pawel Wodkowski Return-path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 957555B34 for ; Tue, 13 Mar 2018 10:08:15 +0100 (CET) In-Reply-To: <20180227162917.35125-2-roy.fan.zhang@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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 > #include > #include > +#include I think this include isn't needed looking at the rest of the patch. > > #include > #include > @@ -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