implement set_mc_addr devop

Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop

Signed-off-by Nelio Laranjeiro <[email protected]>

---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
+++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)

drivers/net/mlx5/mlx5.c index renamed from drivers/net/mlx5/mlx5.c +0 -0

68783c3ac..887924d07 100644

drivers/net/mlx5/mlx5.c +1 -0

@@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@

const struct eth_dev_ops mlx5_dev_ops_isolate = {

drivers/net/mlx5/mlx5.c +1 -0

 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set, diff --git

a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index 6ad41390a..64f025d22 100644

drivers/net/mlx5/mlx5.h +0 -0

@@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
 *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
 struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);  int

mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);

drivers/net/mlx5/mlx5.h +2 -0

+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t

nb_mc_addr);

/* mlx5_rss.c */

drivers/net/mlx5/mlx5_defs.h +0 -0

index e3aa03bef..5973742a7 100644
@@ -15,8 +15,11 @@
 
 /* Maximum number of simultaneous unicast MAC addresses. */  #define

MLX5_MAX_UC_MAC_ADDRESSES 128

drivers/net/mlx5/mlx5_defs.h +2 -0

+/* Maximum number of simultaneous Multicast MAC addresses. */ #define
+MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */ -#define

MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES

drivers/net/mlx5/mlx5_defs.h +2 -0

+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES +

MLX5_MAX_MC_MAC_ADDRESSES)

/* Maximum number of simultaneous VLAN filters. */ #define
MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644

drivers/net/mlx5/mlx5_mac.c +10 -0

@@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
 ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
+
+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.

drivers/net/mlx5/mlx5_mac.c +1 -0

+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,

i++);

drivers/net/mlx5/mlx5_mac.c +7 -1

+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
--

2.17.0

Hi Nelio,

Small comments before I merge.

Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:

Quoted 87 lines

Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop

Signed-off-by Nelio Laranjeiro <[email protected]>

---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
+++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)

drivers/net/mlx5/mlx5.c index renamed from drivers/net/mlx5/mlx5.c +0 -0

68783c3ac..887924d07 100644

drivers/net/mlx5/mlx5.c +1 -0

@@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@

const struct eth_dev_ops mlx5_dev_ops_isolate = {

drivers/net/mlx5/mlx5.c +1 -0

 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set, diff --git

a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index 6ad41390a..64f025d22 100644

drivers/net/mlx5/mlx5.h +0 -0

@@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
 *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
 struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);  int

mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);

drivers/net/mlx5/mlx5.h +2 -0

+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t

nb_mc_addr);

/* mlx5_rss.c */

drivers/net/mlx5/mlx5_defs.h +0 -0

index e3aa03bef..5973742a7 100644
@@ -15,8 +15,11 @@
 
 /* Maximum number of simultaneous unicast MAC addresses. */  #define

MLX5_MAX_UC_MAC_ADDRESSES 128

drivers/net/mlx5/mlx5_defs.h +2 -0

+/* Maximum number of simultaneous Multicast MAC addresses. */ #define
+MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */ -#define

MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES

drivers/net/mlx5/mlx5_defs.h +2 -0

+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES +

MLX5_MAX_MC_MAC_ADDRESSES)

/* Maximum number of simultaneous VLAN filters. */ #define
MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644

drivers/net/mlx5/mlx5_mac.c +15 -0

@@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
 ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
+
+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 224

Quoted 5 lines

drivers/net/mlx5/mlx5_mac.c +5 -0

+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
+	uint32_t i;
+	int ret;
+

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start operate.

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 228

Quoted 5 lines

drivers/net/mlx5/mlx5_mac.c +1 -0

+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=

MLX5_MAX_MAC_ADDRESSES; ++i)

drivers/net/mlx5/mlx5_mac.c +3 -0

+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
+ warn if it is not.

Quoted 11 lines

drivers/net/mlx5/mlx5_mac.c +1 -0

+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,

i++);

drivers/net/mlx5/mlx5_mac.c +7 -1

+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
--

2.17.0

Quoted 13 lines

Quoted 13 lines

+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ * Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ * Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ * Number of entries in the array.
+ *
+ * @return
+ * 0 on success, a negative errno value otherwise and rte_errno is set.
+ */

Quoted 8 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 224

Quoted 6 lines

drivers/net/mlx5/mlx5_mac.c +6 -0

+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
+	uint32_t i;
+	int ret;
+

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES

before we start

Quoted 1 line

operate.

This verification is done in the sub function.

Considering an application calling such API wants to remove/replace the old list with new entries.
That this new one can be just an addition or totally different list or even empty.
This new list can be larger than the amount of MAC addresses the PMD can support.

There are two possibilities:

1. The list is too large: the application will enable the all multicast mode to receive the extra mac addresses it needs.

2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated verification?

Note: if an error happens the new list is not committed yet i.e. the traffic remains untouched.

Quoted 8 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 228

Quoted 5 lines

(quoted diff) +1 -0

+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=

MLX5_MAX_MAC_ADDRESSES; ++i)

drivers/net/mlx5/mlx5_mac.c +3 -0

+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
+ warn if it is not.

Such verification should be done in the public API i.e. ethdev.

Quoted 12 lines

Quoted 11 lines

(quoted diff) +1 -0

+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,

i++);

drivers/net/mlx5/mlx5_mac.c +7 -1

+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
--

Quoted 1 line

2.17.0

Regards,

--
Nélio Laranjeiro
6WIND

Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
[...]

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

drivers/net/mlx5/mlx5_mac.c +9 -0

+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 224

Quoted 10 lines

drivers/net/mlx5/mlx5_mac.c +10 -0

