[3/4] net/enic: support multicast filtering
diff mbox series

Message ID 20181210182857.13043-4-hyonkim@cisco.com
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • net/enic: minor updates
Related show

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Hyong Youb Kim Dec. 10, 2018, 6:28 p.m. UTC
The VIC hardware has 64 MAC filters per vNIC, which can be either
unicast or multicast. Use one half for unicast and the other half for
multicast, as the VIC kernel drivers for Linux and Windows do.

Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 doc/guides/nics/features/enic.ini |  2 +-
 drivers/net/enic/enic.h           |  6 ++-
 drivers/net/enic/enic_ethdev.c    | 96 ++++++++++++++++++++++++++++++++++++++-
 drivers/net/enic/enic_main.c      |  5 +-
 4 files changed, 103 insertions(+), 6 deletions(-)

Comments

Ferruh Yigit Jan. 11, 2019, 3:46 p.m. UTC | #1
On 12/10/2018 6:28 PM, Hyong Youb Kim wrote:
> The VIC hardware has 64 MAC filters per vNIC, which can be either
> unicast or multicast. Use one half for unicast and the other half for
> multicast, as the VIC kernel drivers for Linux and Windows do.
> 
> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> Reviewed-by: John Daley <johndale@cisco.com>

<...>

> +static void debug_log_add_del_addr(struct ether_addr *addr, bool add)
> +{
> +	char mac_str[ETHER_ADDR_FMT_SIZE];
> +	if (rte_log_get_level(enicpmd_logtype_init) == RTE_LOG_DEBUG) {
> +		ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
> +		PMD_INIT_LOG(ERR, " %s address %s\n",
> +			     add ? "add" : "remove", mac_str);
> +	}
> +}

Why logging with 'ERR' level after checking if 'DEBUG' level is set?

And why need that check at all?
Hyong Youb Kim Jan. 12, 2019, 4:49 a.m. UTC | #2
On Fri, Jan 11, 2019 at 03:46:47PM +0000, Ferruh Yigit wrote:
> On 12/10/2018 6:28 PM, Hyong Youb Kim wrote:
> > The VIC hardware has 64 MAC filters per vNIC, which can be either
> > unicast or multicast. Use one half for unicast and the other half for
> > multicast, as the VIC kernel drivers for Linux and Windows do.
> > 
> > Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> > Reviewed-by: John Daley <johndale@cisco.com>
> 
> <...>
> 
> > +static void debug_log_add_del_addr(struct ether_addr *addr, bool add)
> > +{
> > +	char mac_str[ETHER_ADDR_FMT_SIZE];
> > +	if (rte_log_get_level(enicpmd_logtype_init) == RTE_LOG_DEBUG) {
> > +		ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
> > +		PMD_INIT_LOG(ERR, " %s address %s\n",
> > +			     add ? "add" : "remove", mac_str);
> > +	}
> > +}
> 
> Why logging with 'ERR' level after checking if 'DEBUG' level is set?

Thanks for pointing this out. Our mistake. Should be DEBUG.

> And why need that check at all?

The outer check ("is log level debug?") is there to make this function
to do nothing (including ether_format_addr) if log level > debug. I
could not find a good way to combine the level test,
ether_format_addr, and rte_log into one clean macro..

Since this function is on a slow path, I think I can just remove the
level check and do something like the following. Would this satisfy
your concern?

char mac_str[ETHER_ADDR_FMT_SIZE];
ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
PMD_INIT_LOG(DEBUG, " %s address %s\n", add ? "add" : "remove", mac_str);

Thanks.
-Hyong
Ferruh Yigit Jan. 14, 2019, 10:29 a.m. UTC | #3
On 1/12/2019 4:49 AM, Hyong Youb Kim wrote:
> On Fri, Jan 11, 2019 at 03:46:47PM +0000, Ferruh Yigit wrote:
>> On 12/10/2018 6:28 PM, Hyong Youb Kim wrote:
>>> The VIC hardware has 64 MAC filters per vNIC, which can be either
>>> unicast or multicast. Use one half for unicast and the other half for
>>> multicast, as the VIC kernel drivers for Linux and Windows do.
>>>
>>> Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
>>> Reviewed-by: John Daley <johndale@cisco.com>
>>
>> <...>
>>
>>> +static void debug_log_add_del_addr(struct ether_addr *addr, bool add)
>>> +{
>>> +	char mac_str[ETHER_ADDR_FMT_SIZE];
>>> +	if (rte_log_get_level(enicpmd_logtype_init) == RTE_LOG_DEBUG) {
>>> +		ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
>>> +		PMD_INIT_LOG(ERR, " %s address %s\n",
>>> +			     add ? "add" : "remove", mac_str);
>>> +	}
>>> +}
>>
>> Why logging with 'ERR' level after checking if 'DEBUG' level is set?
> 
> Thanks for pointing this out. Our mistake. Should be DEBUG.
> 
>> And why need that check at all?
> 
> The outer check ("is log level debug?") is there to make this function
> to do nothing (including ether_format_addr) if log level > debug. I
> could not find a good way to combine the level test,
> ether_format_addr, and rte_log into one clean macro..
> 
> Since this function is on a slow path, I think I can just remove the
> level check and do something like the following. Would this satisfy
> your concern?
> 
> char mac_str[ETHER_ADDR_FMT_SIZE];
> ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
> PMD_INIT_LOG(DEBUG, " %s address %s\n", add ? "add" : "remove", mac_str);

