[dpdk-dev] [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library
Liu, Changpeng
changpeng.liu at intel.com
Wed Sep 14 06:46:21 CEST 2016
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> Sent: Wednesday, September 14, 2016 11:29 AM
> To: Liu, Changpeng <changpeng.liu at intel.com>
> Cc: dev at dpdk.org; Harris, James R <james.r.harris at intel.com>
> Subject: Re: [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library
>
> 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;
> > +}
> > +
Agreed, cleanup_vq and init_vring_queue can be eliminated as duplicated code here.
>
> [... 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!
Agreed, the 2 patch set I sent as RFC purpose, I will break it into small patches at last.
>
> 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?
Cannot agree with this comments, as you already know that virtio_net and virtio_scsi
are different devices, why should add SCSI related logic into virtio_net file, just because
it's easy for code review?
>
> --yliu
More information about the dev
mailing list