Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf change

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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