Yes, I think this is better, thanks.

Patch
diff mbox series

diff --git a/doc/guides/nics/features/enic.ini b/doc/guides/nics/features/enic.ini
index 020b75cf0..c6d374984 100644
--- a/doc/guides/nics/features/enic.ini
+++ b/doc/guides/nics/features/enic.ini
@@ -15,7 +15,7 @@  TSO                  = Y
 Promiscuous mode     = Y
 Allmulticast mode    = Y
 Unicast MAC filter   = Y
-Multicast MAC filter =
+Multicast MAC filter = Y
 RSS hash             = Y
 RSS key update       = Y
 RSS reta update      = Y
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 7bca3cad2..6c497e9a2 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -25,8 +25,6 @@ 
 #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Poll-mode Driver"
 #define DRV_COPYRIGHT		"Copyright 2008-2015 Cisco Systems, Inc"
 
-#define ENIC_MAX_MAC_ADDR	64
-
 #define VLAN_ETH_HLEN           18
 
 #define ENICPMD_SETTING(enic, f) ((enic->config.flags & VENETF_##f) ? 1 : 0)
@@ -196,6 +194,10 @@  struct enic {
 	uint64_t tx_offload_capa; /* DEV_TX_OFFLOAD flags */
 	uint64_t tx_queue_offload_capa; /* DEV_TX_OFFLOAD flags */
 	uint64_t tx_offload_mask; /* PKT_TX flags accepted */
+
+	/* Multicast MAC addresses added to the NIC */
+	uint32_t mc_count;
+	struct ether_addr mc_addrs[ENIC_MULTICAST_PERFECT_FILTERS];
 };
 
 /* Compute ethdev's max packet size from MTU */
diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 9b206e8f0..fbbec2266 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -367,6 +367,7 @@  static int enicpmd_dev_configure(struct rte_eth_dev *eth_dev)
 		return ret;
 	}
 
+	enic->mc_count = 0;
 	enic->hw_ip_checksum = !!(eth_dev->data->dev_conf.rxmode.offloads &
 				  DEV_RX_OFFLOAD_CHECKSUM);
 	/* All vlan offload masks to apply the current settings */
