[PATCH] deinline a few large functions in vlan code

[PATCH] deinline a few large functions in vlan code

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

Deinline a few functions which produce 150+ bytes of code.

This patch is slightly problematic: it moves __vlan_hwaccel_rx
and __vlan_put_tag from if_vlan.h into vlan_dev.c,
but vlan_dev can be modular, and then a bunch of network drivers
won't link.

What should be done with this?
1) Should I add respective select statements into Kconfigs
   of those drivers?
2) Make vlan_dev non-modular?
3) Move functions to another .c file?
--
vda

[-- Attachment #2: 2.6.16.vlan_inline2.patch --]
[-- Type: text/x-diff, Size: 6345 bytes --]

Size  Uses Wasted Name and definition
===== ==== ====== ================================================
  162   12   1562 vlan_hwaccel_receive_skb      include/linux/if_vlan.h
  162    4    426 vlan_hwaccel_rx       include/linux/if_vlan.h
  241    2    221 vlan_put_tag  include/linux/if_vlan.h

diff -urpN linux-2.6.16.org/include/linux/if_vlan.h linux-2.6.16.vlan/include/linux/if_vlan.h
--- linux-2.6.16.org/include/linux/if_vlan.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/include/linux/if_vlan.h	Thu Apr  6 22:11:23 2006
@@ -149,49 +149,9 @@ struct vlan_skb_tx_cookie {
 #define vlan_tx_tag_get(__skb)	(VLAN_TX_SKB_CB(__skb)->vlan_tag)
 
 /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
-static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
+int __vlan_hwaccel_rx(struct sk_buff *skb,
 				    struct vlan_group *grp,
-				    unsigned short vlan_tag, int polling)
-{
-	struct net_device_stats *stats;
-
-	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion.
-		 */
-		return 0;
-	}
-
-	skb->dev->last_rx = jiffies;
-
-	stats = vlan_dev_get_stats(skb->dev);
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
-
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST:
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST:
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	};
-
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
-}
+				    unsigned short vlan_tag, int polling);
 
 static inline int vlan_hwaccel_rx(struct sk_buff *skb,
 				  struct vlan_group *grp,
@@ -218,43 +178,7 @@ static inline int vlan_hwaccel_receive_s
  * Following the skb_unshare() example, in case of error, the calling function
  * doesn't have to worry about freeing the original skb.
  */
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
-{
-	struct vlan_ethhdr *veth;
-
-	if (skb_headroom(skb) < VLAN_HLEN) {
-		struct sk_buff *sk_tmp = skb;
-		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
-		kfree_skb(sk_tmp);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to realloc headroom\n");
-			return NULL;
-		}
-	} else {
-		skb = skb_unshare(skb, GFP_ATOMIC);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
-			return NULL;
-		}
-	}
-
-	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
-
-	/* Move the mac addresses to the beginning of the new header. */
-	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
-
-	/* first, the ethernet type */
-	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
-
-	/* now, the tag */
-	veth->h_vlan_TCI = htons(tag);
-
-	skb->protocol = __constant_htons(ETH_P_8021Q);
-	skb->mac.raw -= VLAN_HLEN;
-	skb->nh.raw -= VLAN_HLEN;
-
-	return skb;
-}
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag);
 
 /**
  * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
diff -urpN linux-2.6.16.org/net/8021q/vlan_dev.c linux-2.6.16.vlan/net/8021q/vlan_dev.c
--- linux-2.6.16.org/net/8021q/vlan_dev.c	Thu Apr  6 21:41:57 2006
+++ linux-2.6.16.vlan/net/8021q/vlan_dev.c	Thu Apr  6 22:11:52 2006
@@ -37,6 +37,102 @@
 #include <linux/if_vlan.h>
 #include <net/ip.h>
 
+/* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
+int __vlan_hwaccel_rx(struct sk_buff *skb,
+				    struct vlan_group *grp,
+				    unsigned short vlan_tag, int polling)
+{
+	struct net_device_stats *stats;
+
+	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
+	if (skb->dev == NULL) {
+		dev_kfree_skb_any(skb);
+
+		/* Not NET_RX_DROP, this is not being dropped
+		 * due to congestion.
+		 */
+		return 0;
+	}
+
+	skb->dev->last_rx = jiffies;
+
+	stats = vlan_dev_get_stats(skb->dev);
+	stats->rx_packets++;
+	stats->rx_bytes += skb->len;
+
+	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
+	switch (skb->pkt_type) {
+	case PACKET_BROADCAST:
+		break;
+
+	case PACKET_MULTICAST:
+		stats->multicast++;
+		break;
+
+	case PACKET_OTHERHOST:
+		/* Our lower layer thinks this is not local, let's make sure.
+		 * This allows the VLAN to have a different MAC than the underlying
+		 * device, and still route correctly.
+		 */
+		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
+			skb->pkt_type = PACKET_HOST;
+		break;
+	};
+
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+EXPORT_SYMBOL(__vlan_hwaccel_rx);
+
+/**
+ * __vlan_put_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @tag: VLAN tag to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ * 
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
+{
+	struct vlan_ethhdr *veth;
+
+	if (skb_headroom(skb) < VLAN_HLEN) {
+		struct sk_buff *sk_tmp = skb;
+		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
+		kfree_skb(sk_tmp);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to realloc headroom\n");
+			return NULL;
+		}
+	} else {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
+			return NULL;
+		}
+	}
+
+	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
+
+	/* Move the mac addresses to the beginning of the new header. */
+	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
+
+	/* first, the ethernet type */
+	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
+
+	/* now, the tag */
+	veth->h_vlan_TCI = htons(tag);
+
+	skb->protocol = __constant_htons(ETH_P_8021Q);
+	skb->mac.raw -= VLAN_HLEN;
+	skb->nh.raw -= VLAN_HLEN;
+
+	return skb;
+}
+EXPORT_SYMBOL(__vlan_put_tag);
+
 /*
  *	Rebuild the Ethernet MAC header. This is called after an ARP
  *	(or in future other address resolution) has completed on this

Re: [PATCH] deinline a few large functions in vlan code

[David S. Miller]

From: Denis Vlasenko <vda@ilport.com.ua>
Date: Fri, 7 Apr 2006 16:28:30 +0300

> What should be done with this?
> 1) Should I add respective select statements into Kconfigs
>    of those drivers?
> 2) Make vlan_dev non-modular?
> 3) Move functions to another .c file?

4) Leave it inline.

That's why we inline it in the first place.

Re: [PATCH] deinline a few large functions in vlan code

[Denis Vlasenko]

On Friday 07 April 2006 23:25, David S. Miller wrote:
> From: Denis Vlasenko <vda@ilport.com.ua>
> Date: Fri, 7 Apr 2006 16:28:30 +0300
> 
> > What should be done with this?
> > 1) Should I add respective select statements into Kconfigs
> >    of those drivers?
> > 2) Make vlan_dev non-modular?
> > 3) Move functions to another .c file?
> 
> 4) Leave it inline.

Ok, I will leave them alone for now.

BTW, does it make any sense for a network driver to call
these functions in non-VLAN-enbled kernel?

IOW: shouldn't calls to these functions sit in
#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
block? For example, typhoon.c:

                spin_lock(&tp->state_lock);
+#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
                if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
                        vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
                                                 ntohl(rx->vlanTag) & 0xffff);
                else
+#endif
                        netif_receive_skb(new_skb);
                spin_unlock(&tp->state_lock);

Same for s2io.c, chelsio/sge.c, etc...
--
vda

Re: [PATCH] deinline a few large functions in vlan code

[David S. Miller]

From: Denis Vlasenko <vda@ilport.com.ua>
Date: Mon, 10 Apr 2006 08:28:20 +0300

> IOW: shouldn't calls to these functions sit in
> #if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> block? For example, typhoon.c:
> 
>                 spin_lock(&tp->state_lock);
> +#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
>                 if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
>                         vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
>                                                  ntohl(rx->vlanTag) & 0xffff);
>                 else
> +#endif
>                         netif_receive_skb(new_skb);
>                 spin_unlock(&tp->state_lock);
> 
> Same for s2io.c, chelsio/sge.c, etc...

Very likely yes.  tp->vlgrp will never be non-NULL in such situations
so it's not a correctness issue, but rather an optimization :-)

[PATCH] deinline a few large functions in vlan code v2

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Monday 10 April 2006 08:45, David S. Miller wrote:
> From: Denis Vlasenko <vda@ilport.com.ua>
> Date: Mon, 10 Apr 2006 08:28:20 +0300
> 
> > IOW: shouldn't calls to these functions sit in
> > #if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> > block? For example, typhoon.c:
> > 
> >                 spin_lock(&tp->state_lock);
> > +#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> >                 if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
> >                         vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
> >                                                  ntohl(rx->vlanTag) & 0xffff);
> >                 else
> > +#endif
> >                         netif_receive_skb(new_skb);
> >                 spin_unlock(&tp->state_lock);
> > 
> > Same for s2io.c, chelsio/sge.c, etc...
> 
> Very likely yes.  tp->vlgrp will never be non-NULL in such situations
> so it's not a correctness issue, but rather an optimization :-)

Done. Please see attached.

Deinline a few functions which produce 150+ bytes of code.

They are moved into new file, net/core/dev_vlan.c, which
is compiled to nothing if VLAN support is not enabled at all
(even as a module).

A few drivers which were trying to call vlan routines
on non-vlan-enabled kernel are fixed.

Compile tested (allyesconfig + "# CONFIG_VLAN_8021Q is not set").

Signed-off-by: Denis Vlasenko <vda@ilport.com.ua>
--
vda

[-- Attachment #2: 2.6.16.1.vlan.patch --]
[-- Type: text/x-diff, Size: 16437 bytes --]

# size vmlinux.org vmlinux.new
    text    data     bss      dec     hex filename
20477010 6152182 1729804 28358996 1b0b954 vmlinux.org
20472545 6152150 1729804 28354499 1b0a7c3 vmlinux.new

diff -urpN linux-2.6.16.1.org/drivers/net/bnx2.c linux-2.6.16.1.vlan/drivers/net/bnx2.c
--- linux-2.6.16.1.org/drivers/net/bnx2.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/bnx2.c	Mon Apr 10 16:01:01 2006
@@ -4436,10 +4436,13 @@ bnx2_start_xmit(struct sk_buff *skb, str
 		vlan_tag_flags |= TX_BD_FLAGS_TCP_UDP_CKSUM;
 	}
 
+#ifdef BCM_VLAN
 	if (bp->vlgrp != 0 && vlan_tx_tag_present(skb)) {
 		vlan_tag_flags |=
 			(TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
 	}
+#endif
+
 #ifdef BCM_TSO 
 	if ((mss = skb_shinfo(skb)->tso_size) &&
 		(skb->len > (bp->dev->mtu + ETH_HLEN))) {
diff -urpN linux-2.6.16.1.org/drivers/net/bnx2.h linux-2.6.16.1.vlan/drivers/net/bnx2.h
--- linux-2.6.16.1.org/drivers/net/bnx2.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/bnx2.h	Mon Apr 10 16:01:01 2006
@@ -40,7 +40,9 @@
 #include <linux/mii.h>
 #ifdef NETIF_F_HW_VLAN_TX
 #include <linux/if_vlan.h>
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 #define BCM_VLAN 1
+#endif
 #endif
 #ifdef NETIF_F_TSO
 #include <net/ip.h>
diff -urpN linux-2.6.16.1.org/drivers/net/bonding/bond_alb.c linux-2.6.16.1.vlan/drivers/net/bonding/bond_alb.c
--- linux-2.6.16.1.org/drivers/net/bonding/bond_alb.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/bonding/bond_alb.c	Mon Apr 10 16:01:01 2006
@@ -502,6 +502,7 @@ static void rlb_update_client(struct rlb
 
 		skb->dev = client_info->slave->dev;
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (client_info->tag) {
 			skb = vlan_put_tag(skb, client_info->vlan_id);
 			if (!skb) {
@@ -511,7 +512,7 @@ static void rlb_update_client(struct rlb
 				continue;
 			}
 		}
-
+#endif
 		arp_xmit(skb);
 	}
 }
@@ -870,7 +871,9 @@ static void rlb_clear_vlan(struct bondin
 
 static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 {
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	struct bonding *bond = bond_get_bond_by_slave(slave);
+#endif
 	struct learning_pkt pkt;
 	int size = sizeof(struct learning_pkt);
 	int i;
@@ -898,6 +901,7 @@ static void alb_send_learning_packets(st
 		skb->priority = TC_PRIO_CONTROL;
 		skb->dev = slave->dev;
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (!list_empty(&bond->vlan_list)) {
 			struct vlan_entry *vlan;
 
@@ -918,7 +922,7 @@ static void alb_send_learning_packets(st
 				continue;
 			}
 		}
-
+#endif
 		dev_queue_xmit(skb);
 	}
 }
diff -urpN linux-2.6.16.1.org/drivers/net/bonding/bond_main.c linux-2.6.16.1.vlan/drivers/net/bonding/bond_main.c
--- linux-2.6.16.1.org/drivers/net/bonding/bond_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/bonding/bond_main.c	Mon Apr 10 16:01:01 2006
@@ -366,6 +366,7 @@ struct vlan_entry *bond_next_vlan(struct
  */
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev)
 {
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	unsigned short vlan_id;
 
 	if (!list_empty(&bond->vlan_list) &&
@@ -380,9 +381,9 @@ int bond_dev_queue_xmit(struct bonding *
 			 */
 			return 0;
 		}
-	} else {
+	} else
+#endif
 		skb->dev = slave_dev;
-	}
 
 	skb->priority = 1;
 	dev_queue_xmit(skb);
@@ -2270,6 +2271,7 @@ static void bond_arp_send(struct net_dev
 		printk(KERN_ERR DRV_NAME ": ARP packet allocation failed\n");
 		return;
 	}
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (vlan_id) {
 		skb = vlan_put_tag(skb, vlan_id);
 		if (!skb) {
@@ -2277,6 +2279,7 @@ static void bond_arp_send(struct net_dev
 			return;
 		}
 	}
+#endif
 	arp_xmit(skb);
 }
 
