Re: [dpdk-dev] Why nothing since 1.8.0?

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jan 16, 2015 at 07:18:19PM +0100, Thomas Monjalon wrote:
> 2015-01-16 12:20, Neil Horman:
> > On Thu, Jan 15, 2015 at 11:23:28PM +0100, Thomas Monjalon wrote:
> > > 2015-01-15 13:51, Neil Horman:
> > > > On Thu, Jan 15, 2015 at 06:25:33PM +0100, Thomas Monjalon wrote:
> > > > > 2015-01-15 08:06, Neil Horman:
> > > > > > Ok, I think what you're saying here is you're too busy to handle all the patches
> > > > > > comming in at the moment.  As such I'd like to propose a sub-tree encompassing
> > > > > > all the pmds in DPDK.  I would envision that including all the acutal pmd's in
> > > > > > the tree, as well as the infrastructure that is used to interface them to the
> > > > > > core (i.e. the ethdev/rte_ether library).  I'll gladly maintain the patch pool
> > > > > > and send you pull requests.
> > > > > 
> > > > > The list of PMDs is increasing:
> > > > > 	librte_pmd_af_packet
> > > > > 	librte_pmd_bond
> > > > > 	librte_pmd_e1000
> > > > > 	librte_pmd_enic
> > > > > 	librte_pmd_i40e
> > > > > 	librte_pmd_ixgbe
> > > > > 	librte_pmd_pcap
> > > > > 	librte_pmd_ring
> > > > > 	librte_pmd_virtio
> > > > > 	librte_pmd_vmxnet3
> > > > > 	librte_pmd_xenvirt
> > > > > There is already some sub-trees for bnx2x, fm10k and i40e:
> > > > > 	http://dpdk.org/browse/
> > > > > 
> > > > Yes, and I've mentioned before that that is an absolutely silly way to break out
> > > > subtrees.  You have to find a balance of workload distribution and developer
> > > > convienience.
> > > 
> > > Intel requested fm10k and i40e sub-trees because there are many developments
> > > in progress. We want to experience this model.
> > > 
> > Ok, but thats not the point.  Just because a given pmd has lots of changes
> > doesn't mean it itself needs its own tree.  With the right separation of
> > responsibilities all the pmds can be managed from one tree more easily and with
> > less distractions to the developers doing the work for those libraries.
> > 
> > > > I also note that these are problematic because you're not merging anything
> > > > from them. Is it your intention to keep bnx2 and fm10k separate in perpituity?
> > > 
> > > No, I'll merge them on pull requests.
> > > Note that they are planned for version 2.0 but not available yet.
> > > 
> > Ok, good on the pull request, but I don't really see that happening.  According
> > to this:
> > http://git.dpdk.org/dev/roadmap#cycle
> > If we plan a 2.0 release for mid march, counting backwards, we should be at the
> > review period stage.
> 
> ?? We are January 16 today, so according to http://dpdk.org/dev/roadmap#dates,
> we are not yet in review period.
> 
Ok, you still have two weeks, but my point still stands.  At least for I40e,
several of the outstanding patches have been on the list since November of last
year, and have been Acked. If we're separating the trees out, why are we waiting
so long to merge them, shouldn't that have been done by the subtree maintainer
by now?  The merge window is called a merge window for a reason, its the time
that code gets merged.


> > As of today in patchwork, I see 6 patches with I40e in the
> > title. Of those 3 have been acked, only one by someone who I think is likely a
> > subject matter expert on I40e.
> > 
> > Some of those patches have been sitting on the list since November 20th of last
> > year.
> > 
> > I think we're missing the point of a subtree.  Its created to both take some of
> > the load off of the upstream tree maintainer when the volume gets too high, and
> > it provides a location for developers to get bleeding edge code for a given
> > aspect of a project they might be interested in.  Neither of these things are
> > happening here.
> 
> Please let the things happen.
> If our experience shows that this subtree is not needed, it is possible to close it.
> I feel it can be convenient for first releases of new drivers.
> 
I'm not disagreeing, I'm advocating for the fact that theres no need to open and
close trees as it becomes (in)convienient.  At least not at such a fine
granularity.  A single subtree for all the pmds with one tree maintainer and
several subject matter experts to do reviews has proven to be highly efficient
and flexible in many other projects (most notably the linux kernel).

