[V5] ethdev: fix one address occupies two indexes in MAC addrs

  The dev->data->mac_addrs[0] will be changed to a new MAC address when
applications modify the default MAC address by .mac_addr_set(). However,
if the new default one has been added as a non-default MAC address by
.mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
list. As a result, one MAC address occupies two indexes in the list. Like:
add(MAC1)
add(MAC2)
add(MAC3)
add(MAC4)
set_default(MAC3)
default=MAC3, filters=MAC1, MAC2, MAC3, MAC4

In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
old default MAC when set default MAC. If user continues to do
set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
but packets with MAC3 aren't actually received by the PMD.

So this patch adds a remove operation in set default MAC API documents
this behavior.

Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v5:
 - merge the second patch into the first patch.
 - add error log when rollback failed.

v4:
  - fix broken in the patchwork

v3:
  - first explicitly remove the non-default MAC, then set default one.
  - document default and non-default MAC address

v2:
  - fixed commit log.

---
 lib/ethdev/ethdev_driver.h |  7 ++++++-
 lib/ethdev/rte_ethdev.c    | 41 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)
  

Kindly ping.

在 2022/10/20 17:31, Huisong Li 写道:
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by .mac_addr_set(). However,
> if the new default one has been added as a non-default MAC address by
> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two indexes in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>
> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.
>
> So this patch adds a remove operation in set default MAC API documents
> this behavior.
>
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v5:
>   - merge the second patch into the first patch.
>   - add error log when rollback failed.
>
> v4:
>    - fix broken in the patchwork
>
> v3:
>    - first explicitly remove the non-default MAC, then set default one.
>    - document default and non-default MAC address
>
> v2:
>    - fixed commit log.
>
> ---
>   lib/ethdev/ethdev_driver.h |  7 ++++++-
>   lib/ethdev/rte_ethdev.c    | 41 ++++++++++++++++++++++++++++++++++++--
>   2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 1300acc95d..6291be783c 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>   
>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>   
> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link address. The index zero of the array is as the
> +	 * index of the default address, and other indexes can't be the same
> +	 * as the address corresponding to index 0.
> +	 * @see rte_eth_dev_release_port()
> +	 */
>   	struct rte_ether_addr *mac_addrs;
>   	/** Bitmap associating MAC addresses to pools */
>   	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..be4b37c9c1 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>   int
>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   {
> +	uint64_t mac_pool_sel_bk = 0;
>   	struct rte_eth_dev *dev;
> +	uint32_t pool;
> +	int index;
>   	int ret;
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   	if (*dev->dev_ops->mac_addr_set == NULL)
>   		return -ENOTSUP;
>   
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* Remove address in dev data structure */
> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> +		if (ret < 0) {
> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> +				       port_id);
> +			return ret;
> +		}
> +		/* Reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/* Update default address in NIC data structure */
>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>   
>   	return 0;
> -}
>   
> +out:
> +	if (index > 0) {
> +		pool = 0;
> +		do {
> +			if (mac_pool_sel_bk & UINT64_C(1)) {
> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> +							     pool) != 0)
> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> +						       pool, port_id);
> +			}
> +			mac_pool_sel_bk >>= 1;
> +			pool++;
> +		} while (mac_pool_sel_bk != 0);
> +	}
> +
> +	return ret;
> +}
>   
>   /*
>    * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
  

Hi Andrew and PMD maintainers,

Can you have a look?

在 2022/10/20 17:31, Huisong Li 写道:
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by .mac_addr_set(). However,
> if the new default one has been added as a non-default MAC address by
> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two indexes in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>
> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.
>
> So this patch adds a remove operation in set default MAC API documents
> this behavior.
>
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v5:
>   - merge the second patch into the first patch.
>   - add error log when rollback failed.
>
> v4:
>    - fix broken in the patchwork
>
> v3:
>    - first explicitly remove the non-default MAC, then set default one.
>    - document default and non-default MAC address
>
> v2:
>    - fixed commit log.
>
> ---
>   lib/ethdev/ethdev_driver.h |  7 ++++++-
>   lib/ethdev/rte_ethdev.c    | 41 ++++++++++++++++++++++++++++++++++++--
>   2 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 1300acc95d..6291be783c 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>   
>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>   
> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link address. The index zero of the array is as the
> +	 * index of the default address, and other indexes can't be the same
> +	 * as the address corresponding to index 0.
> +	 * @see rte_eth_dev_release_port()
> +	 */
>   	struct rte_ether_addr *mac_addrs;
>   	/** Bitmap associating MAC addresses to pools */
>   	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..be4b37c9c1 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>   int
>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   {
> +	uint64_t mac_pool_sel_bk = 0;
>   	struct rte_eth_dev *dev;
> +	uint32_t pool;
> +	int index;
>   	int ret;
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   	if (*dev->dev_ops->mac_addr_set == NULL)
>   		return -ENOTSUP;
>   
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* Remove address in dev data structure */
> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> +		if (ret < 0) {
> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> +				       port_id);
> +			return ret;
> +		}
> +		/* Reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>   	if (ret < 0)
> -		return ret;
> +		goto out;
>   
>   	/* Update default address in NIC data structure */
>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>   
>   	return 0;
> -}
>   
> +out:
> +	if (index > 0) {
> +		pool = 0;
> +		do {
> +			if (mac_pool_sel_bk & UINT64_C(1)) {
> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> +							     pool) != 0)
> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> +						       pool, port_id);
> +			}
> +			mac_pool_sel_bk >>= 1;
> +			pool++;
> +		} while (mac_pool_sel_bk != 0);
> +	}
> +
> +	return ret;
> +}
>   
>   /*
>    * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
  

LGTM
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2022/10/20 17:31, Huisong Li wrote:
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by .mac_addr_set(). However,
> if the new default one has been added as a non-default MAC address by
> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two indexes in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
> 
> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.
> 
> So this patch adds a remove operation in set default MAC API documents
> this behavior.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v5:
>  - merge the second patch into the first patch.
>  - add error log when rollback failed.
> 
> v4:
>   - fix broken in the patchwork
> 
> v3:
>   - first explicitly remove the non-default MAC, then set default one.
>   - document default and non-default MAC address
> 
> v2:
>   - fixed commit log.
> 
> ---
>  lib/ethdev/ethdev_driver.h |  7 ++++++-
>  lib/ethdev/rte_ethdev.c    | 41 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 1300acc95d..6291be783c 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>  
>  	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>  
> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link address. The index zero of the array is as the
> +	 * index of the default address, and other indexes can't be the same
> +	 * as the address corresponding to index 0.
> +	 * @see rte_eth_dev_release_port()
> +	 */
>  	struct rte_ether_addr *mac_addrs;
>  	/** Bitmap associating MAC addresses to pools */
>  	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 5d5e18db1e..be4b37c9c1 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>  int
>  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  {
> +	uint64_t mac_pool_sel_bk = 0;
>  	struct rte_eth_dev *dev;
> +	uint32_t pool;
> +	int index;
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  	if (*dev->dev_ops->mac_addr_set == NULL)
>  		return -ENOTSUP;
>  
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* Remove address in dev data structure */
> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> +		if (ret < 0) {
> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> +				       port_id);
> +			return ret;
> +		}
> +		/* Reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
>  	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
>  	/* Update default address in NIC data structure */
>  	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>  
>  	return 0;
> -}
>  
> +out:
> +	if (index > 0) {
> +		pool = 0;
> +		do {
> +			if (mac_pool_sel_bk & UINT64_C(1)) {
> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> +							     pool) != 0)
> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> +						       pool, port_id);
> +			}
> +			mac_pool_sel_bk >>= 1;
> +			pool++;
> +		} while (mac_pool_sel_bk != 0);
> +	}
> +
> +	return ret;
> +}
>  
>  /*
>   * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
>
  

20/10/2022 11:31, Huisong Li:
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by .mac_addr_set(). However,
> if the new default one has been added as a non-default MAC address by
> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two indexes in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4

I agree it would be simpler to ensure that the addresses are uniques.

> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.

If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
MAC3 packets should be received.
The MAC address should not be removed by the PMD.

> So this patch adds a remove operation in set default MAC API documents
> this behavior.

Let's be clear here: only the new default address is removed from the rest of the list.

> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org

> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>  
>  	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>  
> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link address. The index zero of the array is as the

It should be "addresses" as there can be multiple.

What means "as" above?
Can we say the first entry (index zero) is the default address?

> +	 * index of the default address, and other indexes can't be the same

You can split the sentence in 2 instead of ", and".
indexes -> entries
can't -> cannot

> +	 * as the address corresponding to index 0.

simpler: as the default address.

> +	 * @see rte_eth_dev_release_port()

Why referencing this function here?

> +	 */

> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>  int
>  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  {
> +	uint64_t mac_pool_sel_bk = 0;
>  	struct rte_eth_dev *dev;
> +	uint32_t pool;
> +	int index;
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  	if (*dev->dev_ops->mac_addr_set == NULL)
>  		return -ENOTSUP;
>  
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */

Make is simpler:
"Keep address unique in dev->data->mac_addrs[]."

> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* Remove address in dev data structure */
> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> +		if (ret < 0) {
> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> +				       port_id);

It is not clear with this log that it failed.

> +			return ret;
> +		}
> +		/* Reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;

mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().

> +	}
>  	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>  	if (ret < 0)
> -		return ret;
> +		goto out;
>  
>  	/* Update default address in NIC data structure */
>  	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>  
>  	return 0;
> -}
>  
> +out:
> +	if (index > 0) {
> +		pool = 0;
> +		do {
> +			if (mac_pool_sel_bk & UINT64_C(1)) {
> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> +							     pool) != 0)
> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> +						       pool, port_id);
> +			}
> +			mac_pool_sel_bk >>= 1;
> +			pool++;
> +		} while (mac_pool_sel_bk != 0);
> +	}
> +
> +	return ret;
> +}

Can we avoid this rollback by removing the address after mac_addr_set() succeed?
  

