Re: [PATCH net-next] bonding: attempt to better support longer hw addresses

2017-04-06 Thread Jarod Wilson 

On 2017-04-05 9:45 PM, David Miller wrote:

From: Jarod Wilson 
Date: Tue,  4 Apr 2017 17:32:42 -0400

...

Applied, but:


+static inline void bond_hw_addr_copy(u8 *dst, const u8 *src, unsigned int len)
+{
+   if (len == ETH_ALEN) {
+   ether_addr_copy(dst, src);
+   return;
+   }
+
+   memcpy(dst, src, len);
+}


I wonder how much value there is in trying to conditionally use
ether_addr_copy().  Unless some of these calls are in the data
plane, just a straight memcpy() all the time is fine and much
simpler.


Yeah, I wasn't sure how much gain the bonding driver actually got from 
using the super-optimized ether_addr_copy(), and thought about just 
doing a memcpy all the time, but wanted to go for minimal impact to 
traditional ethernet bonding. Looks like bond_handle_frame() might 
benefit from sticking to ether_addr_copy() when it can, but the majority 
of other callers, at least in bond_main.c, are all in setup, teardown 
and failover paths, which ought to be tolerant of some overhead, though 
optimized failover isn't a bad thing for connection uptime. I do see 
bond_alb.c has one caller in the arp transmit path as well. I think I'm 
inclined to just leave well enough alone for the moment.


--
Jarod Wilson
ja...@redhat.com

Re: [PATCH net-next] bonding: attempt to better support longer hw addresses

2017-04-05 Thread David Miller 
From: Jarod Wilson 
Date: Tue,  4 Apr 2017 17:32:42 -0400

> People are using bonding over Infiniband IPoIB connections, and who knows
> what else. Infiniband has a hardware address length of 20 octets
> (INFINIBAND_ALEN), and the network core defines a MAX_ADDR_LEN of 32.
> Various places in the bonding code are currently hard-wired to 6 octets
> (ETH_ALEN), such as the 3ad code, which I've left untouched here. Besides,
> only alb is currently possible on Infiniband links right now anyway, due
> to commit 1533e7731522, so the alb code is where most of the changes are.
> 
> One major component of this change is the addition of a bond_hw_addr_copy
> function that takes a length argument, instead of using ether_addr_copy
> everywhere that hardware addresses need to be copied about. The other
> major component of this change is converting the bonding code from using
> struct sockaddr for address storage to struct sockaddr_storage, as the
> former has an address storage space of only 14, while the latter is 128
> minus a few, which is necessary to support bonding over device with up to
> MAX_ADDR_LEN octet hardware addresses. Additionally, this probably fixes
> up some memory corruption issues with the current code, where it's
> possible to write an infiniband hardware address into a sockaddr declared
> on the stack.
> 
> Lightly tested on a dual mlx4 IPoIB setup, which properly shows a 20-octet
> hardware address now:
 ...
> CC: Jay Vosburgh 
> CC: Veaceslav Falico 
> CC: Andy Gospodarek 
> CC: netdev@vger.kernel.org
> Signed-off-by: Jarod Wilson 

Applied, but:

> +static inline void bond_hw_addr_copy(u8 *dst, const u8 *src, unsigned int 
> len)
> +{
> + if (len == ETH_ALEN) {
> + ether_addr_copy(dst, src);
> + return;
> + }
> +
> + memcpy(dst, src, len);
> +}

I wonder how much value there is in trying to conditionally use
ether_addr_copy().  Unless some of these calls are in the data
plane, just a straight memcpy() all the time is fine and much
simpler.

Thanks.

[PATCH net-next] bonding: attempt to better support longer hw addresses

2017-04-04 Thread Jarod Wilson 
People are using bonding over Infiniband IPoIB connections, and who knows
what else. Infiniband has a hardware address length of 20 octets
(INFINIBAND_ALEN), and the network core defines a MAX_ADDR_LEN of 32.
Various places in the bonding code are currently hard-wired to 6 octets
(ETH_ALEN), such as the 3ad code, which I've left untouched here. Besides,
only alb is currently possible on Infiniband links right now anyway, due
to commit 1533e7731522, so the alb code is where most of the changes are.

