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

[Andre Oppermann]

Yar Tikhiy wrote:
> 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.

I'd like to keep the ether_ prefix to make it clear who this pkthdr
field belongs to.  What do you think of pkthdr.ether_vtag?  Is that
more descriptive for this purpose to avoid the confusion you are
referring to?

> 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.

What you describe here is not an approach currently done by the BSD
mbuf system (that is having protocol specific mbuf headers).  The
current way of doing these things is to attach mtags to the mbufs
as it is currently done with ethernet vlans.  The trouble here is
the additional overhead for allocating the mtags and the potential
cache busting of following an mtag chain.

Actually I agree that having protocol specific fields in the mbuf
pkthdr is not clean design.  But on the other hand it's always a
tradeoff between clean design, performance and ugliness.  Mach has
a very clean design but simply doesn't perform.  We have to find a
balance without getting into ugly because then the tradeoff gets
negative and causes lots of pain and complicated maintenance in
the future.

> 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.

Good point.  This can certainly be improved.  For the first patch
I tried to be as mechanical as I could be.

> 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? ;-)

Sure.  Thanks for your comments.

-- 
Andre