From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH v2 2/2] vhost: add vhost-scsi support to vhost library Date: Wed, 14 Sep 2016 13:48:28 +0800 Message-ID: <20160914054828.GY23158@yliu-dev.sh.intel.com> References: <1473855300-3066-1-git-send-email-changpeng.liu@intel.com> <1473899298-4580-1-git-send-email-changpeng.liu@intel.com> <1473899298-4580-2-git-send-email-changpeng.liu@intel.com> <20160914032842.GB10323@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "dev@dpdk.org" , "Harris, James R" To: "Liu, Changpeng" Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id C79248025 for ; Wed, 14 Sep 2016 07:48:01 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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