From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f51.google.com (mail-wg0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 2D8985902 for ; Tue, 30 Sep 2014 10:00:05 +0200 (CEST) Received: by mail-wg0-f51.google.com with SMTP id b13so5721952wgh.34 for ; Tue, 30 Sep 2014 01:06:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=DqX99I6WWmMw/HX+k8ECKWaLpBgVYzg9b6taEwCwZg4=; b=HWr3xb6owgfEdGK+lBeLV8+5NpxFrm/uFKX3NqB5sP3AMkbYjPXZjcYaXgqAVXAaMS UbYwpEfltQAzTkBL5zkWiu6ksfkDSVd3T6gx5xTwJKz/FQQbPA8RMoirdNHF8gdBPKsR AYa9mJd+K34zOO2kevQMyRAuxFMb7Ie/cO43vF3bXF3mJDcmibR7cjVTziRpTQadBtZ2 yxyE8AUpt3kRo9MGlsws0xuK9xxF8jCRgm3BFFr/kKfweagXQa765QseXaixtFftZbcK v4l/hmMfbrYLhQZlxQ+AMKTy0HRixu0LTPxnjE0YtUBV7oAweIkYcXuY4MG9sXRxpxzB AeZw== X-Gm-Message-State: ALoCoQm+5zRxR38mLj+qpSEaTUkoMFL9zimSf5qrbnw1OX1+FmpA1tthsp08T2lJEKLVOlUkKrrr X-Received: by 10.180.24.226 with SMTP id x2mr10014925wif.1.1412064403721; Tue, 30 Sep 2014 01:06:43 -0700 (PDT) Received: from xps13.localnet (110.26.90.92.rev.sfr.net. [92.90.26.110]) by mx.google.com with ESMTPSA id u8sm14248932wia.24.2014.09.30.01.06.42 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Sep 2014 01:06:42 -0700 (PDT) From: Thomas Monjalon To: "Xie, Huawei" Date: Tue, 30 Sep 2014 10:06:21 +0200 Message-ID: <3689877.oAQ04SMz9F@xps13> Organization: 6WIND User-Agent: KMail/4.13.3 (Linux/3.15.8-1-ARCH; KDE/4.13.3; x86_64; ; ) In-Reply-To: References: <1411724758-27488-1-git-send-email-huawei.xie@intel.com> <70634298.6WfLhqUcss@xps13> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 08:00:05 -0000 2014-09-30 06:43, Xie, Huawei: > > 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 patch set > > > based on the example that already has this mbuf change? > > > > Yes > > > > > Some of the background you might not know: > > > I fully understand your concern here to make it a better patch and I fully agree > > > with you total comments. > > > This is really a special case. You know it is transform of thousand lines 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 format > > itself. :(. > > > > > 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. > > > > > Could we possibly treat it specially when we have comment whether the patch > > can be split/merged better? > > > > 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. > > > > 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 tool will n't help here. Please have a quick read through. I will start the rework 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/ > 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 drawback is gitk couldn't recognize main.c and vhost_rxtx.c have the same index. > 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 lib directory. The cons is gitk will treat all files in vhost lib as new files. No please move the files to the lib. It's easier to read. You can drop all the example in 1 patch or make as you did before: drop old example and rework it in subsequent patches. The most important part is the library. > patch 2: > rename virtio-net.c to rte_virtio_net.h It can be done in patch 1. > 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_(merge)_rx/tx and helper functions copy_mbuf_to_rings.. > comment here: > here virtio_dev_(merge)_rx/tx will refer non-existing functions like virtio_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. Yes please try to split and explain these important things. > 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. > Patch 12...: > Vhost example patch OK for the other ones. The global idea is to isolate minor changes in order to make important changes clearly visible in dedicated patches. -- Thomas