+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
+	uint32_t i;
+	int ret;
+

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES

Quoted 4 lines

before we start

Quoted 1 line

operate.

This verification is done in the sub function.

I see only assert. Did I missed anything?

Quoted 7 lines

Considering an application calling such API wants to remove/replace the old list with new entries.
That this new one can be just an addition or totally different list or even empty.
This new list can be larger than the amount of MAC addresses the PMD can support.

Quoted 4 lines

There are two possibilities:

1. The list is too large: the application will enable the all multicast mode to receive the extra mac addresses it needs.

How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

Quoted 6 lines

2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated verification?

At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault.

Quoted 12 lines

Note: if an error happens the new list is not committed yet i.e. the traffic remains untouched.

Quoted 8 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 228

Quoted 5 lines

(quoted diff) +1 -0

+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=

MLX5_MAX_MAC_ADDRESSES; ++i)

drivers/net/mlx5/mlx5_mac.c +3 -0

+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
+ warn if it is not.

Quoted 1 line

Such verification should be done in the public API i.e. ethdev.

I don't understand.
In the first patch of the series you add extra verification to check the mac address validity.

But for the MC you claim it should be done on ethdev layer.

It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

(quoted diff) +1 -0

+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,

i++);

drivers/net/mlx5/mlx5_mac.c +7 -1

+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
--

2.17.0

Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop

Quoted 14 lines · Shahaf Shuler

Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
[...]

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ * Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ * Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ * Number of entries in the array.
+ *
+ * @return
+ * 0 on success, a negative errno value otherwise and rte_errno is

set.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)

Quoted 4 lines

+ */

drivers/net/mlx5/mlx5_mac.c +3 -0

+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t

nb_mc_addr) {

Quoted 11 lines

Quoted 9 lines

Quoted 5 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 224

Quoted 3 lines

drivers/net/mlx5/mlx5_mac.c +3 -0

+	uint32_t i;
+	int ret;
+

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES

before we start

Quoted 1 line

operate.

This verification is done in the sub function.

I see only assert. Did I missed anything?

No.

Quoted 7 lines

Quoted 7 lines

Considering an application calling such API wants to remove/replace the old list with new entries.
That this new one can be just an addition or totally different list or even empty.
This new list can be larger than the amount of MAC addresses the PMD can support.

Quoted 8 lines

Quoted 4 lines

There are two possibilities:

1. The list is too large: the application will enable the all multicast mode to receive the extra mac addresses it needs.

How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

Such information is not reported at all. The application has to guess.

Quoted 9 lines

Quoted 6 lines

2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to clear it before/after enabling the "all multicast" mode. The final result being the same, does it worse to add a duplicated verification?

At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault.

Right it needs a verification.

Quoted 13 lines

Quoted 13 lines

Note: if an error happens the new list is not committed yet i.e. the traffic remains untouched.

Quoted 9 lines

drivers/net/mlx5/mlx5_mac.c mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr) · line 228

Quoted 5 lines

(quoted diff) +1 -0

+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=

MLX5_MAX_MAC_ADDRESSES; ++i)

drivers/net/mlx5/mlx5_mac.c +3 -0

+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
+ warn if it is not.

Quoted 5 lines

Quoted 1 line

Such verification should be done in the public API i.e. ethdev.

I don't understand.
In the first patch of the series you add extra verification to check the mac address validity.

It only verify the MAC address is not zero, it does not verify the MAC address is valid in the function context (e.g. unicast in mlx5_mac_addr_add()).

Quoted 1 line

But for the MC you claim it should be done on ethdev layer.

Dito.

Quoted 3 lines

It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch.

It is consistent, the PMD only verify the MAC address is not zero and this in both API.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+		ret = mlx5_internal_mac_addr_add(dev,

mc_addr_set++,

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

i++);

drivers/net/mlx5/mlx5_mac.c +7 -1

+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
--

2.17.0

Regards,

--
Nélio Laranjeiro
6WIND

Support eth_dev_ops.set_mc_addr_list().

Changes in v2:

Simplify the code and are even more verifications.

Nelio Laranjeiro (3):
net/mlx5: more checks on MAC addresses
net/mlx5: split MAC address add/remove code
net/mlx5: implement multicast add list devop

 drivers/net/mlx5/mlx5.c        |   2 +
 drivers/net/mlx5/mlx5.h        |   2 +
 drivers/net/mlx5/mlx5_defs.h   |   7 +-
 drivers/net/mlx5/mlx5_ethdev.c |   2 +-
 drivers/net/mlx5/mlx5_mac.c    | 117 ++++++++++++++++++++++++++++-----
 5 files changed, 112 insertions(+), 18 deletions(-)

Subject: [dpdk-dev] [PATCH v3 0/3] net/mlx5: implement set_mc_addr devop

Support eth_dev_ops.set_mc_addr_list().

Changes in v3:

Check PMD as enough resources to process the Multicast address list.

Changes in v2:

Simplify the code and are even more verifications.

Nelio Laranjeiro (3):
net/mlx5: more checks on MAC addresses
net/mlx5: split MAC address add/remove code
net/mlx5: implement multicast add list devop

 drivers/net/mlx5/mlx5.c        |   2 +
 drivers/net/mlx5/mlx5.h        |   2 +
 drivers/net/mlx5/mlx5_defs.h   |   7 +-
 drivers/net/mlx5/mlx5_ethdev.c |   2 +-
 drivers/net/mlx5/mlx5_mac.c    | 113 ++++++++++++++++++++++++++++---
--
 5 files changed, 108 insertions(+), 18 deletions(-)

--
2.17.0