18/01/2023 09:26, Thomas Monjalon:
> 20/10/2022 11:31, Huisong Li:
> > The dev->data->mac_addrs[0] will be changed to a new MAC address when
> > applications modify the default MAC address by .mac_addr_set(). However,
> > if the new default one has been added as a non-default MAC address by
> > .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> > list. As a result, one MAC address occupies two indexes in the list. Like:
> > add(MAC1)
> > add(MAC2)
> > add(MAC3)
> > add(MAC4)
> > set_default(MAC3)
> > default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
> 
> I agree it may be simpler to ensure that the addresses are uniques.
> 
> > In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> > old default MAC when set default MAC. If user continues to do
> > set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> > MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> > but packets with MAC3 aren't actually received by the PMD.
> 
> If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
> MAC3 packets should be received.
> The MAC address should not be removed by the PMD.

Sorry for the confusion. I mean the opposite.
With the new behavior, if we want the old default address to be kept
in the list, we need to add it explicitly with rte_eth_dev_mac_addr_add.
This is a behavior change and must be highlighted in the release notes.

> > So this patch adds a remove operation in set default MAC API documents
> > this behavior.
> 
> Let's be clear here: only the new default address is removed from the rest of the list.
> 
> > Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> > Cc: stable@dpdk.org
> 
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
> >  
> >  	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
> >  
> > -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> > +	/**
> > +	 * Device Ethernet link address. The index zero of the array is as the
> 
> It should be "addresses" as there can be multiple.
> 
> What means "as" above?
> Can we say the first entry (index zero) is the default address?
> 
> > +	 * index of the default address, and other indexes can't be the same
> 
> You can split the sentence in 2 instead of ", and".
> indexes -> entries
> can't -> cannot
> 
> > +	 * as the address corresponding to index 0.
> 
> simpler: as the default address.
> 
> > +	 * @see rte_eth_dev_release_port()
> 
> Why referencing this function here?
> 
> > +	 */
> 
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
> >  int
> >  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
> >  {
> > +	uint64_t mac_pool_sel_bk = 0;
> >  	struct rte_eth_dev *dev;
> > +	uint32_t pool;
> > +	int index;
> >  	int ret;
> >  
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
> >  	if (*dev->dev_ops->mac_addr_set == NULL)
> >  		return -ENOTSUP;
> >  
> > +	/*
> > +	 * If the address has been added as a non-default MAC address by
> > +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> > +	 * dev->data->mac_addrs[].
> > +	 */
> 
> Make is simpler:
> "Keep address unique in dev->data->mac_addrs[]."
> 
> > +	index = eth_dev_get_mac_addr_index(port_id, addr);
> > +	if (index > 0) {
> > +		/* Remove address in dev data structure */
> > +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> > +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> > +		if (ret < 0) {
> > +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> > +				       port_id);
> 
> It is not clear with this log that it failed.
> 
> > +			return ret;
> > +		}
> > +		/* Reset pool bitmap */
> > +		dev->data->mac_pool_sel[index] = 0;
> 
> mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().
> 
> > +	}
> >  	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> >  	if (ret < 0)
> > -		return ret;
> > +		goto out;
> >  
> >  	/* Update default address in NIC data structure */
> >  	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
> >  
> >  	return 0;
> > -}
> >  
> > +out:
> > +	if (index > 0) {
> > +		pool = 0;
> > +		do {
> > +			if (mac_pool_sel_bk & UINT64_C(1)) {
> > +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> > +							     pool) != 0)
> > +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> > +						       pool, port_id);
> > +			}
> > +			mac_pool_sel_bk >>= 1;
> > +			pool++;
> > +		} while (mac_pool_sel_bk != 0);
> > +	}
> > +
> > +	return ret;
> > +}
> 
> Can we avoid this rollback by removing the address after mac_addr_set() succeed?
  

