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

From: Denis Vlasenko
Date: Tue Apr 11 2006 - 03:29:33 EST


> > > > 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/