@@ -473,7 +474,7 @@  static void enicpmd_dev_info_get(struct rte_eth_dev *eth_dev,
 	 * ignoring vNIC mtu.
 	 */
 	device_info->max_rx_pktlen = enic_mtu_to_max_rx_pktlen(enic->max_mtu);
-	device_info->max_mac_addrs = ENIC_MAX_MAC_ADDR;
+	device_info->max_mac_addrs = ENIC_UNICAST_PERFECT_FILTERS;
 	device_info->rx_offload_capa = enic->rx_offload_capa;
 	device_info->tx_offload_capa = enic->tx_offload_capa;
 	device_info->tx_queue_offload_capa = enic->tx_queue_offload_capa;
@@ -643,6 +644,98 @@  static int enicpmd_set_mac_addr(struct rte_eth_dev *eth_dev,
 	return enic_set_mac_address(enic, addr->addr_bytes);
 }
 
+static void debug_log_add_del_addr(struct ether_addr *addr, bool add)
+{
+	char mac_str[ETHER_ADDR_FMT_SIZE];
+	if (rte_log_get_level(enicpmd_logtype_init) == RTE_LOG_DEBUG) {
+		ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
+		PMD_INIT_LOG(ERR, " %s address %s\n",
+			     add ? "add" : "remove", mac_str);
+	}
+}
+
+static int enicpmd_set_mc_addr_list(struct rte_eth_dev *eth_dev,
+				    struct ether_addr *mc_addr_set,
+				    uint32_t nb_mc_addr)
+{
+	struct enic *enic = pmd_priv(eth_dev);
+	char mac_str[ETHER_ADDR_FMT_SIZE];
+	struct ether_addr *addr;
+	uint32_t i, j;
+	int ret;
+
+	ENICPMD_FUNC_TRACE();
+
+	/* Validate the given addresses first */
+	for (i = 0; i < nb_mc_addr && mc_addr_set != NULL; i++) {
+		addr = &mc_addr_set[i];
+		if (!is_multicast_ether_addr(addr) ||
+		    is_broadcast_ether_addr(addr)) {
+			ether_format_addr(mac_str, ETHER_ADDR_FMT_SIZE, addr);
+			PMD_INIT_LOG(ERR, " invalid multicast address %s\n",
+				     mac_str);
+			return -EINVAL;
+		}
+	}
+
+	/* Flush all if requested */
+	if (nb_mc_addr == 0 || mc_addr_set == NULL) {
+		PMD_INIT_LOG(DEBUG, " flush multicast addresses\n");
+		for (i = 0; i < enic->mc_count; i++) {
+			addr = &enic->mc_addrs[i];
+			debug_log_add_del_addr(addr, false);
+			ret = vnic_dev_del_addr(enic->vdev, addr->addr_bytes);
+			if (ret)
+				return ret;
+		}
+		enic->mc_count = 0;
+		return 0;
+	}
+
+	if (nb_mc_addr > ENIC_MULTICAST_PERFECT_FILTERS) {
+		PMD_INIT_LOG(ERR, " too many multicast addresses: max=%d\n",
+			     ENIC_MULTICAST_PERFECT_FILTERS);
+		return -ENOSPC;
+	}
+	/*
+	 * devcmd is slow, so apply the difference instead of flushing and
+	 * adding everything.
+	 * 1. Delete addresses on the NIC but not on the host
+	 */
+	for (i = 0; i < enic->mc_count; i++) {
+		addr = &enic->mc_addrs[i];
+		for (j = 0; j < nb_mc_addr; j++) {
+			if (is_same_ether_addr(addr, &mc_addr_set[j]))
+				break;
+		}
+		if (j < nb_mc_addr)
+			continue;
+		debug_log_add_del_addr(addr, false);
+		ret = vnic_dev_del_addr(enic->vdev, addr->addr_bytes);
+		if (ret)
+			return ret;
+	}
+	/* 2. Add addresses on the host but not on the NIC */
+	for (i = 0; i < nb_mc_addr; i++) {
+		addr = &mc_addr_set[i];
+		for (j = 0; j < enic->mc_count; j++) {
+			if (is_same_ether_addr(addr, &enic->mc_addrs[j]))
+				break;
+		}
+		if (j < enic->mc_count)
+			continue;
+		debug_log_add_del_addr(addr, true);
+		ret = vnic_dev_add_addr(enic->vdev, addr->addr_bytes);
+		if (ret)
+			return ret;
+	}
+	/* Keep a copy so we can flush/apply later on.. */
+	memcpy(enic->mc_addrs, mc_addr_set,
+	       nb_mc_addr * sizeof(struct ether_addr));
+	enic->mc_count = nb_mc_addr;
+	return 0;
+}
+
 static int enicpmd_mtu_set(struct rte_eth_dev *eth_dev, uint16_t mtu)
 {
 	struct enic *enic = pmd_priv(eth_dev);
@@ -951,6 +1044,7 @@  static const struct eth_dev_ops enicpmd_eth_dev_ops = {
 	.mac_addr_add         = enicpmd_add_mac_addr,
 	.mac_addr_remove      = enicpmd_remove_mac_addr,
 	.mac_addr_set         = enicpmd_set_mac_addr,
+	.set_mc_addr_list     = enicpmd_set_mc_addr_list,
 	.filter_ctrl          = enicpmd_dev_filter_ctrl,
 	.reta_query           = enicpmd_dev_rss_reta_query,
 	.reta_update          = enicpmd_dev_rss_reta_update,
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index f45717374..621a984a0 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1667,8 +1667,9 @@  static int enic_dev_init(struct enic *enic)
 	/* Get the supported filters */
 	enic_fdir_info(enic);
 
-	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr", ETH_ALEN
-						* ENIC_MAX_MAC_ADDR, 0);
+	eth_dev->data->mac_addrs = rte_zmalloc("enic_mac_addr",
+					sizeof(struct ether_addr) *
+					ENIC_UNICAST_PERFECT_FILTERS, 0);
 	if (!eth_dev->data->mac_addrs) {
 		dev_err(enic, "mac addr storage alloc failed, aborting.\n");
 		return -1;