From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: "Liu, Changpeng" <changpeng.liu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Harris, James R" <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 13:48:28 +0800 [thread overview]
Message-ID: <20160914054828.GY23158@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <FF7FC980937D6342B9D289F5F3C7C2625AAB85F1@shsmsx102.ccr.corp.intel.com>
On Wed, Sep 14, 2016 at 04:46:21AM +0000, Liu, Changpeng wrote:
> > 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 want to let others to get your point easily, you should breat it
in the beginning, even for RFC.
>
> >
> > 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,
Not really, I'd think most of them are common. Looking at your implemention,
you just added "struct vhost_scsi_target scsi_target" for vhost-scsi device,
and changed virt_qp_nb to virt_q_nb. You may say, I hid few more fields
from virtio_net for vhost_scsi. Well, you are using 'union', I see no big
difference.
> just because
> it's easy for code review?
No, and I said, "I don't object to make the change now, only if you can
do it elegantly". And unfortunately, you were not heading that way.
--yliu
prev parent reply other threads:[~2016-09-14 5:48 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
2016-09-14 4:46 ` Liu, Changpeng
2016-09-14 5:48 ` Yuanhan Liu [this message]
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=20160914054828.GY23158@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).