在 2023/1/18 16:26, Thomas Monjalon 写道:
> 20/10/2022 11:31, Huisong Li:
>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>> applications modify the default MAC address by .mac_addr_set(). However,
>> if the new default one has been added as a non-default MAC address by
>> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
>> list. As a result, one MAC address occupies two indexes in the list. Like:
>> add(MAC1)
>> add(MAC2)
>> add(MAC3)
>> add(MAC4)
>> set_default(MAC3)
>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
> I agree it would be simpler to ensure that the addresses are uniques.
>
>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>> old default MAC when set default MAC. If user continues to do
>> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
>> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
>> but packets with MAC3 aren't actually received by the PMD.
> If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
> MAC3 packets should be received.
> The MAC address should not be removed by the PMD.
>
>> So this patch adds a remove operation in set default MAC API documents
>> this behavior.
> Let's be clear here: only the new default address is removed from the rest of the list.
>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>>   
>>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>>   
>> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
>> +	/**
>> +	 * Device Ethernet link address. The index zero of the array is as the
> It should be "addresses" as there can be multiple.
>
> What means "as" above?
> Can we say the first entry (index zero) is the default address?
>
>> +	 * index of the default address, and other indexes can't be the same
> You can split the sentence in 2 instead of ", and".
> indexes -> entries
> can't -> cannot
>
>> +	 * as the address corresponding to index 0.
> simpler: as the default address.
>
>> +	 * @see rte_eth_dev_release_port()
> Why referencing this function here?
>
>> +	 */
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>>   int
>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   {
>> +	uint64_t mac_pool_sel_bk = 0;
>>   	struct rte_eth_dev *dev;
>> +	uint32_t pool;
>> +	int index;
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   	if (*dev->dev_ops->mac_addr_set == NULL)
>>   		return -ENOTSUP;
>>   
>> +	/*
>> +	 * If the address has been added as a non-default MAC address by
>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>> +	 * dev->data->mac_addrs[].
>> +	 */
> Make is simpler:
> "Keep address unique in dev->data->mac_addrs[]."
Ack
>
>> +	index = eth_dev_get_mac_addr_index(port_id, addr);
>> +	if (index > 0) {
>> +		/* Remove address in dev data structure */
>> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>> +		if (ret < 0) {
>> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
>> +				       port_id);
> It is not clear with this log that it failed.
Adjust as follows?
"Delete old address entry from the MAC list of ethdev port %u.\n"
>
>> +			return ret;
>> +		}
>> +		/* Reset pool bitmap */
>> +		dev->data->mac_pool_sel[index] = 0;
> mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().
Yes, this can be deleted.
>
>> +	}
>>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>   	if (ret < 0)
>> -		return ret;
>> +		goto out;
>>   
>>   	/* Update default address in NIC data structure */
>>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>   
>>   	return 0;
>> -}
>>   
>> +out:
>> +	if (index > 0) {
>> +		pool = 0;
>> +		do {
>> +			if (mac_pool_sel_bk & UINT64_C(1)) {
>> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
>> +							     pool) != 0)
>> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
>> +						       pool, port_id);
>> +			}
>> +			mac_pool_sel_bk >>= 1;
>> +			pool++;
>> +		} while (mac_pool_sel_bk != 0);
>> +	}
>> +
>> +	return ret;
>> +}
> Can we avoid this rollback by removing the address after mac_addr_set() succeed?
No, the new default address will be removed if we do that.
we need to remove it first if the new default address has been added as a
non-default MAC address to the MAC list.
>
>
>
> .
  

