From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Changpeng Liu <changpeng.liu@intel.com>
Cc: dev@dpdk.org, james.r.harris@intel.com
Subject: Re: [dpdk-dev] [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library
Date: Wed, 14 Sep 2016 11:28:42 +0800 [thread overview]
Message-ID: <20160914032842.GB10323@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <1473899298-4580-2-git-send-email-changpeng.liu@intel.com>
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
next prev parent reply other threads:[~2016-09-14 3:28 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-14 12:15 [dpdk-dev] [PATCH] vhost: change the vhost library to a common framework which can support more VIRTIO devices Changpeng Liu
2016-09-13 12:58 ` Yuanhan Liu
2016-09-13 13:24 ` Thomas Monjalon
2016-09-13 13:49 ` Yuanhan Liu
2016-09-15 0:28 ` [dpdk-dev] [PATCH v2 1/2] " Changpeng Liu
2016-09-15 0:28 ` [dpdk-dev] [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library Changpeng Liu
2016-09-14 3:28 ` Yuanhan Liu [this message]
2016-09-14 4:46 ` Liu, Changpeng
2016-09-14 5:48 ` Yuanhan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160914032842.GB10323@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=changpeng.liu@intel.com \
--cc=dev@dpdk.org \
--cc=james.r.harris@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).