diff mbox series

[V8] ethdev: fix one address occupies two entries in MAC addrs

Message ID 20230202123625.14975-1-lihuisong@huawei.com (mailing list archive)
State Not Applicable, archived
Delegated to: Ferruh Yigit
Headers
Series [V8] ethdev: fix one address occupies two entries in MAC addrs |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS

Commit Message

lihuisong (C) Feb. 2, 2023, 12:36 p.m. UTC 
  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 entries in the list. Like:
add(MAC1)
add(MAC2)
add(MAC3)
add(MAC4)
set_default(MAC3)
default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
Note: MAC3 occupies two entries.

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 need to ensure that the new default address is removed from the rest of
the list if the address was already in the list.

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

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
v8: fix some comments.
v7: add announcement in the release notes and document this behavior.
v6: fix commit log and some code comments.
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.
---
 doc/guides/rel_notes/release_23_03.rst |  6 +++++
 lib/ethdev/ethdev_driver.h             |  6 ++++-
 lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
 lib/ethdev/rte_ethdev.h                |  3 +++
 4 files changed, 47 insertions(+), 3 deletions(-)

Comments

Thomas Monjalon Feb. 2, 2023, 1:11 p.m. UTC | #1 
02/02/2023 13:36, 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 entries in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> Note: MAC3 occupies two entries.
> 
> 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 need to ensure that the new default address is removed from the rest of
> the list if the address was already in the list.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Thank you
Ferruh Yigit Feb. 2, 2023, 6:09 p.m. UTC | #2 
On 2/2/2023 12:36 PM, 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 entries in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> Note: MAC3 occupies two entries.
> 
> 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 need to ensure that the new default address is removed from the rest of
> the list if the address was already in the list.
> 

Same comment from past seems already valid, I am not looking to the set
for a while, sorry if this is already discussed and decided,
if not, I am referring to the side effect that setting MAC addresses
cause to remove MAC addresses, think following case:

add(MAC1) -> MAC1
add(MAC2) -> MAC1, MAC2
add(MAC3) -> MAC1, MAC2, MAC3
add(MAC4) -> MAC1, MAC2, MAC3, MAC4
set(MAC3) -> MAC3, MAC2, MAC4
set(MAC4) -> MAC4, MAC2
set(MAC2) -> MAC2

I am not exactly clear what is the intention with set(), if there is
single MAC I guess intention is to replace it with new one, but if there
are multiple MACs and one of them are already in the list intention may
be just to change the default MAC.

If above assumption is correct, what about following:

set(MAC) {
    if only_default_mac_exist
        replace_default_mac

    if MAC exists in list
	swap MAC and list[0]
    else
	replace_default_mac
}

This swap prevents removing MAC side affect, does it make sense?


> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
> v8: fix some comments.
> v7: add announcement in the release notes and document this behavior.
> v6: fix commit log and some code comments.
> 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.
> ---
>  doc/guides/rel_notes/release_23_03.rst |  6 +++++
>  lib/ethdev/ethdev_driver.h             |  6 ++++-
>  lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
>  lib/ethdev/rte_ethdev.h                |  3 +++
>  4 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index 84b112a8b1..1c9b9912c2 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -105,6 +105,12 @@ API Changes
>     Also, make sure to start the actual text at the margin.
>     =======================================================
>  
> +* ethdev: ensured all entries in MAC address list are uniques.
> +  When setting a default MAC address with the function
> +  ``rte_eth_dev_default_mac_addr_set``,
> +  the address is now removed from the rest of the address list
> +  in order to ensure it is only at index 0 of the list.
> +
>  
>  ABI Changes
>  -----------
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index dde3ec84ef..3994c61b86 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -117,7 +117,11 @@ 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 addresses.
> +	 * All entries are unique.
> +	 * The first entry (index zero) is the default address.
> +	 */
>  	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 86ca303ab5..de25183619 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,44 @@ 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;
>  
> +	/* 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, "Cannot remove the port %u address from the rest of list.\n",
> +				       port_id);
> +			return ret;
> +		}
> +	}
>  	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
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d22de196db..2456153457 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>  
>  /**
>   * Set the default MAC address.
> + * It replaces the address at index 0 of the MAC address list.
> + * If the address was already in the MAC address list,
> + * it is removed from the rest of the list.
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
Thomas Monjalon Feb. 2, 2023, 9:10 p.m. UTC | #3 
02/02/2023 19:09, Ferruh Yigit:
> On 2/2/2023 12:36 PM, 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 entries in the list. Like:
> > add(MAC1)
> > add(MAC2)
> > add(MAC3)
> > add(MAC4)
> > set_default(MAC3)
> > default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> > Note: MAC3 occupies two entries.
> > 
> > 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 need to ensure that the new default address is removed from the rest of
> > the list if the address was already in the list.
> > 
> 
> Same comment from past seems already valid, I am not looking to the set
> for a while, sorry if this is already discussed and decided,
> if not, I am referring to the side effect that setting MAC addresses
> cause to remove MAC addresses, think following case:
> 
> add(MAC1) -> MAC1
> add(MAC2) -> MAC1, MAC2
> add(MAC3) -> MAC1, MAC2, MAC3
> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
> set(MAC3) -> MAC3, MAC2, MAC4
> set(MAC4) -> MAC4, MAC2
> set(MAC2) -> MAC2
> 
> I am not exactly clear what is the intention with set(),

That's the problem, nobody is clear with the current behavior.
The doc says "Set the default MAC address." and nothing else.

> if there is
> single MAC I guess intention is to replace it with new one, but if there
> are multiple MACs and one of them are already in the list intention may
> be just to change the default MAC.

The assumption in this patch is that "Set" means "Replace", not "Swap".
So this patch takes the approach 1/ Replace and keep Unique.

> If above assumption is correct, what about following:
> 
> set(MAC) {
>     if only_default_mac_exist
>         replace_default_mac
> 
>     if MAC exists in list
> 	swap MAC and list[0]
>     else
> 	replace_default_mac
> }

This approach 2/ is a mix of Swap and Replace.
The old default MAC destiny depends on whether
we have added the new MAC as "secondary" before setting as new default.

> This swap prevents removing MAC side affect, does it make sense?

Another approach would be 3/ to do an "Always Swap"
even if the new MAC didn't exist before,
you keep the old default MAC as a secondary MAC.

And the current approach 0/ is to Replace default MAC address
without touching the secondary addresses at all.

So we have 4 choices.
We could vote, roll a dice, or find a strong argument?
Morten Brørup Feb. 2, 2023, 9:50 p.m. UTC | #4 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 2 February 2023 22.11
> 
> 02/02/2023 19:09, Ferruh Yigit:
> > On 2/2/2023 12:36 PM, 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 entries in the
> list. Like:
> > > add(MAC1)
> > > add(MAC2)
> > > add(MAC3)
> > > add(MAC4)
> > > set_default(MAC3)
> > > default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> > > Note: MAC3 occupies two entries.
> > >
> > > 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 need to ensure that the new default address is removed from the
> rest of
> > > the list if the address was already in the list.
> > >
> >
> > Same comment from past seems already valid, I am not looking to the
> set
> > for a while, sorry if this is already discussed and decided,
> > if not, I am referring to the side effect that setting MAC addresses
> > cause to remove MAC addresses, think following case:
> >
> > add(MAC1) -> MAC1
> > add(MAC2) -> MAC1, MAC2
> > add(MAC3) -> MAC1, MAC2, MAC3
> > add(MAC4) -> MAC1, MAC2, MAC3, MAC4
> > set(MAC3) -> MAC3, MAC2, MAC4
> > set(MAC4) -> MAC4, MAC2
> > set(MAC2) -> MAC2
> >
> > I am not exactly clear what is the intention with set(),
> 
> That's the problem, nobody is clear with the current behavior.
> The doc says "Set the default MAC address." and nothing else.
> 
> > if there is
> > single MAC I guess intention is to replace it with new one, but if
> there
> > are multiple MACs and one of them are already in the list intention
> may
> > be just to change the default MAC.
> 
> The assumption in this patch is that "Set" means "Replace", not "Swap".
> So this patch takes the approach 1/ Replace and keep Unique.
> 
> > If above assumption is correct, what about following:
> >
> > set(MAC) {
> >     if only_default_mac_exist
> >         replace_default_mac
> >
> >     if MAC exists in list
> > 	swap MAC and list[0]
> >     else
> > 	replace_default_mac
> > }
> 
> This approach 2/ is a mix of Swap and Replace.
> The old default MAC destiny depends on whether
> we have added the new MAC as "secondary" before setting as new default.
> 
> > This swap prevents removing MAC side affect, does it make sense?
> 
> Another approach would be 3/ to do an "Always Swap"
> even if the new MAC didn't exist before,
> you keep the old default MAC as a secondary MAC.
> 
> And the current approach 0/ is to Replace default MAC address
> without touching the secondary addresses at all.
> 
> So we have 4 choices.
> We could vote, roll a dice, or find a strong argument?

If there is only one MAC, MAC1, I would certainly expect Set(MAC2) to replace MAC1 with MAC2.

If there is a list of MACs, I might expect Set() to select one in the list. And, if calling Set() with a MAC not in the list, we could either 1. return an error, or 2. implicitly add it to the list and then set it.

If we implicitly add it to the list (i.e. follow option 2 described above), it should not be implicitly be removed from the list if another MAC is Set(). This behavior seems weird, which speaks for returning an error if setting a MAC not in the list (i.e. option 1).

Disclaimer: I have no experience configuring NICs with multiple MACs - we promiscuous mode or one MAC address. So my input regarding multiple MACs is purely theoretical.
lihuisong (C) Feb. 3, 2023, 1:56 a.m. UTC | #5 
在 2023/2/3 5:10, Thomas Monjalon 写道:
> 02/02/2023 19:09, Ferruh Yigit:
>> On 2/2/2023 12:36 PM, 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 entries in the list. Like:
>>> add(MAC1)
>>> add(MAC2)
>>> add(MAC3)
>>> add(MAC4)
>>> set_default(MAC3)
>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>> Note: MAC3 occupies two entries.
>>>
>>> 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 need to ensure that the new default address is removed from the rest of
>>> the list if the address was already in the list.
>>>
>> Same comment from past seems already valid, I am not looking to the set
>> for a while, sorry if this is already discussed and decided,
>> if not, I am referring to the side effect that setting MAC addresses
>> cause to remove MAC addresses, think following case:
>>
>> add(MAC1) -> MAC1
>> add(MAC2) -> MAC1, MAC2
>> add(MAC3) -> MAC1, MAC2, MAC3
>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>> set(MAC3) -> MAC3, MAC2, MAC4
>> set(MAC4) -> MAC4, MAC2
>> set(MAC2) -> MAC2
>>
>> I am not exactly clear what is the intention with set(),
> That's the problem, nobody is clear with the current behavior.
> The doc says "Set the default MAC address." and nothing else.
Indeed. But we can see the following information.
 From the ethdev layer, this set() API always replaces the old default 
address (index 0) without adding the old one.
 From the PMD layer, set() interface of some PMDs, such as i40e, ice, 
hns3 and so on (as far as I know),
also do remove the hardware entry of the old default address.
>
>> if there is
>> single MAC I guess intention is to replace it with new one, but if there
>> are multiple MACs and one of them are already in the list intention may
>> be just to change the default MAC.
> The assumption in this patch is that "Set" means "Replace", not "Swap".
> So this patch takes the approach 1/ Replace and keep Unique.
>
>> If above assumption is correct, what about following:
>>
>> set(MAC) {
>>      if only_default_mac_exist
>>          replace_default_mac
>>
>>      if MAC exists in list
>> 	swap MAC and list[0]
>>      else
>> 	replace_default_mac
>> }
> This approach 2/ is a mix of Swap and Replace.
> The old default MAC destiny depends on whether
> we have added the new MAC as "secondary" before setting as new default.
>
>> This swap prevents removing MAC side affect, does it make sense?
> Another approach would be 3/ to do an "Always Swap"
> even if the new MAC didn't exist before,
> you keep the old default MAC as a secondary MAC.
>
> And the current approach 0/ is to Replace default MAC address
> without touching the secondary addresses at all.
>
> So we have 4 choices.
> We could vote, roll a dice, or find a strong argument?
According to the implement of set() in ethdev and PMD layer, it always 
use "Replace", not "Swap".
If we use "Swap" now, the behavior of this API will be changed.
I'm not sure if the application can accept this change or has other effects.

BTW, it seems that the ethernet port in kernel also replaces the old 
address if we modify the one.
Use the test command: ifconfig eth0 hw ether new_mac
>
>
> .
Ferruh Yigit Feb. 3, 2023, 12:58 p.m. UTC | #6 
On 2/3/2023 1:56 AM, lihuisong (C) wrote:
> 
> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>> 02/02/2023 19:09, Ferruh Yigit:
>>> On 2/2/2023 12:36 PM, 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 entries in the list.
>>>> Like:
>>>> add(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> set_default(MAC3)
>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>> Note: MAC3 occupies two entries.
>>>>
>>>> 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 need to ensure that the new default address is removed from the
>>>> rest of
>>>> the list if the address was already in the list.
>>>>
>>> Same comment from past seems already valid, I am not looking to the set
>>> for a while, sorry if this is already discussed and decided,
>>> if not, I am referring to the side effect that setting MAC addresses
>>> cause to remove MAC addresses, think following case:
>>>
>>> add(MAC1) -> MAC1
>>> add(MAC2) -> MAC1, MAC2
>>> add(MAC3) -> MAC1, MAC2, MAC3
>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>> set(MAC3) -> MAC3, MAC2, MAC4
>>> set(MAC4) -> MAC4, MAC2
>>> set(MAC2) -> MAC2
>>>
>>> I am not exactly clear what is the intention with set(),
>> That's the problem, nobody is clear with the current behavior.
>> The doc says "Set the default MAC address." and nothing else.
> Indeed. But we can see the following information.
> From the ethdev layer, this set() API always replaces the old default
> address (index 0) without adding the old one.
> From the PMD layer, set() interface of some PMDs, such as i40e, ice,
> hns3 and so on (as far as I know),
> also do remove the hardware entry of the old default address.

If we define behavior clearly, I think we can adapt PMD implementation
according it, unless there is HW limitation.

>>
>>> if there is
>>> single MAC I guess intention is to replace it with new one, but if there
>>> are multiple MACs and one of them are already in the list intention may
>>> be just to change the default MAC.
>> The assumption in this patch is that "Set" means "Replace", not "Swap".
>> So this patch takes the approach 1/ Replace and keep Unique.
>>
>>> If above assumption is correct, what about following:
>>>
>>> set(MAC) {
>>>      if only_default_mac_exist
>>>          replace_default_mac
>>>
>>>      if MAC exists in list
>>>     swap MAC and list[0]
>>>      else
>>>     replace_default_mac
>>> }
>> This approach 2/ is a mix of Swap and Replace.
>> The old default MAC destiny depends on whether
>> we have added the new MAC as "secondary" before setting as new default.
>>
>>> This swap prevents removing MAC side affect, does it make sense?
>> Another approach would be 3/ to do an "Always Swap"
>> even if the new MAC didn't exist before,
>> you keep the old default MAC as a secondary MAC.
>>
>> And the current approach 0/ is to Replace default MAC address
>> without touching the secondary addresses at all.
>>
>> So we have 4 choices.
>> We could vote, roll a dice, or find a strong argument?
> According to the implement of set() in ethdev and PMD layer, it always
> use "Replace", not "Swap".
> If we use "Swap" now, the behavior of this API will be changed.
> I'm not sure if the application can accept this change or has other
> effects.
> 

This patch is also changing behavior, because of implied remove address,
same concern is valid with this patch.


As I checked again current implementation may have one more problem
(this from reading code, I did not test this):
add(MAC1) -> MAC1
add(MAC2) -> MAC1, MAC2
set(MAC2) -> MAC2, MAC2
del(MAC2) -> FAILS

This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
MAC, and it only tries to remove first address it finds, it can't find
and remove second 'MAC2'.
I wasn't too much bothered with wasting one MAC address slot, so wasn't
sure if a change is required at all, but if above analysis is correct I
think this is more serious problem to justify the change.


I don't think always swap (option /3) is good idea, specially for single
MAC address exists case, and current case has (option 0/) has mentioned
problems.

Remaining ones are mix of swap and replace (option 2/) and this patch
(option /1).

I think mix of swap and replace (option 2/ above) has some benefits:
- It always replaces default MAC
- Prevents duplication MAC address in the list
- Doesn't implicitly remove address from list


BUT, if the agreement is this patch (option 1/) I am OK with that too, I
just want to make sure that it is discussed.


> BTW, it seems that the ethernet port in kernel also replaces the old
> address if we modify the one.
> Use the test command: ifconfig eth0 hw ether new_mac

For default MAC address it is more clear that intention is to replace
it, but question is about what to do with the list of MAC addresses.
lihuisong (C) Feb. 4, 2023, 2:57 a.m. UTC | #7 
在 2023/2/3 20:58, Ferruh Yigit 写道:
> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>> 02/02/2023 19:09, Ferruh Yigit:
>>>> On 2/2/2023 12:36 PM, 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 entries in the list.
>>>>> Like:
>>>>> add(MAC1)
>>>>> add(MAC2)
>>>>> add(MAC3)
>>>>> add(MAC4)
>>>>> set_default(MAC3)
>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>> Note: MAC3 occupies two entries.
>>>>>
>>>>> 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 need to ensure that the new default address is removed from the
>>>>> rest of
>>>>> the list if the address was already in the list.
>>>>>
>>>> Same comment from past seems already valid, I am not looking to the set
>>>> for a while, sorry if this is already discussed and decided,
>>>> if not, I am referring to the side effect that setting MAC addresses
>>>> cause to remove MAC addresses, think following case:
>>>>
>>>> add(MAC1) -> MAC1
>>>> add(MAC2) -> MAC1, MAC2
>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>> set(MAC4) -> MAC4, MAC2
>>>> set(MAC2) -> MAC2
>>>>
>>>> I am not exactly clear what is the intention with set(),
>>> That's the problem, nobody is clear with the current behavior.
>>> The doc says "Set the default MAC address." and nothing else.
>> Indeed. But we can see the following information.
>>  From the ethdev layer, this set() API always replaces the old default
>> address (index 0) without adding the old one.
>>  From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>> hns3 and so on (as far as I know),
>> also do remove the hardware entry of the old default address.
> If we define behavior clearly, I think we can adapt PMD implementation
> according it, unless there is HW limitation.
Right. I think this is another point (issue 2/) to be discussed.
Namely, whether the old default address should be removed when set new 
default one.
If we want to explicitly unify the behavior of all PMDs in ethdev layer 
as described above,
there may be no problem if do the following:
1) In the ethdev layer, remove the old default address if the old one is 
exist.
2) For PMD i40e, ice and hns3, remvoe the code of deleting the old 
default address before adding the new one.
    For other PMDs, we probably don't need to do anything because they 
have supported remove_addr() API.
    (Without explicitly removing the old default address, I don't know 
if their hardware or firmware
     removes the old one when set a new address. But, we explicitly 
remove the old one in ethdev layer now,
     I'm not sure if this has an effect on these PMDs.)
>>>> if there is
>>>> single MAC I guess intention is to replace it with new one, but if there
>>>> are multiple MACs and one of them are already in the list intention may
>>>> be just to change the default MAC.
>>> The assumption in this patch is that "Set" means "Replace", not "Swap".
>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>
>>>> If above assumption is correct, what about following:
>>>>
>>>> set(MAC) {
>>>>       if only_default_mac_exist
>>>>           replace_default_mac
>>>>
>>>>       if MAC exists in list
>>>>      swap MAC and list[0]
>>>>       else
>>>>      replace_default_mac
>>>> }
>>> This approach 2/ is a mix of Swap and Replace.
>>> The old default MAC destiny depends on whether
>>> we have added the new MAC as "secondary" before setting as new default.
>>>
>>>> This swap prevents removing MAC side affect, does it make sense?
>>> Another approach would be 3/ to do an "Always Swap"
>>> even if the new MAC didn't exist before,
>>> you keep the old default MAC as a secondary MAC.
>>>
>>> And the current approach 0/ is to Replace default MAC address
>>> without touching the secondary addresses at all.
>>>
>>> So we have 4 choices.
>>> We could vote, roll a dice, or find a strong argument?
>> According to the implement of set() in ethdev and PMD layer, it always
>> use "Replace", not "Swap".
>> If we use "Swap" now, the behavior of this API will be changed.
>> I'm not sure if the application can accept this change or has other
>> effects.
>>
> This patch is also changing behavior, because of implied remove address,
> same concern is valid with this patch.
Indeed, it changes the behavior.
But this patch only resolves the problem (issue 1/) that the entries of 
the MAC address list possibly are not uniques.
Fixing it may be little impact on the application.
>
>
> As I checked again current implementation may have one more problem
> (this from reading code, I did not test this):
> add(MAC1) -> MAC1
> add(MAC2) -> MAC1, MAC2
> set(MAC2) -> MAC2, MAC2
> del(MAC2) -> FAILS
>
> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
> MAC, and it only tries to remove first address it finds, it can't find
> and remove second 'MAC2'.
> I wasn't too much bothered with wasting one MAC address slot, so wasn't
> sure if a change is required at all, but if above analysis is correct I
> think this is more serious problem to justify the change.
Your analysis is fully correct.
>
>
> I don't think always swap (option /3) is good idea, specially for single
> MAC address exists case, and current case has (option 0/) has mentioned
> problems.
+1
> Remaining ones are mix of swap and replace (option 2/) and this patch
> (option /1).
>
> I think mix of swap and replace (option 2/ above) has some benefits:
> - It always replaces default MAC
> - Prevents duplication MAC address in the list
> - Doesn't implicitly remove address from list
As far as I know, the first entry (index 0) always be the default 
address in all PMDs,
but it's not documented. (So this patch did it, that's what was 
discussed earlier).
The 'Swap' may be inappropriate. It may need to be discussed.
>
> BUT, if the agreement is this patch (option 1/) I am OK with that too, I
> just want to make sure that it is discussed.
>
>
>> BTW, it seems that the ethernet port in kernel also replaces the old
>> address if we modify the one.
>> Use the test command: ifconfig eth0 hw ether new_mac
> For default MAC address it is more clear that intention is to replace
> it, but question is about what to do with the list of MAC addresses.
Hi Ferruh and Thomas,

As mentioned above, they are actually two problems (issue /1 and issue /2).
Can we deal with them separately?
#1 For issue /1, it's really a problem. This patch is responsible for it.
#2 For issue /2, I will send a RFC to discuss as described above.
      It may require the participation of all PMD maintainers.

What do you think?

/Huisong
>
> .
lihuisong (C) Feb. 9, 2023, 8:32 a.m. UTC | #8 
在 2023/2/4 10:57, lihuisong (C) 写道:
>
> 在 2023/2/3 20:58, Ferruh Yigit 写道:
>> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>>> 02/02/2023 19:09, Ferruh Yigit:
>>>>> On 2/2/2023 12:36 PM, 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 entries in the list.
>>>>>> Like:
>>>>>> add(MAC1)
>>>>>> add(MAC2)
>>>>>> add(MAC3)
>>>>>> add(MAC4)
>>>>>> set_default(MAC3)
>>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>>> Note: MAC3 occupies two entries.
>>>>>>
>>>>>> 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 need to ensure that the new default address is removed from the
>>>>>> rest of
>>>>>> the list if the address was already in the list.
>>>>>>
>>>>> Same comment from past seems already valid, I am not looking to 
>>>>> the set
>>>>> for a while, sorry if this is already discussed and decided,
>>>>> if not, I am referring to the side effect that setting MAC addresses
>>>>> cause to remove MAC addresses, think following case:
>>>>>
>>>>> add(MAC1) -> MAC1
>>>>> add(MAC2) -> MAC1, MAC2
>>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>>> set(MAC4) -> MAC4, MAC2
>>>>> set(MAC2) -> MAC2
>>>>>
>>>>> I am not exactly clear what is the intention with set(),
>>>> That's the problem, nobody is clear with the current behavior.
>>>> The doc says "Set the default MAC address." and nothing else.
>>> Indeed. But we can see the following information.
>>>  From the ethdev layer, this set() API always replaces the old default
>>> address (index 0) without adding the old one.
>>>  From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>>> hns3 and so on (as far as I know),
>>> also do remove the hardware entry of the old default address.
>> If we define behavior clearly, I think we can adapt PMD implementation
>> according it, unless there is HW limitation.
> Right. I think this is another point (issue 2/) to be discussed.
> Namely, whether the old default address should be removed when set new 
> default one.
> If we want to explicitly unify the behavior of all PMDs in ethdev 
> layer as described above,
> there may be no problem if do the following:
> 1) In the ethdev layer, remove the old default address if the old one 
> is exist.
> 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old 
> default address before adding the new one.
>    For other PMDs, we probably don't need to do anything because they 
> have supported remove_addr() API.
>    (Without explicitly removing the old default address, I don't know 
> if their hardware or firmware
>     removes the old one when set a new address. But, we explicitly 
> remove the old one in ethdev layer now,
>     I'm not sure if this has an effect on these PMDs.)
>>>>> if there is
>>>>> single MAC I guess intention is to replace it with new one, but if 
>>>>> there
>>>>> are multiple MACs and one of them are already in the list 
>>>>> intention may
>>>>> be just to change the default MAC.
>>>> The assumption in this patch is that "Set" means "Replace", not 
>>>> "Swap".
>>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>>
>>>>> If above assumption is correct, what about following:
>>>>>
>>>>> set(MAC) {
>>>>>       if only_default_mac_exist
>>>>>           replace_default_mac
>>>>>
>>>>>       if MAC exists in list
>>>>>      swap MAC and list[0]
>>>>>       else
>>>>>      replace_default_mac
>>>>> }
>>>> This approach 2/ is a mix of Swap and Replace.
>>>> The old default MAC destiny depends on whether
>>>> we have added the new MAC as "secondary" before setting as new 
>>>> default.
>>>>
>>>>> This swap prevents removing MAC side affect, does it make sense?
>>>> Another approach would be 3/ to do an "Always Swap"
>>>> even if the new MAC didn't exist before,
>>>> you keep the old default MAC as a secondary MAC.
>>>>
>>>> And the current approach 0/ is to Replace default MAC address
>>>> without touching the secondary addresses at all.
>>>>
>>>> So we have 4 choices.
>>>> We could vote, roll a dice, or find a strong argument?
>>> According to the implement of set() in ethdev and PMD layer, it always
>>> use "Replace", not "Swap".
>>> If we use "Swap" now, the behavior of this API will be changed.
>>> I'm not sure if the application can accept this change or has other
>>> effects.
>>>
>> This patch is also changing behavior, because of implied remove address,
>> same concern is valid with this patch.
> Indeed, it changes the behavior.
> But this patch only resolves the problem (issue 1/) that the entries 
> of the MAC address list possibly are not uniques.
> Fixing it may be little impact on the application.
>>
>>
>> As I checked again current implementation may have one more problem
>> (this from reading code, I did not test this):
>> add(MAC1) -> MAC1
>> add(MAC2) -> MAC1, MAC2
>> set(MAC2) -> MAC2, MAC2
>> del(MAC2) -> FAILS
>>
>> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
>> MAC, and it only tries to remove first address it finds, it can't find
>> and remove second 'MAC2'.
>> I wasn't too much bothered with wasting one MAC address slot, so wasn't
>> sure if a change is required at all, but if above analysis is correct I
>> think this is more serious problem to justify the change.
> Your analysis is fully correct.
>>
>>
>> I don't think always swap (option /3) is good idea, specially for single
>> MAC address exists case, and current case has (option 0/) has mentioned
>> problems.
> +1
>> Remaining ones are mix of swap and replace (option 2/) and this patch
>> (option /1).
>>
>> I think mix of swap and replace (option 2/ above) has some benefits:
>> - It always replaces default MAC
>> - Prevents duplication MAC address in the list
>> - Doesn't implicitly remove address from list
> As far as I know, the first entry (index 0) always be the default 
> address in all PMDs,
> but it's not documented. (So this patch did it, that's what was 
> discussed earlier).
> The 'Swap' may be inappropriate. It may need to be discussed.
>>
>> BUT, if the agreement is this patch (option 1/) I am OK with that too, I
>> just want to make sure that it is discussed.
>>
>>
>>> BTW, it seems that the ethernet port in kernel also replaces the old
>>> address if we modify the one.
>>> Use the test command: ifconfig eth0 hw ether new_mac
>> For default MAC address it is more clear that intention is to replace
>> it, but question is about what to do with the list of MAC addresses.
> Hi Ferruh and Thomas,
>
> As mentioned above, they are actually two problems (issue /1 and issue 
> /2).
> Can we deal with them separately?
> #1 For issue /1, it's really a problem. This patch is responsible for it.
> #2 For issue /2, I will send a RFC to discuss as described above.
>      It may require the participation of all PMD maintainers.
>
> What do you think?
>
>
Hi Ferruh and Thomas,

What do you think of the above proposal?
Looking forward to your reply.

/Huisong
>>
>> .
> .
Ferruh Yigit Feb. 9, 2023, 12:45 p.m. UTC | #9 
On 2/4/2023 2:57 AM, lihuisong (C) wrote:
> 
> 在 2023/2/3 20:58, Ferruh Yigit 写道:
>> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>>> 02/02/2023 19:09, Ferruh Yigit:
>>>>> On 2/2/2023 12:36 PM, 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 entries in the list.
>>>>>> Like:
>>>>>> add(MAC1)
>>>>>> add(MAC2)
>>>>>> add(MAC3)
>>>>>> add(MAC4)
>>>>>> set_default(MAC3)
>>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>>> Note: MAC3 occupies two entries.
>>>>>>
>>>>>> 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 need to ensure that the new default address is removed from the
>>>>>> rest of
>>>>>> the list if the address was already in the list.
>>>>>>
>>>>> Same comment from past seems already valid, I am not looking to the
>>>>> set
>>>>> for a while, sorry if this is already discussed and decided,
>>>>> if not, I am referring to the side effect that setting MAC addresses
>>>>> cause to remove MAC addresses, think following case:
>>>>>
>>>>> add(MAC1) -> MAC1
>>>>> add(MAC2) -> MAC1, MAC2
>>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>>> set(MAC4) -> MAC4, MAC2
>>>>> set(MAC2) -> MAC2
>>>>>
>>>>> I am not exactly clear what is the intention with set(),
>>>> That's the problem, nobody is clear with the current behavior.
>>>> The doc says "Set the default MAC address." and nothing else.
>>> Indeed. But we can see the following information.
>>>  From the ethdev layer, this set() API always replaces the old default
>>> address (index 0) without adding the old one.
>>>  From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>>> hns3 and so on (as far as I know),
>>> also do remove the hardware entry of the old default address.
>> If we define behavior clearly, I think we can adapt PMD implementation
>> according it, unless there is HW limitation.
> Right. I think this is another point (issue 2/) to be discussed.
> Namely, whether the old default address should be removed when set new
> default one.
> If we want to explicitly unify the behavior of all PMDs in ethdev layer
> as described above,
> there may be no problem if do the following:
> 1) In the ethdev layer, remove the old default address if the old one is
> exist.
> 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old
> default address before adding the new one.
>    For other PMDs, we probably don't need to do anything because they
> have supported remove_addr() API.
>    (Without explicitly removing the old default address, I don't know if
> their hardware or firmware
>     removes the old one when set a new address. But, we explicitly
> remove the old one in ethdev layer now,
>     I'm not sure if this has an effect on these PMDs.)
>>>>> if there is
>>>>> single MAC I guess intention is to replace it with new one, but if
>>>>> there
>>>>> are multiple MACs and one of them are already in the list intention
>>>>> may
>>>>> be just to change the default MAC.
>>>> The assumption in this patch is that "Set" means "Replace", not "Swap".
>>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>>
>>>>> If above assumption is correct, what about following:
>>>>>
>>>>> set(MAC) {
>>>>>       if only_default_mac_exist
>>>>>           replace_default_mac
>>>>>
>>>>>       if MAC exists in list
>>>>>      swap MAC and list[0]
>>>>>       else
>>>>>      replace_default_mac
>>>>> }
>>>> This approach 2/ is a mix of Swap and Replace.
>>>> The old default MAC destiny depends on whether
>>>> we have added the new MAC as "secondary" before setting as new default.
>>>>
>>>>> This swap prevents removing MAC side affect, does it make sense?
>>>> Another approach would be 3/ to do an "Always Swap"
>>>> even if the new MAC didn't exist before,
>>>> you keep the old default MAC as a secondary MAC.
>>>>
>>>> And the current approach 0/ is to Replace default MAC address
>>>> without touching the secondary addresses at all.
>>>>
>>>> So we have 4 choices.
>>>> We could vote, roll a dice, or find a strong argument?
>>> According to the implement of set() in ethdev and PMD layer, it always
>>> use "Replace", not "Swap".
>>> If we use "Swap" now, the behavior of this API will be changed.
>>> I'm not sure if the application can accept this change or has other
>>> effects.
>>>
>> This patch is also changing behavior, because of implied remove address,
>> same concern is valid with this patch.
> Indeed, it changes the behavior.
> But this patch only resolves the problem (issue 1/) that the entries of
> the MAC address list possibly are not uniques.
> Fixing it may be little impact on the application.
>>
>>
>> As I checked again current implementation may have one more problem
>> (this from reading code, I did not test this):
>> add(MAC1) -> MAC1
>> add(MAC2) -> MAC1, MAC2
>> set(MAC2) -> MAC2, MAC2
>> del(MAC2) -> FAILS
>>
>> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
>> MAC, and it only tries to remove first address it finds, it can't find
>> and remove second 'MAC2'.
>> I wasn't too much bothered with wasting one MAC address slot, so wasn't
>> sure if a change is required at all, but if above analysis is correct I
>> think this is more serious problem to justify the change.
> Your analysis is fully correct.
>>
>>
>> I don't think always swap (option /3) is good idea, specially for single
>> MAC address exists case, and current case has (option 0/) has mentioned
>> problems.
> +1
>> Remaining ones are mix of swap and replace (option 2/) and this patch
>> (option /1).
>>
>> I think mix of swap and replace (option 2/ above) has some benefits:
>> - It always replaces default MAC
>> - Prevents duplication MAC address in the list
>> - Doesn't implicitly remove address from list
> As far as I know, the first entry (index 0) always be the default
> address in all PMDs,
> but it's not documented. (So this patch did it, that's what was
> discussed earlier).
> The 'Swap' may be inappropriate. It may need to be discussed.

Yes, index 0 always holds the default MAC, +1 to document this.

In option /2, MAC swap is done only if the MAC address is already in the
list.


Another option can be to store an enabled/disabled flag for each MAC
address in the list.
In set(MAC), if MAC is already in the list, the existing one in the list
can be disabled. Next time default MAC changed, disabled MAC can be
enabled again. Like:

(d) => disabled

add(MAC1) -> MAC1
add(MAC2) -> MAC1, MAC2
add(MAC3) -> MAC1, MAC2, MAC3
add(MAC4) -> MAC1, MAC2, MAC3, MAC4
set(MAC3) -> MAC3, MAC2, MAC3(d), MAC4
set(MAC4) -> MAC4, MAC2, MAC3, MAC4(d)
set(MAC2) -> MAC2, MAC2(d), MAC3, MAC4
set(MAC5) -> MAC5, MAC2, MAC3, MAC4

The ones disabled in the ethdev layer can be explicitly removed in
drivers, and the ones enabled in the ethdev layer can be explicitly
added back in drives. This simplifies the driver implementation a lot.


>>
>> BUT, if the agreement is this patch (option 1/) I am OK with that too, I
>> just want to make sure that it is discussed.
>>
>>
>>> BTW, it seems that the ethernet port in kernel also replaces the old
>>> address if we modify the one.
>>> Use the test command: ifconfig eth0 hw ether new_mac
>> For default MAC address it is more clear that intention is to replace
>> it, but question is about what to do with the list of MAC addresses.
> Hi Ferruh and Thomas,
> 
> As mentioned above, they are actually two problems (issue /1 and issue /2).
> Can we deal with them separately?
> #1 For issue /1, it's really a problem. This patch is responsible for it.
> #2 For issue /2, I will send a RFC to discuss as described above.
>      It may require the participation of all PMD maintainers.
> 
> What do you think?
> 

Agree to separate fixing drivers (issue /2) and ethdev (issue /1), and
drivers can be fixed after ethdev clarified.

Sorry that this patch is taking time because expected behavior is not
clear, and we are not getting enough feedback for it. Also issue is not
a major or a blocking issue.
lihuisong (C) Feb. 10, 2023, 9:54 a.m. UTC | #10 
在 2023/2/9 20:45, Ferruh Yigit 写道:
> On 2/4/2023 2:57 AM, lihuisong (C) wrote:
>> 在 2023/2/3 20:58, Ferruh Yigit 写道:
>>> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>>>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>>>> 02/02/2023 19:09, Ferruh Yigit:
>>>>>> On 2/2/2023 12:36 PM, 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 entries in the list.
>>>>>>> Like:
>>>>>>> add(MAC1)
>>>>>>> add(MAC2)
>>>>>>> add(MAC3)
>>>>>>> add(MAC4)
>>>>>>> set_default(MAC3)
>>>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>>>> Note: MAC3 occupies two entries.
>>>>>>>
>>>>>>> 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 need to ensure that the new default address is removed from the
>>>>>>> rest of
>>>>>>> the list if the address was already in the list.
>>>>>>>
>>>>>> Same comment from past seems already valid, I am not looking to the
>>>>>> set
>>>>>> for a while, sorry if this is already discussed and decided,
>>>>>> if not, I am referring to the side effect that setting MAC addresses
>>>>>> cause to remove MAC addresses, think following case:
>>>>>>
>>>>>> add(MAC1) -> MAC1
>>>>>> add(MAC2) -> MAC1, MAC2
>>>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>>>> set(MAC4) -> MAC4, MAC2
>>>>>> set(MAC2) -> MAC2
>>>>>>
>>>>>> I am not exactly clear what is the intention with set(),
>>>>> That's the problem, nobody is clear with the current behavior.
>>>>> The doc says "Set the default MAC address." and nothing else.
>>>> Indeed. But we can see the following information.
>>>>   From the ethdev layer, this set() API always replaces the old default
>>>> address (index 0) without adding the old one.
>>>>   From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>>>> hns3 and so on (as far as I know),
>>>> also do remove the hardware entry of the old default address.
>>> If we define behavior clearly, I think we can adapt PMD implementation
>>> according it, unless there is HW limitation.
>> Right. I think this is another point (issue 2/) to be discussed.
>> Namely, whether the old default address should be removed when set new
>> default one.
>> If we want to explicitly unify the behavior of all PMDs in ethdev layer
>> as described above,
>> there may be no problem if do the following:
>> 1) In the ethdev layer, remove the old default address if the old one is
>> exist.
>> 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old
>> default address before adding the new one.
>>     For other PMDs, we probably don't need to do anything because they
>> have supported remove_addr() API.
>>     (Without explicitly removing the old default address, I don't know if
>> their hardware or firmware
>>      removes the old one when set a new address. But, we explicitly
>> remove the old one in ethdev layer now,
>>      I'm not sure if this has an effect on these PMDs.)
>>>>>> if there is
>>>>>> single MAC I guess intention is to replace it with new one, but if
>>>>>> there
>>>>>> are multiple MACs and one of them are already in the list intention
>>>>>> may
>>>>>> be just to change the default MAC.
>>>>> The assumption in this patch is that "Set" means "Replace", not "Swap".
>>>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>>>
>>>>>> If above assumption is correct, what about following:
>>>>>>
>>>>>> set(MAC) {
>>>>>>        if only_default_mac_exist
>>>>>>            replace_default_mac
>>>>>>
>>>>>>        if MAC exists in list
>>>>>>       swap MAC and list[0]
>>>>>>        else
>>>>>>       replace_default_mac
>>>>>> }
>>>>> This approach 2/ is a mix of Swap and Replace.
>>>>> The old default MAC destiny depends on whether
>>>>> we have added the new MAC as "secondary" before setting as new default.
>>>>>
>>>>>> This swap prevents removing MAC side affect, does it make sense?
>>>>> Another approach would be 3/ to do an "Always Swap"
>>>>> even if the new MAC didn't exist before,
>>>>> you keep the old default MAC as a secondary MAC.
>>>>>
>>>>> And the current approach 0/ is to Replace default MAC address
>>>>> without touching the secondary addresses at all.
>>>>>
>>>>> So we have 4 choices.
>>>>> We could vote, roll a dice, or find a strong argument?
>>>> According to the implement of set() in ethdev and PMD layer, it always
>>>> use "Replace", not "Swap".
>>>> If we use "Swap" now, the behavior of this API will be changed.
>>>> I'm not sure if the application can accept this change or has other
>>>> effects.
>>>>
>>> This patch is also changing behavior, because of implied remove address,
>>> same concern is valid with this patch.
>> Indeed, it changes the behavior.
>> But this patch only resolves the problem (issue 1/) that the entries of
>> the MAC address list possibly are not uniques.
>> Fixing it may be little impact on the application.
>>>
>>> As I checked again current implementation may have one more problem
>>> (this from reading code, I did not test this):
>>> add(MAC1) -> MAC1
>>> add(MAC2) -> MAC1, MAC2
>>> set(MAC2) -> MAC2, MAC2
>>> del(MAC2) -> FAILS
>>>
>>> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
>>> MAC, and it only tries to remove first address it finds, it can't find
>>> and remove second 'MAC2'.
>>> I wasn't too much bothered with wasting one MAC address slot, so wasn't
>>> sure if a change is required at all, but if above analysis is correct I
>>> think this is more serious problem to justify the change.
>> Your analysis is fully correct.
>>>
>>> I don't think always swap (option /3) is good idea, specially for single
>>> MAC address exists case, and current case has (option 0/) has mentioned
>>> problems.
>> +1
>>> Remaining ones are mix of swap and replace (option 2/) and this patch
>>> (option /1).
>>>
>>> I think mix of swap and replace (option 2/ above) has some benefits:
>>> - It always replaces default MAC
>>> - Prevents duplication MAC address in the list
>>> - Doesn't implicitly remove address from list
>> As far as I know, the first entry (index 0) always be the default
>> address in all PMDs,
>> but it's not documented. (So this patch did it, that's what was
>> discussed earlier).
>> The 'Swap' may be inappropriate. It may need to be discussed.
> Yes, index 0 always holds the default MAC, +1 to document this.
>
> In option /2, MAC swap is done only if the MAC address is already in the
> list.
>
Whether the 'swap' is required in this case depends on the assumption 
that the
old default address needs to be 'secondary' address when set new default 
one, right?
It is still the same issue as issue /2 to be discussed.
> Another option can be to store an enabled/disabled flag for each MAC
> address in the list.
> In set(MAC), if MAC is already in the list, the existing one in the list
> can be disabled. Next time default MAC changed, disabled MAC can be
> enabled again. Like:
>
> (d) => disabled
>
> add(MAC1) -> MAC1
> add(MAC2) -> MAC1, MAC2
> add(MAC3) -> MAC1, MAC2, MAC3
> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
> set(MAC3) -> MAC3, MAC2, MAC3(d), MAC4
> set(MAC4) -> MAC4, MAC2, MAC3, MAC4(d)
> set(MAC2) -> MAC2, MAC2(d), MAC3, MAC4
> set(MAC5) -> MAC5, MAC2, MAC3, MAC4
>
> The ones disabled in the ethdev layer can be explicitly removed in
> drivers, and the ones enabled in the ethdev layer can be explicitly
> added back in drives. This simplifies the driver implementation a lot.
>
Good idea, but it's a little complicated.
If I understand correctly, all of the above are done under the condition 
that 'swap' is accepted.
>>> BUT, if the agreement is this patch (option 1/) I am OK with that too, I
>>> just want to make sure that it is discussed.
>>>
>>>
>>>> BTW, it seems that the ethernet port in kernel also replaces the old
>>>> address if we modify the one.
>>>> Use the test command: ifconfig eth0 hw ether new_mac
>>> For default MAC address it is more clear that intention is to replace
>>> it, but question is about what to do with the list of MAC addresses.
>> Hi Ferruh and Thomas,
>>
>> As mentioned above, they are actually two problems (issue /1 and issue /2).
>> Can we deal with them separately?
>> #1 For issue /1, it's really a problem. This patch is responsible for it.
>> #2 For issue /2, I will send a RFC to discuss as described above.
>>       It may require the participation of all PMD maintainers.
>>
>> What do you think?
>>
> Agree to separate fixing drivers (issue /2) and ethdev (issue /1), and
> drivers can be fixed after ethdev clarified.
Yeah, we are in the same boat.😁
>
> Sorry that this patch is taking time because expected behavior is not
I cannot understand the behavior determined by this patch is not expected.
Wouldn't it be simpler and more reasonable to make sure that the entries 
in the MAC list are uniques?
> clear, and we are not getting enough feedback for it. Also issue is not
> a major or a blocking issue.
IMO, the last point to be discussed and have enough feedback is the 
issue /2 instead of issue /1.
Whether the old default address needs to be removed or swapped when set 
new one is the major or a blocking issue.

We can't go back and forth between them. we should break the cycle.
For issue /1 (duplicate entries), please forget what to do with the old 
default address. I'm sure you'll think it's ok.
Let's take a new discussion for the old default address in set() API to 
get enough feedback.

what do you think, Ferruh and Thomas?
>
> .
Ferruh Yigit Feb. 10, 2023, 12:27 p.m. UTC | #11 
On 2/10/2023 9:54 AM, lihuisong (C) wrote:
> 
> 在 2023/2/9 20:45, Ferruh Yigit 写道:
>> On 2/4/2023 2:57 AM, lihuisong (C) wrote:
>>> 在 2023/2/3 20:58, Ferruh Yigit 写道:
>>>> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>>>>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>>>>> 02/02/2023 19:09, Ferruh Yigit:
>>>>>>> On 2/2/2023 12:36 PM, 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 entries in the
>>>>>>>> list.
>>>>>>>> Like:
>>>>>>>> add(MAC1)
>>>>>>>> add(MAC2)
>>>>>>>> add(MAC3)
>>>>>>>> add(MAC4)
>>>>>>>> set_default(MAC3)
>>>>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>>>>> Note: MAC3 occupies two entries.
>>>>>>>>
>>>>>>>> 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 need to ensure that the new default address is removed from the
>>>>>>>> rest of
>>>>>>>> the list if the address was already in the list.
>>>>>>>>
>>>>>>> Same comment from past seems already valid, I am not looking to the
>>>>>>> set
>>>>>>> for a while, sorry if this is already discussed and decided,
>>>>>>> if not, I am referring to the side effect that setting MAC addresses
>>>>>>> cause to remove MAC addresses, think following case:
>>>>>>>
>>>>>>> add(MAC1) -> MAC1
>>>>>>> add(MAC2) -> MAC1, MAC2
>>>>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>>>>> set(MAC4) -> MAC4, MAC2
>>>>>>> set(MAC2) -> MAC2
>>>>>>>
>>>>>>> I am not exactly clear what is the intention with set(),
>>>>>> That's the problem, nobody is clear with the current behavior.
>>>>>> The doc says "Set the default MAC address." and nothing else.
>>>>> Indeed. But we can see the following information.
>>>>>   From the ethdev layer, this set() API always replaces the old
>>>>> default
>>>>> address (index 0) without adding the old one.
>>>>>   From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>>>>> hns3 and so on (as far as I know),
>>>>> also do remove the hardware entry of the old default address.
>>>> If we define behavior clearly, I think we can adapt PMD implementation
>>>> according it, unless there is HW limitation.
>>> Right. I think this is another point (issue 2/) to be discussed.
>>> Namely, whether the old default address should be removed when set new
>>> default one.
>>> If we want to explicitly unify the behavior of all PMDs in ethdev layer
>>> as described above,
>>> there may be no problem if do the following:
>>> 1) In the ethdev layer, remove the old default address if the old one is
>>> exist.
>>> 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old
>>> default address before adding the new one.
>>>     For other PMDs, we probably don't need to do anything because they
>>> have supported remove_addr() API.
>>>     (Without explicitly removing the old default address, I don't
>>> know if
>>> their hardware or firmware
>>>      removes the old one when set a new address. But, we explicitly
>>> remove the old one in ethdev layer now,
>>>      I'm not sure if this has an effect on these PMDs.)
>>>>>>> if there is
>>>>>>> single MAC I guess intention is to replace it with new one, but if
>>>>>>> there
>>>>>>> are multiple MACs and one of them are already in the list intention
>>>>>>> may
>>>>>>> be just to change the default MAC.
>>>>>> The assumption in this patch is that "Set" means "Replace", not
>>>>>> "Swap".
>>>>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>>>>
>>>>>>> If above assumption is correct, what about following:
>>>>>>>
>>>>>>> set(MAC) {
>>>>>>>        if only_default_mac_exist
>>>>>>>            replace_default_mac
>>>>>>>
>>>>>>>        if MAC exists in list
>>>>>>>       swap MAC and list[0]
>>>>>>>        else
>>>>>>>       replace_default_mac
>>>>>>> }
>>>>>> This approach 2/ is a mix of Swap and Replace.
>>>>>> The old default MAC destiny depends on whether
>>>>>> we have added the new MAC as "secondary" before setting as new
>>>>>> default.
>>>>>>
>>>>>>> This swap prevents removing MAC side affect, does it make sense?
>>>>>> Another approach would be 3/ to do an "Always Swap"
>>>>>> even if the new MAC didn't exist before,
>>>>>> you keep the old default MAC as a secondary MAC.
>>>>>>
>>>>>> And the current approach 0/ is to Replace default MAC address
>>>>>> without touching the secondary addresses at all.
>>>>>>
>>>>>> So we have 4 choices.
>>>>>> We could vote, roll a dice, or find a strong argument?
>>>>> According to the implement of set() in ethdev and PMD layer, it always
>>>>> use "Replace", not "Swap".
>>>>> If we use "Swap" now, the behavior of this API will be changed.
>>>>> I'm not sure if the application can accept this change or has other
>>>>> effects.
>>>>>
>>>> This patch is also changing behavior, because of implied remove
>>>> address,
>>>> same concern is valid with this patch.
>>> Indeed, it changes the behavior.
>>> But this patch only resolves the problem (issue 1/) that the entries of
>>> the MAC address list possibly are not uniques.
>>> Fixing it may be little impact on the application.
>>>>
>>>> As I checked again current implementation may have one more problem
>>>> (this from reading code, I did not test this):
>>>> add(MAC1) -> MAC1
>>>> add(MAC2) -> MAC1, MAC2
>>>> set(MAC2) -> MAC2, MAC2
>>>> del(MAC2) -> FAILS
>>>>
>>>> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
>>>> MAC, and it only tries to remove first address it finds, it can't find
>>>> and remove second 'MAC2'.
>>>> I wasn't too much bothered with wasting one MAC address slot, so wasn't
>>>> sure if a change is required at all, but if above analysis is correct I
>>>> think this is more serious problem to justify the change.
>>> Your analysis is fully correct.
>>>>
>>>> I don't think always swap (option /3) is good idea, specially for
>>>> single
>>>> MAC address exists case, and current case has (option 0/) has mentioned
>>>> problems.
>>> +1
>>>> Remaining ones are mix of swap and replace (option 2/) and this patch
>>>> (option /1).
>>>>
>>>> I think mix of swap and replace (option 2/ above) has some benefits:
>>>> - It always replaces default MAC
>>>> - Prevents duplication MAC address in the list
>>>> - Doesn't implicitly remove address from list
>>> As far as I know, the first entry (index 0) always be the default
>>> address in all PMDs,
>>> but it's not documented. (So this patch did it, that's what was
>>> discussed earlier).
>>> The 'Swap' may be inappropriate. It may need to be discussed.
>> Yes, index 0 always holds the default MAC, +1 to document this.
>>
>> In option /2, MAC swap is done only if the MAC address is already in the
>> list.
>>
> Whether the 'swap' is required in this case depends on the assumption
> that the
> old default address needs to be 'secondary' address when set new default
> one, right?


We don't know if default address needs to be 'secondary' address or not,
swap is just a way to both keep list unique and don't implicit remove
any MAC address.


> It is still the same issue as issue /2 to be discussed.>> Another option can be to store an enabled/disabled flag for each MAC
>> address in the list.
>> In set(MAC), if MAC is already in the list, the existing one in the list
>> can be disabled. Next time default MAC changed, disabled MAC can be
>> enabled again. Like:
>>
>> (d) => disabled
>>
>> add(MAC1) -> MAC1
>> add(MAC2) -> MAC1, MAC2
>> add(MAC3) -> MAC1, MAC2, MAC3
>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>> set(MAC3) -> MAC3, MAC2, MAC3(d), MAC4
>> set(MAC4) -> MAC4, MAC2, MAC3, MAC4(d)
>> set(MAC2) -> MAC2, MAC2(d), MAC3, MAC4
>> set(MAC5) -> MAC5, MAC2, MAC3, MAC4
>>
>> The ones disabled in the ethdev layer can be explicitly removed in
>> drivers, and the ones enabled in the ethdev layer can be explicitly
>> added back in drives. This simplifies the driver implementation a lot.
>>
> Good idea, but it's a little complicated.
> If I understand correctly, all of the above are done under the condition
> that 'swap' is accepted.


Agree it is more complicated, and no this doesn't require swap.

Instead of implicitly removing duplicated MAC address it keeps in the
ethdev list as disabled, and adds it back when default MAC is changed.


>>>> BUT, if the agreement is this patch (option 1/) I am OK with that
>>>> too, I
>>>> just want to make sure that it is discussed.
>>>>
>>>>
>>>>> BTW, it seems that the ethernet port in kernel also replaces the old
>>>>> address if we modify the one.
>>>>> Use the test command: ifconfig eth0 hw ether new_mac
>>>> For default MAC address it is more clear that intention is to replace
>>>> it, but question is about what to do with the list of MAC addresses.
>>> Hi Ferruh and Thomas,
>>>
>>> As mentioned above, they are actually two problems (issue /1 and
>>> issue /2).
>>> Can we deal with them separately?
>>> #1 For issue /1, it's really a problem. This patch is responsible for
>>> it.
>>> #2 For issue /2, I will send a RFC to discuss as described above.
>>>       It may require the participation of all PMD maintainers.
>>>
>>> What do you think?
>>>
>> Agree to separate fixing drivers (issue /2) and ethdev (issue /1), and
>> drivers can be fixed after ethdev clarified.
> Yeah, we are in the same boat.😁
>>
>> Sorry that this patch is taking time because expected behavior is not
> I cannot understand the behavior determined by this patch is not expected.
> Wouldn't it be simpler and more reasonable to make sure that the entries
> in the MAC list are uniques?


Yes, above part is OK. Only problem is it may cause shrinking the MAC
list by setting MAC address, that may not be the expected behavior.


>> clear, and we are not getting enough feedback for it. Also issue is not
>> a major or a blocking issue.
> IMO, the last point to be discussed and have enough feedback is the
> issue /2 instead of issue /1.
> Whether the old default address needs to be removed or swapped when set
> new one is the major or a blocking issue.
> 
> We can't go back and forth between them. we should break the cycle.
> For issue /1 (duplicate entries), please forget what to do with the old
> default address. I'm sure you'll think it's ok.
> Let's take a new discussion for the old default address in set() API to
> get enough feedback.
> 
> what do you think, Ferruh and Thomas?

Does it work to present options to techboard, get a decision and proceed
with it?
lihuisong (C) Feb. 10, 2023, 1:20 p.m. UTC | #12 
在 2023/2/10 20:27, Ferruh Yigit 写道:
> On 2/10/2023 9:54 AM, lihuisong (C) wrote:
>> 在 2023/2/9 20:45, Ferruh Yigit 写道:
>>> On 2/4/2023 2:57 AM, lihuisong (C) wrote:
>>>> 在 2023/2/3 20:58, Ferruh Yigit 写道:
>>>>> On 2/3/2023 1:56 AM, lihuisong (C) wrote:
>>>>>> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>>>>>>> 02/02/2023 19:09, Ferruh Yigit:
>>>>>>>> On 2/2/2023 12:36 PM, 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 entries in the
>>>>>>>>> list.
>>>>>>>>> Like:
>>>>>>>>> add(MAC1)
>>>>>>>>> add(MAC2)
>>>>>>>>> add(MAC3)
>>>>>>>>> add(MAC4)
>>>>>>>>> set_default(MAC3)
>>>>>>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>>>>>>> Note: MAC3 occupies two entries.
>>>>>>>>>
>>>>>>>>> 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 need to ensure that the new default address is removed from the
>>>>>>>>> rest of
>>>>>>>>> the list if the address was already in the list.
>>>>>>>>>
>>>>>>>> Same comment from past seems already valid, I am not looking to the
>>>>>>>> set
>>>>>>>> for a while, sorry if this is already discussed and decided,
>>>>>>>> if not, I am referring to the side effect that setting MAC addresses
>>>>>>>> cause to remove MAC addresses, think following case:
>>>>>>>>
>>>>>>>> add(MAC1) -> MAC1
>>>>>>>> add(MAC2) -> MAC1, MAC2
>>>>>>>> add(MAC3) -> MAC1, MAC2, MAC3
>>>>>>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>>>>>>> set(MAC3) -> MAC3, MAC2, MAC4
>>>>>>>> set(MAC4) -> MAC4, MAC2
>>>>>>>> set(MAC2) -> MAC2
>>>>>>>>
>>>>>>>> I am not exactly clear what is the intention with set(),
>>>>>>> That's the problem, nobody is clear with the current behavior.
>>>>>>> The doc says "Set the default MAC address." and nothing else.
>>>>>> Indeed. But we can see the following information.
>>>>>>    From the ethdev layer, this set() API always replaces the old
>>>>>> default
>>>>>> address (index 0) without adding the old one.
>>>>>>    From the PMD layer, set() interface of some PMDs, such as i40e, ice,
>>>>>> hns3 and so on (as far as I know),
>>>>>> also do remove the hardware entry of the old default address.
>>>>> If we define behavior clearly, I think we can adapt PMD implementation
>>>>> according it, unless there is HW limitation.
>>>> Right. I think this is another point (issue 2/) to be discussed.
>>>> Namely, whether the old default address should be removed when set new
>>>> default one.
>>>> If we want to explicitly unify the behavior of all PMDs in ethdev layer
>>>> as described above,
>>>> there may be no problem if do the following:
>>>> 1) In the ethdev layer, remove the old default address if the old one is
>>>> exist.
>>>> 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old
>>>> default address before adding the new one.
>>>>      For other PMDs, we probably don't need to do anything because they
>>>> have supported remove_addr() API.
>>>>      (Without explicitly removing the old default address, I don't
>>>> know if
>>>> their hardware or firmware
>>>>       removes the old one when set a new address. But, we explicitly
>>>> remove the old one in ethdev layer now,
>>>>       I'm not sure if this has an effect on these PMDs.)
>>>>>>>> if there is
>>>>>>>> single MAC I guess intention is to replace it with new one, but if
>>>>>>>> there
>>>>>>>> are multiple MACs and one of them are already in the list intention
>>>>>>>> may
>>>>>>>> be just to change the default MAC.
>>>>>>> The assumption in this patch is that "Set" means "Replace", not
>>>>>>> "Swap".
>>>>>>> So this patch takes the approach 1/ Replace and keep Unique.
>>>>>>>
>>>>>>>> If above assumption is correct, what about following:
>>>>>>>>
>>>>>>>> set(MAC) {
>>>>>>>>         if only_default_mac_exist
>>>>>>>>             replace_default_mac
>>>>>>>>
>>>>>>>>         if MAC exists in list
>>>>>>>>        swap MAC and list[0]
>>>>>>>>         else
>>>>>>>>        replace_default_mac
>>>>>>>> }
>>>>>>> This approach 2/ is a mix of Swap and Replace.
>>>>>>> The old default MAC destiny depends on whether
>>>>>>> we have added the new MAC as "secondary" before setting as new
>>>>>>> default.
>>>>>>>
>>>>>>>> This swap prevents removing MAC side affect, does it make sense?
>>>>>>> Another approach would be 3/ to do an "Always Swap"
>>>>>>> even if the new MAC didn't exist before,
>>>>>>> you keep the old default MAC as a secondary MAC.
>>>>>>>
>>>>>>> And the current approach 0/ is to Replace default MAC address
>>>>>>> without touching the secondary addresses at all.
>>>>>>>
>>>>>>> So we have 4 choices.
>>>>>>> We could vote, roll a dice, or find a strong argument?
>>>>>> According to the implement of set() in ethdev and PMD layer, it always
>>>>>> use "Replace", not "Swap".
>>>>>> If we use "Swap" now, the behavior of this API will be changed.
>>>>>> I'm not sure if the application can accept this change or has other
>>>>>> effects.
>>>>>>
>>>>> This patch is also changing behavior, because of implied remove
>>>>> address,
>>>>> same concern is valid with this patch.
>>>> Indeed, it changes the behavior.
>>>> But this patch only resolves the problem (issue 1/) that the entries of
>>>> the MAC address list possibly are not uniques.
>>>> Fixing it may be little impact on the application.
>>>>> As I checked again current implementation may have one more problem
>>>>> (this from reading code, I did not test this):
>>>>> add(MAC1) -> MAC1
>>>>> add(MAC2) -> MAC1, MAC2
>>>>> set(MAC2) -> MAC2, MAC2
>>>>> del(MAC2) -> FAILS
>>>>>
>>>>> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default
>>>>> MAC, and it only tries to remove first address it finds, it can't find
>>>>> and remove second 'MAC2'.
>>>>> I wasn't too much bothered with wasting one MAC address slot, so wasn't
>>>>> sure if a change is required at all, but if above analysis is correct I
>>>>> think this is more serious problem to justify the change.
>>>> Your analysis is fully correct.
>>>>> I don't think always swap (option /3) is good idea, specially for
>>>>> single
>>>>> MAC address exists case, and current case has (option 0/) has mentioned
>>>>> problems.
>>>> +1
>>>>> Remaining ones are mix of swap and replace (option 2/) and this patch
>>>>> (option /1).
>>>>>
>>>>> I think mix of swap and replace (option 2/ above) has some benefits:
>>>>> - It always replaces default MAC
>>>>> - Prevents duplication MAC address in the list
>>>>> - Doesn't implicitly remove address from list
>>>> As far as I know, the first entry (index 0) always be the default
>>>> address in all PMDs,
>>>> but it's not documented. (So this patch did it, that's what was
>>>> discussed earlier).
>>>> The 'Swap' may be inappropriate. It may need to be discussed.
>>> Yes, index 0 always holds the default MAC, +1 to document this.
>>>
>>> In option /2, MAC swap is done only if the MAC address is already in the
>>> list.
>>>
>> Whether the 'swap' is required in this case depends on the assumption
>> that the
>> old default address needs to be 'secondary' address when set new default
>> one, right?
>
> We don't know if default address needs to be 'secondary' address or not,
> swap is just a way to both keep list unique and don't implicit remove
> any MAC address.
>
>
>> It is still the same issue as issue /2 to be discussed.>> Another option can be to store an enabled/disabled flag for each MAC
>>> address in the list.
>>> In set(MAC), if MAC is already in the list, the existing one in the list
>>> can be disabled. Next time default MAC changed, disabled MAC can be
>>> enabled again. Like:
>>>
>>> (d) => disabled
>>>
>>> add(MAC1) -> MAC1
>>> add(MAC2) -> MAC1, MAC2
>>> add(MAC3) -> MAC1, MAC2, MAC3
>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>> set(MAC3) -> MAC3, MAC2, MAC3(d), MAC4
>>> set(MAC4) -> MAC4, MAC2, MAC3, MAC4(d)
>>> set(MAC2) -> MAC2, MAC2(d), MAC3, MAC4
>>> set(MAC5) -> MAC5, MAC2, MAC3, MAC4
>>>
>>> The ones disabled in the ethdev layer can be explicitly removed in
>>> drivers, and the ones enabled in the ethdev layer can be explicitly
>>> added back in drives. This simplifies the driver implementation a lot.
>>>
>> Good idea, but it's a little complicated.
>> If I understand correctly, all of the above are done under the condition
>> that 'swap' is accepted.
>
> Agree it is more complicated, and no this doesn't require swap.
>
> Instead of implicitly removing duplicated MAC address it keeps in the
> ethdev list as disabled, and adds it back when default MAC is changed.
>
>
>>>>> BUT, if the agreement is this patch (option 1/) I am OK with that
>>>>> too, I
>>>>> just want to make sure that it is discussed.
>>>>>
>>>>>
>>>>>> BTW, it seems that the ethernet port in kernel also replaces the old
>>>>>> address if we modify the one.
>>>>>> Use the test command: ifconfig eth0 hw ether new_mac
>>>>> For default MAC address it is more clear that intention is to replace
>>>>> it, but question is about what to do with the list of MAC addresses.
>>>> Hi Ferruh and Thomas,
>>>>
>>>> As mentioned above, they are actually two problems (issue /1 and
>>>> issue /2).
>>>> Can we deal with them separately?
>>>> #1 For issue /1, it's really a problem. This patch is responsible for
>>>> it.
>>>> #2 For issue /2, I will send a RFC to discuss as described above.
>>>>        It may require the participation of all PMD maintainers.
>>>>
>>>> What do you think?
>>>>
>>> Agree to separate fixing drivers (issue /2) and ethdev (issue /1), and
>>> drivers can be fixed after ethdev clarified.
>> Yeah, we are in the same boat.😁
>>> Sorry that this patch is taking time because expected behavior is not
>> I cannot understand the behavior determined by this patch is not expected.
>> Wouldn't it be simpler and more reasonable to make sure that the entries
>> in the MAC list are uniques?
>
> Yes, above part is OK. Only problem is it may cause shrinking the MAC
> list by setting MAC address, that may not be the expected behavior.
The root cause of shrinking the MAC list is that the old default address 
is removed when set MAC address,
rather than removing the address being already in the rest of the list.
>
>
>>> clear, and we are not getting enough feedback for it. Also issue is not
>>> a major or a blocking issue.
>> IMO, the last point to be discussed and have enough feedback is the
>> issue /2 instead of issue /1.
>> Whether the old default address needs to be removed or swapped when set
>> new one is the major or a blocking issue.
>>
>> We can't go back and forth between them. we should break the cycle.
>> For issue /1 (duplicate entries), please forget what to do with the old
>> default address. I'm sure you'll think it's ok.
>> Let's take a new discussion for the old default address in set() API to
>> get enough feedback.
>>
>> what do you think, Ferruh and Thomas?
> Does it work to present options to techboard, get a decision and proceed
> with it?
Agreed. At least, there's a decision soon.
> .
lihuisong (C) May 16, 2023, 11:47 a.m. UTC | #13 
Hi Ferruh,

There is no result on techboard.
How to deal with this problem next?

/Huisong

在 2023/2/2 20:36, 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 entries in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> Note: MAC3 occupies two entries.
>
> 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 need to ensure that the new default address is removed from the rest of
> the list if the address was already in the list.
>
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
> v8: fix some comments.
> v7: add announcement in the release notes and document this behavior.
> v6: fix commit log and some code comments.
> 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.
> ---
>   doc/guides/rel_notes/release_23_03.rst |  6 +++++
>   lib/ethdev/ethdev_driver.h             |  6 ++++-
>   lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
>   lib/ethdev/rte_ethdev.h                |  3 +++
>   4 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index 84b112a8b1..1c9b9912c2 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -105,6 +105,12 @@ API Changes
>      Also, make sure to start the actual text at the margin.
>      =======================================================
>   
> +* ethdev: ensured all entries in MAC address list are uniques.
> +  When setting a default MAC address with the function
> +  ``rte_eth_dev_default_mac_addr_set``,
> +  the address is now removed from the rest of the address list
> +  in order to ensure it is only at index 0 of the list.
> +
>   
>   ABI Changes
>   -----------
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index dde3ec84ef..3994c61b86 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -117,7 +117,11 @@ 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 addresses.
> +	 * All entries are unique.
> +	 * The first entry (index zero) is the default address.
> +	 */
>   	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 86ca303ab5..de25183619 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,44 @@ 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;
>   
> +	/* 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, "Cannot remove the port %u address from the rest of list.\n",
> +				       port_id);
> +			return ret;
> +		}
> +	}
>   	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
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index d22de196db..2456153457 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>   
>   /**
>    * Set the default MAC address.
> + * It replaces the address at index 0 of the MAC address list.
> + * If the address was already in the MAC address list,
> + * it is removed from the rest of the list.
>    *
>    * @param port_id
>    *   The port identifier of the Ethernet device.
Ferruh Yigit May 16, 2023, 2:13 p.m. UTC | #14 
On 5/16/2023 12:47 PM, lihuisong (C) wrote:
> Hi Ferruh,
> 
> There is no result on techboard.
> How to deal with this problem next?

+techboard for comment.


Btw, what was your positioning to Bruce's suggestion,
when a MAC address is in the list, fail to set it as default and enforce
user do the corrective action (delete MAC explicitly etc...).
If you are OK with it, that is good for me too, unless techboard objects
we can proceed with that one.


> 
> /Huisong
> 
> 在 2023/2/2 20:36, 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 entries in the list.
>> Like:
>> add(MAC1)
>> add(MAC2)
>> add(MAC3)
>> add(MAC4)
>> set_default(MAC3)
>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>> Note: MAC3 occupies two entries.
>>
>> 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 need to ensure that the new default address is removed from the
>> rest of
>> the list if the address was already in the list.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>> v8: fix some comments.
>> v7: add announcement in the release notes and document this behavior.
>> v6: fix commit log and some code comments.
>> 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.
>> ---
>>   doc/guides/rel_notes/release_23_03.rst |  6 +++++
>>   lib/ethdev/ethdev_driver.h             |  6 ++++-
>>   lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
>>   lib/ethdev/rte_ethdev.h                |  3 +++
>>   4 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_23_03.rst
>> b/doc/guides/rel_notes/release_23_03.rst
>> index 84b112a8b1..1c9b9912c2 100644
>> --- a/doc/guides/rel_notes/release_23_03.rst
>> +++ b/doc/guides/rel_notes/release_23_03.rst
>> @@ -105,6 +105,12 @@ API Changes
>>      Also, make sure to start the actual text at the margin.
>>      =======================================================
>>   +* ethdev: ensured all entries in MAC address list are uniques.
>> +  When setting a default MAC address with the function
>> +  ``rte_eth_dev_default_mac_addr_set``,
>> +  the address is now removed from the rest of the address list
>> +  in order to ensure it is only at index 0 of the list.
>> +
>>     ABI Changes
>>   -----------
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index dde3ec84ef..3994c61b86 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -117,7 +117,11 @@ 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 addresses.
>> +     * All entries are unique.
>> +     * The first entry (index zero) is the default address.
>> +     */
>>       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 86ca303ab5..de25183619 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,44 @@ 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;
>>   +    /* 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, "Cannot remove the port %u address
>> from the rest of list.\n",
>> +                       port_id);
>> +            return ret;
>> +        }
>> +    }
>>       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
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index d22de196db..2456153457 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>     /**
>>    * Set the default MAC address.
>> + * It replaces the address at index 0 of the MAC address list.
>> + * If the address was already in the MAC address list,
>> + * it is removed from the rest of the list.
>>    *
>>    * @param port_id
>>    *   The port identifier of the Ethernet device.
lihuisong (C) May 17, 2023, 7:45 a.m. UTC | #15 
在 2023/5/16 22:13, Ferruh Yigit 写道:
> On 5/16/2023 12:47 PM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> There is no result on techboard.
>> How to deal with this problem next?
> +techboard for comment.
>
>
> Btw, what was your positioning to Bruce's suggestion,
> when a MAC address is in the list, fail to set it as default and enforce
> user do the corrective action (delete MAC explicitly etc...).
If a MAC address is in the list, rte_eth_dev_default_mac_addr_set 
returns failure?
> If you are OK with it, that is good for me too, unless techboard objects
> we can proceed with that one.
>
>
>> /Huisong
>>
>> 在 2023/2/2 20:36, 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 entries in the list.
>>> Like:
>>> add(MAC1)
>>> add(MAC2)
>>> add(MAC3)
>>> add(MAC4)
>>> set_default(MAC3)
>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>> Note: MAC3 occupies two entries.
>>>
>>> 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 need to ensure that the new default address is removed from the
>>> rest of
>>> the list if the address was already in the list.
>>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>> ---
>>> v8: fix some comments.
>>> v7: add announcement in the release notes and document this behavior.
>>> v6: fix commit log and some code comments.
>>> 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.
>>> ---
>>>    doc/guides/rel_notes/release_23_03.rst |  6 +++++
>>>    lib/ethdev/ethdev_driver.h             |  6 ++++-
>>>    lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
>>>    lib/ethdev/rte_ethdev.h                |  3 +++
>>>    4 files changed, 47 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/release_23_03.rst
>>> b/doc/guides/rel_notes/release_23_03.rst
>>> index 84b112a8b1..1c9b9912c2 100644
>>> --- a/doc/guides/rel_notes/release_23_03.rst
>>> +++ b/doc/guides/rel_notes/release_23_03.rst
>>> @@ -105,6 +105,12 @@ API Changes
>>>       Also, make sure to start the actual text at the margin.
>>>       =======================================================
>>>    +* ethdev: ensured all entries in MAC address list are uniques.
>>> +  When setting a default MAC address with the function
>>> +  ``rte_eth_dev_default_mac_addr_set``,
>>> +  the address is now removed from the rest of the address list
>>> +  in order to ensure it is only at index 0 of the list.
>>> +
>>>      ABI Changes
>>>    -----------
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index dde3ec84ef..3994c61b86 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -117,7 +117,11 @@ 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 addresses.
>>> +     * All entries are unique.
>>> +     * The first entry (index zero) is the default address.
>>> +     */
>>>        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 86ca303ab5..de25183619 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,44 @@ 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;
>>>    +    /* 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, "Cannot remove the port %u address
>>> from the rest of list.\n",
>>> +                       port_id);
>>> +            return ret;
>>> +        }
>>> +    }
>>>        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
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index d22de196db..2456153457 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>>      /**
>>>     * Set the default MAC address.
>>> + * It replaces the address at index 0 of the MAC address list.
>>> + * If the address was already in the MAC address list,
>>> + * it is removed from the rest of the list.
>>>     *
>>>     * @param port_id
>>>     *   The port identifier of the Ethernet device.
> .
Ferruh Yigit May 17, 2023, 8:53 a.m. UTC | #16 
On 5/17/2023 8:45 AM, lihuisong (C) wrote:
> 
> 在 2023/5/16 22:13, Ferruh Yigit 写道:
>> On 5/16/2023 12:47 PM, lihuisong (C) wrote:
>>> Hi Ferruh,
>>>
>>> There is no result on techboard.
>>> How to deal with this problem next?
>> +techboard for comment.
>>
>>
>> Btw, what was your positioning to Bruce's suggestion,
>> when a MAC address is in the list, fail to set it as default and enforce
>> user do the corrective action (delete MAC explicitly etc...).
> If a MAC address is in the list, rte_eth_dev_default_mac_addr_set
> returns failure?

Yes.
In that case API can return EEXIST or similar. In this case user need to
call 'rte_eth_dev_mac_addr_remove()' first and call
'rte_eth_dev_default_mac_addr_set()' again, if this is the intention.

>> If you are OK with it, that is good for me too, unless techboard objects
>> we can proceed with that one.
>>
>>
>>> /Huisong
>>>
>>> 在 2023/2/2 20:36, 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 entries in the list.
>>>> Like:
>>>> add(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> set_default(MAC3)
>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>> Note: MAC3 occupies two entries.
>>>>
>>>> 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 need to ensure that the new default address is removed from the
>>>> rest of
>>>> the list if the address was already in the list.
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---
>>>> v8: fix some comments.
>>>> v7: add announcement in the release notes and document this behavior.
>>>> v6: fix commit log and some code comments.
>>>> 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.
>>>> ---
>>>>    doc/guides/rel_notes/release_23_03.rst |  6 +++++
>>>>    lib/ethdev/ethdev_driver.h             |  6 ++++-
>>>>    lib/ethdev/rte_ethdev.c                | 35
>>>> ++++++++++++++++++++++++--
>>>>    lib/ethdev/rte_ethdev.h                |  3 +++
>>>>    4 files changed, 47 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/doc/guides/rel_notes/release_23_03.rst
>>>> b/doc/guides/rel_notes/release_23_03.rst
>>>> index 84b112a8b1..1c9b9912c2 100644
>>>> --- a/doc/guides/rel_notes/release_23_03.rst
>>>> +++ b/doc/guides/rel_notes/release_23_03.rst
>>>> @@ -105,6 +105,12 @@ API Changes
>>>>       Also, make sure to start the actual text at the margin.
>>>>       =======================================================
>>>>    +* ethdev: ensured all entries in MAC address list are uniques.
>>>> +  When setting a default MAC address with the function
>>>> +  ``rte_eth_dev_default_mac_addr_set``,
>>>> +  the address is now removed from the rest of the address list
>>>> +  in order to ensure it is only at index 0 of the list.
>>>> +
>>>>      ABI Changes
>>>>    -----------
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index dde3ec84ef..3994c61b86 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -117,7 +117,11 @@ 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 addresses.
>>>> +     * All entries are unique.
>>>> +     * The first entry (index zero) is the default address.
>>>> +     */
>>>>        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 86ca303ab5..de25183619 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,44 @@ 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;
>>>>    +    /* 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, "Cannot remove the port %u address
>>>> from the rest of list.\n",
>>>> +                       port_id);
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>>        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
>>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>>> index d22de196db..2456153457 100644
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>>>      /**
>>>>     * Set the default MAC address.
>>>> + * It replaces the address at index 0 of the MAC address list.
>>>> + * If the address was already in the MAC address list,
>>>> + * it is removed from the rest of the list.
>>>>     *
>>>>     * @param port_id
>>>>     *   The port identifier of the Ethernet device.
>> .
lihuisong (C) May 17, 2023, 11:46 a.m. UTC | #17 
在 2023/5/17 16:53, Ferruh Yigit 写道:
> On 5/17/2023 8:45 AM, lihuisong (C) wrote:
>> 在 2023/5/16 22:13, Ferruh Yigit 写道:
>>> On 5/16/2023 12:47 PM, lihuisong (C) wrote:
>>>> Hi Ferruh,
>>>>
>>>> There is no result on techboard.
>>>> How to deal with this problem next?
>>> +techboard for comment.
>>>
>>>
>>> Btw, what was your positioning to Bruce's suggestion,
>>> when a MAC address is in the list, fail to set it as default and enforce
>>> user do the corrective action (delete MAC explicitly etc...).
>> If a MAC address is in the list, rte_eth_dev_default_mac_addr_set
>> returns failure?
> Yes.
> In that case API can return EEXIST or similar. In this case user need to
> call 'rte_eth_dev_mac_addr_remove()' first and call
> 'rte_eth_dev_default_mac_addr_set()' again, if this is the intention.

lgtm.
send V9 to do this?

>
>>> If you are OK with it, that is good for me too, unless techboard objects
>>> we can proceed with that one.
>>>
>>>
Ferruh Yigit May 17, 2023, 1:43 p.m. UTC | #18 
On 5/17/2023 12:46 PM, lihuisong (C) wrote:
> 
> 在 2023/5/17 16:53, Ferruh Yigit 写道:
>> On 5/17/2023 8:45 AM, lihuisong (C) wrote:
>>> 在 2023/5/16 22:13, Ferruh Yigit 写道:
>>>> On 5/16/2023 12:47 PM, lihuisong (C) wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> There is no result on techboard.
>>>>> How to deal with this problem next?
>>>> +techboard for comment.
>>>>
>>>>
>>>> Btw, what was your positioning to Bruce's suggestion,
>>>> when a MAC address is in the list, fail to set it as default and
>>>> enforce
>>>> user do the corrective action (delete MAC explicitly etc...).
>>> If a MAC address is in the list, rte_eth_dev_default_mac_addr_set
>>> returns failure?
>> Yes.
>> In that case API can return EEXIST or similar. In this case user need to
>> call 'rte_eth_dev_mac_addr_remove()' first and call
>> 'rte_eth_dev_default_mac_addr_set()' again, if this is the intention.
> 
> lgtm.
> send V9 to do this?
> 

I am happy to proceed with a v9 unless there is no objection from techboard.
diff mbox series

Patch

diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
index 84b112a8b1..1c9b9912c2 100644
--- a/doc/guides/rel_notes/release_23_03.rst
+++ b/doc/guides/rel_notes/release_23_03.rst
@@ -105,6 +105,12 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =======================================================
 
+* ethdev: ensured all entries in MAC address list are uniques.
+  When setting a default MAC address with the function
+  ``rte_eth_dev_default_mac_addr_set``,
+  the address is now removed from the rest of the address list
+  in order to ensure it is only at index 0 of the list.
+
 
 ABI Changes
 -----------
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index dde3ec84ef..3994c61b86 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -117,7 +117,11 @@  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 addresses.
+	 * All entries are unique.
+	 * The first entry (index zero) is the default address.
+	 */
 	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 86ca303ab5..de25183619 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,44 @@  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;
 
+	/* 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, "Cannot remove the port %u address from the rest of list.\n",
+				       port_id);
+			return ret;
+		}
+	}
 	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
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index d22de196db..2456153457 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -4356,6 +4356,9 @@  int rte_eth_dev_mac_addr_remove(uint16_t port_id,
 
 /**
  * Set the default MAC address.
+ * It replaces the address at index 0 of the MAC address list.
+ * If the address was already in the MAC address list,
+ * it is removed from the rest of the list.
  *
  * @param port_id
  *   The port identifier of the Ethernet device.