在 2023/1/18 16:38, Thomas Monjalon 写道:
> 18/01/2023 09:26, Thomas Monjalon:
>> 20/10/2022 11:31, Huisong Li:
>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>> applications modify the default MAC address by .mac_addr_set(). However,
>>> if the new default one has been added as a non-default MAC address by
>>> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
>>> list. As a result, one MAC address occupies two indexes in the list. Like:
>>> add(MAC1)
>>> add(MAC2)
>>> add(MAC3)
>>> add(MAC4)
>>> set_default(MAC3)
>>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>> I agree it may be simpler to ensure that the addresses are uniques.
>>
>>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>>> old default MAC when set default MAC. If user continues to do
>>> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
>>> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
>>> but packets with MAC3 aren't actually received by the PMD.
>> If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
>> MAC3 packets should be received.
>> The MAC address should not be removed by the PMD.
> Sorry for the confusion. I mean the opposite.
> With the new behavior, if we want the old default address to be kept
> in the list, we need to add it explicitly with rte_eth_dev_mac_addr_add.
> This is a behavior change and must be highlighted in the release notes.
Hi Thomas,

Sorry for your confusion. Maybe my cmmmit log is misleading.
Please ignore the second paragraph description in my commit.

It has nothing to do with the old defalut address.

The purpose of this patch is to solve the problem that one
address occupies two entries in MAC list.
All network engines have this problem. If the new defalut address
has been added as a non-default MAC address to the MAC list, this
address will occupy two entries in MAC list.

It's still a problem for some network engines, like, hns3 and i40e.
That's what I described in the second paragraph in my commit.

/Huisong
>>> So this patch adds a remove operation in set default MAC API documents
>>> this behavior.
>> Let's be clear here: only the new default address is removed from the rest of the list.
>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>>>   
>>>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>>>   
>>> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
>>> +	/**
>>> +	 * Device Ethernet link address. The index zero of the array is as the
>> It should be "addresses" as there can be multiple.
>>
>> What means "as" above?
>> Can we say the first entry (index zero) is the default address?
>>
>>> +	 * index of the default address, and other indexes can't be the same
>> You can split the sentence in 2 instead of ", and".
>> indexes -> entries
>> can't -> cannot
>>
>>> +	 * as the address corresponding to index 0.
>> simpler: as the default address.
>>
>>> +	 * @see rte_eth_dev_release_port()
>> Why referencing this function here?
>>
>>> +	 */
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>>>   int
>>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>   {
>>> +	uint64_t mac_pool_sel_bk = 0;
>>>   	struct rte_eth_dev *dev;
>>> +	uint32_t pool;
>>> +	int index;
>>>   	int ret;
>>>   
>>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>   	if (*dev->dev_ops->mac_addr_set == NULL)
>>>   		return -ENOTSUP;
>>>   
>>> +	/*
>>> +	 * If the address has been added as a non-default MAC address by
>>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>>> +	 * dev->data->mac_addrs[].
>>> +	 */
>> Make is simpler:
>> "Keep address unique in dev->data->mac_addrs[]."
>>
>>> +	index = eth_dev_get_mac_addr_index(port_id, addr);
>>> +	if (index > 0) {
>>> +		/* Remove address in dev data structure */
>>> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>>> +		if (ret < 0) {
>>> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
>>> +				       port_id);
>> It is not clear with this log that it failed.
>>
>>> +			return ret;
>>> +		}
>>> +		/* Reset pool bitmap */
>>> +		dev->data->mac_pool_sel[index] = 0;
>> mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().
>>
>>> +	}
>>>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>   	if (ret < 0)
>>> -		return ret;
>>> +		goto out;
>>>   
>>>   	/* Update default address in NIC data structure */
>>>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>   
>>>   	return 0;
>>> -}
>>>   
>>> +out:
>>> +	if (index > 0) {
>>> +		pool = 0;
>>> +		do {
>>> +			if (mac_pool_sel_bk & UINT64_C(1)) {
>>> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
>>> +							     pool) != 0)
>>> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
>>> +						       pool, port_id);
>>> +			}
>>> +			mac_pool_sel_bk >>= 1;
>>> +			pool++;
>>> +		} while (mac_pool_sel_bk != 0);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>> Can we avoid this rollback by removing the address after mac_addr_set() succeed?
>
>
> .
  

Hi,

You missed some questions and comments below.