> > > 
> > > > If so, thats a real problem, because then we effectively just have several out
> > > > of tree drivers, and thats just unacceptible.
> > > 
> > > I don't understand what make you thinking that. They are -next tree, not out of tree.
> > > 
> > If they are the -next tree, then I apologize, because it certainly doesn't seem
> > that way so far.  But if they are, so be it.  That still leaves the outstanding
> > question though of, why one tree per pmd?  As I noted in other notes, the roles
> > of tree maintainer and driver maintainer (or as I would refer to it, subject
> > matter expert, for clairity) are separate ones.  The former is focused on the
> > merging of patches and general SCM process, while the latter is focused on
> > reviewing code within their purview.  When done properly, a single tree
> > maintainer can simply rely on the ACKs of the SME's to gate the merging of code,
> > and both parties can do their jobs much more efficiently.
> > 
> > > > > > If you could set me up with a login to dpdk.org, I'd appreciate it.
> > > > > 
> > > > > It is preferred to have 1 sub-tree per module.
> > > > > What do you think of managing contributions for af_packet and/or virtio?
> > > > > It would make sense as virtio is a RedHat technology.
> > > > > Maybe it could include vhost lib and example.
> > > > > 
> > > > No, for reasons I've mentioned before.  If you take each pmd/library and create
> > > > a subtree for it, you've created the most fine grained control of subtrees you
> > > > could ask for, but you've created a nighmare of a burden on developers who want
> > > > to update any code, especially if they have patches that hit multiple trees.
> > > 
> > > It's not planned to have a sub-tree for each library.
> > > And some sub-trees can be closed when activity decrease.
> > > 
> > But that need not happen.  If you create a single sub-tree for all the PMD's, it
> > will have a long life, and developers will have a long lived canonical source
> > from which to get the latest pmd code, and will enjoy the benefits of more
> > rapidly merged/reviewed code, if we follow the dual role approach that I've been
> > advocating.
> > 
> > > > Look at some of the stats in the dpdk tree:
> > > > 
> > > > Library		Commits between 1.7.0 and 1.8.0
> > > > librte_acl		5
> > > > librte_cfgfile		0
> > > > librte_cmdline		4
> > > > librte_compat		0
> > > > librte_distributor 	5
> > > > librte_eal 		125
> > > > librte_ether 		31
> > > > librte_hash 		1
> > > > librte_ip_frag 		5
> > > > librte_ivshmem 		0
> > > > librte_kni 		2
> > > > librte_kvargs 		0
> > > > librte_lpm 		1
> > > > librte_malloc 		1
> > > > librte_mbuf 		39
> > > > librte_mempool 		4
> > > > librte_meter 		0
> > > > librte_net 		4
> > > > librte_pipeline 	0
> > > > librte_pmd_af_packet 	4
> > > > librte_pmd_bond 	20
> > > > librte_pmd_e1000 	21
> > > > librte_pmd_enic 	12
> > > > librte_pmd_i40e 	90
> > > > librte_pmd_ixgbe 	83
> > > > librte_pmd_pcap 	4
> > > > librte_pmd_ring 	0
> > > > librte_pmd_virtio 	21
> > > > librte_pmd_vmxnet3 	21
> > > > librte_pmd_xenvirt 	6
> > > > librte_port 		6
> > > > librte_power 		3
> > > > librte_ring 		2
> > > > librte_sched 		1
> > > > librte_table 		7
> > > > librte_timer 		0
> > > > librte_vhost 		30
> > > > 
> > > > If you look at all of the pmds in the dpdk tree, we're talking about ~300
> > > > patches per release.  If you look at the net-next tree for the linux kernel,
> > > > Dave Miller merged 569 patches on his own (based on the following command:
> > > > git log --pretty=format:%H v3.17..v3.18 -- drivers/net/ethernet/ net/core/ | wc
> > > > -l)
> > > > 
> > > > And that doesn't account for the ~500 patches that come in via pull request from
> > > > the wireless subtree.  Nor does it account for the merge window for net-next
> > > > being 2 months instead of dpdk's 6 months.  Theres no need in any way for 12
> > > > maintainers to be twiddling their thumbs waiting on ~20 patches each, and for
> > > > that split, you've forced developers to potentially develop patches against 12
> > > > trees (12 being the current number of PMD's that are in the dpdk).
> > > 
> > > Please stop on this wrong assumption. We keep only 1 mailing-list and use only
> > > few sub-trees.
> > I never said you needed multiple mailing lists, and you can't assert a few
> > sub-trees, because you don't know how many more high volume pmd's you'll have in
> > the future.  A single pmd-subtree with a single maintainer and multiple SME's
> > mitigates that problem, just like it has in the kernel.
> > 
> > > 
> > > > The right answer here is balance.  Let me split out the pmd's and ethernet
> > > > infrastructure libraries to a subtree.  I'll pull in patches posted regarding
> > > > pmd's and librte_ether/ip_frag etc, and send you a pull requests after each
> > > > release so you get all the latest bits, and then pulls for stabilization on each
> > > > -rc. I can manage 300 patches without issue, and that takes a load off your
> > > > shoulders.  I'll get fm10k integrated, as well as bnx2.  That gives us a single
> > > > alternate tree for developers to go to for pmd and pmd infrastructure updates.
> > > > Its a win-win.
> > > 
> > > You misunderstood some things but I understand the global idea in your hard words.
> > > You're right when you say balance is important and we have to experience
> > > some solutions to find the right balance.
> > > 
> > > Note that the real challenge is not to push patches but to have them carefully
> > > reviewed. The answer is to make sure each area of DPDK is covered by at least
> > > one reviewer. Probably that a MAINTAINERS file could help here.
> > > 
> > YES!  That very thing!  We definately agree on that.  What I'm trying to point
> > out is that rewview/subject matter expert roles don't also have to be tree
> > maintainer roles.  In fact, they specficially should not be.  The trivial but
> > high volume work that is patch management distracts subject matter experts from
> > the detailed time consuming role of careful patch review.  If you split the
> > roles in two (like the kernel has done for years), you allows SME's to do their
> > job throughly and well, while allowing the tree maintainers to handle multiple
> > SME's and their code very efficiently.
> 
> So we agree on the most important thing.
> I'd like to try solving the review challenge first and see what else can be
> done after that. Step by step.
ok, the review challenge is solved by identifing a maintainer for each PMD, and
recording their name in a MAINTAINERS file, then requiring that they review
patches that touch files within their purview.  I propose we create such a file
and add in the names/emails of the individuals who are responsible for those
PMD's.  I'm happy to submit a patch to that effect.  I know Stephen Hemminger
has stepped up for bnx2x, who is the responsible person for I40e?

> During that time, few sub-trees are allowed to experience it.
> I suggest to continue this discussion after release 2.0. At that time, we'll
> be able to give some conclusions.
I really don't see why you need to devise some alternate workflow here. You
have a good corollary case to guide your actions (the linux kernel workflow),
but your choosing to ignore it despite having evidence that an alternate workflow
will be beneficial.  Why not start with something that you know if functional
and efficient, and start experimenting with alterations from there?

Neil

> 
> -- 
> Thomas
>