[dpdk-dev] [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library

[Yuanhan Liu]

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