19/01/2023 10:57, lihuisong (C):
> 在 2023/1/18 16:26, Thomas Monjalon 写道:
> > 20/10/2022 11:31, Huisong Li:
> >> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> >> applications modify the default MAC address by .mac_addr_set(). However,
> >> if the new default one has been added as a non-default MAC address by
> >> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> >> list. As a result, one MAC address occupies two indexes in the list. Like:
> >> add(MAC1)
> >> add(MAC2)
> >> add(MAC3)
> >> add(MAC4)
> >> set_default(MAC3)
> >> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
> > I agree it would be simpler to ensure that the addresses are uniques.
> >
> >> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> >> old default MAC when set default MAC. If user continues to do
> >> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> >> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> >> but packets with MAC3 aren't actually received by the PMD.
> > If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
> > MAC3 packets should be received.
> > The MAC address should not be removed by the PMD.
> >
> >> So this patch adds a remove operation in set default MAC API documents
> >> this behavior.
> > Let's be clear here: only the new default address is removed from the rest of the list.
> >
> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> >> Cc: stable@dpdk.org
> >> --- a/lib/ethdev/ethdev_driver.h
> >> +++ b/lib/ethdev/ethdev_driver.h
> >> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
> >>   
> >>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
> >>   
> >> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> >> +	/**
> >> +	 * Device Ethernet link address. The index zero of the array is as the
> > It should be "addresses" as there can be multiple.
> >
> > What means "as" above?
> > Can we say the first entry (index zero) is the default address?
> >
> >> +	 * index of the default address, and other indexes can't be the same
> > You can split the sentence in 2 instead of ", and".
> > indexes -> entries
> > can't -> cannot
> >
> >> +	 * as the address corresponding to index 0.
> > simpler: as the default address.
> >
> >> +	 * @see rte_eth_dev_release_port()
> > Why referencing this function here?
> >
> >> +	 */
> >> --- a/lib/ethdev/rte_ethdev.c
> >> +++ b/lib/ethdev/rte_ethdev.c
> >> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
> >>   int
> >>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
> >>   {
> >> +	uint64_t mac_pool_sel_bk = 0;
> >>   	struct rte_eth_dev *dev;
> >> +	uint32_t pool;
> >> +	int index;
> >>   	int ret;
> >>   
> >>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
> >>   	if (*dev->dev_ops->mac_addr_set == NULL)
> >>   		return -ENOTSUP;
> >>   
> >> +	/*
> >> +	 * If the address has been added as a non-default MAC address by
> >> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> >> +	 * dev->data->mac_addrs[].
> >> +	 */
> > Make is simpler:
> > "Keep address unique in dev->data->mac_addrs[]."
> Ack
> >
> >> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> >> +	if (index > 0) {
> >> +		/* Remove address in dev data structure */
> >> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> >> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> >> +		if (ret < 0) {
> >> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
> >> +				       port_id);
> > It is not clear with this log that it failed.
> Adjust as follows?
> "Delete old address entry from the MAC list of ethdev port %u.\n"
> >
> >> +			return ret;
> >> +		}
> >> +		/* Reset pool bitmap */
> >> +		dev->data->mac_pool_sel[index] = 0;
> > mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().
> Yes, this can be deleted.
> >
> >> +	}
> >>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> >>   	if (ret < 0)
> >> -		return ret;
> >> +		goto out;
> >>   
> >>   	/* Update default address in NIC data structure */
> >>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
> >>   
> >>   	return 0;
> >> -}
> >>   
> >> +out:
> >> +	if (index > 0) {
> >> +		pool = 0;
> >> +		do {
> >> +			if (mac_pool_sel_bk & UINT64_C(1)) {
> >> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
> >> +							     pool) != 0)
> >> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
> >> +						       pool, port_id);
> >> +			}
> >> +			mac_pool_sel_bk >>= 1;
> >> +			pool++;
> >> +		} while (mac_pool_sel_bk != 0);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> > Can we avoid this rollback by removing the address after mac_addr_set() succeed?
> No, the new default address will be removed if we do that.
> we need to remove it first if the new default address has been added as a
> non-default MAC address to the MAC list.

OK yes.
  