One major component of this change is the addition of a bond_hw_addr_copy
function that takes a length argument, instead of using ether_addr_copy
everywhere that hardware addresses need to be copied about. The other
major component of this change is converting the bonding code from using
struct sockaddr for address storage to struct sockaddr_storage, as the
former has an address storage space of only 14, while the latter is 128
minus a few, which is necessary to support bonding over device with up to
MAX_ADDR_LEN octet hardware addresses. Additionally, this probably fixes
up some memory corruption issues with the current code, where it's
possible to write an infiniband hardware address into a sockaddr declared
on the stack.

Lightly tested on a dual mlx4 IPoIB setup, which properly shows a 20-octet
hardware address now:

$ cat /proc/net/bonding/bond0
Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)

Bonding Mode: fault-tolerance (active-backup) (fail_over_mac active)
Primary Slave: mlx4_ib0 (primary_reselect always)
Currently Active Slave: mlx4_ib0
MII Status: up
MII Polling Interval (ms): 100
Up Delay (ms): 100
Down Delay (ms): 100

Slave Interface: mlx4_ib0
MII Status: up
Speed: Unknown
Duplex: Unknown
Link Failure Count: 0
Permanent HW addr:
80:00:02:08:fe:80:00:00:00:00:00:00:e4:1d:2d:03:00:1d:67:01
Slave queue ID: 0

Slave Interface: mlx4_ib1
MII Status: up
Speed: Unknown
Duplex: Unknown
Link Failure Count: 0
Permanent HW addr:
80:00:02:09:fe:80:00:00:00:00:00:01:e4:1d:2d:03:00:1d:67:02
Slave queue ID: 0

Also tested with a standard 1Gbps NIC bonding setup (with a mix of
e1000 and e1000e cards), running LNST's bonding tests.

CC: Jay Vosburgh 
CC: Veaceslav Falico 
CC: Andy Gospodarek 
CC: netdev@vger.kernel.org
Signed-off-by: Jarod Wilson 
---
 drivers/net/bonding/bond_alb.c| 88 +++
 drivers/net/bonding/bond_main.c   | 73 ++--
 drivers/net/bonding/bond_procfs.c |  3 +-
 include/net/bonding.h | 12 +-
 4 files changed, 108 insertions(+), 68 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index c80b023092dd..7d7a3cec149a 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -687,7 +687,8 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, 
struct bonding *bond)
/* the arp must be sent on the selected rx channel */
tx_slave = rlb_choose_channel(skb, bond);
if (tx_slave)
-   ether_addr_copy(arp->mac_src, tx_slave->dev->dev_addr);
+   bond_hw_addr_copy(arp->mac_src, tx_slave->dev->dev_addr,
+ tx_slave->dev->addr_len);
netdev_dbg(bond->dev, "Server sent ARP Reply packet\n");
} else if (arp->op_code == htons(ARPOP_REQUEST)) {
/* Create an entry in the rx_hashtbl for this client as a
@@ -1017,22 +1018,23 @@ static void alb_send_learning_packets(struct slave 
*slave, u8 mac_addr[],
rcu_read_unlock();
 }
 
-static int alb_set_slave_mac_addr(struct slave *slave, u8 addr[])
+static int alb_set_slave_mac_addr(struct slave *slave, u8 addr[],
+ unsigned int len)
 {
struct net_device *dev = slave->dev;
-   struct sockaddr s_addr;
+   struct sockaddr_storage ss;
 
if (BOND_MODE(slave->bond) == BOND_MODE_TLB) {
-   memcpy(dev->dev_addr, addr, dev->addr_len);
+   memcpy(dev->dev_addr, addr, len);
return 0;
}
 
/* for rlb each slave must have a unique hw mac addresses so that
 * each slave will receive packets destined to a different mac
 */
-   memcpy(s_addr.sa_data, addr, dev->addr_len);
-   s_addr.sa_family = dev->type;
-   if (dev_set_mac_address(dev, &s_addr)) {
+   memcpy(ss.__data, addr, len);
+   ss.ss_family = dev->type;
+   if (dev_set_mac_address(dev, (struct sockaddr *)&ss)) {
netdev_err(slave->bond->dev, "dev_set_mac_address of dev %s 
failed! ALB mode requires that the base driver support setting the hw address 
also when the network device's interface is open\n",
   dev->name);
return -EOPNOTSUPP;
@@ -1046,11 +1048,14 @@ static int alb_set_slave_mac_addr(struct slave *slave, 
u8 addr[])
  */
 static void alb_swap_mac_addr(struct slave *slave1, struct slave *slave2)
 {