[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