diff mbox

[tbench,regression,fixes] : digging out smelly deadmen.

Message ID 22495.1225498593@death.nxdomain.ibm.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Jay Vosburgh Nov. 1, 2008, 12:16 a.m. UTC 
Stephen Hemminger <shemminger@vyatta.com> wrote:

>On Fri, 31 Oct 2008 16:51:44 -0700 (PDT)
>David Miller <davem@davemloft.net> wrote:
[...]
>> However, I do like Stephen's suggestion that maybe we can get rid of
>> this ->last_rx thing by encapsulating the logic completely in the
>> bonding driver.
>
>Since bonding driver doesn't actually see the rx packets, that isn't
>really possible.  But it would be possible to change last_rx from a variable
>to an function pointer, so that device's could apply other logic to derive
>the last value.  One example would be to keep it per cpu and then take the
>maximum.

	I suspect it could also be tucked away in skb_bond_should_drop,
which is called both by the standard input path and the VLAN accelerated
path to see if the packet should be tossed (e.g., it arrived on an
inactive bonding slave).

	Since last_rx is part of struct net_device, I don't think any
additional bonding internals knowledge would be needed.  It could be
arranged to only update last_rx for devices that are actually bonding
slaves.

	Just off the top of my head (haven't tested this), something
like this:



	That doesn't move the storage out of struct net_device, but it
does stop the updates for devices that aren't bonding slaves.  It could
probably be refined further to only update when the ARP monitor is
running (the gizmo that uses last_rx).

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller Nov. 2, 2008, 4:40 a.m. UTC | #1 
From: Jay Vosburgh <fubar@us.ibm.com>
Date: Fri, 31 Oct 2008 17:16:33 -0700

> 	I suspect it could also be tucked away in skb_bond_should_drop,
> which is called both by the standard input path and the VLAN accelerated
> path to see if the packet should be tossed (e.g., it arrived on an
> inactive bonding slave).
> 
> 	Since last_rx is part of struct net_device, I don't think any
> additional bonding internals knowledge would be needed.  It could be
> arranged to only update last_rx for devices that are actually bonding
> slaves.
> 
> 	Just off the top of my head (haven't tested this), something
> like this:
 ...
> 
> 	That doesn't move the storage out of struct net_device, but it
> does stop the updates for devices that aren't bonding slaves.  It could
> probably be refined further to only update when the ARP monitor is
> running (the gizmo that uses last_rx).

I like this very much.

Jay can you give this a quick test by just trying this patch
and removing the ->last_rx setting in the driver you use for
your test?

Once you do that, I'll apply this to net-next-2.6 and do the
leg work to zap all of the ->last_rx updates from the entire tree.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c8bcb59..ed1e58f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1743,22 +1743,24 @@  static inline int skb_bond_should_drop(struct sk_buff *skb)
 	struct net_device *dev = skb->dev;
 	struct net_device *master = dev->master;
 
-	if (master &&
-	    (dev->priv_flags & IFF_SLAVE_INACTIVE)) {
-		if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
-		    skb->protocol == __constant_htons(ETH_P_ARP))
-			return 0;
-
-		if (master->priv_flags & IFF_MASTER_ALB) {
-			if (skb->pkt_type != PACKET_BROADCAST &&
-			    skb->pkt_type != PACKET_MULTICAST)
+	if (master) {
+		dev->last_rx = jiffies;
+		if (dev->priv_flags & IFF_SLAVE_INACTIVE)) {
+			if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
+			    skb->protocol == __constant_htons(ETH_P_ARP))
 				return 0;
-		}
-		if (master->priv_flags & IFF_MASTER_8023AD &&
-		    skb->protocol == __constant_htons(ETH_P_SLOW))
-			return 0;
 
-		return 1;
+			if (master->priv_flags & IFF_MASTER_ALB) {
+				if (skb->pkt_type != PACKET_BROADCAST &&
+				    skb->pkt_type != PACKET_MULTICAST)
+					return 0;
+			}
+			if (master->priv_flags & IFF_MASTER_8023AD &&
+			    skb->protocol == __constant_htons(ETH_P_SLOW))
+				return 0;
+
+			return 1;
+		}
 	}
 	return 0;
 }