diff -urpN linux-2.6.16.1.org/drivers/net/chelsio/sge.c linux-2.6.16.1.vlan/drivers/net/chelsio/sge.c
--- linux-2.6.16.1.org/drivers/net/chelsio/sge.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/chelsio/sge.c	Mon Apr 10 16:01:01 2006
@@ -978,6 +978,7 @@ static int sge_rx(struct sge *sge, struc
 	} else
 		skb->ip_summed = CHECKSUM_NONE;
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (unlikely(adapter->vlan_grp && p->vlan_valid)) {
 		sge->port_stats[p->iff].vlan_xtract++;
 		if (adapter->params.sge.polling)
@@ -986,7 +987,9 @@ static int sge_rx(struct sge *sge, struc
 		else
 			vlan_hwaccel_rx(skb, adapter->vlan_grp,
 					ntohs(p->vlan));
-	} else if (adapter->params.sge.polling)
+	} else
+#endif
+	if (adapter->params.sge.polling)
 		netif_receive_skb(skb);
 	else
 		netif_rx(skb);
diff -urpN linux-2.6.16.1.org/drivers/net/e1000/e1000_main.c linux-2.6.16.1.vlan/drivers/net/e1000/e1000_main.c
--- linux-2.6.16.1.org/drivers/net/e1000/e1000_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/e1000/e1000_main.c	Mon Apr 10 16:01:01 2006
@@ -3714,23 +3714,25 @@ e1000_clean_rx_irq(struct e1000_adapter 
 
 		skb->protocol = eth_type_trans(skb, netdev);
 #ifdef CONFIG_E1000_NAPI
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (unlikely(adapter->vlgrp &&
 			    (status & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 						 le16_to_cpu(rx_desc->special) &
 						 E1000_RXD_SPC_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_receive_skb(skb);
-		}
 #else /* CONFIG_E1000_NAPI */
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (unlikely(adapter->vlgrp &&
 			    (status & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 					le16_to_cpu(rx_desc->special) &
 					E1000_RXD_SPC_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_rx(skb);
-		}
 #endif /* CONFIG_E1000_NAPI */
 		netdev->last_rx = jiffies;
 #ifdef CONFIG_E1000_MQ
@@ -3861,21 +3863,23 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 			   cpu_to_le16(E1000_RXDPS_HDRSTAT_HDRSP)))
 			adapter->rx_hdr_split++;
 #ifdef CONFIG_E1000_NAPI
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->wb.middle.vlan) &
 				E1000_RXD_SPC_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_receive_skb(skb);
-		}
 #else /* CONFIG_E1000_NAPI */
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if (unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->wb.middle.vlan) &
 				E1000_RXD_SPC_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_rx(skb);
-		}
 #endif /* CONFIG_E1000_NAPI */
 		netdev->last_rx = jiffies;
 #ifdef CONFIG_E1000_MQ
diff -urpN linux-2.6.16.1.org/drivers/net/ixgb/ixgb_main.c linux-2.6.16.1.vlan/drivers/net/ixgb/ixgb_main.c
--- linux-2.6.16.1.org/drivers/net/ixgb/ixgb_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/ixgb/ixgb_main.c	Mon Apr 10 16:01:01 2006
@@ -1905,21 +1905,23 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a
 
 		skb->protocol = eth_type_trans(skb, netdev);
 #ifdef CONFIG_IXGB_NAPI
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if(adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->special) &
 					IXGB_RX_DESC_SPECIAL_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_receive_skb(skb);
-		}
 #else /* CONFIG_IXGB_NAPI */
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if(adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->special) &
 					IXGB_RX_DESC_SPECIAL_VLAN_MASK);
-		} else {
+		} else
+#endif
 			netif_rx(skb);
-		}
 #endif /* CONFIG_IXGB_NAPI */
 		netdev->last_rx = jiffies;
 
diff -urpN linux-2.6.16.1.org/drivers/net/s2io.c linux-2.6.16.1.vlan/drivers/net/s2io.c
--- linux-2.6.16.1.org/drivers/net/s2io.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/s2io.c	Mon Apr 10 16:01:01 2006
@@ -5680,21 +5680,23 @@ static int rx_osm_handler(ring_info_t *r
 
 	skb->protocol = eth_type_trans(skb, dev);
 #ifdef CONFIG_S2IO_NAPI
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
 		/* Queueing the vlan frame to the upper layer */
 		vlan_hwaccel_receive_skb(skb, sp->vlgrp,
 			RXD_GET_VLAN_TAG(rxdp->Control_2));
-	} else {
+	} else
+#endif
 		netif_receive_skb(skb);
-	}
 #else
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if (sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
 		/* Queueing the vlan frame to the upper layer */
 		vlan_hwaccel_rx(skb, sp->vlgrp,
 			RXD_GET_VLAN_TAG(rxdp->Control_2));
-	} else {
+	} else
+#endif
 		netif_rx(skb);
-	}
 #endif
 	dev->last_rx = jiffies;
 	atomic_dec(&sp->rx_bufs_left[ring_no]);
diff -urpN linux-2.6.16.1.org/drivers/net/typhoon.c linux-2.6.16.1.vlan/drivers/net/typhoon.c
--- linux-2.6.16.1.org/drivers/net/typhoon.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/drivers/net/typhoon.c	Mon Apr 10 16:01:01 2006
@@ -1745,10 +1745,12 @@ typhoon_rx(struct typhoon *tp, struct ba
 			new_skb->ip_summed = CHECKSUM_NONE;
 
 		spin_lock(&tp->state_lock);
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 		if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
 			vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
 						 ntohl(rx->vlanTag) & 0xffff);
 		else
+#endif
 			netif_receive_skb(new_skb);
 		spin_unlock(&tp->state_lock);
 
diff -urpN linux-2.6.16.1.org/include/linux/if_vlan.h linux-2.6.16.1.vlan/include/linux/if_vlan.h
--- linux-2.6.16.1.org/include/linux/if_vlan.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/include/linux/if_vlan.h	Mon Apr 10 16:01:01 2006
@@ -149,49 +149,9 @@ struct vlan_skb_tx_cookie {
 #define vlan_tx_tag_get(__skb)	(VLAN_TX_SKB_CB(__skb)->vlan_tag)
 
 /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
-static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
+int __vlan_hwaccel_rx(struct sk_buff *skb,
 				    struct vlan_group *grp,
-				    unsigned short vlan_tag, int polling)
-{
-	struct net_device_stats *stats;
-
-	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion.
-		 */
-		return 0;
-	}
-
-	skb->dev->last_rx = jiffies;
-
-	stats = vlan_dev_get_stats(skb->dev);
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
-
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST:
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST:
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	};
-
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
-}
+				    unsigned short vlan_tag, int polling);
 
 static inline int vlan_hwaccel_rx(struct sk_buff *skb,
 				  struct vlan_group *grp,
@@ -218,43 +178,7 @@ static inline int vlan_hwaccel_receive_s
  * Following the skb_unshare() example, in case of error, the calling function
  * doesn't have to worry about freeing the original skb.
  */
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
-{
-	struct vlan_ethhdr *veth;
-
-	if (skb_headroom(skb) < VLAN_HLEN) {
-		struct sk_buff *sk_tmp = skb;
-		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
-		kfree_skb(sk_tmp);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to realloc headroom\n");
-			return NULL;
-		}
-	} else {
-		skb = skb_unshare(skb, GFP_ATOMIC);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
-			return NULL;
-		}
-	}
-
-	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
-
-	/* Move the mac addresses to the beginning of the new header. */
-	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
-
-	/* first, the ethernet type */
-	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
-
-	/* now, the tag */
-	veth->h_vlan_TCI = htons(tag);
-
-	skb->protocol = __constant_htons(ETH_P_8021Q);
-	skb->mac.raw -= VLAN_HLEN;
-	skb->nh.raw -= VLAN_HLEN;
-
-	return skb;
-}
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag);
 
 /**
  * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
diff -urpN linux-2.6.16.1.org/net/core/Makefile linux-2.6.16.1.vlan/net/core/Makefile
--- linux-2.6.16.1.org/net/core/Makefile	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.1.vlan/net/core/Makefile	Mon Apr 10 16:01:01 2006
@@ -7,7 +7,7 @@ obj-y := sock.o request_sock.o skbuff.o 
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
 
-obj-y		     += dev.o ethtool.o dev_mcast.o dst.o \
+obj-y		     += dev.o ethtool.o dev_mcast.o dev_vlan.o dst.o \
 			neighbour.o rtnetlink.o utils.o link_watch.o filter.o
 
 obj-$(CONFIG_XFRM) += flow.o
diff -urpN linux-2.6.16.1.org/net/core/dev_vlan.c linux-2.6.16.1.vlan/net/core/dev_vlan.c
--- linux-2.6.16.1.org/net/core/dev_vlan.c	Thu Jan  1 03:00:00 1970
+++ linux-2.6.16.1.vlan/net/core/dev_vlan.c	Mon Apr 10 16:01:01 2006
@@ -0,0 +1,110 @@
+/* 802.1q helpers.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
+
+#include <linux/skbuff.h>
+#include <linux/if_vlan.h>
+
+/* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
+int __vlan_hwaccel_rx(struct sk_buff *skb,
+				    struct vlan_group *grp,
+				    unsigned short vlan_tag, int polling)
+{
+	struct net_device_stats *stats;
+
+	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
+	if (skb->dev == NULL) {
+		dev_kfree_skb_any(skb);
+
+		/* Not NET_RX_DROP, this is not being dropped
+		 * due to congestion.
+		 */
+		return 0;
+	}
+
+	skb->dev->last_rx = jiffies;
+
+	stats = vlan_dev_get_stats(skb->dev);
+	stats->rx_packets++;
+	stats->rx_bytes += skb->len;
+
+	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
+	switch (skb->pkt_type) {
+	case PACKET_BROADCAST:
+		break;
+
+	case PACKET_MULTICAST:
+		stats->multicast++;
+		break;
+
+	case PACKET_OTHERHOST:
+		/* Our lower layer thinks this is not local, let's make sure.
+		 * This allows the VLAN to have a different MAC than the underlying
+		 * device, and still route correctly.
+		 */
+		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
+			skb->pkt_type = PACKET_HOST;
+		break;
+	};
+
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+EXPORT_SYMBOL(__vlan_hwaccel_rx);
+
+/**
+ * __vlan_put_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @tag: VLAN tag to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ * 
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
+{
+	struct vlan_ethhdr *veth;
+
+	if (skb_headroom(skb) < VLAN_HLEN) {
+		struct sk_buff *sk_tmp = skb;
+		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
+		kfree_skb(sk_tmp);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to realloc headroom\n");
+			return NULL;
+		}
+	} else {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
+			return NULL;
+		}
+	}
+
+	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
+
+	/* Move the mac addresses to the beginning of the new header. */
+	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
+
+	/* first, the ethernet type */
+	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
+
+	/* now, the tag */
+	veth->h_vlan_TCI = htons(tag);
+
+	skb->protocol = __constant_htons(ETH_P_8021Q);
+	skb->mac.raw -= VLAN_HLEN;
+	skb->nh.raw -= VLAN_HLEN;
+
+	return skb;
+}
+EXPORT_SYMBOL(__vlan_put_tag);
+
+#endif

Re: [PATCH] deinline a few large functions in vlan code v2

[Auke Kok]

Denis Vlasenko wrote:
> On Monday 10 April 2006 08:45, David S. Miller wrote:
>> From: Denis Vlasenko <vda@ilport.com.ua>
>> Date: Mon, 10 Apr 2006 08:28:20 +0300
>>
>>> IOW: shouldn't calls to these functions sit in
>>> #if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
>>> block? For example, typhoon.c:
>>>


this should have gone to netdev instead of linux-net.

Cheers,

Auke

Re: [PATCH] deinline a few large functions in vlan code v2

[Dave Dillow]

On Mon, 2006-04-10 at 17:16 +0300, Denis Vlasenko wrote:
> On Monday 10 April 2006 08:45, David S. Miller wrote:
> > From: Denis Vlasenko <vda@ilport.com.ua>
> > Date: Mon, 10 Apr 2006 08:28:20 +0300
> > 
> > > IOW: shouldn't calls to these functions sit in
> > > #if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> > > block? For example, typhoon.c:
> > > 
> > >                 spin_lock(&tp->state_lock);
> > > +#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> > >                 if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
> > >                         vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
> > >                                                  ntohl(rx->vlanTag) & 0xffff);
> > >                 else
> > > +#endif
> > >                         netif_receive_skb(new_skb);
> > >                 spin_unlock(&tp->state_lock);
> > > 
> > > Same for s2io.c, chelsio/sge.c, etc...
> > 
> > Very likely yes.  tp->vlgrp will never be non-NULL in such situations
> > so it's not a correctness issue, but rather an optimization :-)
> 
> Done. Please see attached.

I see this as a minor optimization, at the cost uglifying the body of a
function. Besides, if you're going to do this, you can get rid of the
spin_lock functions around it to, since they only protect tp->vlgrp in
this instance.

Please don't apply this to typhoon.c
-- 
Dave Dillow <dave@thedillows.org>

Re: [PATCH] deinline a few large functions in vlan code v2

[Denis Vlasenko]

> > > > block? For example, typhoon.c:
> > > > 
> > > >                 spin_lock(&tp->state_lock);
> > > > +#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> > > >                 if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
> > > >                         vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
> > > >                                                  ntohl(rx->vlanTag) & 0xffff);
> > > >                 else
> > > > +#endif
> > > >                         netif_receive_skb(new_skb);
> > > >                 spin_unlock(&tp->state_lock);
> > > > 
> > > > Same for s2io.c, chelsio/sge.c, etc...
> > > 
> > > Very likely yes.  tp->vlgrp will never be non-NULL in such situations
> > > so it's not a correctness issue, but rather an optimization :-)
> 
> I see this as a minor optimization, at the cost uglifying the body of a
> function.

But it saves some text (~5k total in all network drivers)
and removes a branch on rx path on non-VLAN enabled kernels...

> Besides, if you're going to do this, you can get rid of the 
> spin_lock functions around it to, since they only protect tp->vlgrp in
> this instance.

Done.

> Please don't apply this to typhoon.c

Please comment on the below patch fragment, is it ok with you?

