From owner-freebsd-net@FreeBSD.ORG Tue Sep 12 11:43:17 2006 Return-Path: X-Original-To: freebsd-net@freebsd.org Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 01DE616A40F; Tue, 12 Sep 2006 11:43:17 +0000 (UTC) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (comp.chem.msu.su [158.250.32.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id F119143D6B; Tue, 12 Sep 2006 11:43:15 +0000 (GMT) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.13.4/8.13.3) with ESMTP id k8CBhDQ8009486; Tue, 12 Sep 2006 15:43:13 +0400 (MSD) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.13.4/8.13.3/Submit) id k8CBhDEi009485; Tue, 12 Sep 2006 15:43:13 +0400 (MSD) (envelope-from yar) Date: Tue, 12 Sep 2006 15:43:13 +0400 From: Yar Tikhiy To: Andre Oppermann Message-ID: <20060912114312.GE8639@comp.chem.msu.su> References: <450035AD.3040600@freebsd.org> <20060908010835.GA6334@heff.fud.org.nz> <45012EAA.4010303@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <45012EAA.4010303@freebsd.org> User-Agent: Mutt/1.5.9i Cc: freebsd-net@freebsd.org, Andrew Thompson , freebsd-arch@freebsd.org Subject: Re: Moving ethernet VLAN tags into the mbuf packet header (from mtags) X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2006 11:43:17 -0000 On Fri, Sep 08, 2006 at 10:49:46AM +0200, Andre Oppermann wrote: > Andrew Thompson wrote: > >On Thu, Sep 07, 2006 at 05:07:25PM +0200, Andre Oppermann wrote: > >>With the recent addition of a 16bit field for TSO into the mbuf packet > >>header we've got 16bits left over. I've reserved these bits for the > >>ethernet VLAN tagging of packet to do away with the allocated mbuf mtag. > >> > >>The change is rather mechanical. Patch available here: > >> > >> http://people.freebsd.org/~andre/vlan_pkthdr-20060907.diff > >> > > > >RCS file: /home/ncvs/src/sys/netgraph/ng_vlan.c,v > >retrieving revision 1.3 > >diff -u -p -r1.3 ng_vlan.c > >--- netgraph/ng_vlan.c 20 Apr 2005 14:19:20 -0000 1.3 > >+++ netgraph/ng_vlan.c 7 Sep 2006 15:03:58 -0000 > > > ><...> > > > >- vlan = EVL_VLANOFTAG(VLAN_TAG_VALUE(mtag)); > >+ vlan = m->m_pkthdr.ether_vlan; > > (void)&evl; /* XXX silence GCC */ > > > >I think this is a typeo, EVL_VLANOFTAG is still needed. I like the > >change and it helps out a few related projects that people are working > >on. > > Fixed. Thanks for the review! It seems to me that the typo just highlighted not-so-optimal naming of the new field. We must distinguish between VLAN ID's and VLAN tags clearly. A VLAN ID is what can be passed to "ifconfig foo0 vlan ___". A VLAN tag is a 16-bit value ready to be stored in the respective field of the 802.1Q header, except for its byte order. I'd rather have the new field renamed to just "vlan_tag" to avoid further confusion. In long perspective, however, stuffing protocol-specific fields in the generic mbuf header is no good. I share Julian's point here. We already can allocate simple mbufs as well as mbufs with m_pkthdr. This scheme is begging to be generalised. Then we should be able to allocate mbufs crafted for a particular protcol at once. Now I can't but do some nitpicking :-) In if_vlan.c, the "inenc" variable is set to 0 or 1 depending on the branch taken in the if-else clause. Then why to initialize it at its definition? I think that the better style would be: int inenc; if (m->m_flags & M_VLAN) { ... inenc = 0; } else { ... inenc = 1; } AFAIK, C compilers are clever enough to avoid false "uninitialized variable" warning for the code. Lastly, I've just noticed that the M_VLAN flag isn't documented in mbuf(9) yet. Could you document it along with the results of your work when it's finished? ;-) -- Yar