From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 745276A7D for ; Tue, 30 Sep 2014 08:36:45 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 29 Sep 2014 23:43:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="393397445" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by FMSMGA003.fm.intel.com with ESMTP; 29 Sep 2014 23:37:06 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 29 Sep 2014 23:43:22 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.203]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.204]) with mapi id 14.03.0195.001; Tue, 30 Sep 2014 14:43:21 +0800 From: "Xie, Huawei" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change Thread-Index: AQHP3B3Ts2Zmxo5h6kuMpa16GYv4yJwY9BnA//+ggQCAAJEn4A== Date: Tue, 30 Sep 2014 06:43:20 +0000 Message-ID: References: <1411724758-27488-1-git-send-email-huawei.xie@intel.com> <9382804.Ypo5if4EZ6@xps13> <70634298.6WfLhqUcss@xps13> In-Reply-To: <70634298.6WfLhqUcss@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Sep 2014 06:36:46 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Tuesday, September 30, 2014 12:46 PM > To: Xie, Huawei > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's= mbuf > change >=20 > 2014-09-30 02:41, Xie, Huawei: > > I would rework the patch according to your comment. > > I don't get clear about this comment. Do you mean that recreate the pat= ch set > > based on the example that already has this mbuf change? >=20 > Yes >=20 > > Some of the background you might not know: > > I fully understand your concern here to make it a better patch and I fu= lly agree > > with you total comments. > > This is really a special case. You know it is transform of thousand lin= es of code > with modifications. > > Sometimes a simple change could take me more than one day to rework the > patch, lines of lines manual check. > > I have already spent more than one week of time merely on the patch fo= rmat > itself. :(. >=20 > I know. I think you are learning (the hard way) how to use git. > As Ouyang said in this thread, you should use "git rebase" > and especially the --interactive mode to update your changes. > And you should make small commits at first. It's easier to squash > commits than splitting them. >=20 > > Could we possibly treat it specially when we have comment whether the p= atch > can be split/merged better? >=20 > I thought it many times because I see it causes you many troubles. > But I still think that vhost is an important feature and we probably > want to be able to understand what are the reasons behond the changes > by looking at the git history. That's why I'd like you to make smaller > refactoring commits with explanations in commit logs. >=20 > That's said, we should continue working together on it. > Send me your drafts and I'll help you to split them. The part I cannot do > by myself is about the explanations in commit logs. > Based on the principle "small commit", this is the rough idea. Btw, git too= l will n't help here. Please have a quick read through. I will start the re= work asap. Patch 1: copy examples/vhost/main.c /lib/librte_vhost/vhost/vhost_rxtx.c git mv examples/vhost/eventfd_link /lib/librte_vhost/ git mv examples/vhost/libvirt /lib/librte/vhost/libvirt git mv examples/vhost/vhost-net-cdev.c /lib/librte_vhost/ git mv examples/vhost/vhost-net-cdev.h /lib/librte_vhost/ =20 git mv examples/vhost/virtio-net.* /lib/librte_vhost/ comment: As in previous patch set, vhost_rxtx.c is partly copied from main.c, here I= decide to "copy" rather than "mv" main.c from example to vhost lib. The dr= awback is gitk couldn't recognize main.c and vhost_rxtx.c have the same ind= ex. The pros is even with mv, gitk couldn't recognize partly copy of vhost_rxtx= .c, and later we could patch vhost example based on existing files. Will emphasize that vhost_rxtx.c is a purely copy without any modification = in commit message. delete examples/vhost/Makefile as vhost example could no longer be compiled= from here after until the example patch. Another option is leave all example files, and copy needed ones to vhost li= b directory. The cons is gitk will treat all files in vhost lib as new file= s.=20 patch 2: rename virtio-net.c to rte_virtio_net.h patch 3: remove zero copy logic in related files. patch 4: remove switching related logics in related files. patch 5: delete all functions in vhost_rxtx.c except four functions virtio_dev_(mer= ge)_rx/tx and helper functions copy_mbuf_to_rings.. comment here: here virtio_dev_(merge)_rx/tx will refer non-existing functions like virti= o_tx_route. I think it is ok, right? patch 6: remove virtio_dev_tx, and rename virtio_dev_merge_tx to virtio_dev_tx. patch virtio_dev_rx, virtio_dev_merge_rx and virtio_dev_tx will see if this can be further divided. patch 7: Other minior fixes, like change global vars to static vars patch 8: fixes plenty of serious coding style issues Patch 9: Vhost API patch Patch 10: Added identified TODO or FIXME patch 11: Add vhost lib makefiles.=09 Patch 12...: Vhost example patch =20 > Thanks > -- > Thomas