diff -urpN linux-2.6.16.org/drivers/net/typhoon.c linux-2.6.16.vlan/drivers/net/typhoon.c
--- linux-2.6.16.org/drivers/net/typhoon.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/typhoon.c	Tue Apr 11 10:21:19 2006
@@ -285,7 +285,9 @@ struct typhoon {
 	struct pci_dev *	pdev;
 	struct net_device *	dev;
 	spinlock_t		state_lock;
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	struct vlan_group *	vlgrp;
+#endif
 	struct basic_ring	rxHiRing;
 	struct basic_ring	rxBuffRing;
 	struct rxbuff_ent	rxbuffers[RXENT_ENTRIES];
@@ -707,6 +709,7 @@ out:
 	return err;
 }
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 static void
 typhoon_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 {
@@ -754,6 +757,7 @@ typhoon_vlan_rx_kill_vid(struct net_devi
 		tp->vlgrp->vlan_devices[vid] = NULL;
 	spin_unlock_bh(&tp->state_lock);
 }
+#endif
 
 static inline void
 typhoon_tso_fill(struct sk_buff *skb, struct transmit_ring *txRing,
@@ -837,6 +841,7 @@ typhoon_start_tx(struct sk_buff *skb, st
 		first_txd->processFlags |= TYPHOON_TX_PF_IP_CHKSUM;
 	}
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	if(vlan_tx_tag_present(skb)) {
 		first_txd->processFlags |=
 		    TYPHOON_TX_PF_INSERT_VLAN | TYPHOON_TX_PF_VLAN_PRIORITY;
@@ -844,6 +849,7 @@ typhoon_start_tx(struct sk_buff *skb, st
 		    cpu_to_le32(htons(vlan_tx_tag_get(skb)) <<
 				TYPHOON_TX_PF_VLAN_TAG_SHIFT);
 	}
+#endif
 
 	if(skb_tso_size(skb)) {
 		first_txd->processFlags |= TYPHOON_TX_PF_TCP_SEGMENT;
@@ -1744,13 +1750,15 @@ typhoon_rx(struct typhoon *tp, struct ba
 		} else
 			new_skb->ip_summed = CHECKSUM_NONE;
 
-		spin_lock(&tp->state_lock);
-		if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+		if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN) {
+			spin_lock(&tp->state_lock);
 			vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
 						 ntohl(rx->vlanTag) & 0xffff);
-		else
+			spin_unlock(&tp->state_lock);
+		} else
+#endif
 			netif_receive_skb(new_skb);
-		spin_unlock(&tp->state_lock);
 
 		tp->dev->last_rx = jiffies;
 		received++;
@@ -2232,6 +2240,7 @@ typhoon_suspend(struct pci_dev *pdev, pm
 	if(!netif_running(dev))
 		return 0;
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	spin_lock_bh(&tp->state_lock);
 	if(tp->vlgrp && tp->wol_events & TYPHOON_WAKE_MAGIC_PKT) {
 		spin_unlock_bh(&tp->state_lock);
@@ -2240,6 +2249,7 @@ typhoon_suspend(struct pci_dev *pdev, pm
 		return -EBUSY;
 	}
 	spin_unlock_bh(&tp->state_lock);
+#endif
 
 	netif_device_detach(dev);
 
@@ -2549,8 +2559,10 @@ typhoon_init_one(struct pci_dev *pdev, c
 	dev->watchdog_timeo	= TX_TIMEOUT;
 	dev->get_stats		= typhoon_get_stats;
 	dev->set_mac_address	= typhoon_set_mac_address;
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	dev->vlan_rx_register	= typhoon_vlan_rx_register;
 	dev->vlan_rx_kill_vid	= typhoon_vlan_rx_kill_vid;
+#endif
 	SET_ETHTOOL_OPS(dev, &typhoon_ethtool_ops);
 
 	/* We can handle scatter gather, up to 16 entries, and

--
vda

Re: [PATCH] deinline a few large functions in vlan code v2

[David S. Miller]

From: Denis Vlasenko <vda@ilport.com.ua>
Date: Tue, 11 Apr 2006 10:28:54 +0300

> But it saves some text (~5k total in all network drivers)
> and removes a branch on rx path on non-VLAN enabled kernels...

It removes "5K total" when you build every single networking driver
statically into the main kernel image.

Nobody does that except for build sanity testing.  Folks will
enable the drivers they need for their testing into a static
kernel, or build all the net drivers they want modular.

Please resist the urge to work on things like this in a robotic
fashion.  Consider whether the thing you're working on really matters
in real life, and whether the alternative you're providing is really
an overall improvement.  In this case you're "improving" a case only
kernel build testers will use, and the amount of ifdef's added by
your patches uglify the code immensely.  To me, that's a clear net
loss.

Re: [PATCH] deinline a few large functions in vlan code v2

[Ingo Oeser]

Hi Denis,

Denis Vlasenko wrote:
> +#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
>  	if(vlan_tx_tag_present(skb)) {
>  		first_txd->processFlags |=
>  		    TYPHOON_TX_PF_INSERT_VLAN | TYPHOON_TX_PF_VLAN_PRIORITY;
> @@ -844,6 +849,7 @@ typhoon_start_tx(struct sk_buff *skb, st
>  		    cpu_to_le32(htons(vlan_tx_tag_get(skb)) <<
>  				TYPHOON_TX_PF_VLAN_TAG_SHIFT);
>  	}
> +#endif

Wouldn't it be much easier to just do

#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
static inline int  vlan_tx_tag_present(...) {
 /** get VLAN tag */
}
#else
static inline  int vlan_tx_tag_present(...) {return 0;}
#endif

in some header file?

Similiar in typhoon.c:

#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
static inline  has_vlan_group(...) {
  /* get VLAN group */
}
#else
static inline  has_vlan_group(...) {return 0;}
#endif

With this and similiar changes in the drivers, 
your patch might be less intrusive and thus more acceptable to maintainers.

Just let the compiler remove the extra code with constant folding and dead
code elemination. The result will be even cleaner code, I think.

What do you think?


Regards

Ingo Oeser

Re: [PATCH] deinline a few large functions in vlan code v2

[Ingo Oeser]

Hi Denis,

Denis Vlasenko wrote:
> On Tuesday 11 April 2006 12:49, Ingo Oeser wrote:
> > #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
> > static inline  has_vlan_group(...) {
> >   /* get VLAN group */
> > }
> > #else
> > static inline  has_vlan_group(...) {return 0;}
> > #endif
> > 
> > With this and similiar changes in the drivers, 
> > your patch might be less intrusive and thus more acceptable to maintainers.
> > 
> > Just let the compiler remove the extra code with constant folding and dead
> > code elemination. The result will be even cleaner code, I think.
> > 
> > What do you think?

I thought more of introducing functions, which just fold away 
all the "if ()" blocks in normal code paths, 
which you wrapped into "#if" here. 

I don't think people have problems, if you #ifdef out complete functions,
linear setup code or structure members. People seem to have more problems
with #ifdef in control flow code, because there the condition is nothing else but
a compile time constant (the CONFIG_FOO value) which should be expressed as such.

Instead of 

#ifdef CONFIG_FOO
if (condition) {
}
#endif

just do

#ifdef CONFIG_FOO
#define foo 1
#else
#define foo 0
#endif

if  (foo && condition) {
}

Just do nothing or return a compile time constant in those inlines.

> 
> Addresses of these functions are stored into netdevice members:
>
> @@ -2549,8 +2559,10 @@ typhoon_init_one(struct pci_dev *pdev, c
>         dev->watchdog_timeo     = TX_TIMEOUT;
>         dev->get_stats          = typhoon_get_stats;
>         dev->set_mac_address    = typhoon_set_mac_address;
> +#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
>         dev->vlan_rx_register   = typhoon_vlan_rx_register;
>         dev->vlan_rx_kill_vid   = typhoon_vlan_rx_kill_vid;
> +#endif
> 
> Even empty inline would not be "optimized out to nothing" here.

No problem, just optimize out the assignment itself:

#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
static inline void typhoon_setup_vlan_hooks(struct netdev *dev) {
         dev->vlan_rx_register = typhoon_vlan_rx_register;
         dev->vlan_rx_kill_vid = typhoon_vlan_rx_kill_vid;
}
#else
static inline void typhoon_setup_vlan_hooks(struct netdev *dev) { (void)dev; }
#endif

The whole linux/if_vlan.h stuff misses this style of code.
There is simply no fallback code for CONFIG_VLAN_8021Q not being defined.


Regards

Ingo Oeser

[RFD][PATCH] typhoon and core sample for folding away VLAN stuff (was: Re: [PATCH] deinline a few large functions in vlan code v2)

[Ingo Oeser]

Hi Denis,

here is a sample patch for the vlan core and API plus
typhoon driver converted as example.

Just so you can see, what I meant with "No #if in control flow code."

I couldn't resist cleaning up the vlan core, while I'm at it. 
Of course I can seperate this, if you want the pure unilining stuff only.

So I hope this style is clear enough that you can do the rest of 
the conversion without any problems.

Dave Dillow: Is this style clean enough to have it in your driver?

This kind of changes are important, because bloat creeps in byte by byte
of unused features. So I really appreciate your work here Denis.


Regards

Ingo Oeser

diff --git a/drivers/net/typhoon.c b/drivers/net/typhoon.c
index c1ce87a..aab24b8 100644
--- a/drivers/net/typhoon.c
+++ b/drivers/net/typhoon.c
@@ -285,7 +285,9 @@ struct typhoon {
 	struct pci_dev *	pdev;
 	struct net_device *	dev;
 	spinlock_t		state_lock;
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	struct vlan_group *	vlgrp;
+#endif
 	struct basic_ring	rxHiRing;
 	struct basic_ring	rxBuffRing;
 	struct rxbuff_ent	rxbuffers[RXENT_ENTRIES];
@@ -350,6 +352,19 @@ enum state_values {
 #define TSO_OFFLOAD_ON		0
 #endif
 
+
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+static inline struct vlan_group *typhoon_get_vlgrp(struct typhoon *tp)
+{
+	return tp->vlgrp;
+}
+#else
+static inline struct vlan_group *typhoon_get_vlgrp(struct typhoon *tp)
+{
+	return NULL;
+}
+#endif
+
 static inline void
 typhoon_inc_index(u32 *index, const int count, const int num_entries)
 {
@@ -707,6 +722,7 @@ out:
 	return err;
 }
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 static void
 typhoon_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 {
@@ -754,6 +770,12 @@ typhoon_vlan_rx_kill_vid(struct net_devi
 		tp->vlgrp->vlan_devices[vid] = NULL;
 	spin_unlock_bh(&tp->state_lock);
 }
+#else
+
+#define typhoon_vlan_rx_register NULL
+#define typhoon_vlan_rx_kill_vid NULL
+
+#endif
 
 static inline void
 typhoon_tso_fill(struct sk_buff *skb, struct transmit_ring *txRing,
@@ -1686,6 +1708,7 @@ typhoon_rx(struct typhoon *tp, struct ba
 	struct rx_desc *rx;
 	struct sk_buff *skb, *new_skb;
 	struct rxbuff_ent *rxb;
+	struct vlan_group *vlgrp;
 	dma_addr_t dma_addr;
 	u32 local_ready;
 	u32 rxaddr;
@@ -1745,8 +1768,9 @@ typhoon_rx(struct typhoon *tp, struct ba
 			new_skb->ip_summed = CHECKSUM_NONE;
 
 		spin_lock(&tp->state_lock);
-		if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
-			vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
+		vlgrp = typhoon_get_vlgrp(tp);
+		if(vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
+			vlan_hwaccel_receive_skb(new_skb, vlgrp,
 						 ntohl(rx->vlanTag) & 0xffff);
 		else
 			netif_receive_skb(new_skb);
@@ -2233,7 +2257,7 @@ typhoon_suspend(struct pci_dev *pdev, pm
 		return 0;
 
 	spin_lock_bh(&tp->state_lock);
-	if(tp->vlgrp && tp->wol_events & TYPHOON_WAKE_MAGIC_PKT) {
+	if(typhoon_get_vlgrp(tp) && tp->wol_events & TYPHOON_WAKE_MAGIC_PKT) {
 		spin_unlock_bh(&tp->state_lock);
 		printk(KERN_ERR "%s: cannot do WAKE_MAGIC with VLANS\n",
 				dev->name);
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index eef0876..4ed541f 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -14,6 +14,8 @@
 #define _LINUX_IF_VLAN_H_
 
 #ifdef __KERNEL__
+#define HAVE_VLAN_PUT_TAG
+#define HAVE_VLAN_GET_TAG
 
 /* externally defined structs */
 struct vlan_group;
@@ -119,79 +121,29 @@ struct vlan_dev_info {
 	struct net_device_stats dev_stats; /* Device stats (rx-bytes, tx-pkts, etc...) */
 };
 
-#define VLAN_DEV_INFO(x) ((struct vlan_dev_info *)(x->priv))
-
-/* inline functions */
-
-static inline struct net_device_stats *vlan_dev_get_stats(struct net_device *dev)
-{
-	return &(VLAN_DEV_INFO(dev)->dev_stats);
-}
-
-static inline __u32 vlan_get_ingress_priority(struct net_device *dev,
-					      unsigned short vlan_tag)
-{
-	struct vlan_dev_info *vip = VLAN_DEV_INFO(dev);
-
-	return vip->ingress_priority_map[(vlan_tag >> 13) & 0x7];
-}
-
 /* VLAN tx hw acceleration helpers. */
 struct vlan_skb_tx_cookie {
 	u32	magic;
 	u32	vlan_tag;
 };
 
-#define VLAN_TX_COOKIE_MAGIC	0x564c414e	/* "VLAN" in ascii. */
 #define VLAN_TX_SKB_CB(__skb)	((struct vlan_skb_tx_cookie *)&((__skb)->cb[0]))
-#define vlan_tx_tag_present(__skb) \
-	(VLAN_TX_SKB_CB(__skb)->magic == VLAN_TX_COOKIE_MAGIC)
+
 #define vlan_tx_tag_get(__skb)	(VLAN_TX_SKB_CB(__skb)->vlan_tag)
 
-/* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
-static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
-				    struct vlan_group *grp,
-				    unsigned short vlan_tag, int polling)
-{
-	struct net_device_stats *stats;
-
-	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion.
-		 */
-		return 0;
-	}
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+#define VLAN_DEV_INFO(x) ((struct vlan_dev_info *)(x->priv))
+#define VLAN_REAL_DEV(x) (VLAN_DEV_INFO(x)->real_dev)
 
-	skb->dev->last_rx = jiffies;
+#define VLAN_TX_COOKIE_MAGIC	0x564c414e	/* "VLAN" in ascii. */
 
-	stats = vlan_dev_get_stats(skb->dev);
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
-
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST:
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST:
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	};
 
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
-}
+#define vlan_tx_tag_present(__skb) \
+	(VLAN_TX_SKB_CB(__skb)->magic == VLAN_TX_COOKIE_MAGIC)
+
+
+int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
+				    unsigned short vlan_tag, int polling);
 
 static inline int vlan_hwaccel_rx(struct sk_buff *skb,
 				  struct vlan_group *grp,
@@ -207,55 +159,7 @@ static inline int vlan_hwaccel_receive_s
 	return __vlan_hwaccel_rx(skb, grp, vlan_tag, 1);
 }
 
-/**
- * __vlan_put_tag - regular VLAN tag inserting
- * @skb: skbuff to tag
- * @tag: VLAN tag to insert
- *
- * Inserts the VLAN tag into @skb as part of the payload
- * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
- * 
- * Following the skb_unshare() example, in case of error, the calling function
- * doesn't have to worry about freeing the original skb.
- */
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
-{
-	struct vlan_ethhdr *veth;
-
-	if (skb_headroom(skb) < VLAN_HLEN) {
-		struct sk_buff *sk_tmp = skb;
-		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
-		kfree_skb(sk_tmp);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to realloc headroom\n");
-			return NULL;
-		}
-	} else {
-		skb = skb_unshare(skb, GFP_ATOMIC);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
-			return NULL;
-		}
-	}
-
-	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
-
-	/* Move the mac addresses to the beginning of the new header. */
-	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
-
-	/* first, the ethernet type */
-	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
-
-	/* now, the tag */
-	veth->h_vlan_TCI = htons(tag);
-
-	skb->protocol = __constant_htons(ETH_P_8021Q);
-	skb->mac.raw -= VLAN_HLEN;
-	skb->nh.raw -= VLAN_HLEN;
-
-	return skb;
-}
-
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag);
 /**
  * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
  * @skb: skbuff to tag
@@ -274,8 +178,6 @@ static inline struct sk_buff *__vlan_hwa
 	return skb;
 }
 
-#define HAVE_VLAN_PUT_TAG
-
 /**
  * vlan_put_tag - inserts VLAN tag according to device features
  * @skb: skbuff to tag
@@ -304,7 +206,7 @@ static inline int __vlan_get_tag(struct 
 {
 	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)skb->data;
 
-	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
+	if (veth->h_vlan_proto != htons(ETH_P_8021Q)) {
 		return -EINVAL;
 	}
 
@@ -334,8 +236,6 @@ static inline int __vlan_hwaccel_get_tag
 	}
 }
 
-#define HAVE_VLAN_GET_TAG
-
 /**
  * vlan_get_tag - get the VLAN ID from the skb
  * @skb: skbuff to query
@@ -352,6 +252,41 @@ static inline int vlan_get_tag(struct sk
 	}
 }
 
+#else
+
+#define VLAN_REAL_DEV(x) x
+#define vlan_tx_tag_present(__skb) 0
+
+static inline int vlan_hwaccel_rx(struct sk_buff *skb,
+				  struct vlan_group *grp,
+				  unsigned short vlan_tag)
+{
+	return netif_rx(skb);
+}
+
+static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb,
+				  struct vlan_group *grp,
+				  unsigned short vlan_tag)
+{
+	return netif_receive_skb(skb);
+}
+
+static inline struct sk_buff *vlan_put_tag(struct sk_buff *skb, unsigned short tag)
+{
+	return skb;
+}
+
+static inline int vlan_get_tag(struct sk_buff *skb, unsigned short *tag)
+{
+	*tag = 0;
+	return -EINVAL;
+}
+
+#endif /* defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE) */
+
+
+
+
 #endif /* __KERNEL__ */
 
 /* VLAN IOCTLs are found in sockios.h */
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index da9cfe9..d7c4eb1 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -37,6 +37,21 @@
 #include <linux/if_vlan.h>
 #include <net/ip.h>
 
+/* inline functions */
+
+static inline struct net_device_stats *vlan_dev_get_stats(struct net_device *dev)
+{
+	return &(VLAN_DEV_INFO(dev)->dev_stats);
+}
+
+static inline __u32 vlan_get_ingress_priority(struct net_device *dev,
+					      unsigned short vlan_tag)
+{
+	struct vlan_dev_info *vip = VLAN_DEV_INFO(dev);
+
+	return vip->ingress_priority_map[(vlan_tag >> 13) & 0x7];
+}
+
 /*
  *	Rebuild the Ethernet MAC header. This is called after an ARP
  *	(or in future other address resolution) has completed on this
@@ -71,7 +86,7 @@ int vlan_dev_rebuild_header(struct sk_bu
 	return 0;
 }
 
-static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
+static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
 {
 	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
 		if (skb_shared(skb) || skb_cloned(skb)) {
@@ -90,6 +105,104 @@ static inline struct sk_buff *vlan_check
 	return skb;
 }
 
+/**
+ * __vlan_put_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @tag: VLAN tag to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ * 
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
+{
+	struct vlan_ethhdr *veth;
+
+	if (skb_headroom(skb) < VLAN_HLEN) {
+		struct sk_buff *sk_tmp = skb;
+		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
+		kfree_skb(sk_tmp);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to realloc headroom\n");
+			return NULL;
+		}
+	} else {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
+			return NULL;
+		}
+	}
+
+	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
+
+	/* Move the mac addresses to the beginning of the new header. */
+	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
+
+	/* first, the ethernet type */
+	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
+
+	/* now, the tag */
+	veth->h_vlan_TCI = htons(tag);
+
+	skb->protocol = __constant_htons(ETH_P_8021Q);
+	skb->mac.raw -= VLAN_HLEN;
+	skb->nh.raw -= VLAN_HLEN;
+
+	return skb;
+}
+
+static void vlan_rx_common(struct sk_buff *skb, unsigned short vlan_tag)
+{
+	struct net_device_stats *stats;
+
+	stats = vlan_dev_get_stats(skb->dev);
+	stats->rx_packets++;
+	stats->rx_bytes += skb->len;
+
+	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
+	switch (skb->pkt_type) {
+	case PACKET_BROADCAST:
+		break;
+
+	case PACKET_MULTICAST:
+		stats->multicast++;
+		break;
+
+	case PACKET_OTHERHOST:
+		/* Our lower layer thinks this is not local, let's make sure.
+		 * This allows the VLAN to have a different MAC than the underlying
+		 * device, and still route correctly.
+		 */
+		if (!compare_ether_addr(eth_hdr(skb)->h_dest, skb->dev->dev_addr)) {
+			skb->pkt_type = PACKET_HOST;
+		break;
+	};
+}
+
+/* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
+int __vlan_hwaccel_rx(struct sk_buff *skb,
+				    struct vlan_group *grp,
+				    unsigned short vlan_tag, int polling)
+{
+	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
+	if (skb->dev == NULL) {
+		dev_kfree_skb_any(skb);
+
+		/* Not NET_RX_DROP, this is not being dropped
+		 * due to congestion.
+		 */
+		return 0;
+	}
+
+
+	skb->dev->last_rx = jiffies;
+	vlan_rx_common(skb, vlan_tag);
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+
 /*
  *	Determine the packet's protocol ID. The rule here is that we 
  *	assume 802.3 if the type field is short enough to be a length.
@@ -158,11 +271,6 @@ int vlan_skb_recv(struct sk_buff *skb, s
 
 	skb->dev->last_rx = jiffies;
 
-	/* Bump the rx counters for the VLAN device. */
-	stats = vlan_dev_get_stats(skb->dev);
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
-
 	/* Take off the VLAN header (4 bytes currently) */
 	skb_pull_rcsum(skb, VLAN_HLEN);
 
@@ -184,42 +292,7 @@ int vlan_skb_recv(struct sk_buff *skb, s
 		return -1;
 	}
 
-	/*
-	 * Deal with ingress priority mapping.
-	 */
-	skb->priority = vlan_get_ingress_priority(skb->dev, ntohs(vhdr->h_vlan_TCI));
-
-#ifdef VLAN_DEBUG
-	printk(VLAN_DBG "%s: priority: %lu  for TCI: %hu (hbo)\n",
-		__FUNCTION__, (unsigned long)(skb->priority), 
-		ntohs(vhdr->h_vlan_TCI));
-#endif
-
-	/* The ethernet driver already did the pkt_type calculations
-	 * for us...
-	 */
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST: /* Yeah, stats collect these together.. */
-		// stats->broadcast ++; // no such counter :-(
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST: 
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!compare_ether_addr(eth_hdr(skb)->h_dest, skb->dev->dev_addr)) {
-			/* It is for our (changed) MAC-address! */
-			skb->pkt_type = PACKET_HOST;
-		}
-		break;
-	default:
-		break;
-	};
+	vlan_rx_common(skb, vlan_tag);
 
 	/*  Was a VLAN packet, grab the encapsulated protocol, which the layer
 	 * three protocols care about.
@@ -258,7 +331,7 @@ int vlan_skb_recv(struct sk_buff *skb, s
 	 * won't work for fault tolerant netware but does for the rest.
 	 */
 	if (*(unsigned short *)rawp == 0xFFFF) {
-		skb->protocol = __constant_htons(ETH_P_802_3);
+		skb->protocol = htons(ETH_P_802_3);
 		/* place it back on the queue to be handled by true layer 3 protocols.
 		 */
 
@@ -281,7 +354,7 @@ int vlan_skb_recv(struct sk_buff *skb, s
 	/*
 	 *	Real 802.2 LLC
 	 */
-	skb->protocol = __constant_htons(ETH_P_802_2);
+	skb->protocol = htons(ETH_P_802_2);
 	/* place it back on the queue to be handled by upper layer protocols.
 	 */
 
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 3da9264..dec3495 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -879,10 +879,8 @@ static unsigned int ip_sabotage_out(unsi
 	if ((out->hard_start_xmit == br_dev_xmit &&
 	     okfn != br_nf_forward_finish &&
 	     okfn != br_nf_local_out_finish && okfn != br_nf_dev_queue_xmit)
-#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	    || ((out->priv_flags & IFF_802_1Q_VLAN) &&
-		VLAN_DEV_INFO(out)->real_dev->hard_start_xmit == br_dev_xmit)
-#endif
+		VLAN_REAL_DEV(out)->hard_start_xmit == br_dev_xmit)
 	    ) {
 		struct nf_bridge_info *nf_bridge;
 

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[Ben Greear]

What is the reasoning for this change?  Is the compiler
able to optomize the right-hand-side to a constant with your
change in place?

> -	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
> +	if (veth->h_vlan_proto != htons(ETH_P_8021Q)) {
>  		return -EINVAL;
>  	}




-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[Stephen Hemminger]

On Wed, 12 Apr 2006 13:10:06 -0700
Ben Greear <greearb@candelatech.com> wrote:

> What is the reasoning for this change?  Is the compiler
> able to optomize the right-hand-side to a constant with your
> change in place?
> 
> > -	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
> > +	if (veth->h_vlan_proto != htons(ETH_P_8021Q)) {
> >  		return -EINVAL;
> >  	}

Read the source, the macro handles it.

For i386:
	htons maps to __cpu_to_be16
	cpu_to_be16 maps to swab16 which is defined in swab.h

#  define __swab16(x) \
(__builtin_constant_p((__u16)(x)) ? \
 ___swab16((x)) : \
 __fswab16((x)))

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[David S. Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Wed, 12 Apr 2006 13:10:06 -0700

> What is the reasoning for this change?  Is the compiler
> able to optomize the right-hand-side to a constant with your
> change in place?
> 
> > -	if (veth->h_vlan_proto != __constant_htons(ETH_P_8021Q)) {
> > +	if (veth->h_vlan_proto != htons(ETH_P_8021Q)) {
> >  		return -EINVAL;
> >  	}

As a policy, Ben, we only use __constant_htons() in compile
time initializers of data structures.  Otherwise we use the
normal htons().

That's why.

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[Dave Dillow]

Ingo Oeser wrote:
> Dave Dillow: Is this style clean enough to have it in your driver?

Though I'm not real fond of Denis's last patch, I think I prefer it's 
style to this, solely because it removes more code when VLANs are 
disabled -- you've left the spin_locks in, and have more #ifdefs.

Regardless, I remain opposed to this particular instance of bloat 
busting. While both patches have improved in style, they remove a useful 
feature and make the code less clean, for no net gain.

> This kind of changes are important, because bloat creeps in byte by byte
> of unused features. So I really appreciate your work here Denis.

On SMP FC4, typhoon.ko has a text size of 68330, so you need to cut 2794 
bytes to see an actual difference in memory usage for a module. Non-SMP 
it is 67741, so there you only need to cut 2205 bytes to get a win.

Every byte counts, except when it doesn't.

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[Denis Vlasenko]

On Thursday 13 April 2006 04:24, Dave Dillow wrote:
> Regardless, I remain opposed to this particular instance of bloat 
> busting. While both patches have improved in style, they remove a useful 
> feature and make the code less clean, for no net gain.

What happened to non-modular build? "no net gain" is not true.
 
> > This kind of changes are important, because bloat creeps in byte by byte
> > of unused features. So I really appreciate your work here Denis.
> 
> On SMP FC4, typhoon.ko has a text size of 68330, so you need to cut 2794 
> bytes to see an actual difference in memory usage for a module. Non-SMP 
> it is 67741, so there you only need to cut 2205 bytes to get a win.

This is silly. Should I go this route and try a dozen of different gcc
versions and "-O2 versus -Os" things to demonstrate that sometimes
it will matter?
--
vda

Re: [RFD][PATCH] typhoon and core sample for folding away VLAN stuff

[Dave Dillow]

On Thu, 2006-04-13 at 11:38 +0300, Denis Vlasenko wrote:
> On Thursday 13 April 2006 04:24, Dave Dillow wrote:
> > Regardless, I remain opposed to this particular instance of bloat 
> > busting. While both patches have improved in style, they remove a useful 
> > feature and make the code less clean, for no net gain.
> 
> What happened to non-modular build? "no net gain" is not true.

Ok, so you saved what, 200 bytes? On a few drivers that may save you a
small amount -- you basically said you had to have everything loaded to
see 5K.

Weren't most of those savings from moving a big function out-of-line?
The part I agree with?
 
> > > This kind of changes are important, because bloat creeps in byte by byte
> > > of unused features. So I really appreciate your work here Denis.
> > 
> > On SMP FC4, typhoon.ko has a text size of 68330, so you need to cut 2794 
> > bytes to see an actual difference in memory usage for a module. Non-SMP 
> > it is 67741, so there you only need to cut 2205 bytes to get a win.
> 
> This is silly. Should I go this route and try a dozen of different gcc
> versions and "-O2 versus -Os" things to demonstrate that sometimes
> it will matter?

Quit being dense. No one has said that there are cases will it make a
difference, just that that case is far removed from the usual case.

I think I'm done on this topic. You've got more important people to
convince than me, and they've already clear stated their position.
-- 
Dave Dillow <dave@thedillows.org>

Re: [PATCH] deinline a few large functions in vlan code v2

[Dave Dillow]

On Tue, 2006-04-11 at 10:28 +0300, Denis Vlasenko wrote:
> > > > > block? For example, typhoon.c:
> > > > > 
> > > > >                 spin_lock(&tp->state_lock);
> > > > > +#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> > > > >                 if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
> > > > >                         vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
> > > > >                                                  ntohl(rx->vlanTag) & 0xffff);
> > > > >                 else
> > > > > +#endif
> > > > >                         netif_receive_skb(new_skb);
> > > > >                 spin_unlock(&tp->state_lock);
> > > > > 
> > > > > Same for s2io.c, chelsio/sge.c, etc...
> > > > 
> > > > Very likely yes.  tp->vlgrp will never be non-NULL in such situations
> > > > so it's not a correctness issue, but rather an optimization :-)
> > 
> > I see this as a minor optimization, at the cost uglifying the body of a
> > function.
> 
> But it saves some text (~5k total in all network drivers)
> and removes a branch on rx path on non-VLAN enabled kernels...

DaveM beat me to it, but as he said, it saves 5K only if you have all
the drivers built in or loaded. And even if it saves 200 bytes in one
module, unless that module text was already less than 200 bytes into a
page, you've saved no memory -- a 4300 byte module takes 2 pages on x86,
as does a 4100 byte module.

The only savings is in instruction cache, and it would be better to
perhaps uninline vlan_hwaccel_receive_skb() or __vlan_hwaccel_rx() and
make vlan_tx_tag_present be

#if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
#define vlan_tx_tag_present(__skb) \
        (VLAN_TX_SKB_CB(__skb)->magic == VLAN_TX_COOKIE_MAGIC)
#else
#define vlan_tx_tag_present(x) 	0
#endif

to get the cache savings on the hot paths without the ugliness.

> > Besides, if you're going to do this, you can get rid of the 
> > spin_lock functions around it to, since they only protect tp->vlgrp in
> > this instance.
> 
> Done.
> 
> > Please don't apply this to typhoon.c
> 
> Please comment on the below patch fragment, is it ok with you?

NAK. The #ifdefs are ugly, for no significant gain.

And you introduced a race condition when you moved the spin_locks. The
test for tp->vlgrp is no longer protected.
-- 
Dave Dillow <dave@thedillows.org>

Re: [PATCH] deinline a few large functions in vlan code v2

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

On Tuesday 11 April 2006 16:59, Dave Dillow wrote:
> DaveM beat me to it, but as he said, it saves 5K only if you have all
> the drivers built in

I have most of network drivers built in.
I want network card to work right away in early boot,
and I prefer to not regenerate initrd with new nic modules for
each new kernel which I build for diskless stations.

> or loaded. And even if it saves 200 bytes in one 
> module, unless that module text was already less than 200 bytes into a
> page, you've saved no memory -- a 4300 byte module takes 2 pages on x86,
> as does a 4100 byte module.

Sometimes, those 200 bytes can bring module size just under 4096.
Thus on the average, on many modules you get the same size savings
as on built-in code. (Not that we have THAT many network modules...)

> The only savings is in instruction cache, and it would be better to
> perhaps uninline vlan_hwaccel_receive_skb() or __vlan_hwaccel_rx() and
> make vlan_tx_tag_present be
> 
> #if defined(CONFIG_VLAN_8021Q) || defined (CONFIG_VLAN_8021Q_MODULE)
> #define vlan_tx_tag_present(__skb) \
>         (VLAN_TX_SKB_CB(__skb)->magic == VLAN_TX_COOKIE_MAGIC)
> #else
> #define vlan_tx_tag_present(x) 	0
> #endif
> 
> to get the cache savings on the hot paths without the ugliness.

Good idea.

> > > Besides, if you're going to do this, you can get rid of the 
> > > spin_lock functions around it to, since they only protect tp->vlgrp in
> > > this instance.
> > 
> > Done.
> > 
> > > Please don't apply this to typhoon.c
> > 
> > Please comment on the below patch fragment, is it ok with you?
> 
> NAK. The #ifdefs are ugly, for no significant gain.
> 
> And you introduced a race condition when you moved the spin_locks. The
> test for tp->vlgrp is no longer protected.

See attached. Much less #ifdefs (most of the time I test for compile time
constant VLAN_ENABLED in the if()s instead), the race is fixed.

Please apply your ugli-o-meter to it...
--
vda

[-- Attachment #2: 2.6.16.vlan_inline8.patch --]
[-- Type: text/x-diff, Size: 36840 bytes --]

diff -urpN linux-2.6.16.org/drivers/net/bnx2.c linux-2.6.16.vlan/drivers/net/bnx2.c
--- linux-2.6.16.org/drivers/net/bnx2.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/bnx2.c	Wed Apr 12 11:29:34 2006
@@ -4436,10 +4436,13 @@ bnx2_start_xmit(struct sk_buff *skb, str
 		vlan_tag_flags |= TX_BD_FLAGS_TCP_UDP_CKSUM;
 	}
 
+#ifdef BCM_VLAN
 	if (bp->vlgrp != 0 && vlan_tx_tag_present(skb)) {
 		vlan_tag_flags |=
 			(TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
 	}
+#endif
+
 #ifdef BCM_TSO 
 	if ((mss = skb_shinfo(skb)->tso_size) &&
 		(skb->len > (bp->dev->mtu + ETH_HLEN))) {
diff -urpN linux-2.6.16.org/drivers/net/bnx2.h linux-2.6.16.vlan/drivers/net/bnx2.h
--- linux-2.6.16.org/drivers/net/bnx2.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/bnx2.h	Wed Apr 12 11:29:34 2006
@@ -40,7 +40,9 @@
 #include <linux/mii.h>
 #ifdef NETIF_F_HW_VLAN_TX
 #include <linux/if_vlan.h>
+#if VLAN_ENABLED
 #define BCM_VLAN 1
+#endif
 #endif
 #ifdef NETIF_F_TSO
 #include <net/ip.h>
diff -urpN linux-2.6.16.org/drivers/net/bonding/bond_alb.c linux-2.6.16.vlan/drivers/net/bonding/bond_alb.c
--- linux-2.6.16.org/drivers/net/bonding/bond_alb.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/bonding/bond_alb.c	Wed Apr 12 11:29:34 2006
@@ -502,7 +502,7 @@ static void rlb_update_client(struct rlb
 
 		skb->dev = client_info->slave->dev;
 
-		if (client_info->tag) {
+		if (VLAN_ENABLED && client_info->tag) {
 			skb = vlan_put_tag(skb, client_info->vlan_id);
 			if (!skb) {
 				printk(KERN_ERR DRV_NAME
@@ -511,7 +511,6 @@ static void rlb_update_client(struct rlb
 				continue;
 			}
 		}
-
 		arp_xmit(skb);
 	}
 }
@@ -671,7 +670,7 @@ static struct slave *rlb_choose_channel(
 			client_info->ntt = 0;
 		}
 
-		if (!list_empty(&bond->vlan_list)) {
+		if (VLAN_ENABLED && !list_empty(&bond->vlan_list)) {
 			unsigned short vlan_id;
 			int res = vlan_get_tag(skb, &vlan_id);
 			if (!res) {
@@ -833,6 +832,7 @@ static void rlb_deinitialize(struct bond
 	_unlock_rx_hashtbl(bond);
 }
 
+#if VLAN_ENABLED
 static void rlb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
 	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
@@ -865,16 +865,20 @@ static void rlb_clear_vlan(struct bondin
 
 	_unlock_rx_hashtbl(bond);
 }
+#endif
 
 /*********************** tlb/rlb shared functions *********************/
 
 static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
 {
-	struct bonding *bond = bond_get_bond_by_slave(slave);
+	struct bonding *bond;
 	struct learning_pkt pkt;
 	int size = sizeof(struct learning_pkt);
 	int i;
 
+	if (VLAN_ENABLED)
+		bond = bond_get_bond_by_slave(slave);
+
 	memset(&pkt, 0, size);
 	memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
 	memcpy(pkt.mac_src, mac_addr, ETH_ALEN);
@@ -898,7 +902,7 @@ static void alb_send_learning_packets(st
 		skb->priority = TC_PRIO_CONTROL;
 		skb->dev = slave->dev;
 
-		if (!list_empty(&bond->vlan_list)) {
+		if (VLAN_ENABLED && !list_empty(&bond->vlan_list)) {
 			struct vlan_entry *vlan;
 
 			vlan = bond_next_vlan(bond,
@@ -918,7 +922,6 @@ static void alb_send_learning_packets(st
 				continue;
 			}
 		}
-
 		dev_queue_xmit(skb);
 	}
 }
@@ -1665,6 +1668,7 @@ int bond_alb_set_mac_address(struct net_
 	return 0;
 }
 
+#if VLAN_ENABLED
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id)
 {
 	if (bond->alb_info.current_alb_vlan &&
@@ -1676,4 +1680,4 @@ void bond_alb_clear_vlan(struct bonding 
 		rlb_clear_vlan(bond, vlan_id);
 	}
 }
-
+#endif
diff -urpN linux-2.6.16.org/drivers/net/bonding/bond_main.c linux-2.6.16.vlan/drivers/net/bonding/bond_main.c
--- linux-2.6.16.org/drivers/net/bonding/bond_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/bonding/bond_main.c	Wed Apr 12 11:45:33 2006
@@ -199,6 +199,7 @@ const char *bond_mode_name(int mode)
 	}
 }
 
+#if VLAN_ENABLED
 /*---------------------------------- VLAN -----------------------------------*/
 
 /**
@@ -349,6 +350,10 @@ struct vlan_entry *bond_next_vlan(struct
 
 	return next;
 }
+#else
+/* never called */
+int bond_has_challenged_slaves(struct bonding *bond);
+#endif /* VLAN_ENABLED */
 
 /**
  * bond_dev_queue_xmit - Prepare skb for xmit.
@@ -368,7 +373,7 @@ int bond_dev_queue_xmit(struct bonding *
 {
 	unsigned short vlan_id;
 
-	if (!list_empty(&bond->vlan_list) &&
+	if (VLAN_ENABLED && !list_empty(&bond->vlan_list) &&
 	    !(slave_dev->features & NETIF_F_HW_VLAN_TX) &&
 	    vlan_get_tag(skb, &vlan_id) == 0) {
 		skb->dev = slave_dev;
@@ -390,6 +395,7 @@ int bond_dev_queue_xmit(struct bonding *
 	return 0;
 }
 
+#if VLAN_ENABLED
 /*
  * In the following 3 functions, bond_vlan_rx_register(), bond_vlan_rx_add_vid
  * and bond_vlan_rx_kill_vid, We don't protect the slave list iteration with a
@@ -555,6 +561,11 @@ unreg:
 out:
 	write_unlock_bh(&bond->lock);
 }
+#else
+/* never called */
+void bond_add_vlans_on_slave(struct bonding *bond, struct net_device *slave_dev);
+void bond_del_vlans_from_slave(struct bonding *bond, struct net_device *slave_dev);
+#endif /* VLAN_ENABLED */
 
 /*------------------------------- Link status -------------------------------*/
 
@@ -1217,7 +1228,7 @@ int bond_enslave(struct net_device *bond
 
 	/* vlan challenged mutual exclusion */
 	/* no need to lock since we're protected by rtnl_lock */
-	if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {
+	if (VLAN_ENABLED && (slave_dev->features & NETIF_F_VLAN_CHALLENGED)) {
 		dprintk("%s: NETIF_F_VLAN_CHALLENGED\n", slave_dev->name);
 		if (!list_empty(&bond->vlan_list)) {
 			printk(KERN_ERR DRV_NAME
@@ -1235,7 +1246,7 @@ int bond_enslave(struct net_device *bond
 			       bond_dev->name);
 			bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
 		}
-	} else {
+	} else if (VLAN_ENABLED) {
 		dprintk("%s: ! NETIF_F_VLAN_CHALLENGED\n", slave_dev->name);
 		if (bond->slave_cnt == 0) {
 			/* First slave, and it is not VLAN challenged,
@@ -1356,7 +1367,8 @@ int bond_enslave(struct net_device *bond
 		dev_mc_add(slave_dev, lacpdu_multicast, ETH_ALEN, 0);
 	}
 
-	bond_add_vlans_on_slave(bond, slave_dev);
+	if (VLAN_ENABLED)
+		bond_add_vlans_on_slave(bond, slave_dev);
 
 	write_lock_bh(&bond->lock);
 
@@ -1667,7 +1679,7 @@ int bond_release(struct net_device *bond
 		 */
 		memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
 
-		if (list_empty(&bond->vlan_list)) {
+		if (!VLAN_ENABLED || list_empty(&bond->vlan_list)) {
 			bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
 		} else {
 			printk(KERN_WARNING DRV_NAME
@@ -1679,7 +1691,7 @@ int bond_release(struct net_device *bond
 			       "HW address matches its VLANs'.\n",
 			       bond_dev->name);
 		}
-	} else if ((bond_dev->features & NETIF_F_VLAN_CHALLENGED) &&
+	} else if (VLAN_ENABLED && (bond_dev->features & NETIF_F_VLAN_CHALLENGED) &&
 		   !bond_has_challenged_slaves(bond)) {
 		printk(KERN_INFO DRV_NAME
 		       ": %s: last VLAN challenged slave %s "
@@ -1693,7 +1705,8 @@ int bond_release(struct net_device *bond
 	/* must do this from outside any spinlocks */
 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
 
-	bond_del_vlans_from_slave(bond, slave_dev);
+	if (VLAN_ENABLED)
+		bond_del_vlans_from_slave(bond, slave_dev);
 
 	/* If the mode USES_PRIMARY, then we should only remove its
 	 * promisc and mc settings if it was the curr_active_slave, but that was
@@ -1785,7 +1798,8 @@ static int bond_release_all(struct net_d
 		write_unlock_bh(&bond->lock);
 
 		bond_destroy_slave_symlinks(bond_dev, slave_dev);
-		bond_del_vlans_from_slave(bond, slave_dev);
+		if (VLAN_ENABLED)
+			bond_del_vlans_from_slave(bond, slave_dev);
 
 		/* If the mode USES_PRIMARY, then we should only remove its
 		 * promisc and mc settings if it was the curr_active_slave, but that was
@@ -1835,7 +1849,7 @@ static int bond_release_all(struct net_d
 	 */
 	memset(bond_dev->dev_addr, 0, bond_dev->addr_len);
 
-	if (list_empty(&bond->vlan_list)) {
+	if (!VLAN_ENABLED || list_empty(&bond->vlan_list)) {
 		bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
 	} else {
 		printk(KERN_WARNING DRV_NAME
@@ -2239,7 +2253,7 @@ static int bond_has_ip(struct bonding *b
 	if (bond->master_ip)
 		return 1;
 
-	if (list_empty(&bond->vlan_list))
+	if (!VLAN_ENABLED || list_empty(&bond->vlan_list))
 		return 0;
 
 	list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
@@ -2270,7 +2284,7 @@ static void bond_arp_send(struct net_dev
 		printk(KERN_ERR DRV_NAME ": ARP packet allocation failed\n");
 		return;
 	}
-	if (vlan_id) {
+	if (VLAN_ENABLED && vlan_id) {
 		skb = vlan_put_tag(skb, vlan_id);
 		if (!skb) {
 			printk(KERN_ERR DRV_NAME ": failed to insert VLAN tag\n");
@@ -2283,7 +2297,7 @@ static void bond_arp_send(struct net_dev
 
 static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 {
-	int i, vlan_id, rv;
+	int i,vlan_id, rv;
 	u32 *targets = bond->params.arp_targets;
 	struct vlan_entry *vlan, *vlan_next;
 	struct net_device *vlan_dev;
@@ -2294,7 +2308,7 @@ static void bond_arp_send_all(struct bon
 		if (!targets[i])
 			continue;
 		dprintk("basa: target %x\n", targets[i]);
-		if (list_empty(&bond->vlan_list)) {
+		if (!VLAN_ENABLED || list_empty(&bond->vlan_list)) {
 			dprintk("basa: empty vlan: arp_send\n");
 			bond_arp_send(slave->dev, ARPOP_REQUEST, targets[i],
 				      bond->master_ip, 0);
@@ -2369,7 +2383,6 @@ static void bond_send_gratuitous_arp(str
 	struct slave *slave = bond->curr_active_slave;
 	struct vlan_entry *vlan;
 	struct net_device *vlan_dev;
-
 	dprintk("bond_send_grat_arp: bond %s slave %s\n", bond->dev->name,
 				slave ? slave->dev->name : "NULL");
 	if (!slave)
@@ -2380,11 +2393,13 @@ static void bond_send_gratuitous_arp(str
 				  bond->master_ip, 0);
 	}
 
-	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
-		vlan_dev = bond->vlgrp->vlan_devices[vlan->vlan_id];
-		if (vlan->vlan_ip) {
-			bond_arp_send(slave->dev, ARPOP_REPLY, vlan->vlan_ip,
-				      vlan->vlan_ip, vlan->vlan_id);
+	if (VLAN_ENABLED) {
+		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
+			vlan_dev = bond->vlgrp->vlan_devices[vlan->vlan_id];
+			if (vlan->vlan_ip) {
+				bond_arp_send(slave->dev, ARPOP_REPLY, vlan->vlan_ip,
+					      vlan->vlan_ip, vlan->vlan_id);
+			}
 		}
 	}
 }
@@ -3215,7 +3230,7 @@ static int bond_inetaddr_event(struct no
 			}
 		}
 
-		if (list_empty(&bond->vlan_list))
+		if (!VLAN_ENABLED || list_empty(&bond->vlan_list))
 			continue;
 
 		list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
@@ -4120,7 +4135,8 @@ static int bond_init(struct net_device *
 	bond->current_arp_slave = NULL;
 	bond->primary_slave = NULL;
 	bond->dev = bond_dev;
-	INIT_LIST_HEAD(&bond->vlan_list);
+	if (VLAN_ENABLED)
+		INIT_LIST_HEAD(&bond->vlan_list);
 
 	/* Initialize the device entry points */
 	bond_dev->open = bond_open;
@@ -4151,6 +4167,7 @@ static int bond_init(struct net_device *
 	 * transmitting */
 	bond_dev->features |= NETIF_F_LLTX;
 
+#if VLAN_ENABLED
 	/* By default, we declare the bond to be fully
 	 * VLAN hardware accelerated capable. Special
 	 * care is taken in the various xmit functions
@@ -4163,6 +4180,7 @@ static int bond_init(struct net_device *
 	bond_dev->features |= (NETIF_F_HW_VLAN_TX |
 			       NETIF_F_HW_VLAN_RX |
 			       NETIF_F_HW_VLAN_FILTER);
+#endif
 
 #ifdef CONFIG_PROC_FS
 	bond_create_proc_entry(bond);
diff -urpN linux-2.6.16.org/drivers/net/chelsio/sge.c linux-2.6.16.vlan/drivers/net/chelsio/sge.c
--- linux-2.6.16.org/drivers/net/chelsio/sge.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/chelsio/sge.c	Wed Apr 12 11:29:34 2006
@@ -978,7 +978,7 @@ static int sge_rx(struct sge *sge, struc
 	} else
 		skb->ip_summed = CHECKSUM_NONE;
 
-	if (unlikely(adapter->vlan_grp && p->vlan_valid)) {
+	if (VLAN_ENABLED && unlikely(adapter->vlan_grp && p->vlan_valid)) {
 		sge->port_stats[p->iff].vlan_xtract++;
 		if (adapter->params.sge.polling)
 			vlan_hwaccel_receive_skb(skb, adapter->vlan_grp,
diff -urpN linux-2.6.16.org/drivers/net/e1000/e1000_main.c linux-2.6.16.vlan/drivers/net/e1000/e1000_main.c
--- linux-2.6.16.org/drivers/net/e1000/e1000_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/e1000/e1000_main.c	Wed Apr 12 11:39:31 2006
@@ -250,10 +250,16 @@ static void e1000_smartspeed(struct e100
 static inline int e1000_82547_fifo_workaround(struct e1000_adapter *adapter,
 					      struct sk_buff *skb);
 
+#if VLAN_ENABLED
 static void e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp);
 static void e1000_vlan_rx_add_vid(struct net_device *netdev, uint16_t vid);
 static void e1000_vlan_rx_kill_vid(struct net_device *netdev, uint16_t vid);
 static void e1000_restore_vlan(struct e1000_adapter *adapter);
+#else
+/* never called */
+void e1000_vlan_rx_kill_vid(struct net_device *netdev, uint16_t vid);
+void e1000_restore_vlan(struct e1000_adapter *adapter);
+#endif
 
 #ifdef CONFIG_PM
 static int e1000_suspend(struct pci_dev *pdev, pm_message_t state);
@@ -361,6 +367,7 @@ e1000_irq_enable(struct e1000_adapter *a
 	}
 }
 
+#if VLAN_ENABLED
 static void
 e1000_update_mng_vlan(struct e1000_adapter *adapter)
 {
@@ -383,6 +390,10 @@ e1000_update_mng_vlan(struct e1000_adapt
 		}
 	}
 }
+#else
+/* never called */
+void e1000_update_mng_vlan(struct e1000_adapter *adapter);
+#endif
 
 /**
  * e1000_release_hw_control - release control of the h/w to f/w
@@ -470,7 +481,8 @@ e1000_up(struct e1000_adapter *adapter)
 
 	e1000_set_multi(netdev);
 
-	e1000_restore_vlan(adapter);
+	if (VLAN_ENABLED)
+		e1000_restore_vlan(adapter);
 
 	e1000_configure_tx(adapter);
 	e1000_setup_rctl(adapter);
@@ -629,9 +641,12 @@ e1000_reset(struct e1000_adapter *adapte
 		E1000_WRITE_REG(&adapter->hw, WUC, 0);
 	if (e1000_init_hw(&adapter->hw))
 		DPRINTK(PROBE, ERR, "Hardware Error\n");
-	e1000_update_mng_vlan(adapter);
-	/* Enable h/w to recognize an 802.1Q VLAN Ethernet packet */
-	E1000_WRITE_REG(&adapter->hw, VET, ETHERNET_IEEE_VLAN_TYPE);
+
+	if (VLAN_ENABLED) {
+		e1000_update_mng_vlan(adapter);
+		/* Enable h/w to recognize an 802.1Q VLAN Ethernet packet */
+		E1000_WRITE_REG(&adapter->hw, VET, ETHERNET_IEEE_VLAN_TYPE);
+	}
 
 	e1000_reset_adaptive(&adapter->hw);
 	e1000_phy_get_info(&adapter->hw, &adapter->phy_info);
@@ -733,9 +748,11 @@ e1000_probe(struct pci_dev *pdev,
 	netdev->poll = &e1000_clean;
 	netdev->weight = 64;
 #endif
+#if VLAN_ENABLED
 	netdev->vlan_rx_register = e1000_vlan_rx_register;
 	netdev->vlan_rx_add_vid = e1000_vlan_rx_add_vid;
 	netdev->vlan_rx_kill_vid = e1000_vlan_rx_kill_vid;
+#endif
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	netdev->poll_controller = e1000_netpoll;
 #endif
@@ -1228,10 +1245,13 @@ e1000_open(struct net_device *netdev)
 
 	if ((err = e1000_up(adapter)))
 		goto err_up;
-	adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
-	if ((adapter->hw.mng_cookie.status &
-			  E1000_MNG_DHCP_COOKIE_STATUS_VLAN_SUPPORT)) {
-		e1000_update_mng_vlan(adapter);
+
+	if (VLAN_ENABLED) {
+		adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
+		if ((adapter->hw.mng_cookie.status &
+				  E1000_MNG_DHCP_COOKIE_STATUS_VLAN_SUPPORT)) {
+			e1000_update_mng_vlan(adapter);
+		}
 	}
 
 	/* If AMT is enabled, let the firmware know that the network
@@ -1274,7 +1294,7 @@ e1000_close(struct net_device *netdev)
 	e1000_free_all_tx_resources(adapter);
 	e1000_free_all_rx_resources(adapter);
 
-	if ((adapter->hw.mng_cookie.status &
+	if (VLAN_ENABLED && (adapter->hw.mng_cookie.status &
 			  E1000_MNG_DHCP_COOKIE_STATUS_VLAN_SUPPORT)) {
 		e1000_vlan_rx_kill_vid(netdev, adapter->mng_vlan_id);
 	}
@@ -2397,7 +2417,7 @@ e1000_watchdog_task(struct e1000_adapter
 	e1000_check_for_link(&adapter->hw);
 	if (adapter->hw.mac_type == e1000_82573) {
 		e1000_enable_tx_pkt_filtering(&adapter->hw);
-		if (adapter->mng_vlan_id != adapter->hw.mng_cookie.vlan_id)
+		if (VLAN_ENABLED && adapter->mng_vlan_id != adapter->hw.mng_cookie.vlan_id)
 			e1000_update_mng_vlan(adapter);
 	}
 
@@ -3714,7 +3734,7 @@ e1000_clean_rx_irq(struct e1000_adapter 
 
 		skb->protocol = eth_type_trans(skb, netdev);
 #ifdef CONFIG_E1000_NAPI
-		if (unlikely(adapter->vlgrp &&
+		if (VLAN_ENABLED && unlikely(adapter->vlgrp &&
 			    (status & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 						 le16_to_cpu(rx_desc->special) &
@@ -3723,7 +3743,7 @@ e1000_clean_rx_irq(struct e1000_adapter 
 			netif_receive_skb(skb);
 		}
 #else /* CONFIG_E1000_NAPI */
-		if (unlikely(adapter->vlgrp &&
+		if (VLAN_ENABLED && unlikely(adapter->vlgrp &&
 			    (status & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 					le16_to_cpu(rx_desc->special) &
@@ -3861,7 +3881,7 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 			   cpu_to_le16(E1000_RXDPS_HDRSTAT_HDRSP)))
 			adapter->rx_hdr_split++;
 #ifdef CONFIG_E1000_NAPI
-		if (unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
+		if (VLAN_ENABLED && unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->wb.middle.vlan) &
 				E1000_RXD_SPC_VLAN_MASK);
@@ -3869,7 +3889,7 @@ e1000_clean_rx_irq_ps(struct e1000_adapt
 			netif_receive_skb(skb);
 		}
 #else /* CONFIG_E1000_NAPI */
-		if (unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
+		if (VLAN_ENABLED && unlikely(adapter->vlgrp && (staterr & E1000_RXD_STAT_VP))) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->wb.middle.vlan) &
 				E1000_RXD_SPC_VLAN_MASK);
@@ -4351,6 +4371,7 @@ e1000_io_write(struct e1000_hw *hw, unsi
 	outl(value, port);
 }
 
+#if VLAN_ENABLED
 static void
 e1000_vlan_rx_register(struct net_device *netdev, struct vlan_group *grp)
 {
@@ -4382,7 +4403,7 @@ e1000_vlan_rx_register(struct net_device
 		rctl = E1000_READ_REG(&adapter->hw, RCTL);
 		rctl &= ~E1000_RCTL_VFE;
 		E1000_WRITE_REG(&adapter->hw, RCTL, rctl);
-		if (adapter->mng_vlan_id != (uint16_t)E1000_MNG_VLAN_NONE) {
+		if (VLAN_ENABLED && adapter->mng_vlan_id != (uint16_t)E1000_MNG_VLAN_NONE) {
 			e1000_vlan_rx_kill_vid(netdev, adapter->mng_vlan_id);
 			adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
 		}
@@ -4450,6 +4471,7 @@ e1000_restore_vlan(struct e1000_adapter 
 		}
 	}
 }
+#endif
 
 int
 e1000_set_spd_dplx(struct e1000_adapter *adapter, uint16_t spddplx)
diff -urpN linux-2.6.16.org/drivers/net/gianfar.c linux-2.6.16.vlan/drivers/net/gianfar.c
--- linux-2.6.16.org/drivers/net/gianfar.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/gianfar.c	Wed Apr 12 11:41:09 2006
@@ -270,15 +270,17 @@ static int gfar_probe(struct platform_de
 	} else
 		priv->rx_csum_enable = 0;
 
-	priv->vlgrp = NULL;
+	if (VLAN_ENABLED) {
+		priv->vlgrp = NULL;
 
-	if (priv->einfo->device_flags & FSL_GIANFAR_DEV_HAS_VLAN) {
-		dev->vlan_rx_register = gfar_vlan_rx_register;
-		dev->vlan_rx_kill_vid = gfar_vlan_rx_kill_vid;
+		if (priv->einfo->device_flags & FSL_GIANFAR_DEV_HAS_VLAN) {
+			dev->vlan_rx_register = gfar_vlan_rx_register;
+			dev->vlan_rx_kill_vid = gfar_vlan_rx_kill_vid;
 
-		dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
+			dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
 
-		priv->vlan_enable = 1;
+			priv->vlan_enable = 1;
+		}
 	}
 
 	if (priv->einfo->device_flags & FSL_GIANFAR_DEV_HAS_EXTENDED_HASH) {
@@ -792,7 +794,7 @@ int startup_gfar(struct net_device *dev)
 		rctrl |= RCTRL_EMEN;
 	}
 
-	if (priv->vlan_enable)
+	if (VLAN_ENABLED && priv->vlan_enable)
 		rctrl |= RCTRL_VLAN;
 
 	if (priv->padding) {
@@ -1037,6 +1039,7 @@ int gfar_set_mac_address(struct net_devi
 }
 
 
+#if VLAN_ENABLED
 /* Enables and disables VLAN insertion/extraction */
 static void gfar_vlan_rx_register(struct net_device *dev,
 		struct vlan_group *grp)
@@ -1088,6 +1091,7 @@ static void gfar_vlan_rx_kill_vid(struct
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
+#endif
 
 
 static int gfar_change_mtu(struct net_device *dev, int new_mtu)
@@ -1097,7 +1101,7 @@ static int gfar_change_mtu(struct net_de
 	int oldsize = priv->rx_buffer_size;
 	int frame_size = new_mtu + ETH_HLEN;
 
-	if (priv->vlan_enable)
+	if (VLAN_ENABLED && priv->vlan_enable)
 		frame_size += VLAN_ETH_HLEN;
 
 	if (gfar_uses_fcb(priv))
@@ -1409,7 +1413,7 @@ static int gfar_process_frame(struct net
 		skb->protocol = eth_type_trans(skb, dev);
 
 		/* Send the packet up the stack */
-		if (unlikely(priv->vlgrp && (fcb->flags & RXFCB_VLN)))
+		if (VLAN_ENABLED && unlikely(priv->vlgrp && (fcb->flags & RXFCB_VLN)))
 			ret = gfar_rx_vlan(skb, priv->vlgrp, fcb->vlctl);
 		else
 			ret = RECEIVE(skb);
diff -urpN linux-2.6.16.org/drivers/net/gianfar.h linux-2.6.16.vlan/drivers/net/gianfar.h
--- linux-2.6.16.org/drivers/net/gianfar.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/gianfar.h	Wed Apr 12 11:29:34 2006
@@ -702,7 +702,9 @@ struct gfar_private {
 		extended_hash:1,
 		bd_stash_en:1;
 	unsigned short padding;
+#if VLAN_ENABLED
 	struct vlan_group *vlgrp;
+#endif
 	/* Info structure initialized by board setup code */
 	unsigned int interruptTransmit;
 	unsigned int interruptReceive;
diff -urpN linux-2.6.16.org/drivers/net/ixgb/ixgb_main.c linux-2.6.16.vlan/drivers/net/ixgb/ixgb_main.c
--- linux-2.6.16.org/drivers/net/ixgb/ixgb_main.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/ixgb/ixgb_main.c	Wed Apr 12 11:39:25 2006
@@ -121,11 +121,16 @@ static void ixgb_alloc_rx_buffers(struct
 void ixgb_set_ethtool_ops(struct net_device *netdev);
 static void ixgb_tx_timeout(struct net_device *dev);
 static void ixgb_tx_timeout_task(struct net_device *dev);
+#if VLAN_ENABLED
 static void ixgb_vlan_rx_register(struct net_device *netdev,
 				  struct vlan_group *grp);
 static void ixgb_vlan_rx_add_vid(struct net_device *netdev, uint16_t vid);
 static void ixgb_vlan_rx_kill_vid(struct net_device *netdev, uint16_t vid);
 static void ixgb_restore_vlan(struct ixgb_adapter *adapter);
+#else
+/* never called */
+void ixgb_restore_vlan(struct ixgb_adapter *adapter);
+#endif
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 /* for netdump / net console */
@@ -233,7 +238,8 @@ ixgb_up(struct ixgb_adapter *adapter)
 
 	ixgb_set_multi(netdev);
 
-	ixgb_restore_vlan(adapter);
+	if (VLAN_ENABLED)
+		ixgb_restore_vlan(adapter);
 
 	ixgb_configure_tx(adapter);
 	ixgb_setup_rctl(adapter);
@@ -419,9 +425,11 @@ ixgb_probe(struct pci_dev *pdev,
 	netdev->poll = &ixgb_clean;
 	netdev->weight = 64;
 #endif
+#if VLAN_ENABLED
 	netdev->vlan_rx_register = ixgb_vlan_rx_register;
 	netdev->vlan_rx_add_vid = ixgb_vlan_rx_add_vid;
 	netdev->vlan_rx_kill_vid = ixgb_vlan_rx_kill_vid;
+#endif
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	netdev->poll_controller = ixgb_netpoll;
 #endif
@@ -1905,7 +1913,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a
 
 		skb->protocol = eth_type_trans(skb, netdev);
 #ifdef CONFIG_IXGB_NAPI
-		if(adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
+		if(VLAN_ENABLED && adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
 			vlan_hwaccel_receive_skb(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->special) &
 					IXGB_RX_DESC_SPECIAL_VLAN_MASK);
@@ -1913,7 +1921,7 @@ ixgb_clean_rx_irq(struct ixgb_adapter *a
 			netif_receive_skb(skb);
 		}
 #else /* CONFIG_IXGB_NAPI */
-		if(adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
+		if(VLAN_ENABLED && adapter->vlgrp && (status & IXGB_RX_DESC_STATUS_VP)) {
 			vlan_hwaccel_rx(skb, adapter->vlgrp,
 				le16_to_cpu(rx_desc->special) &
 					IXGB_RX_DESC_SPECIAL_VLAN_MASK);
@@ -2014,6 +2022,7 @@ ixgb_alloc_rx_buffers(struct ixgb_adapte
 	rx_ring->next_to_use = i;
 }
 
+#if VLAN_ENABLED
 /**
  * ixgb_vlan_rx_register - enables or disables vlan tagging/stripping.
  * 
@@ -2107,6 +2116,7 @@ ixgb_restore_vlan(struct ixgb_adapter *a
 		}
 	}
 }
+#endif /* VLAN_ENABLED */
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
 /*
diff -urpN linux-2.6.16.org/drivers/net/s2io.c linux-2.6.16.vlan/drivers/net/s2io.c
--- linux-2.6.16.org/drivers/net/s2io.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/s2io.c	Wed Apr 12 11:40:33 2006
@@ -182,6 +182,7 @@ static char ethtool_stats_keys[][ETH_GST
 			timer.data = (unsigned long) arg;	\
 			mod_timer(&timer, (jiffies + exp))	\
 
+#if VLAN_ENABLED
 /* Add the vlan */
 static void s2io_vlan_rx_register(struct net_device *dev,
 					struct vlan_group *grp)
@@ -205,6 +206,7 @@ static void s2io_vlan_rx_kill_vid(struct
 		nic->vlgrp->vlan_devices[vid] = NULL;
 	spin_unlock_irqrestore(&nic->tx_lock, flags);
 }
+#endif
 
 /*
  * Constants to be programmed into the Xena's registers, to configure
@@ -3470,7 +3472,6 @@ static int s2io_xmit(struct sk_buff *skb
 	int mss;
 #endif
 	u16 vlan_tag = 0;
-	int vlan_priority = 0;
 	mac_info_t *mac_control;
 	struct config_param *config;
 
@@ -3491,6 +3492,7 @@ static int s2io_xmit(struct sk_buff *skb
 
 	/* Get Fifo number to Transmit based on vlan priority */
 	if (sp->vlgrp && vlan_tx_tag_present(skb)) {
+		int vlan_priority;
 		vlan_tag = vlan_tx_tag_get(skb);
 		vlan_priority = vlan_tag >> 13;
 		queue = config->fifo_mapping[vlan_priority];
@@ -5680,7 +5682,7 @@ static int rx_osm_handler(ring_info_t *r
 
 	skb->protocol = eth_type_trans(skb, dev);
 #ifdef CONFIG_S2IO_NAPI
-	if (sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
+	if (VLAN_ENABLED && sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
 		/* Queueing the vlan frame to the upper layer */
 		vlan_hwaccel_receive_skb(skb, sp->vlgrp,
 			RXD_GET_VLAN_TAG(rxdp->Control_2));
@@ -5688,7 +5690,7 @@ static int rx_osm_handler(ring_info_t *r
 		netif_receive_skb(skb);
 	}
 #else
-	if (sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
+	if (VLAN_ENABLED && sp->vlgrp && RXD_GET_VLAN_TAG(rxdp->Control_2)) {
 		/* Queueing the vlan frame to the upper layer */
 		vlan_hwaccel_rx(skb, sp->vlgrp,
 			RXD_GET_VLAN_TAG(rxdp->Control_2));
@@ -6051,9 +6053,11 @@ Defaulting to INTA\n");
 	dev->do_ioctl = &s2io_ioctl;
 	dev->change_mtu = &s2io_change_mtu;
 	SET_ETHTOOL_OPS(dev, &netdev_ethtool_ops);
+#if VLAN_ENABLED
 	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_RX;
 	dev->vlan_rx_register = s2io_vlan_rx_register;
 	dev->vlan_rx_kill_vid = (void *)s2io_vlan_rx_kill_vid;
+#endif
 
 	/*
 	 * will use eth_mac_addr() for  dev->set_mac_address
diff -urpN linux-2.6.16.org/drivers/net/typhoon.c linux-2.6.16.vlan/drivers/net/typhoon.c
--- linux-2.6.16.org/drivers/net/typhoon.c	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/drivers/net/typhoon.c	Wed Apr 12 11:40:48 2006
@@ -707,6 +707,7 @@ out:
 	return err;
 }
 
+#if VLAN_ENABLED
 static void
 typhoon_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 {
@@ -754,6 +755,7 @@ typhoon_vlan_rx_kill_vid(struct net_devi
 		tp->vlgrp->vlan_devices[vid] = NULL;
 	spin_unlock_bh(&tp->state_lock);
 }
+#endif
 
 static inline void
 typhoon_tso_fill(struct sk_buff *skb, struct transmit_ring *txRing,
@@ -1744,13 +1746,15 @@ typhoon_rx(struct typhoon *tp, struct ba
 		} else
 			new_skb->ip_summed = CHECKSUM_NONE;
 
-		spin_lock(&tp->state_lock);
-		if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN)
-			vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
+		if(VLAN_ENABLED) {
+			spin_lock(&tp->state_lock);
+			if(tp->vlgrp != NULL && rx->rxStatus & TYPHOON_RX_VLAN) {
+				vlan_hwaccel_receive_skb(new_skb, tp->vlgrp,
 						 ntohl(rx->vlanTag) & 0xffff);
-		else
+			}
+			spin_unlock(&tp->state_lock);
+		} else
 			netif_receive_skb(new_skb);
-		spin_unlock(&tp->state_lock);
 
 		tp->dev->last_rx = jiffies;
 		received++;
@@ -2232,14 +2236,16 @@ typhoon_suspend(struct pci_dev *pdev, pm
 	if(!netif_running(dev))
 		return 0;
 
-	spin_lock_bh(&tp->state_lock);
-	if(tp->vlgrp && tp->wol_events & TYPHOON_WAKE_MAGIC_PKT) {
+	if(VLAN_ENABLED) {
+		spin_lock_bh(&tp->state_lock);
+		if(tp->vlgrp && tp->wol_events & TYPHOON_WAKE_MAGIC_PKT) {
+			spin_unlock_bh(&tp->state_lock);
+			printk(KERN_ERR "%s: cannot do WAKE_MAGIC with VLANS\n",
+					dev->name);
+			return -EBUSY;
+		}
 		spin_unlock_bh(&tp->state_lock);
-		printk(KERN_ERR "%s: cannot do WAKE_MAGIC with VLANS\n",
-				dev->name);
-		return -EBUSY;
 	}
-	spin_unlock_bh(&tp->state_lock);
 
 	netif_device_detach(dev);
 
@@ -2549,8 +2555,10 @@ typhoon_init_one(struct pci_dev *pdev, c
 	dev->watchdog_timeo	= TX_TIMEOUT;
 	dev->get_stats		= typhoon_get_stats;
 	dev->set_mac_address	= typhoon_set_mac_address;
+#if VLAN_ENABLED
 	dev->vlan_rx_register	= typhoon_vlan_rx_register;
 	dev->vlan_rx_kill_vid	= typhoon_vlan_rx_kill_vid;
+#endif
 	SET_ETHTOOL_OPS(dev, &typhoon_ethtool_ops);
 
 	/* We can handle scatter gather, up to 16 entries, and
diff -urpN linux-2.6.16.org/include/linux/if_vlan.h linux-2.6.16.vlan/include/linux/if_vlan.h
--- linux-2.6.16.org/include/linux/if_vlan.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/include/linux/if_vlan.h	Wed Apr 12 11:38:14 2006
@@ -23,6 +23,12 @@ struct vlan_collection;
 struct vlan_dev_info;
 struct hlist_node;
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+#define VLAN_ENABLED 1
+#else
+#define VLAN_ENABLED 0
+#endif
+
 #include <linux/proc_fs.h> /* for proc_dir_entry */
 #include <linux/netdevice.h>
 
@@ -144,54 +150,18 @@ struct vlan_skb_tx_cookie {
 
 #define VLAN_TX_COOKIE_MAGIC	0x564c414e	/* "VLAN" in ascii. */
 #define VLAN_TX_SKB_CB(__skb)	((struct vlan_skb_tx_cookie *)&((__skb)->cb[0]))
+#if VLAN_ENABLED 1
 #define vlan_tx_tag_present(__skb) \
 	(VLAN_TX_SKB_CB(__skb)->magic == VLAN_TX_COOKIE_MAGIC)
+#else
+#define vlan_tx_tag_present(__skb) 0
+#endif
 #define vlan_tx_tag_get(__skb)	(VLAN_TX_SKB_CB(__skb)->vlan_tag)
 
 /* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
-static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
+int __vlan_hwaccel_rx(struct sk_buff *skb,
 				    struct vlan_group *grp,
-				    unsigned short vlan_tag, int polling)
-{
-	struct net_device_stats *stats;
-
-	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
-	if (skb->dev == NULL) {
-		dev_kfree_skb_any(skb);
-
-		/* Not NET_RX_DROP, this is not being dropped
-		 * due to congestion.
-		 */
-		return 0;
-	}
-
-	skb->dev->last_rx = jiffies;
-
-	stats = vlan_dev_get_stats(skb->dev);
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
-
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST:
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST:
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	};
-
-	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
-}
+				    unsigned short vlan_tag, int polling);
 
 static inline int vlan_hwaccel_rx(struct sk_buff *skb,
 				  struct vlan_group *grp,
@@ -218,43 +188,7 @@ static inline int vlan_hwaccel_receive_s
  * Following the skb_unshare() example, in case of error, the calling function
  * doesn't have to worry about freeing the original skb.
  */
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
-{
-	struct vlan_ethhdr *veth;
-
-	if (skb_headroom(skb) < VLAN_HLEN) {
-		struct sk_buff *sk_tmp = skb;
-		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
-		kfree_skb(sk_tmp);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to realloc headroom\n");
-			return NULL;
-		}
-	} else {
-		skb = skb_unshare(skb, GFP_ATOMIC);
-		if (!skb) {
-			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
-			return NULL;
-		}
-	}
-
-	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
-
-	/* Move the mac addresses to the beginning of the new header. */
-	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
-
-	/* first, the ethernet type */
-	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
-
-	/* now, the tag */
-	veth->h_vlan_TCI = htons(tag);
-
-	skb->protocol = __constant_htons(ETH_P_8021Q);
-	skb->mac.raw -= VLAN_HLEN;
-	skb->nh.raw -= VLAN_HLEN;
-
-	return skb;
-}
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag);
 
 /**
  * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
diff -urpN linux-2.6.16.org/include/linux/netdevice.h linux-2.6.16.vlan/include/linux/netdevice.h
--- linux-2.6.16.org/include/linux/netdevice.h	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/include/linux/netdevice.h	Wed Apr 12 11:29:34 2006
@@ -475,12 +475,14 @@ struct net_device
 #define HAVE_TX_TIMEOUT
 	void			(*tx_timeout) (struct net_device *dev);
 
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
 	void			(*vlan_rx_register)(struct net_device *dev,
 						    struct vlan_group *grp);
 	void			(*vlan_rx_add_vid)(struct net_device *dev,
 						   unsigned short vid);
 	void			(*vlan_rx_kill_vid)(struct net_device *dev,
 						    unsigned short vid);
+#endif
 
 	int			(*hard_header_parse)(struct sk_buff *skb,
 						     unsigned char *haddr);
diff -urpN linux-2.6.16.org/net/8021q/Makefile linux-2.6.16.vlan/net/8021q/Makefile
--- linux-2.6.16.org/net/8021q/Makefile	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/net/8021q/Makefile	Wed Apr 12 11:29:34 2006
@@ -2,6 +2,8 @@
 # Makefile for the Linux VLAN layer.
 #
 
+obj-y			 += if_vlan.o 
+
 obj-$(CONFIG_VLAN_8021Q) += 8021q.o
 
 8021q-objs := vlan.o vlan_dev.o
diff -urpN linux-2.6.16.org/net/8021q/if_vlan.c linux-2.6.16.vlan/net/8021q/if_vlan.c
--- linux-2.6.16.org/net/8021q/if_vlan.c	Thu Jan  1 03:00:00 1970
+++ linux-2.6.16.vlan/net/8021q/if_vlan.c	Wed Apr 12 11:29:34 2006
@@ -0,0 +1,111 @@
+/* 802.1q helpers.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+
+#include <linux/skbuff.h>
+#include <linux/if_vlan.h>
+
+#if VLAN_ENABLED
+
+/* VLAN rx hw acceleration helper.  This acts like netif_{rx,receive_skb}(). */
+int __vlan_hwaccel_rx(struct sk_buff *skb,
+				    struct vlan_group *grp,
+				    unsigned short vlan_tag, int polling)
+{
+	struct net_device_stats *stats;
+
+	skb->dev = grp->vlan_devices[vlan_tag & VLAN_VID_MASK];
+	if (skb->dev == NULL) {
+		dev_kfree_skb_any(skb);
+
+		/* Not NET_RX_DROP, this is not being dropped
+		 * due to congestion.
+		 */
+		return 0;
+	}
+
+	skb->dev->last_rx = jiffies;
+
+	stats = vlan_dev_get_stats(skb->dev);
+	stats->rx_packets++;
+	stats->rx_bytes += skb->len;
+
+	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
+	switch (skb->pkt_type) {
+	case PACKET_BROADCAST:
+		break;
+
+	case PACKET_MULTICAST:
+		stats->multicast++;
+		break;
+
+	case PACKET_OTHERHOST:
+		/* Our lower layer thinks this is not local, let's make sure.
+		 * This allows the VLAN to have a different MAC than the underlying
+		 * device, and still route correctly.
+		 */
+		if (!memcmp(eth_hdr(skb)->h_dest, skb->dev->dev_addr, ETH_ALEN))
+			skb->pkt_type = PACKET_HOST;
+		break;
+	};
+
+	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
+}
+EXPORT_SYMBOL(__vlan_hwaccel_rx);
+
+/**
+ * __vlan_put_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @tag: VLAN tag to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ * 
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+struct sk_buff *__vlan_put_tag(struct sk_buff *skb, unsigned short tag)
+{
+	struct vlan_ethhdr *veth;
+
+	if (skb_headroom(skb) < VLAN_HLEN) {
+		struct sk_buff *sk_tmp = skb;
+		skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
+		kfree_skb(sk_tmp);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to realloc headroom\n");
+			return NULL;
+		}
+	} else {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb) {
+			printk(KERN_ERR "vlan: failed to unshare skbuff\n");
+			return NULL;
+		}
+	}
+
+	veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
+
+	/* Move the mac addresses to the beginning of the new header. */
+	memmove(skb->data, skb->data + VLAN_HLEN, 2 * VLAN_ETH_ALEN);
+
+	/* first, the ethernet type */
+	veth->h_vlan_proto = __constant_htons(ETH_P_8021Q);
+
+	/* now, the tag */
+	veth->h_vlan_TCI = htons(tag);
+
+	skb->protocol = __constant_htons(ETH_P_8021Q);
+	skb->mac.raw -= VLAN_HLEN;
+	skb->nh.raw -= VLAN_HLEN;
+
+	return skb;
+}
+EXPORT_SYMBOL(__vlan_put_tag);
+
+#endif
diff -urpN linux-2.6.16.org/net/Makefile linux-2.6.16.vlan/net/Makefile
--- linux-2.6.16.org/net/Makefile	Mon Mar 20 07:53:29 2006
+++ linux-2.6.16.vlan/net/Makefile	Wed Apr 12 11:29:34 2006
@@ -41,7 +41,7 @@ obj-$(CONFIG_RXRPC)		+= rxrpc/
 obj-$(CONFIG_ATM)		+= atm/
 obj-$(CONFIG_DECNET)		+= decnet/
 obj-$(CONFIG_ECONET)		+= econet/
-obj-$(CONFIG_VLAN_8021Q)	+= 8021q/
+obj-y				+= 8021q/
 obj-$(CONFIG_IP_DCCP)		+= dccp/
 obj-$(CONFIG_IP_SCTP)		+= sctp/
 obj-$(CONFIG_IEEE80211)		+= ieee80211/

Re: [PATCH] deinline a few large functions in vlan code v2

[Dave Dillow]

On Wed, 2006-04-12 at 11:55 +0300, Denis Vlasenko wrote:
> On Tuesday 11 April 2006 16:59, Dave Dillow wrote:
> > DaveM beat me to it, but as he said, it saves 5K only if you have all
> > the drivers built in
> 
> I have most of network drivers built in.
> I want network card to work right away in early boot,
> and I prefer to not regenerate initrd with new nic modules for
> each new kernel which I build for diskless stations.

Do you really intend to say that you are the common case, and that this
5K really matters? I'm all for removing bloat, but not without looking
at the real savings vs the cost.

> > or loaded. And even if it saves 200 bytes in one 
> > module, unless that module text was already less than 200 bytes into a
> > page, you've saved no memory -- a 4300 byte module takes 2 pages on x86,
> > as does a 4100 byte module.
> 
> Sometimes, those 200 bytes can bring module size just under 4096.
> Thus on the average, on many modules you get the same size savings
> as on built-in code. (Not that we have THAT many network modules...)

You're making a bogus leap from "sometimes" to "average".

Assuming an even distribution of lengths mod 4096, less than 5% of the
time will 200 bytes save any memory on a module.

Since we're both making big assumptions, only real numbers from built
modules would be convincing.

> > NAK. The #ifdefs are ugly, for no significant gain.
> > 
> > And you introduced a race condition when you moved the spin_locks. The
> > test for tp->vlgrp is no longer protected.
> 
> See attached. Much less #ifdefs (most of the time I test for compile time
> constant VLAN_ENABLED in the if()s instead), the race is fixed.

Testing for VLAN_ENABLED in an if statement is better than the ifdefs, I
agree, but I still find it ugly. Probably not ugly enough to argue about
it any more.

I don't like "VLAN_ENABLED" as a global define -- it looks too much like
something local. The CONFIG_VLAN... defines were more descriptive.

I didn't think about this before, but I'm pretty sure you're taking away
functionality. When I wrote the typhoon driver, ISTR that I looked
through the vlan implantation, and determined that all the #ifdefs on
CONFIG_VLAN_8021Q were not really needed, since all the hooks were
there. You could just load the 8021q module (even perhaps building it at
a later date), and it would work if you had filled in the hooks in
struct net_device. So I didn't #ifdef out code in my driver to let the
user have the option.

You're taking that away in the name of a total of 5K, which most users
won't actually get back?
-- 
Dave Dillow <dave@thedillows.org>

Re: [PATCH] deinline a few large functions in vlan code v2

[Denis Vlasenko]

On Wednesday 12 April 2006 20:18, Dave Dillow wrote:
> > > or loaded. And even if it saves 200 bytes in one 
> > > module, unless that module text was already less than 200 bytes into a
> > > page, you've saved no memory -- a 4300 byte module takes 2 pages on x86,
> > > as does a 4100 byte module.
> > 
> > Sometimes, those 200 bytes can bring module size just under 4096.
> > Thus on the average, on many modules you get the same size savings
> > as on built-in code. (Not that we have THAT many network modules...)
> 
> You're making a bogus leap from "sometimes" to "average".

It's not bogus. See below.

> Assuming an even distribution of lengths mod 4096, less than 5% of the
> time will 200 bytes save any memory on a module.

Ok, imagine perfect module size distribution, and suppose we shave 204
bytes off each module. 5% of the modules will have their size reduced
so that they occupy one 4k page less.

Those 5% of modules will have their RAM footprint reduced not
by just 204 bytes, but by 4096 bytes.

Total RAM footprint of all modules, in kb (N=number of modules):
before: sum_of_orig_module_size
after, modular: sum_of_orig_module_size - 0.05*N * 4096
after, builtin: sum_of_orig_module_size - N * 204

0.05*4096 = 204.53

> I don't like "VLAN_ENABLED" as a global define -- it looks too much like
> something local. The CONFIG_VLAN... defines were more descriptive.

That will require Kconfig changes. Maybe you would like "VLAN_ENABLED_KERNER"
name? It doesn't sound local...

> I didn't think about this before, but I'm pretty sure you're taking away
> functionality. When I wrote the typhoon driver, ISTR that I looked
> through the vlan implantation, and determined that all the #ifdefs on
> CONFIG_VLAN_8021Q were not really needed, since all the hooks were
> there. You could just load the 8021q module (even perhaps building it at
> a later date), and it would work if you had filled in the hooks in
> struct net_device. So I didn't #ifdef out code in my driver to let the
> user have the option.

IOW: currently most of VLAN code is already in kernel.
Then why do we have VLAN as a config option? Let's make it unconditional
(will add only 10k to core kernel)?

# size net/8021q/8021q.o
   text    data     bss     dec     hex filename
   9379     484     136    9999    270f net/8021q/8021q.o

> You're taking that away in the name of a total of 5K, which most users
> won't actually get back?

It started as a kernel-wide audit for huge inlines, I was not aiming
at VLAN particularly. But yes, when I saw other (not related to inlines)
opportunities to make code smaller, I decided to do it.

You know, people say that even Linux is getting fat these days.
--
vda

Re: [PATCH] deinline a few large functions in vlan code v2

[Dave Dillow]

On Thu, 2006-04-13 at 09:04 +0300, Denis Vlasenko wrote:
> On Wednesday 12 April 2006 20:18, Dave Dillow wrote:
> > > > or loaded. And even if it saves 200 bytes in one 
> > > > module, unless that module text was already less than 200 bytes into a
> > > > page, you've saved no memory -- a 4300 byte module takes 2 pages on x86,
> > > > as does a 4100 byte module.
> > > 
> > > Sometimes, those 200 bytes can bring module size just under 4096.
> > > Thus on the average, on many modules you get the same size savings
> > > as on built-in code. (Not that we have THAT many network modules...)
> > 
> > You're making a bogus leap from "sometimes" to "average".
> 
> It's not bogus. See below.

My bad, I used a different notion of "average". You're using a
mathematical definition, and are of course correct in that case.

But to get your average, you have to either build all modules in, or
load every module. I'm saying the "average" user won't do that.

> IOW: currently most of VLAN code is already in kernel.

No, I'm saying most of the code is in 8021q.ko. The exceptions are the
big inlines you targeted, and I agree with moving them out-of-line.

-- 
Dave Dillow <dave@thedillows.org>

Re: [PATCH] deinline a few large functions in vlan code v2

[Ingo Oeser]

Hi Dave,

On Wednesday, 12. April 2006 19:18, Dave Dillow wrote:
> you've left the spin_locks in, and have more #ifdefs.

Ok, I can refactor your driver to even remove this and reduce it to exaxtly 
two ifdef sections for your driver. Acceptable?
 
> Regardless, I remain opposed to this particular instance of bloat 
> busting. While both patches have improved in style, they remove a useful 
> feature and make the code less clean, for no net gain.

s/useful feature/unreachable code/g

> On SMP FC4, typhoon.ko has a text size of 68330, so you need to cut 2794 
> bytes to see an actual difference in memory usage for a module. Non-SMP 
> it is 67741, so there you only need to cut 2205 bytes to get a win.

   text    data     bss     dec     hex filename
  62079     973       4   63056    f650 no-vlan/drivers/net/typhoon.o
  62654     973       4   63631    f88f vlan/drivers/net/typhoon.o

with allyesconfig (minus CONFIG_INFO) and my patches applied.
So maybe the uninlining is enough. Gain after this is just 575 bytes here.


Regards

Ingo Oeser