Re: [PATCH net-next-2.6 1/2] bonding: add keep_all parameter

Andy Gospodarek [off-list ref] wrote:

On Tue, May 11, 2010 at 10:18:21AM -0700, Jay Vosburgh wrote:

quoted

Andy Gospodarek [off-list ref] wrote:

quoted

In an effort to suppress duplicate frames on certain bonding modes
(specifically the modes that do not require additional configuration on
the switch or switches connected to the host),

	Strictly speaking, the above is incorrect, as the duplicate
suppression is turned on for the active-backup inactive slaves as well
as 802.3ad ports that are disabled (any slave that gets the "inactive"
flag bit set).

It is also effective when using ALB and TLB, right?  I can change the
language if you would like to increase the description's accuracy.

	Yah, I forgot about that; the ALB/TLB modes suppress broadcast
and multicast traffic on "inactive" slaves, although that's kind of a
misnomer, since in those modes, "inactive" slaves are active for unicast
traffic.

quoted

quoted

[...] code was added in the
generic receive patch in 2.6.16.  The current behavior works quite well
for most users, but there are some times it would be nice to restore old
functionality and allow all frames to make their way up the stack.

	Reading netdev lately, it sure looks like everybody wants ways
to shut off or bypass the duplicate suppression.

I see that too, which was part of the reason to add a configuration
option.  I know many of the people that complained that they were seeing
dups will complain again if they show up in the future, so a config
option seemed like the best way to satisfy both.

quoted

quoted

This patch adds support for a new module option and sysfs file called
'keep_all' that will restore pre-2.6.16 functionality if the user
desires.  The default value is '0' and retains existing behavior, but
the user can set it to '1' and allow all frames up if desired.

	Since this is really meant for the queue tagging stuff in the
next patch, should this really be something that's enabled automatically
if the queues are configured in such a way that the inactive slave is
going to receive traffic?

Part of the reason not to have it happen automatically is that the
second patch *should* allow simple pass-through of queue-mapping (though
I didn't mention that specifically) from bond device to underlying
slaves if the user is aware of the number of output queues in their
NIC and doesn't set the queue_ids for any of the slaves.

Another reason not to turn it on automatically is if the network patch
for transmission and reception are actually different.  The 'keep_all=1'
flag might not be needed if transmission is happening on an inactive
interface, but the active interface will receive all responses due to
the way the network is designed.

Again, a big part of the motivation patch was bringing back that
old-functionality to those that desire it and was why I split this out
from the next patch.

	I think I addressed a lot of this in my big honkin' reply to the
other patch, so I'll forbear further commment until you're read through
all that.

quoted

	I also wonder if something like this would satisfy the FCOE guys
without making __netif_receive_skb / skb_bond_should_drop even more
complicated than they already are.

I'd love to think so, but you never know.

quoted

quoted

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
---
Documentation/networking/bonding.txt |   15 ++++++++++++
drivers/net/bonding/bond_main.c      |   15 ++++++++++++
drivers/net/bonding/bond_sysfs.c     |   43 +++++++++++++++++++++++++++++++++-
drivers/net/bonding/bonding.h        |    1 +
include/linux/if.h                   |    1 +
net/core/dev.c                       |   26 +++++++++++---------
6 files changed, 88 insertions(+), 13 deletions(-)

diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 61f516b..d64fd2f 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt

@@ -399,6 +399,21 @@ fail_over_mac

	This option was added in bonding version 3.2.0.  The "follow"
	policy was added in bonding version 3.3.0.

+keep_all
+
+	Option to specify whether or not you will keep all frames
+	received on an interface that is a member of a bond.  Right
+	now checking is done to ensure that most frames ultimately
+	classified as duplicates are dropped to keep noise to a
+	minimum.  The feature to drop duplicates was added in kernel
+	version 2.6.16 (bonding driver version 3.0.2) and this will
+	allow that original behavior to be restored if desired.
+
+	A value of 0 (default) will preserve the current behavior and
+	will drop all duplicate frames the bond may receive.  A value
+	of 1 will not attempt to avoid duplicate frames and pass all
+	of them up the stack.

	Two thoughts (presuming for the moment that this doesn't
change): first, bump the driver version and mention when it was added;
second, mention that this only applies to active-backup mode.

Happy to update the version.  But shouldn't this impact ALB and TLB
modes too since they have a concept of 'active' slaves?

quoted

quoted

lacp_rate

	Option specifying the rate in which we'll ask our link partner

<snip>

quoted

quoted

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2758,21 +2758,23 @@ int __skb_bond_should_drop(struct sk_buff *skb, struct net_device *master)

		skb_bond_set_mac_by_master(skb, master);
	}

-	if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
-		if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
-		    skb->protocol == __cpu_to_be16(ETH_P_ARP))
-			return 0;
+	if (unlikely(!(master->priv_flags & IFF_BONDING_KEEP_ALL))) {

	So it's unlikely that "keep all" will be turned off?

Grrrr.  That should be an if(likely!(....  Good catch.

quoted

quoted

+		if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
+			if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
+			    skb->protocol == __cpu_to_be16(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->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 == __cpu_to_be16(ETH_P_SLOW))
				return 0;
-		}
-		if (master->priv_flags & IFF_MASTER_8023AD &&
-		    skb->protocol == __cpu_to_be16(ETH_P_SLOW))
-			return 0;

-		return 1;
+			return 1;
+		}
	}
	return 0;
}
-- 
1.6.2.5

 	-J
 
 ---
 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com