From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 772317EEF for ; Wed, 14 Sep 2016 05:28:16 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP; 13 Sep 2016 20:28:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,332,1470726000"; d="scan'208";a="8125706" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga006.fm.intel.com with ESMTP; 13 Sep 2016 20:28:14 -0700 Date: Wed, 14 Sep 2016 11:28:42 +0800 From: Yuanhan Liu To: Changpeng Liu Cc: dev@dpdk.org, james.r.harris@intel.com Message-ID: <20160914032842.GB10323@yliu-dev.sh.intel.com> References: <1473855300-3066-1-git-send-email-changpeng.liu@intel.com> <1473899298-4580-1-git-send-email-changpeng.liu@intel.com> <1473899298-4580-2-git-send-email-changpeng.liu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473899298-4580-2-git-send-email-changpeng.liu@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library 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: Wed, 14 Sep 2016 03:28:17 -0000 On Thu, Sep 15, 2016 at 08:28:18AM +0800, Changpeng Liu wrote: > Since we changed the vhost library as a common framework to add other As I said in my earlier email, I don't see how common it becomes after your refactoring. __Another__ for example, I just saw a bunch of duplicated code below that should not even be there (vhost-scsi.c). Assuming we may add vhost-crypto in future, don't we have to duplicate again in vhost-crypto.c in your way? The answer is obviously NO. > +static void > +cleanup_vq(struct vhost_virtqueue *vq, int destroy) > +{ > + if ((vq->callfd >= 0) && (destroy != 0)) > + close(vq->callfd); > + if (vq->kickfd >= 0) > + close(vq->kickfd); > +} > + > +/* > + * Unmap any memory, close any file descriptors and > + * free any memory owned by a device. > + */ > +static void > +cleanup_device(struct virtio_dev *device, int destroy) > +{ > + struct virtio_scsi *dev = get_scsi_device(device); > + uint32_t i; > + > + dev->features = 0; > + dev->protocol_features = 0; > + > + for (i = 0; i < dev->virt_q_nb; i++) { > + cleanup_vq(dev->virtqueue[i], destroy); > + } > +} > + > +static void > +init_vring_queue(struct vhost_virtqueue *vq) > +{ > + memset(vq, 0, sizeof(struct vhost_virtqueue)); > + > + vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD; > + vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD; > + > + /* Backends are set to -1 indicating an inactive device. */ > + vq->backend = -1; > + vq->enabled = 1; > +} > + [... snipped a bunch of duplicated code ...] > +int > +rte_vhost_scsi_pop_request(int vid, uint16_t queue_id, > + struct virtio_scsi_cmd_req **request, struct virtio_scsi_cmd_resp **response, struct iovec *iovs, int *iov_cnt, uint32_t *desc_idx, uint32_t *xfer_direction) We definitely don't want to introduce a new such API for each vhost device. The proposal I gave is something like rte_vhost_vring_dequeue_burst(), which, as the name explains, just dequeues some vring entries and let the application to consume it. The application then could be a virtio-scsi device, virtio-crypto device, and even, a virtio-net device. Few more generic comments: - you touched way more code than necessary. - you should split your patches into some small patches: one patch just does one tiny logic. Doing one bunch of stuff in one patch is really hard for review. For example, in patch 1, you did: * move bunch of code from here and there * besides that, you even modified the code you moved. * introduce virtio_dev_table * split virtio_net_dev and introduce virtio_dev * change some vhost user message handler, say VHOST_USER_GET_QUEUE_NUM. * ... That's way too much for a single patch! If you think some functions are not well placed, that you want to move them to somewhere else, fine, just move it. And if you want to modify few of them, that's fine, too. But you should make the changes in another patch. This helps review, and what's more importantly, it helps us to locate buggy code if any. Just assume you introduced a bug in patch 1, it's so big a patch that it's hard for human to spot it. Later, someone reported that something is broken and he make a bisect and show this patch is the culprit. However, it's so big a patch, that even we know there is a bug there, it may take a lot of time to figure out which change breaks it. If you're splitting patches properly, the bug code could even be spotted in review time. That are some generic comments about making patches to introduce something big. Besides, I'd like to state again, it seems you are heading the wrong direction: again, you touched way too much code than necessary to add vhost-scsi support. In a rough thinking, it could be simple as: - handle vring queues correctly for vhost-scsi; currently, it sticks to virtio-net queue pairs. - add vring operation functions, such as dequeue/enqueue vrings, update used rings, ... - add vhost-scsi messages - may need change they way to trigger new_device() callback for vhost-scsi device. Above should be enough (I guess). And again, please make one patch for each item. Besides the 2nd item may introduce some code, others should be small changes. And, let us forget about the names so far, just reuse what we have. Say, don't bother to introduce virtio_dev, just use virtio_net (well, I don't object to make the change now, only if you can do it elegantly). Also, let's stick to the rte_virtio_net.h as well: let's make it right later. So far, just let us focus on what's need be done to make vhost-scsi work. Okay to you guys? --yliu