netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [net-next PATCH 0/3] ixgbe: Fix FDB handling
@ 2015-10-22 23:26 Alexander Duyck
  2015-10-22 23:26 ` [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code Alexander Duyck
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Alexander Duyck @ 2015-10-22 23:26 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher

This patch series addresses a number of issues in the FDB handling code of
the ixgbe driver.  Specifically there were 3 issues.

1.  The addresses were being allocated but not freed from the MAC address
tables.  As a result cycling in and out various addresses would end up
exhausing the table.

2.  The entries weren't being counted correclty and as a result only half
of the MAC address entries could be used at any given point in time.

3.  The FDB entries were being limited to only 15 while there were 126
RAL/H entries available.

With this patch series in place it is possible ot maintain a decent sized
set of FDB entries and they can cycle in and out without incurring any
heavy cost as previously seen.

---

Alexander Duyck (3):
      ixgbe: Refactor MAC address configuration code
      ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses
      ixgbe: Allow FDB entries access to more RAR filters


 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    7 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  196 ++++++++++++++++---------
 2 files changed, 129 insertions(+), 74 deletions(-)

--

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code
  2015-10-22 23:26 [net-next PATCH 0/3] ixgbe: Fix FDB handling Alexander Duyck
@ 2015-10-22 23:26 ` Alexander Duyck
  2015-11-11 23:34   ` [Intel-wired-lan] " Miller, Darin J
  2015-10-22 23:26 ` [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses Alexander Duyck
  2015-10-22 23:26 ` [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters Alexander Duyck
  2 siblings, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2015-10-22 23:26 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher

In the process of tracking down a memory leak when adding/removing fdb
entries I had to go through the MAC address configuration code for ixgbe.
In the process of doing so I found a number of issues that impacted
readability and performance.  This change updates the code in general to
clean it up so it becomes clear what each step is doing.  From what I can
tell there a couple of bugs cleaned up in this code.

First is the fact that the MAC addresses were being double counted for the
PF.  As a result once entries up to 63 had been used you could no longer
add additional filters.

A simple test case for this:
  for i in `seq 0 96`
  do
    ip link add link ens8 name mv$i type macvlan
    ip link set dev mv$i up
  done

Test script:
  ethregs -s 0:8.0 | grep -e "RAH" | grep 8000....$

When things are working correctly RAL/H registers 1 - 97 will be consumed.
In the failing case it will stop at 63 and prevent any further filters from
being added.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    7 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  163 +++++++++++++++----------
 2 files changed, 100 insertions(+), 70 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index dda0f678339a..44ee7f778b07 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -579,9 +579,10 @@ static inline u16 ixgbe_desc_unused(struct ixgbe_ring *ring)
 
 struct ixgbe_mac_addr {
 	u8 addr[ETH_ALEN];
-	u16 queue;
+	u16 pool;
 	u16 state; /* bitmask */
 };
+
 #define IXGBE_MAC_STATE_DEFAULT		0x1
 #define IXGBE_MAC_STATE_MODIFIED	0x2
 #define IXGBE_MAC_STATE_IN_USE		0x4
@@ -875,9 +876,9 @@ int ixgbe_wol_supported(struct ixgbe_adapter *adapter, u16 device_id,
 void ixgbe_full_sync_mac_table(struct ixgbe_adapter *adapter);
 #endif
 int ixgbe_add_mac_filter(struct ixgbe_adapter *adapter,
-			 u8 *addr, u16 queue);
+			 const u8 *addr, u16 queue);
 int ixgbe_del_mac_filter(struct ixgbe_adapter *adapter,
-			 u8 *addr, u16 queue);
+			 const u8 *addr, u16 queue);
 void ixgbe_clear_interrupt_scheme(struct ixgbe_adapter *adapter);
 netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *, struct ixgbe_adapter *,
 				  struct ixgbe_ring *);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 677cde8599de..650c9526bbde 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4034,124 +4034,156 @@ static int ixgbe_write_mc_addr_list(struct net_device *netdev)
 #ifdef CONFIG_PCI_IOV
 void ixgbe_full_sync_mac_table(struct ixgbe_adapter *adapter)
 {
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
 	int i;
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		if (adapter->mac_table[i].state & IXGBE_MAC_STATE_IN_USE)
-			hw->mac.ops.set_rar(hw, i, adapter->mac_table[i].addr,
-					    adapter->mac_table[i].queue,
+
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		mac_table->state &= ~IXGBE_MAC_STATE_MODIFIED;
+
+		if (mac_table->state & IXGBE_MAC_STATE_IN_USE)
+			hw->mac.ops.set_rar(hw, i,
+					    mac_table->addr,
+					    mac_table->pool,
 					    IXGBE_RAH_AV);
 		else
 			hw->mac.ops.clear_rar(hw, i);
-
-		adapter->mac_table[i].state &= ~(IXGBE_MAC_STATE_MODIFIED);
 	}
 }
-#endif
 
+#endif
 static void ixgbe_sync_mac_table(struct ixgbe_adapter *adapter)
 {
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
 	int i;
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		if (adapter->mac_table[i].state & IXGBE_MAC_STATE_MODIFIED) {
-			if (adapter->mac_table[i].state &
-			    IXGBE_MAC_STATE_IN_USE)
-				hw->mac.ops.set_rar(hw, i,
-						adapter->mac_table[i].addr,
-						adapter->mac_table[i].queue,
-						IXGBE_RAH_AV);
-			else
-				hw->mac.ops.clear_rar(hw, i);
 
-			adapter->mac_table[i].state &=
-						~(IXGBE_MAC_STATE_MODIFIED);
-		}
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		if (!(mac_table->state & IXGBE_MAC_STATE_MODIFIED))
+			continue;
+
+		mac_table->state &= ~IXGBE_MAC_STATE_MODIFIED;
+
+		if (mac_table->state & IXGBE_MAC_STATE_IN_USE)
+			hw->mac.ops.set_rar(hw, i,
+					    mac_table->addr,
+					    mac_table->pool,
+					    IXGBE_RAH_AV);
+		else
+			hw->mac.ops.clear_rar(hw, i);
 	}
 }
 
 static void ixgbe_flush_sw_mac_table(struct ixgbe_adapter *adapter)
 {
-	int i;
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
+	int i;
 
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		adapter->mac_table[i].state |= IXGBE_MAC_STATE_MODIFIED;
-		adapter->mac_table[i].state &= ~IXGBE_MAC_STATE_IN_USE;
-		eth_zero_addr(adapter->mac_table[i].addr);
-		adapter->mac_table[i].queue = 0;
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		mac_table->state |= IXGBE_MAC_STATE_MODIFIED;
+		mac_table->state &= ~IXGBE_MAC_STATE_IN_USE;
 	}
+
 	ixgbe_sync_mac_table(adapter);
 }
 
-static int ixgbe_available_rars(struct ixgbe_adapter *adapter)
+static int ixgbe_available_rars(struct ixgbe_adapter *adapter, u16 pool)
 {
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
 	int i, count = 0;
 
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		if (adapter->mac_table[i].state == 0)
-			count++;
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		/* do not count default RAR as available */
+		if (mac_table->state & IXGBE_MAC_STATE_DEFAULT)
+			continue;
+
+		/* only count unused and addresses that belong to us */
+		if (mac_table->state & IXGBE_MAC_STATE_IN_USE) {
+			if (mac_table->pool != pool)
+				continue;
+		}
+
+		count++;
 	}
+
 	return count;
 }
 
 /* this function destroys the first RAR entry */
-static void ixgbe_mac_set_default_filter(struct ixgbe_adapter *adapter,
-					 u8 *addr)
+static void ixgbe_mac_set_default_filter(struct ixgbe_adapter *adapter)
 {
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
 
-	memcpy(&adapter->mac_table[0].addr, addr, ETH_ALEN);
-	adapter->mac_table[0].queue = VMDQ_P(0);
-	adapter->mac_table[0].state = (IXGBE_MAC_STATE_DEFAULT |
-				       IXGBE_MAC_STATE_IN_USE);
-	hw->mac.ops.set_rar(hw, 0, adapter->mac_table[0].addr,
-			    adapter->mac_table[0].queue,
+	memcpy(&mac_table->addr, hw->mac.addr, ETH_ALEN);
+	mac_table->pool = VMDQ_P(0);
+
+	mac_table->state = IXGBE_MAC_STATE_DEFAULT | IXGBE_MAC_STATE_IN_USE;
+
+	hw->mac.ops.set_rar(hw, 0, mac_table->addr, mac_table->pool,
 			    IXGBE_RAH_AV);
 }
 
-int ixgbe_add_mac_filter(struct ixgbe_adapter *adapter, u8 *addr, u16 queue)
+int ixgbe_add_mac_filter(struct ixgbe_adapter *adapter,
+			 const u8 *addr, u16 pool)
 {
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
 	int i;
 
 	if (is_zero_ether_addr(addr))
 		return -EINVAL;
 
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		if (adapter->mac_table[i].state & IXGBE_MAC_STATE_IN_USE)
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		if (mac_table->state & IXGBE_MAC_STATE_IN_USE)
 			continue;
-		adapter->mac_table[i].state |= (IXGBE_MAC_STATE_MODIFIED |
-						IXGBE_MAC_STATE_IN_USE);
-		ether_addr_copy(adapter->mac_table[i].addr, addr);
-		adapter->mac_table[i].queue = queue;
+
+		ether_addr_copy(mac_table->addr, addr);
+		mac_table->pool = pool;
+
+		mac_table->state |= IXGBE_MAC_STATE_MODIFIED |
+				    IXGBE_MAC_STATE_IN_USE;
+
 		ixgbe_sync_mac_table(adapter);
+
 		return i;
 	}
+
 	return -ENOMEM;
 }
 
-int ixgbe_del_mac_filter(struct ixgbe_adapter *adapter, u8 *addr, u16 queue)
+int ixgbe_del_mac_filter(struct ixgbe_adapter *adapter,
+			 const u8 *addr, u16 pool)
 {
-	/* search table for addr, if found, set to 0 and sync */
-	int i;
+	struct ixgbe_mac_addr *mac_table = &adapter->mac_table[0];
 	struct ixgbe_hw *hw = &adapter->hw;
+	int i;
 
 	if (is_zero_ether_addr(addr))
 		return -EINVAL;
 
-	for (i = 0; i < hw->mac.num_rar_entries; i++) {
-		if (ether_addr_equal(addr, adapter->mac_table[i].addr) &&
-		    adapter->mac_table[i].queue == queue) {
-			adapter->mac_table[i].state |= IXGBE_MAC_STATE_MODIFIED;
-			adapter->mac_table[i].state &= ~IXGBE_MAC_STATE_IN_USE;
-			eth_zero_addr(adapter->mac_table[i].addr);
-			adapter->mac_table[i].queue = 0;
-			ixgbe_sync_mac_table(adapter);
-			return 0;
-		}
+	/* search table for addr, if found clear IN_USE flag and sync */
+	for (i = 0; i < hw->mac.num_rar_entries; i++, mac_table++) {
+		/* we can only delete an entry if it is in use */
+		if (!(mac_table->state & IXGBE_MAC_STATE_IN_USE))
+			continue;
+		/* we only care about entries that belong to the given pool */
+		if (mac_table->pool != pool)
+			continue;
+		/* we only care about a specific MAC address */
+		if (!ether_addr_equal(addr, mac_table->addr))
+			continue;
+
+		mac_table->state |= IXGBE_MAC_STATE_MODIFIED;
+		mac_table->state &= ~IXGBE_MAC_STATE_IN_USE;
+
+		ixgbe_sync_mac_table(adapter);
+
+		return 0;
 	}
+
 	return -ENOMEM;
 }
 /**
@@ -4169,7 +4201,7 @@ static int ixgbe_write_uc_addr_list(struct net_device *netdev, int vfn)
 	int count = 0;
 
 	/* return ENOMEM indicating insufficient memory for addresses */
-	if (netdev_uc_count(netdev) > ixgbe_available_rars(adapter))
+	if (netdev_uc_count(netdev) > ixgbe_available_rars(adapter, vfn))
 		return -ENOMEM;
 
 	if (!netdev_uc_empty(netdev)) {
@@ -5042,7 +5074,6 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 	struct ixgbe_hw *hw = &adapter->hw;
 	struct net_device *netdev = adapter->netdev;
 	int err;
-	u8 old_addr[ETH_ALEN];
 
 	if (ixgbe_removed(hw->hw_addr))
 		return;
@@ -5079,9 +5110,8 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 
 	clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state);
 	/* do not flush user set addresses */
-	memcpy(old_addr, &adapter->mac_table[0].addr, netdev->addr_len);
 	ixgbe_flush_sw_mac_table(adapter);
-	ixgbe_mac_set_default_filter(adapter, old_addr);
+	ixgbe_mac_set_default_filter(adapter);
 
 	/* update SAN MAC vmdq pool selection */
 	if (hw->mac.san_mac_rar_index)
@@ -7659,17 +7689,16 @@ static int ixgbe_set_mac(struct net_device *netdev, void *p)
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
 	struct sockaddr *addr = p;
-	int ret;
 
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	ixgbe_del_mac_filter(adapter, hw->mac.addr, VMDQ_P(0));
 	memcpy(netdev->dev_addr, addr->sa_data, netdev->addr_len);
 	memcpy(hw->mac.addr, addr->sa_data, netdev->addr_len);
 
-	ret = ixgbe_add_mac_filter(adapter, hw->mac.addr, VMDQ_P(0));
-	return ret > 0 ? 0 : ret;
+	ixgbe_mac_set_default_filter(adapter);
+
+	return 0;
 }
 
 static int
@@ -8872,7 +8901,7 @@ skip_sriov:
 		goto err_sw_init;
 	}
 
-	ixgbe_mac_set_default_filter(adapter, hw->mac.perm_addr);
+	ixgbe_mac_set_default_filter(adapter);
 
 	setup_timer(&adapter->service_timer, &ixgbe_service_timer,
 		    (unsigned long) adapter);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses
  2015-10-22 23:26 [net-next PATCH 0/3] ixgbe: Fix FDB handling Alexander Duyck
  2015-10-22 23:26 ` [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code Alexander Duyck
@ 2015-10-22 23:26 ` Alexander Duyck
  2015-11-04 18:39   ` [Intel-wired-lan] " Miller, Darin J
  2015-11-12  1:35   ` Stephen Hemminger
  2015-10-22 23:26 ` [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters Alexander Duyck
  2 siblings, 2 replies; 9+ messages in thread
From: Alexander Duyck @ 2015-10-22 23:26 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher

This change replaces the ixgbe_write_uc_addr_list call in ixgbe_set_rx_mode
with a call to __dev_uc_sync instead.  This works much better with the MAC
addr list code that was already in place and solves an issue in which you
couldn't remove an fdb address without having to reset the port.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   28 ++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 650c9526bbde..ba5c0d5b95b9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -4215,6 +4215,25 @@ static int ixgbe_write_uc_addr_list(struct net_device *netdev, int vfn)
 	return count;
 }
 
+static int ixgbe_uc_sync(struct net_device *netdev, const unsigned char *addr)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	int ret;
+
+	ret = ixgbe_add_mac_filter(adapter, addr, VMDQ_P(0));
+
+	return min_t(int, ret, 0);
+}
+
+static int ixgbe_uc_unsync(struct net_device *netdev, const unsigned char *addr)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+
+	ixgbe_del_mac_filter(adapter, addr, VMDQ_P(0));
+
+	return 0;
+}
+
 /**
  * ixgbe_set_rx_mode - Unicast, Multicast and Promiscuous mode set
  * @netdev: network interface device structure
@@ -4270,8 +4289,7 @@ void ixgbe_set_rx_mode(struct net_device *netdev)
 	 * sufficient space to store all the addresses then enable
 	 * unicast promiscuous mode
 	 */
