From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 2FB0258E1 for ; Tue, 13 Sep 2016 14:58:17 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP; 13 Sep 2016 05:58:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,328,1470726000"; d="scan'208";a="760219808" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by FMSMGA003.fm.intel.com with ESMTP; 13 Sep 2016 05:58:15 -0700 Date: Tue, 13 Sep 2016 20:58:47 +0800 From: Yuanhan Liu To: Changpeng Liu Cc: dev@dpdk.org, james.r.harris@intel.com, Thomas Monjalon Message-ID: <20160913125847.GA10323@yliu-dev.sh.intel.com> References: <1473855300-3066-1-git-send-email-changpeng.liu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473855300-3066-1-git-send-email-changpeng.liu@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] vhost: change the vhost library to a common framework which can support more VIRTIO devices X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Sep 2016 12:58:17 -0000 On Wed, Sep 14, 2016 at 08:15:00PM +0800, Changpeng Liu wrote: > For storage virtualization use cases, vhost-scsi becomes a more popular > solution to support VMs. However a user space vhost-scsi-user solution > does not exist currently. SPDK(Storage Performance Development Kit, > https://github.com/spdk/spdk) will provide a user space vhost-scsi target > to support multiple VMs through Qemu. Originally SPDK is built on top > of DPDK libraries, so we would like to use DPDK vhost library as the > communication channel between Qemu and vhost-scsi target application. > > Currently DPDK vhost library can only support VIRTIO_ID_NET device type, > we would like to extend the library to support VIRTIO_ID_SCSI and > VIRTIO_ID_BLK. Most of DPDK vhost library can be reused only several > differences: > 1. VIRTIO SCSI device has different vring queues compared with VIRTIO NET > device, at least 3 vring queues needed for SCSI device type; > 2. VIRTIO SCSI will need several extra message operation code, such as > SCSI_SET_ENDPIONT/SCSI_CLEAR_ENDPOINT; > > First, we would like to extend DPDK vhost library as a common framework I don't see how common it becomes with this patch applied. > which be friendly to add other VIRTIO device types, to implement this feature, > we add a new data structure virtio_dev, which can deliver socket messages > to different VIRTIO devices, each specific VIRTIO device will register > callback to virtio_dev. > > Secondly, we would to upstream a patch to Qemu community to add vhost-scsi > specific operation command such as SCSI_SET_ENDPOINT and SCSI_CLEAR_ENDOINT, > and user space feature bits. > > Finally, after the Qemu patch set was merged, we will add VIRTIO_ID_SCSI > support to DPDK vhost library You actually should send this part out with this patchset. You are making changes for adding the vhost-scsi support, however, you don't show us how the code to support vhost-scsi looks like. That means, it's hard for us to understand why you are doing those changes. What I said is DPDK will not consider merging vhost-scsi patches unless QEMU have merged the vhost-scsi part. This doesn't mean you can't send out the DPDK vhost-scsi patches before that. > and an example vhost-scsi target which can > add a SCSI device to VM through this example application. > > This patch set changed the vhost library as a common framework which > can add other VIRTIO device type in future. > > Signed-off-by: Changpeng Liu > --- > lib/librte_vhost/Makefile | 4 +- > lib/librte_vhost/rte_virtio_dev.h | 140 ++++++++ > lib/librte_vhost/rte_virtio_net.h | 97 +----- rte_virtio_net.h is the header file will be exported for applications. Every change there would mean either an API or ABI breakage. Thus, we should try to avoid touching it. Don't even to say you added yet another header file, rte_virtio_dev.h. I confess that the rte_virtio_net.h filename isn't that right: it sticks to virtio-net so tightly. We may could rename it to rte_vhost.h, but I doubt it's worthwhile: as said, it breaks the API. The fortunate thing about this file is that, the context is actually not sticking to virtio-net too much. I mean, all the APIs are using the "vid", which is just a number. Well, except the virtio_net_device_ops() structure, which also should be renamed to vhost_device_ops(). Besides that, the three ops, "new_device", "destroy_device" and "vring_state_changed", are actually not limited to virtio-net device. That is to say, we could have two options here: - rename the header file and the structure properly, to not limited to virtio-net - live with it, let it be a legacy issue, and document it at somewhere, say, "due to history reason, that virtio-net is the first one supported in DPDK, we kept the header filename as rte_virtio_net.h, but not ..." I personally would prefer the later one, which saves us from breaking applications again. I don't have strong objection to the first one though. Thomas, any comments? > lib/librte_vhost/socket.c | 6 +- > lib/librte_vhost/vhost.c | 421 ------------------------ > lib/librte_vhost/vhost.h | 288 ----------------- > lib/librte_vhost/vhost_device.h | 230 +++++++++++++ > lib/librte_vhost/vhost_net.c | 659 ++++++++++++++++++++++++++++++++++++++ > lib/librte_vhost/vhost_net.h | 126 ++++++++ > lib/librte_vhost/vhost_user.c | 451 +++++++++++++------------- > lib/librte_vhost/vhost_user.h | 17 +- > lib/librte_vhost/virtio_net.c | 37 ++- That basically means you are heading the wrong way. For example, > +struct virtio_dev_table { > + int (*vhost_dev_ready)(struct virtio_dev *dev); > + struct vhost_virtqueue* (*vhost_dev_get_queues)(struct virtio_dev *dev, uint16_t queue_id); > + void (*vhost_dev_cleanup)(struct virtio_dev *dev, int destroy); > + void (*vhost_dev_free)(struct virtio_dev *dev); > + void (*vhost_dev_reset)(struct virtio_dev *dev); > + uint64_t (*vhost_dev_get_features)(struct virtio_dev *dev); > + int (*vhost_dev_set_features)(struct virtio_dev *dev, uint64_t features); > + uint64_t (*vhost_dev_get_protocol_features)(struct virtio_dev *dev); > + int (*vhost_dev_set_protocol_features)(struct virtio_dev *dev, uint64_t features); > + uint32_t (*vhost_dev_get_default_queue_num)(struct virtio_dev *dev); > + uint32_t (*vhost_dev_get_queue_num)(struct virtio_dev *dev); > + uint16_t (*vhost_dev_get_avail_entries)(struct virtio_dev *dev, uint16_t queue_id); > + int (*vhost_dev_get_vring_base)(struct virtio_dev *dev, struct vhost_virtqueue *vq); > + int (*vhost_dev_set_vring_num)(struct virtio_dev *dev, struct vhost_virtqueue *vq); > + int (*vhost_dev_set_vring_call)(struct virtio_dev *dev, struct vhost_vring_file *file); > + int (*vhost_dev_set_log_base)(struct virtio_dev *dev, int fd, uint64_t size, uint64_t off); > +}; This looks wrong. Most of them (if not all) should be same, regardless it's for virtio-net, or virtio-scsi. I don't understand why you should even touch this. Those are for handling *vhost-user* messages, but not virtio-net, nor virtio-scsi. They should be same no matter which virtio device we are dealing with. Well, virtio-scsi may just have few more messages than virtio-net. --yliu