From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Liu, Changpeng" Subject: Re: [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library Date: Wed, 14 Sep 2016 04:46:21 +0000 Message-ID: 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> <20160914032842.GB10323@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "Harris, James R" To: Yuanhan Liu Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 780127F14 for ; Wed, 14 Sep 2016 06:46:25 +0200 (CEST) In-Reply-To: <20160914032842.GB10323@yliu-dev.sh.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > Sent: Wednesday, September 14, 2016 11:29 AM > To: Liu, Changpeng > Cc: dev@dpdk.org; Harris, James R > Subject: Re: [PATCH v2 2/2] vhost: add vhost-scsi support to vhost librar= y >=20 > 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 >=20 > 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). >=20 > 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. >=20 > > +static void > > +cleanup_vq(struct vhost_virtqueue *vq, int destroy) > > +{ > > + if ((vq->callfd >=3D 0) && (destroy !=3D 0)) > > + close(vq->callfd); > > + if (vq->kickfd >=3D 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 =3D get_scsi_device(device); > > + uint32_t i; > > + > > + dev->features =3D 0; > > + dev->protocol_features =3D 0; > > + > > + for (i =3D 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 =3D VIRTIO_UNINITIALIZED_EVENTFD; > > + vq->callfd =3D VIRTIO_UNINITIALIZED_EVENTFD; > > + > > + /* Backends are set to -1 indicating an inactive device. */ > > + vq->backend =3D -1; > > + vq->enabled =3D 1; > > +} > > + Agreed, cleanup_vq and init_vring_queue can be eliminated as duplicated cod= e here. >=20 > [... snipped a bunch of duplicated code ...] >=20 > > +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) >=20 > We definitely don't want to introduce a new such API for each vhost devic= e. > 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. >=20 > Few more generic comments: >=20 > - you touched way more code than necessary. >=20 > - 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: >=20 > * 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. > * ... >=20 > That's way too much for a single patch! Agreed, the 2 patch set I sent as RFC purpose, I will break it into small p= atches at last.=20 >=20 > 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 anoth= er > patch. >=20 > 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. >=20 > If you're splitting patches properly, the bug code could even be spotte= d > in review time. >=20 > That are some generic comments about making patches to introduce someth= ing > big. >=20 >=20 > 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: >=20 > - handle vring queues correctly for vhost-scsi; currently, it sticks to > virtio-net queue pairs. >=20 > - add vring operation functions, such as dequeue/enqueue vrings, update > used rings, ... >=20 > - add vhost-scsi messages >=20 > - may need change they way to trigger new_device() callback for > vhost-scsi device. >=20 > Above should be enough (I guess). And again, please make one patch for ea= ch > item. Besides the 2nd item may introduce some code, others should be sma= ll > changes. >=20 > 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, le= t's > stick to the rte_virtio_net.h as well: let's make it right later. >=20 > So far, just let us focus on what's need be done to make vhost-scsi work. > Okay to you guys? Cannot agree with this comments, as you already know that virtio_net and vi= rtio_scsi are different devices, why should add SCSI related logic into virtio_net fi= le, just because it's easy for code review? >=20 > --yliu