-	count = ixgbe_write_uc_addr_list(netdev, VMDQ_P(0));
-	if (count < 0) {
+	if (__dev_uc_sync(netdev, ixgbe_uc_sync, ixgbe_uc_unsync)) {
 		fctrl |= IXGBE_FCTRL_UPE;
 		vmolr |= IXGBE_VMOLR_ROPE;
 	}
@@ -5109,8 +5127,12 @@ void ixgbe_reset(struct ixgbe_adapter *adapter)
 	}
 
 	clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state);
-	/* do not flush user set addresses */
+
+	/* flush entries out of MAC table */
 	ixgbe_flush_sw_mac_table(adapter);
+	__dev_uc_unsync(netdev, NULL);
+
+	/* do not flush user set addresses */
 	ixgbe_mac_set_default_filter(adapter);
 
 	/* update SAN MAC vmdq pool selection */

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters
  2015-10-22 23:26 [net-next PATCH 0/3] ixgbe: Fix FDB handling Alexander Duyck
  2015-10-22 23:26 ` [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code Alexander Duyck
  2015-10-22 23:26 ` [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses Alexander Duyck
@ 2015-10-22 23:26 ` Alexander Duyck
  2015-11-04 18:40   ` [Intel-wired-lan] " Miller, Darin J
  2 siblings, 1 reply; 9+ messages in thread
From: Alexander Duyck @ 2015-10-22 23:26 UTC (permalink / raw)
  To: netdev, intel-wired-lan, jeffrey.t.kirsher

This change makes it so that we allow the PF to make use of all free RAR
entries for FDB use if needed.

Previously the code limited us to 16 unicast entries, however this was
shared between MACVLAN which wasn't limited and the FDB code which was.  So
instead of treating the FDB code as a second class citizen I have updated
it so that it has access to just as many entries as the MACVLAN filters.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ba5c0d5b95b9..4befe637b9be 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8206,7 +8206,10 @@ static int ixgbe_ndo_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 {
 	/* guarantee we can provide a unique filter for the unicast address */
 	if (is_unicast_ether_addr(addr) || is_link_local_ether_addr(addr)) {
-		if (IXGBE_MAX_PF_MACVLANS <= netdev_uc_count(dev))
+		struct ixgbe_adapter *adapter = netdev_priv(dev);
+		u16 pool = VMDQ_P(0);
+
+		if (netdev_uc_count(dev) >= ixgbe_available_rars(adapter, pool))
 			return -ENOMEM;
 	}
 

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [Intel-wired-lan] [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses
  2015-10-22 23:26 ` [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses Alexander Duyck
@ 2015-11-04 18:39   ` Miller, Darin J
  2015-11-12  1:35   ` Stephen Hemminger
  1 sibling, 0 replies; 9+ messages in thread
From: Miller, Darin J @ 2015-11-04 18:39 UTC (permalink / raw)
  To: 'Alexander Duyck', netdev@vger.kernel.org,
	intel-wired-lan@lists.osuosl.org, Kirsher, Jeffrey T



-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, October 22, 2015 4:27 PM
To: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
Subject: [Intel-wired-lan] [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses

This change replaces the ixgbe_write_uc_addr_list call in ixgbe_set_rx_mode with a call to __dev_uc_sync instead.  This works much better with the MAC addr list code that was already in place and solves an issue in which you couldn't remove an fdb address without having to reset the port.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   28 ++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

Tested-by: Darin Miller <darin.j.miller@intel.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [Intel-wired-lan] [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters
  2015-10-22 23:26 ` [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters Alexander Duyck
@ 2015-11-04 18:40   ` Miller, Darin J
  0 siblings, 0 replies; 9+ messages in thread
From: Miller, Darin J @ 2015-11-04 18:40 UTC (permalink / raw)
  To: 'Alexander Duyck', netdev@vger.kernel.org,
	intel-wired-lan@lists.osuosl.org, Kirsher, Jeffrey T



-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, October 22, 2015 4:27 PM
To: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
Subject: [Intel-wired-lan] [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters

This change makes it so that we allow the PF to make use of all free RAR entries for FDB use if needed.

Previously the code limited us to 16 unicast entries, however this was shared between MACVLAN which wasn't limited and the FDB code which was.  So instead of treating the FDB code as a second class citizen I have updated it so that it has access to just as many entries as the MACVLAN filters.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Tested-by: Darin Miller <darin.j.miller@intel.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [Intel-wired-lan] [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code
  2015-10-22 23:26 ` [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code Alexander Duyck
@ 2015-11-11 23:34   ` Miller, Darin J
  0 siblings, 0 replies; 9+ messages in thread
From: Miller, Darin J @ 2015-11-11 23:34 UTC (permalink / raw)
  To: 'Alexander Duyck', netdev@vger.kernel.org,
	intel-wired-lan@lists.osuosl.org, Kirsher, Jeffrey T



-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, October 22, 2015 4:27 PM
To: netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Kirsher, Jeffrey T
Subject: [Intel-wired-lan] [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code

In the process of tracking down a memory leak when adding/removing fdb entries I had to go through the MAC address configuration code for ixgbe.
In the process of doing so I found a number of issues that impacted readability and performance.  This change updates the code in general to clean it up so it becomes clear what each step is doing.  From what I can tell there a couple of bugs cleaned up in this code.

First is the fact that the MAC addresses were being double counted for the PF.  As a result once entries up to 63 had been used you could no longer add additional filters.

A simple test case for this:
  for i in `seq 0 96`
  do
    ip link add link ens8 name mv$i type macvlan
    ip link set dev mv$i up
  done

Test script:
  ethregs -s 0:8.0 | grep -e "RAH" | grep 8000....$

When things are working correctly RAL/H registers 1 - 97 will be consumed.
In the failing case it will stop at 63 and prevent any further filters from being added.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    7 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  163 +++++++++++++++----------
 2 files changed, 100 insertions(+), 70 deletions(-)

Tested-by: Darin Miller <darin.j.miller@intel.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses
  2015-10-22 23:26 ` [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses Alexander Duyck
  2015-11-04 18:39   ` [Intel-wired-lan] " Miller, Darin J
@ 2015-11-12  1:35   ` Stephen Hemminger
  2015-11-12  5:21     ` [Intel-wired-lan] " Alexander Duyck
  1 sibling, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2015-11-12  1:35 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: netdev, intel-wired-lan, jeffrey.t.kirsher

On Thu, 22 Oct 2015 16:26:36 -0700
Alexander Duyck <aduyck@mirantis.com> wrote:

> +static int ixgbe_uc_unsync(struct net_device *netdev, const unsigned char *addr)
> +{
> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> +
> +	ixgbe_del_mac_filter(adapter, addr, VMDQ_P(0));
> +
> +	return 0;

Why add an internal function that always returns 0?
Rather than making it void.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Intel-wired-lan] [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses
  2015-11-12  1:35   ` Stephen Hemminger
@ 2015-11-12  5:21     ` Alexander Duyck
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Duyck @ 2015-11-12  5:21 UTC (permalink / raw)
  To: Stephen Hemminger, Alexander Duyck; +Cc: netdev, intel-wired-lan

On 11/11/2015 05:35 PM, Stephen Hemminger wrote:
> On Thu, 22 Oct 2015 16:26:36 -0700
> Alexander Duyck <aduyck@mirantis.com> wrote:
>
>> +static int ixgbe_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>> +{
>> +	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>> +
>> +	ixgbe_del_mac_filter(adapter, addr, VMDQ_P(0));
>> +
>> +	return 0;
> Why add an internal function that always returns 0?
> Rather than making it void.

Because the function pointer is passed to the __dev_uc_sync call and it 
requires a return value on the unsync function.  Basically if we 
returned an error it would delay flushing the address from the device 
until we could complete the call successfully, or __dev_uc_unsysnc was 
called without a function pointer.

- Alex

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-11-12  5:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-22 23:26 [net-next PATCH 0/3] ixgbe: Fix FDB handling Alexander Duyck
2015-10-22 23:26 ` [net-next PATCH 1/3] ixgbe: Refactor MAC address configuration code Alexander Duyck
2015-11-11 23:34   ` [Intel-wired-lan] " Miller, Darin J
2015-10-22 23:26 ` [net-next PATCH 2/3] ixgbe: Use __dev_uc_sync and __dev_uc_unsync for unicast addresses Alexander Duyck
2015-11-04 18:39   ` [Intel-wired-lan] " Miller, Darin J
2015-11-12  1:35   ` Stephen Hemminger
2015-11-12  5:21     ` [Intel-wired-lan] " Alexander Duyck
2015-10-22 23:26 ` [net-next PATCH 3/3] ixgbe: Allow FDB entries access to more RAR filters Alexander Duyck
2015-11-04 18:40   ` [Intel-wired-lan] " Miller, Darin J

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).