Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 21 Feb 2002 01:12:10 -0800
From:      Terry Lambert <tlambert2@mindspring.com>
To:        Matthew Dillon <dillon@apollo.backplane.com>
Cc:        Greg Lehey <grog@FreeBSD.ORG>, Jake Burkholder <jake@locore.ca>, David O'Brien <obrien@FreeBSD.ORG>, current@FreeBSD.ORG
Subject:   Re: Patch to improve mutex collision performance
Message-ID:  <3C74B9EA.9692E27E@mindspring.com>
References:  <200202181912.g1IJCGK32122@apollo.backplane.com> <20020218114326.A98974@dragon.nuxi.com> <200202181951.g1IJpip33604@apollo.backplane.com> <20020218153807.E96115@locore.ca> <20020221111915.N65817@wantadilla.lemis.com> <200202210146.g1L1kqg91511@apollo.backplane.com> <3C746FC1.897E759C@mindspring.com> <200202210435.g1L4Z0H92642@apollo.backplane.com>

next in thread | previous in thread | raw e-mail | index | archive | help
Matthew Dillon wrote:
[ ... ]
> :How do you reconcile these divergent points of view?
> 
>     These are not divergent points of view.  I am saying quite clearly that
>     the ucred code and proc-locking code can be committed in a piecemeal
>     fashion.  In fact, good chunk of the proc locking code already has been
>     with no ill effect on the tree - it might not be completely operational
>     due to Giant, but just comitting it tests a great deal of infrastructure
>     including potential lock order reversals and mismatched locks.  That's
>     the whole point of doing things this way.

If you'll notice, the post to which you are replying was
posted before your last post on the same topic.


> :Do you have a set of rules, that would let us look at a patch
> :set and instantly decide which of these two categories the
> :code fell into?
> :
> :I'm not trying to be a jerk, but not everything can be broken
> :down into 1 line commits, and not everything can be broken down
> :into 8 line commits, or 64 line commits, or 512 line commits,
> :etc. (if you'll forgive my proof by induction).
> 
>     This is getting a bit absurd.  I am arguing a general principle here,
>     you are not contributing anything by stretching it into an irrational
>     statement.  One line commits?  Oh come on!

I'm asking because I've made changes like this which never
got in, and the review cycle was such that unless they were
reviewed immediately, the patches wouldn't cleanly apply.

In other words, anything that makes an interface change is
going to fall into the category os "too big".


I've also done changes that were very large scale, and could
be broken up, but when they were actually broken up, there
was no obvious benefit to about 3/4's of them, so the broken
up pieces were labelled as "gratuitous", when in fact they
were necessary foundation work for the last 1/4 of the changes.


Rarely, I've made sweeping changes, and they've been sheparded
into the source tree by Julian's dogged (and appreciated)
persistance, e.g. when I rewrote init_main.c and added the
original SYSINIT() construct using linker sets, or reinvented
later by someone else, e.g. my nameifree() changes to make it
so that there was no longer a "caller allocate/callee free"
interface for nameidata.

Other people's code has dropped by the wayside completely, and
been lost; the SACK/TSACK work Luigi did never got integrated
and accepted by the project, and LRP code that Peter Druschel
and Gaurav Banga did at Rice University, which was originally
done against FreeBSD 2.2.  For that matter, we've also lost
out on integration of the IO-Lite code, also from Rice (both
were a result of the ScalaServer project).  Likewise, the CMU
work on TCP Rate Halving (admittedly, it's based on NetBSD 1.3.2,
but that's not that significantly different from FreeBSD to matter),
as well as their FACK/SACK implementation.


It just seems to me that FreeBSD is blocking integration of
important research advances, and I don't see a clear criteria
being used to make this decision... a "general principle".


>     I will say that the work *I* am doing on -current is mostly piecemeal
>     in nature.  I even expect the VM locking to wind up being piecemeal.
>     Everything I have posted to date has been piecemeal.  For example,
>     the ucred patchset I posted does not patch all the ucred functions,
>     it just patched the read-only functions.  But as a side effect that
>     gave me a basis to track down the other uses of Giant in the general
>     syscall path.  That was a good demarkation point for me.  It is by
>     no means the end... it is, as I have said, piecemeal.


Not "everything to date"; "everything recently", perhaps.

It was not so long ago that you had your commit priviledges
yanked for, in effect, producing code faster than it could
be reviewed and vetted, so you checked in anyway.

You've got them back, and have since got the "incremental
changes" religion: there are none so zealous as a recent
convert, it seems.


Right now, you're trying to enforce this world-view, which
was enforced on you, on others.  All I'm asking is "where is
the line in the sand?".


I know that a lot of the people doing work in the P4
repository would just as soon be commiting to the main
CVS repository, instead, but, frankly, they are not
*permitted* to do this.  So they do the work in P4, in
order to build a consensus around the code, so that it
can be jammed in over the "go slooooooooow" objections.


>     The result?  I was able to immediately note the use of Giant in
>     trap.c (the ucred clearing code) as well as its use in userret(),
>     plus I could test a few of the ucred functions like getuid() and test
>     the Giant instrumentation.

Look, I'm not faulting your work, or your choice of the
"interesting problems" you choose to work on, but I do
maintain that not everything can be done as incrementally
as you seem to want it done these days.

You as much as admitted this yourself in your previous
postings, and then went on to say that committing the
prime example of current work of this nature is "too
dangerous".  All I'm asking for is "where is ``dangerous,
but not too dangerous'' so it can be targeted".


>     So you see, a piecemeal commit can have a great ability to move the
>     development process forward.  When you spend weeks or months putting
>     together an UBER-patch you do not get any of that... it's all delayed
>     and when the patch finally goes in people wind up spending a lot of
>     time testing the patch itself rather then working on the ramification
>     of what the patch allows you to move on to.

That works great for incremental changes, which can be
brought in peicemeal.  Interface changes to widely used
interfaces don't fall into that category (CAM is the biggest
recent example of this, IMO).

Patches which are, for lack of a better metaphor, nothing
but road-bed materials, could never get in over the hurdle
of "gratuitous changes".

It's amazingly ironic that major changes (e.g. newbus) are
allowed in without usage documentation at all, whereas
things like SACK or FACK, with dozens of research papers
aren't.


Again, not to be a jerk, but I'd like to see the "dangerous"
code committed to the main source tree.  Willing testers
have not worked in the past, so unwilling testers (like those
for CAM and the character device removal changes) should be
used: people have had their chance so far, and if they have
not availed themselves of it, well, we have the choice of
incrementally making the changes only to find out right before
5.0 ships that there was some part of the problem space not
mapped by the code, that wasn't seen because no one got there
in time.

It's best to fail fast and early, and so have room to try again,
then to fail slow and late, and not have time to retry.

This is basically the only way to reconcile your complaint
about developement going forward in the P4 repository, and
leaving the main CVS repository "out of sync".

-- Terry

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3C74B9EA.9692E27E>