Moving ethernet VLAN tags into the mbuf packet header (from mtags)

[Yar Tikhiy]

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