在 2023/1/19 22:38, Thomas Monjalon 写道:
> Hi,
>
> You missed some questions and comments below.
Sorry for my later reply. I just got back from vacation.
Please take a look again.
>
> 19/01/2023 10:57, lihuisong (C):
>> 在 2023/1/18 16:26, Thomas Monjalon 写道:
>>> 20/10/2022 11:31, Huisong Li:
>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>> applications modify the default MAC address by .mac_addr_set(). However,
>>>> if the new default one has been added as a non-default MAC address by
>>>> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
>>>> list. As a result, one MAC address occupies two indexes in the list. Like:
>>>> add(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> set_default(MAC3)
>>>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>>> I agree it would be simpler to ensure that the addresses are uniques.
>>>
>>>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>>>> old default MAC when set default MAC. If user continues to do
>>>> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
>>>> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
>>>> but packets with MAC3 aren't actually received by the PMD.
>>> If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app,
>>> MAC3 packets should be received.
>>> The MAC address should not be removed by the PMD.
Please review another thread you pulled.
>>>
>>>> So this patch adds a remove operation in set default MAC API documents
>>>> this behavior.
>>> Let's be clear here: only the new default address is removed from the rest of the list.
ok. Your expression is more explicit.
>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -116,7 +116,12 @@ struct rte_eth_dev_data {
>>>>    
>>>>    	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>>>>    
>>>> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
>>>> +	/**
>>>> +	 * Device Ethernet link address. The index zero of the array is as the
>>> It should be "addresses" as there can be multiple.
>>>
>>> What means "as" above?
>>> Can we say the first entry (index zero) is the default address?
>>>
>>>> +	 * index of the default address, and other indexes can't be the same
>>> You can split the sentence in 2 instead of ", and".
>>> indexes -> entries
>>> can't -> cannot
>>>
>>>> +	 * as the address corresponding to index 0.
>>> simpler: as the default address.
Above all will be fixed. Thank you for your suggestion.
>>>
>>>> +	 * @see rte_eth_dev_release_port()
>>> Why referencing this function here?
It was here before, and I didn't care too much, but I think it is redundant.
I will remove it.
>>>
>>>> +	 */
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr)
>>>>    int
>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>    {
>>>> +	uint64_t mac_pool_sel_bk = 0;
>>>>    	struct rte_eth_dev *dev;
>>>> +	uint32_t pool;
>>>> +	int index;
>>>>    	int ret;
>>>>    
>>>>    	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>    	if (*dev->dev_ops->mac_addr_set == NULL)
>>>>    		return -ENOTSUP;
>>>>    
>>>> +	/*
>>>> +	 * If the address has been added as a non-default MAC address by
>>>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>>>> +	 * dev->data->mac_addrs[].
>>>> +	 */
>>> Make is simpler:
>>> "Keep address unique in dev->data->mac_addrs[]."
>> Ack
>>>> +	index = eth_dev_get_mac_addr_index(port_id, addr);
>>>> +	if (index > 0) {
>>>> +		/* Remove address in dev data structure */
>>>> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>>> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>>>> +		if (ret < 0) {
>>>> +			RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n",
>>>> +				       port_id);
>>> It is not clear with this log that it failed.
>> Adjust as follows?
>> "Delete old address entry from the MAC list of ethdev port %u.\n"
>>>> +			return ret;
>>>> +		}
>>>> +		/* Reset pool bitmap */
>>>> +		dev->data->mac_pool_sel[index] = 0;
>>> mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove().
>> Yes, this can be deleted.
>>>> +	}
>>>>    	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>>    	if (ret < 0)
>>>> -		return ret;
>>>> +		goto out;
>>>>    
>>>>    	/* Update default address in NIC data structure */
>>>>    	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>    
>>>>    	return 0;
>>>> -}
>>>>    
>>>> +out:
>>>> +	if (index > 0) {
>>>> +		pool = 0;
>>>> +		do {
>>>> +			if (mac_pool_sel_bk & UINT64_C(1)) {
>>>> +				if (rte_eth_dev_mac_addr_add(port_id, addr,
>>>> +							     pool) != 0)
>>>> +					RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n",
>>>> +						       pool, port_id);
>>>> +			}
>>>> +			mac_pool_sel_bk >>= 1;
>>>> +			pool++;
>>>> +		} while (mac_pool_sel_bk != 0);
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>> Can we avoid this rollback by removing the address after mac_addr_set() succeed?
>> No, the new default address will be removed if we do that.
>> we need to remove it first if the new default address has been added as a
>> non-default MAC address to the MAC list.
> OK yes.
>
>
> .