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

Xie, Huawei huawei.xie at intel.com
Tue Sep 30 08:43:20 CEST 2014



> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Tuesday, September 30, 2014 12:46 PM
> To: Xie, Huawei
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 05/11] lib/librte_vhost: merge Oliver's mbuf
> change
> 
> 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. 

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_(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.
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

 
> Thanks
> --
> Thomas


More information about the dev mailing list