From owner-svn-src-head@FreeBSD.ORG Thu Aug 22 04:47:07 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id CE25A9DC; Thu, 22 Aug 2013 04:47:07 +0000 (UTC) (envelope-from lstewart@freebsd.org) Received: from lauren.room52.net (lauren.room52.net [210.50.193.198]) by mx1.freebsd.org (Postfix) with ESMTP id 4C1BC2C84; Thu, 22 Aug 2013 04:47:06 +0000 (UTC) Received: from lstewart.caia.swin.edu.au (lstewart.caia.swin.edu.au [136.186.229.95]) by lauren.room52.net (Postfix) with ESMTPSA id F0B4F7E81E; Thu, 22 Aug 2013 14:46:57 +1000 (EST) Message-ID: <521597C1.6070805@freebsd.org> Date: Thu, 22 Aug 2013 14:46:57 +1000 From: Lawrence Stewart User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130817 Thunderbird/17.0.8 MIME-Version: 1.0 To: Andre Oppermann Subject: Re: svn commit: r254524 - head/sys/sys References: <201308191356.r7JDuELE075073@svn.freebsd.org> <521257E2.4020502@FreeBSD.org> <5213AAA1.6020700@freebsd.org> <5214DD31.6010205@freebsd.org> <5214E86F.1060102@freebsd.org> In-Reply-To: <5214E86F.1060102@freebsd.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY autolearn=unavailable version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on lauren.room52.net Cc: Davide Italiano , svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Navdeep Parhar X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Aug 2013 04:47:08 -0000 On 08/22/13 02:18, Andre Oppermann wrote: > On 21.08.2013 17:59, Davide Italiano wrote: >> On Wed, Aug 21, 2013 at 5:30 PM, Andre Oppermann >> wrote: >>> On 20.08.2013 20:13, Davide Italiano wrote: >>>> >>>> On Tue, Aug 20, 2013 at 7:42 PM, Andre Oppermann >>>> wrote: >>>>> >>>>> On 19.08.2013 19:37, Navdeep Parhar wrote: >>>>>> >>>>>> Why reuse the freed up bits so soon (at least one of which >>>>>> I think was prematurely GC'ed -- see my other email on >>>>>> M_NOFREE). There was room beyond M_HASHTYPEBITS, no? >>>>> >>>>> >>>>> This is HEAD where kernel and modules have to be (re)compiled >>>>> together at any point in time. On stable this reuse would >>>>> not have been possible. >>>>> >>>>> In a subsequent commit I compacted and ordered the flags. >>>>> There's a couple of free ones remaining. >>>>> >>>>> I have some additional mbuf changes coming up to be posted >>>>> for possible objections later today. The close HEAD freeze >>>>> deadline has got me rushed a bit to allow 10.x backporting of >>>>> the checksum/offload overhaul. >>>>> >>>>> -- Andre >>>>> >>>> >>>> In my opinion the possibility we have about breaking KPI/KBI >>>> should not be abused, even if it's allowed in HEAD. In other >>>> words,people should go for preserving it when (as in this >>>> case) it's easy and without additional costs. Your point about >>>> "this is HEAD, it can be broken at any time" makes relatively >>>> little sense to me. Note that in the worst case such KPI/KBI >>>> breakages are annoying for $VENDORS who maintain out-of-tree >>>> code, which need to rebuild/change their code, or, if they get >>>> pissed off, drop FreeBSD support. In your case, it's just a >>>> matter of "code cleaness" about few lines, which, IMHO and >>>> always IMHO, doesn't justify the breakage. >>> >>> >>> Preserving the API but having to recompile is always fair game in >>> HEAD. In fact it happens all the time and the only supported way >>> to track HEAD is to recompile kernel and modules together. >>> >>> For removing (crufty) functionality I agree that it shouldn't be >>> done just for the sake of it and also shouldn't be done often. >>> Sometimes it is necessary in the name of progress. Having to >>> support old, crufty or broken ways of doing something is a waste >>> of developer time too. >>> >>> It's always a judgement call. In this case there is a perfectly >>> valid alternative way of doing that also is less dangerous to >>> leak mbufs. >>> >>> -- Andre >>> >> >> I don't see in any way how flags reordering might be in any way >> connected to mbufs leaks, alas. There's a similar recent('ish) >> discussion and it was decided to not compact/reorder flags. See >> r253662/r253775 for references. So, I'm not sure what's the >> de-facto policy, but still, as a community FreeBSD should decide >> one strategy and be stuck with that. It's a matter of being >> consistent, which, IMHO, is something that should not be >> undervaluated. > > I fail to see what problem you have with the flag reordering. > Recompile and done. That's why we work with #defines instead of > magic 0x1234 values. On a general note, Navdeep's and Davide's objections are fair and in line with defacto policy and established practice. Breaking the KBI because we allow ourselves to on head doesn't mean we should, especially when there is an alternative approach that preserves it, achieves the same outcome and would be an equivalent amount of effort. As noted by Navdeep, there are 4 bits beyond M_HASHTYPEBITS which could have been used. We want to allow as much time to elapse as possible between deprecating a bit's use/meaning and reusing it for something new. As an aside, embedding the date a bit became "free" in the comment next to the bit would be useful to ensure eventual bit reuse reclaims the oldest free bit(s) first. > If someone outside the tree was using the spare bits for their own > private purposes, yes, they would quickly have to check and possibly > adjust them. That's trivial however and should be expected when > tracking an OpenSource operating system. To be expected, sure, but we should only inflict the pain when it is truly necessary. Determining necessity should be straight forward in this instance: Are there other changes on the immediate horizon for 10.0 that require new bits? If yes, how many bits? If the answer is no, the recently deprecated bits should be reinstated as they were and M_PROTOFLAGS[9-12] should be moved to after M_HASHTYPEBITS. If yes, moving M_PROTOFLAGS[9-12] to after M_HASHTYPEBITS is probably a waste of time/effort (depending on how many bits are needed) as the new functionality will have to recycle some of the free bits anyway. In addition to this, the reordering done in r254527 is also a POLA and KBI issue for no gain. The flags easily fit on a single screen of text so it's not like they couldn't all be seen together easily. Changing bit definitions in such an important subsystem for purely cosmetic gain is a bad idea and I would argue in favour of reverting the reordering done in r254527 (which should have been committed as a separate commit anyway - lumping everything in one commit was unfortunate). The fact these patches don't have a "Reviewed by" line is of concern. "Discussed with" != "Reviewed by". I appreciate some of us (myself included) got a bit caught off guard by the proposed 10.0 timeline, but rushing changes, especially to critical infrastructure, without review is not a good idea. Cheers, Lawrence