Date: Tue, 12 Sep 2006 15:43:13 +0400 From: Yar Tikhiy <yar@comp.chem.msu.su> To: Andre Oppermann <andre@freebsd.org> Cc: freebsd-net@freebsd.org, Andrew Thompson <thompsa@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: Moving ethernet VLAN tags into the mbuf packet header (from mtags) Message-ID: <20060912114312.GE8639@comp.chem.msu.su> In-Reply-To: <45012EAA.4010303@freebsd.org> References: <450035AD.3040600@freebsd.org> <20060908010835.GA6334@heff.fud.org.nz> <45012EAA.4010303@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060912114312.GE8639>