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
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
