DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
@ 2021-09-22  3:36 Min Hu (Connor)
  2021-09-22  6:39 ` Andrew Rybchenko
                   ` (3 more replies)
  0 siblings, 4 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2021-09-22  3:36 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, thomas

From: Huisong Li <lihuisong@huawei.com>

Use the testpmd to perform the following operations:
1) mac_addr add 0 00:18:2D:00:00:90
2) mac_addr add 0 00:18:2D:00:00:91
3) mac_addr add 0 00:18:2D:00:00:92
4) mac_addr set 0 00:18:2D:00:00:91
5) show port 0 macs
Number of MAC address added: 4
  00:18:2D:00:00:91
  00:18:2D:00:00:90
  00:18:2D:00:00:91
  00:18:2D:00:00:92

This is due to the reason that if the address has been added as a
non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
from dev->data->mac_addrs[] when set default MAC address with the same
address.

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

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index daf5ca9242..77657a3314 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4360,6 +4360,7 @@ int
 rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 {
 	struct rte_eth_dev *dev;
+	int index;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -4381,6 +4382,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * If the address has been added as a non-default MAC address by
+	 * rte_eth_dev_mac_addr_add API, it should be removed from
+	 * dev->data->mac_addrs[].
+	 */
+	index = eth_dev_get_mac_addr_index(port_id, addr);
+	if (index > 0) {
+		/* remove address in NIC data structure */
+		rte_ether_addr_copy(&null_mac_addr,
+				    &dev->data->mac_addrs[index]);
+		/* reset pool bitmap */
+		dev->data->mac_pool_sel[index] = 0;
+	}
+
 	/* Update default address in NIC data structure */
 	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
 
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  3:36 [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs Min Hu (Connor)
@ 2021-09-22  6:39 ` Andrew Rybchenko
  2021-09-22  7:43   ` Huisong Li
  2021-10-05 19:21 ` Thomas Monjalon
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 40+ messages in thread
From: Andrew Rybchenko @ 2021-09-22  6:39 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: ferruh.yigit, thomas

On 9/22/21 6:36 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Use the testpmd to perform the following operations:
> 1) mac_addr add 0 00:18:2D:00:00:90
> 2) mac_addr add 0 00:18:2D:00:00:91
> 3) mac_addr add 0 00:18:2D:00:00:92
> 4) mac_addr set 0 00:18:2D:00:00:91
> 5) show port 0 macs
> Number of MAC address added: 4
>   00:18:2D:00:00:91
>   00:18:2D:00:00:90
>   00:18:2D:00:00:91
>   00:18:2D:00:00:92
> 
> This is due to the reason that if the address has been added as a
> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
> from dev->data->mac_addrs[] when set default MAC address with the same
> address.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index daf5ca9242..77657a3314 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4360,6 +4360,7 @@ int
>  rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  {
>  	struct rte_eth_dev *dev;
> +	int index;
>  	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4381,6 +4382,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* remove address in NIC data structure */
> +		rte_ether_addr_copy(&null_mac_addr,
> +				    &dev->data->mac_addrs[index]);
> +		/* reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
> +
>  	/* Update default address in NIC data structure */
>  	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>  
> 

If I change default MAC to something else later, should the old
default MAC be returned to some specific pools? I guess it is
hard to do. If the change is accepted, the behaviour must be
documented in rte_eth_dev_default_mac_addr_set() description.

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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  6:39 ` Andrew Rybchenko
@ 2021-09-22  7:43   ` Huisong Li
  2021-09-22  8:02     ` Andrew Rybchenko
  0 siblings, 1 reply; 40+ messages in thread
From: Huisong Li @ 2021-09-22  7:43 UTC (permalink / raw)
  To: dev


在 2021/9/22 14:39, Andrew Rybchenko 写道:
> On 9/22/21 6:36 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Use the testpmd to perform the following operations:
>> 1) mac_addr add 0 00:18:2D:00:00:90
>> 2) mac_addr add 0 00:18:2D:00:00:91
>> 3) mac_addr add 0 00:18:2D:00:00:92
>> 4) mac_addr set 0 00:18:2D:00:00:91
>> 5) show port 0 macs
>> Number of MAC address added: 4
>>    00:18:2D:00:00:91
>>    00:18:2D:00:00:90
>>    00:18:2D:00:00:91
>>    00:18:2D:00:00:92
>>
>> This is due to the reason that if the address has been added as a
>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
>> from dev->data->mac_addrs[] when set default MAC address with the same
>> address.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index daf5ca9242..77657a3314 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -4360,6 +4360,7 @@ int
>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   {
>>   	struct rte_eth_dev *dev;
>> +	int index;
>>   	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -4381,6 +4382,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	/*
>> +	 * If the address has been added as a non-default MAC address by
>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>> +	 * dev->data->mac_addrs[].
>> +	 */
>> +	index = eth_dev_get_mac_addr_index(port_id, addr);
>> +	if (index > 0) {
>> +		/* remove address in NIC data structure */
>> +		rte_ether_addr_copy(&null_mac_addr,
>> +				    &dev->data->mac_addrs[index]);
>> +		/* reset pool bitmap */
>> +		dev->data->mac_pool_sel[index] = 0;
>> +	}
>> +
>>   	/* Update default address in NIC data structure */
>>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>   
>>
> If I change default MAC to something else later, should the old
> default MAC be returned to some specific pools? I guess it is

Since the old default MAC address is invalid, which has been removed 
from hardware.

This MAC address does not need to be added to some specific pools, and 
occupies

one position in mac addrs.

> hard to do. If the change is accepted, the behaviour must be
> documented in rte_eth_dev_default_mac_addr_set() description.
> .

I think it's not necessary. Because old default MAC is no longer valid 
if we modify

default MAC with a new MAC address. This is a definition of 
rte_eth_dev_default_mac_addr_set().

The current modification does not change the definition of the interface.


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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  7:43   ` Huisong Li
@ 2021-09-22  8:02     ` Andrew Rybchenko
  2021-09-22  9:48       ` Huisong Li
  0 siblings, 1 reply; 40+ messages in thread
From: Andrew Rybchenko @ 2021-09-22  8:02 UTC (permalink / raw)
  To: Huisong Li, dev

On 9/22/21 10:43 AM, Huisong Li wrote:
> 
> 在 2021/9/22 14:39, Andrew Rybchenko 写道:
>> On 9/22/21 6:36 AM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> Use the testpmd to perform the following operations:
>>> 1) mac_addr add 0 00:18:2D:00:00:90
>>> 2) mac_addr add 0 00:18:2D:00:00:91
>>> 3) mac_addr add 0 00:18:2D:00:00:92
>>> 4) mac_addr set 0 00:18:2D:00:00:91
>>> 5) show port 0 macs
>>> Number of MAC address added: 4
>>>    00:18:2D:00:00:91
>>>    00:18:2D:00:00:90
>>>    00:18:2D:00:00:91
>>>    00:18:2D:00:00:92
>>>
>>> This is due to the reason that if the address has been added as a
>>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't
>>> remove
>>> from dev->data->mac_addrs[] when set default MAC address with the same
>>> address.
>>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index daf5ca9242..77657a3314 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -4360,6 +4360,7 @@ int
>>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct
>>> rte_ether_addr *addr)
>>>   {
>>>       struct rte_eth_dev *dev;
>>> +    int index;
>>>       int ret;
>>>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> @@ -4381,6 +4382,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t
>>> port_id, struct rte_ether_addr *addr)
>>>       if (ret < 0)
>>>           return ret;
>>>   +    /*
>>> +     * If the address has been added as a non-default MAC address by
>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>> +     * dev->data->mac_addrs[].
>>> +     */
>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>> +    if (index > 0) {
>>> +        /* remove address in NIC data structure */
>>> +        rte_ether_addr_copy(&null_mac_addr,
>>> +                    &dev->data->mac_addrs[index]);
>>> +        /* reset pool bitmap */
>>> +        dev->data->mac_pool_sel[index] = 0;
>>> +    }
>>> +
>>>       /* Update default address in NIC data structure */
>>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>  
>> If I change default MAC to something else later, should the old
>> default MAC be returned to some specific pools? I guess it is
> 
> Since the old default MAC address is invalid, which has been removed
> from hardware.
> 
> This MAC address does not need to be added to some specific pools, and
> occupies
> 
> one position in mac addrs.
> 
>> hard to do. If the change is accepted, the behaviour must be
>> documented in rte_eth_dev_default_mac_addr_set() description.
>> .
> 
> I think it's not necessary. Because old default MAC is no longer valid
> if we modify
> 
> default MAC with a new MAC address. This is a definition of
> rte_eth_dev_default_mac_addr_set().
> 
> The current modification does not change the definition of the interface.

Not sure that I understand:

set default MAC-A
add MAC-B to pool 1
set default MAC-B
set default MAC-C

since I've not removed MAC-B from pool 1, I'd expect it to be
there.

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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  8:02     ` Andrew Rybchenko
@ 2021-09-22  9:48       ` Huisong Li
  0 siblings, 0 replies; 40+ messages in thread
From: Huisong Li @ 2021-09-22  9:48 UTC (permalink / raw)
  To: Andrew Rybchenko, dev


在 2021/9/22 16:02, Andrew Rybchenko 写道:
> On 9/22/21 10:43 AM, Huisong Li wrote:
>> 在 2021/9/22 14:39, Andrew Rybchenko 写道:
>>> On 9/22/21 6:36 AM, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> Use the testpmd to perform the following operations:
>>>> 1) mac_addr add 0 00:18:2D:00:00:90
>>>> 2) mac_addr add 0 00:18:2D:00:00:91
>>>> 3) mac_addr add 0 00:18:2D:00:00:92
>>>> 4) mac_addr set 0 00:18:2D:00:00:91
>>>> 5) show port 0 macs
>>>> Number of MAC address added: 4
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:90
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:92
>>>>
>>>> This is due to the reason that if the address has been added as a
>>>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't
>>>> remove
>>>> from dev->data->mac_addrs[] when set default MAC address with the same
>>>> address.
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index daf5ca9242..77657a3314 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -4360,6 +4360,7 @@ int
>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct
>>>> rte_ether_addr *addr)
>>>>    {
>>>>        struct rte_eth_dev *dev;
>>>> +    int index;
>>>>        int ret;
>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>> @@ -4381,6 +4382,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t
>>>> port_id, struct rte_ether_addr *addr)
>>>>        if (ret < 0)
>>>>            return ret;
>>>>    +    /*
>>>> +     * If the address has been added as a non-default MAC address by
>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>> +     * dev->data->mac_addrs[].
>>>> +     */
>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>> +    if (index > 0) {
>>>> +        /* remove address in NIC data structure */
>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>> +                    &dev->data->mac_addrs[index]);
>>>> +        /* reset pool bitmap */
>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>> +    }
>>>> +
>>>>        /* Update default address in NIC data structure */
>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>   
>>> If I change default MAC to something else later, should the old
>>> default MAC be returned to some specific pools? I guess it is
>> Since the old default MAC address is invalid, which has been removed
>> from hardware.
>>
>> This MAC address does not need to be added to some specific pools, and
>> occupies
>>
>> one position in mac addrs.
>>
>>> hard to do. If the change is accepted, the behaviour must be
>>> documented in rte_eth_dev_default_mac_addr_set() description.
>>> .
>> I think it's not necessary. Because old default MAC is no longer valid
>> if we modify
>>
>> default MAC with a new MAC address. This is a definition of
>> rte_eth_dev_default_mac_addr_set().
>>
>> The current modification does not change the definition of the interface.
> Not sure that I understand:
>
> set default MAC-A
> add MAC-B to pool 1
> set default MAC-B
> set default MAC-C
>
> since I've not removed MAC-B from pool 1, I'd expect it to be
> there.
> .

Yes. You are right.

But MAC-B is removed from hardware after executing "set default MAC-C" 
cmd, and

new default MAC-C takes effect.

View from interface rte_eth_dev_mac_addr_remove(), if one MAC address is 
removed,

the API removes the MAC from dev->data->mac_addrs[] and reset 
"mac_pool_sel"(that is,

all pools do not have the MAC address.).

This also means that above MAC-B should not exist in any of the pools and

in dev->data->mac_addrs[] after executing "set default MAC-C" cmd.


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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  3:36 [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs Min Hu (Connor)
  2021-09-22  6:39 ` Andrew Rybchenko
@ 2021-10-05 19:21 ` Thomas Monjalon
  2021-10-08  7:02   ` Min Hu (Connor)
  2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
  3 siblings, 1 reply; 40+ messages in thread
From: Thomas Monjalon @ 2021-10-05 19:21 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, ferruh.yigit, andrew.rybchenko

22/09/2021 05:36, Min Hu (Connor):
> From: Huisong Li <lihuisong@huawei.com>
> 
> Use the testpmd to perform the following operations:
> 1) mac_addr add 0 00:18:2D:00:00:90
> 2) mac_addr add 0 00:18:2D:00:00:91
> 3) mac_addr add 0 00:18:2D:00:00:92
> 4) mac_addr set 0 00:18:2D:00:00:91
> 5) show port 0 macs
> Number of MAC address added: 4
>   00:18:2D:00:00:91
>   00:18:2D:00:00:90
>   00:18:2D:00:00:91
>   00:18:2D:00:00:92

Please describe with words.
Reading similar MAC addresses is not a fun game.

> This is due to the reason that if the address has been added as a
> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
> from dev->data->mac_addrs[] when set default MAC address with the same
> address.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>




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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-05 19:21 ` Thomas Monjalon
@ 2021-10-08  7:02   ` Min Hu (Connor)
  2021-10-08 10:04     ` Thomas Monjalon
  0 siblings, 1 reply; 40+ messages in thread
From: Min Hu (Connor) @ 2021-10-08  7:02 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, ferruh.yigit, andrew.rybchenko

Hi, Thomas,

在 2021/10/6 3:21, Thomas Monjalon 写道:
> 22/09/2021 05:36, Min Hu (Connor):
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Use the testpmd to perform the following operations:
>> 1) mac_addr add 0 00:18:2D:00:00:90
>> 2) mac_addr add 0 00:18:2D:00:00:91
>> 3) mac_addr add 0 00:18:2D:00:00:92
>> 4) mac_addr set 0 00:18:2D:00:00:91
>> 5) show port 0 macs
>> Number of MAC address added: 4
>>    00:18:2D:00:00:91
>>    00:18:2D:00:00:90
>>    00:18:2D:00:00:91
>>    00:18:2D:00:00:92
> 
> Please describe with words.
> Reading similar MAC addresses is not a fun game.

I do not catch you, could you please be
more detailed, thanks.

> 
>> This is due to the reason that if the address has been added as a
>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
>> from dev->data->mac_addrs[] when set default MAC address with the same
>> address.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> 
> 
> .
> 

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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-08  7:02   ` Min Hu (Connor)
@ 2021-10-08 10:04     ` Thomas Monjalon
  2021-10-09  9:53       ` Min Hu (Connor)
  0 siblings, 1 reply; 40+ messages in thread
From: Thomas Monjalon @ 2021-10-08 10:04 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, ferruh.yigit, andrew.rybchenko

08/10/2021 09:02, Min Hu (Connor):
> Hi, Thomas,
> 
> 在 2021/10/6 3:21, Thomas Monjalon 写道:
> > 22/09/2021 05:36, Min Hu (Connor):
> >> From: Huisong Li <lihuisong@huawei.com>
> >>
> >> Use the testpmd to perform the following operations:
> >> 1) mac_addr add 0 00:18:2D:00:00:90
> >> 2) mac_addr add 0 00:18:2D:00:00:91
> >> 3) mac_addr add 0 00:18:2D:00:00:92
> >> 4) mac_addr set 0 00:18:2D:00:00:91
> >> 5) show port 0 macs
> >> Number of MAC address added: 4
> >>    00:18:2D:00:00:91
> >>    00:18:2D:00:00:90
> >>    00:18:2D:00:00:91
> >>    00:18:2D:00:00:92
> > 
> > Please describe with words.
> > Reading similar MAC addresses is not a fun game.
> 
> I do not catch you, could you please be
> more detailed, thanks.

Me too, I don't catch you.
Please explain the problem in the commit log
so we can understand without the example.

> >> This is due to the reason that if the address has been added as a
> >> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
> >> from dev->data->mac_addrs[] when set default MAC address with the same
> >> address.
> >>
> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>




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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-08 10:04     ` Thomas Monjalon
@ 2021-10-09  9:53       ` Min Hu (Connor)
  2021-10-11  9:02         ` Thomas Monjalon
  0 siblings, 1 reply; 40+ messages in thread
From: Min Hu (Connor) @ 2021-10-09  9:53 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, ferruh.yigit, andrew.rybchenko

Hi, Thomas,

The dev->data->mac_addrs[0] will be changed to a new MAC address when 
applications modify
the default MAC address by rte_eth_dev_default_mac_addr_set() API. 
However, If the new default
MAC address has been added as a non-default MAC address by 
rte_eth_dev_mac_addr_add() API, the
rte_eth_dev_default_mac_addr_set() API doesn't remove it from 
dev->data->mac_addrs[].
As a result, one MAC address occupies two index capacities in 
dev->data->mac_addrs[].
This patch adds the logic of removing MAC addresses for this scenario.

Is that will be more clear? Hope for your reply

在 2021/10/8 18:04, Thomas Monjalon 写道:
> 08/10/2021 09:02, Min Hu (Connor):
>> Hi, Thomas,
>>
>> 在 2021/10/6 3:21, Thomas Monjalon 写道:
>>> 22/09/2021 05:36, Min Hu (Connor):
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> Use the testpmd to perform the following operations:
>>>> 1) mac_addr add 0 00:18:2D:00:00:90
>>>> 2) mac_addr add 0 00:18:2D:00:00:91
>>>> 3) mac_addr add 0 00:18:2D:00:00:92
>>>> 4) mac_addr set 0 00:18:2D:00:00:91
>>>> 5) show port 0 macs
>>>> Number of MAC address added: 4
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:90
>>>>     00:18:2D:00:00:91
>>>>     00:18:2D:00:00:92
>>>
>>> Please describe with words.
>>> Reading similar MAC addresses is not a fun game.
>>
>> I do not catch you, could you please be
>> more detailed, thanks.
> 
> Me too, I don't catch you.
> Please explain the problem in the commit log
> so we can understand without the example.
> 
>>>> This is due to the reason that if the address has been added as a
>>>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
>>>> from dev->data->mac_addrs[] when set default MAC address with the same
>>>> address.
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> 
> 
> 
> .
> 

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

* Re: [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-09  9:53       ` Min Hu (Connor)
@ 2021-10-11  9:02         ` Thomas Monjalon
  0 siblings, 0 replies; 40+ messages in thread
From: Thomas Monjalon @ 2021-10-11  9:02 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, ferruh.yigit, andrew.rybchenko

09/10/2021 11:53, Min Hu (Connor):
> Hi, Thomas,
> 
> The dev->data->mac_addrs[0] will be changed to a new MAC address when 
> applications modify
> the default MAC address by rte_eth_dev_default_mac_addr_set() API. 
> However, If the new default
> MAC address has been added as a non-default MAC address by 
> rte_eth_dev_mac_addr_add() API, the
> rte_eth_dev_default_mac_addr_set() API doesn't remove it from 
> dev->data->mac_addrs[].
> As a result, one MAC address occupies two index capacities in 
> dev->data->mac_addrs[].
> This patch adds the logic of removing MAC addresses for this scenario.
> 
> Is that will be more clear? Hope for your reply

Yes, that's the explanation I was expecting. Thank you!


> 在 2021/10/8 18:04, Thomas Monjalon 写道:
> > 08/10/2021 09:02, Min Hu (Connor):
> >> Hi, Thomas,
> >>
> >> 在 2021/10/6 3:21, Thomas Monjalon 写道:
> >>> 22/09/2021 05:36, Min Hu (Connor):
> >>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>
> >>>> Use the testpmd to perform the following operations:
> >>>> 1) mac_addr add 0 00:18:2D:00:00:90
> >>>> 2) mac_addr add 0 00:18:2D:00:00:91
> >>>> 3) mac_addr add 0 00:18:2D:00:00:92
> >>>> 4) mac_addr set 0 00:18:2D:00:00:91
> >>>> 5) show port 0 macs
> >>>> Number of MAC address added: 4
> >>>>     00:18:2D:00:00:91
> >>>>     00:18:2D:00:00:90
> >>>>     00:18:2D:00:00:91
> >>>>     00:18:2D:00:00:92
> >>>
> >>> Please describe with words.
> >>> Reading similar MAC addresses is not a fun game.
> >>
> >> I do not catch you, could you please be
> >> more detailed, thanks.
> > 
> > Me too, I don't catch you.
> > Please explain the problem in the commit log
> > so we can understand without the example.
> > 
> >>>> This is due to the reason that if the address has been added as a
> >>>> non-default MAC address by rte_eth_dev_mac_addr_add API, it doesn't remove
> >>>> from dev->data->mac_addrs[] when set default MAC address with the same
> >>>> address.
> >>>>
> >>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>






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

* [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-09-22  3:36 [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs Min Hu (Connor)
  2021-09-22  6:39 ` Andrew Rybchenko
  2021-10-05 19:21 ` Thomas Monjalon
@ 2021-10-11  9:28 ` Min Hu (Connor)
  2021-10-11 10:35   ` Thomas Monjalon
  2021-10-19 17:45   ` Ferruh Yigit
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
  3 siblings, 2 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2021-10-11  9:28 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, thomas

From: Huisong Li <lihuisong@huawei.com>

The dev->data->mac_addrs[0] will be changed to a new MAC address when
applications modify the default MAC address by
rte_eth_dev_default_mac_addr_set() API. However, If the new default
MAC address has been added as a non-default MAC address by
rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
address occupies two index capacities in dev->data->mac_addrs[].

This patch adds the logic of removing MAC addresses for this scenario.

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

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
v2:
* fixed commit log.
---
 lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 028907bc4b..7faff17d9a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4340,6 +4340,7 @@ int
 rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 {
 	struct rte_eth_dev *dev;
+	int index;
 	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
@@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * If the address has been added as a non-default MAC address by
+	 * rte_eth_dev_mac_addr_add API, it should be removed from
+	 * dev->data->mac_addrs[].
+	 */
+	index = eth_dev_get_mac_addr_index(port_id, addr);
+	if (index > 0) {
+		/* remove address in NIC data structure */
+		rte_ether_addr_copy(&null_mac_addr,
+				    &dev->data->mac_addrs[index]);
+		/* reset pool bitmap */
+		dev->data->mac_pool_sel[index] = 0;
+	}
+
 	/* Update default address in NIC data structure */
 	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
 
-- 
2.33.0


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2021-10-11 10:35   ` Thomas Monjalon
  2021-10-12  2:58     ` lihuisong (C)
  2021-10-19 17:45   ` Ferruh Yigit
  1 sibling, 1 reply; 40+ messages in thread
From: Thomas Monjalon @ 2021-10-11 10:35 UTC (permalink / raw)
  To: Min Hu (Connor); +Cc: dev, ferruh.yigit, andrew.rybchenko

11/10/2021 11:28, Min Hu (Connor):
> From: Huisong Li <lihuisong@huawei.com>
> 
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by
> rte_eth_dev_default_mac_addr_set() API. However, If the new default
> MAC address has been added as a non-default MAC address by
> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
> address occupies two index capacities in dev->data->mac_addrs[].
> 
> This patch adds the logic of removing MAC addresses for this scenario.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */

This is the definition of mac_addrs:

    struct rte_ether_addr *mac_addrs;
            /**< Device Ethernet link address.
             *   @see rte_eth_dev_release_port()
             */

I feel we need to explain there can be multiple addresses,
the first one being the default.

Another comment,
If we remove the duplicate, we may have to copy to previous default one
to avoid completely deleting the previous default address.
Not sure what should be the behaviour.



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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-11 10:35   ` Thomas Monjalon
@ 2021-10-12  2:58     ` lihuisong (C)
  2021-10-12  7:14       ` Thomas Monjalon
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2021-10-12  2:58 UTC (permalink / raw)
  To: Thomas Monjalon, Min Hu (Connor); +Cc: dev, ferruh.yigit, andrew.rybchenko


在 2021/10/11 18:35, Thomas Monjalon 写道:
> 11/10/2021 11:28, Min Hu (Connor):
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>> applications modify the default MAC address by
>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>> MAC address has been added as a non-default MAC address by
>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>> address occupies two index capacities in dev->data->mac_addrs[].
>>
>> This patch adds the logic of removing MAC addresses for this scenario.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> +	/*
>> +	 * If the address has been added as a non-default MAC address by
>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>> +	 * dev->data->mac_addrs[].
>> +	 */
> This is the definition of mac_addrs:
>
>      struct rte_ether_addr *mac_addrs;
>              /**< Device Ethernet link address.
>               *   @see rte_eth_dev_release_port()
>               */
>
> I feel we need to explain there can be multiple addresses,
> the first one being the default.

Do you mean that the problem mentioned in this patch is not a problem?

Should we accept this scenario?

>
> Another comment,
> If we remove the duplicate, we may have to copy to previous default one
> to avoid completely deleting the previous default address.

That's not necessary.

Because the previous default address has been removed from hardware.

After the default MAC address is modified, the previous default one is 
invalid.

> Not sure what should be the behaviour.
>
>
> .

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-12  2:58     ` lihuisong (C)
@ 2021-10-12  7:14       ` Thomas Monjalon
  2021-10-15  2:00         ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Thomas Monjalon @ 2021-10-12  7:14 UTC (permalink / raw)
  To: Min Hu (Connor), lihuisong (C); +Cc: dev, ferruh.yigit, andrew.rybchenko

12/10/2021 04:58, lihuisong (C):
> 在 2021/10/11 18:35, Thomas Monjalon 写道:
> > 11/10/2021 11:28, Min Hu (Connor):
> >> From: Huisong Li <lihuisong@huawei.com>
> >>
> >> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> >> applications modify the default MAC address by
> >> rte_eth_dev_default_mac_addr_set() API. However, If the new default
> >> MAC address has been added as a non-default MAC address by
> >> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
> >> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
> >> address occupies two index capacities in dev->data->mac_addrs[].
> >>
> >> This patch adds the logic of removing MAC addresses for this scenario.
> >>
> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> >> ---
> >> +	/*
> >> +	 * If the address has been added as a non-default MAC address by
> >> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> >> +	 * dev->data->mac_addrs[].
> >> +	 */
> > This is the definition of mac_addrs:
> >
> >      struct rte_ether_addr *mac_addrs;
> >              /**< Device Ethernet link address.
> >               *   @see rte_eth_dev_release_port()
> >               */
> >
> > I feel we need to explain there can be multiple addresses,
> > the first one being the default.
> 
> Do you mean that the problem mentioned in this patch is not a problem?

It is not a problem if it is expected,
but nothing is defined.

> Should we accept this scenario?

We need to decide what is the correct behaviour.
I see pros and cons on both sides.

> > Another comment,
> > If we remove the duplicate, we may have to copy to previous default one
> > to avoid completely deleting the previous default address.
> 
> That's not necessary.
> 
> Because the previous default address has been removed from hardware.
> 
> After the default MAC address is modified, the previous default one is 
> invalid.

No you don't get it.
With the current behaviour, the app could keep all the MAC addresses,
and copy one as the default while keeping it in the rest of the list.
You are suggesting a different behaviour where there is no duplicate.

> > Not sure what should be the behaviour.




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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-12  7:14       ` Thomas Monjalon
@ 2021-10-15  2:00         ` lihuisong (C)
  0 siblings, 0 replies; 40+ messages in thread
From: lihuisong (C) @ 2021-10-15  2:00 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, ferruh.yigit, andrew.rybchenko, Min Hu (Connor)


在 2021/10/12 15:14, Thomas Monjalon 写道:
> 12/10/2021 04:58, lihuisong (C):
>> 在 2021/10/11 18:35, Thomas Monjalon 写道:
>>> 11/10/2021 11:28, Min Hu (Connor):
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>> applications modify the default MAC address by
>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>> MAC address has been added as a non-default MAC address by
>>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>
>>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>> +	/*
>>>> +	 * If the address has been added as a non-default MAC address by
>>>> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
>>>> +	 * dev->data->mac_addrs[].
>>>> +	 */
>>> This is the definition of mac_addrs:
>>>
>>>       struct rte_ether_addr *mac_addrs;
>>>               /**< Device Ethernet link address.
>>>                *   @see rte_eth_dev_release_port()
>>>                */
>>>
>>> I feel we need to explain there can be multiple addresses,
>>> the first one being the default.
>> Do you mean that the problem mentioned in this patch is not a problem?
> It is not a problem if it is expected,
> but nothing is defined.
>
>> Should we accept this scenario?
> We need to decide what is the correct behaviour.
> I see pros and cons on both sides.
Where's the bright side of this behavior?
>>> Another comment,
>>> If we remove the duplicate, we may have to copy to previous default one
>>> to avoid completely deleting the previous default address.
>> That's not necessary.
>>
>> Because the previous default address has been removed from hardware.
>>
>> After the default MAC address is modified, the previous default one is
>> invalid.
> No you don't get it.
> With the current behaviour, the app could keep all the MAC addresses,
> and copy one as the default while keeping it in the rest of the list.
> You are suggesting a different behaviour where there is no duplicate.

This behavior seems to be contrary to the rte_eth_dev_mac_addr_add() API.

If the pmd does not support the dev_ops->mac_addr_set, the 
rte_eth_dev_mac_addr_add()

is used to set default MAC in ethdev layer. Namely, the 
rte_eth_dev_mac_addr_add() can

be used to set default MAC in the special scenario and add MAC address.

But the API does not allow the rest of the list to be the same as the 
default MAC which

occupies idx 0.


If the app uses the MAC address in dev->data->mac_addrs[] to set the 
default MAC,

the PMD actually has only one entry.  As far as I know, the 
dev->data->mac_addrs[] in

ethdev layer should be consistent with the hardware entry of the PMD.

So I think the behavior referred to in this patch is inappropriate.

>
>>> Not sure what should be the behaviour.
>
>
> .

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
  2021-10-11 10:35   ` Thomas Monjalon
@ 2021-10-19 17:45   ` Ferruh Yigit
  2021-10-20  6:49     ` lihuisong (C)
  1 sibling, 1 reply; 40+ messages in thread
From: Ferruh Yigit @ 2021-10-19 17:45 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: thomas

On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by
> rte_eth_dev_default_mac_addr_set() API. However, If the new default
> MAC address has been added as a non-default MAC address by
> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
> address occupies two index capacities in dev->data->mac_addrs[].
> 

Hi Connor,

I see the problem, but can you please clarify what is the impact to the end user?

If application does as following:
   rte_eth_dev_mac_addr_add(MAC1);
   rte_eth_dev_mac_addr_add(MAC2);
   rte_eth_dev_mac_addr_add(MAC3);
   rte_eth_dev_default_mac_addr_set(MAC2);

The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.

Will this cause any problem for the application to receive the packets
with 'MAC2' address?

Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
without any other impact to the application?

> This patch adds the logic of removing MAC addresses for this scenario.
> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v2:
> * fixed commit log.
> ---
>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 028907bc4b..7faff17d9a 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4340,6 +4340,7 @@ int
>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   {
>   	struct rte_eth_dev *dev;
> +	int index;
>   	int ret;
>   
>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   	if (ret < 0)
>   		return ret;
>   
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* remove address in NIC data structure */
> +		rte_ether_addr_copy(&null_mac_addr,
> +				    &dev->data->mac_addrs[index]);
> +		/* reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
> +

Here only 'dev->data->mac_addrs[]' array is updated and it assumes
driver removes similar duplication itself, but I am not sure if this is
valid for all drivers.

If driver is not removing the duplicate in the HW configuration, the driver
config and 'dev->data->mac_addrs[]' will diverge, which is not good.

What about following logic to be sure HW configuration and
'dev->data->mac_addrs[]' is same:

   index = eth_dev_get_mac_addr_index(port_id, addr);
   if (index > 0)
       rte_eth_dev_mac_addr_remove(port_id, addr);
   (*dev->dev_ops->mac_addr_set)(dev, addr);

>   	/* Update default address in NIC data structure */
>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>   
> 


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-19 17:45   ` Ferruh Yigit
@ 2021-10-20  6:49     ` lihuisong (C)
  2021-10-20  7:41       ` Ferruh Yigit
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2021-10-20  6:49 UTC (permalink / raw)
  To: Ferruh Yigit, Min Hu (Connor), dev; +Cc: thomas

Hi Ferruh

在 2021/10/20 1:45, Ferruh Yigit 写道:
> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>> applications modify the default MAC address by
>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>> MAC address has been added as a non-default MAC address by
>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>> address occupies two index capacities in dev->data->mac_addrs[].
>>
>
> Hi Connor,
>
> I see the problem, but can you please clarify what is the impact to 
> the end user?
>
> If application does as following:
>   rte_eth_dev_mac_addr_add(MAC1);
>   rte_eth_dev_mac_addr_add(MAC2);
>   rte_eth_dev_mac_addr_add(MAC3);
>   rte_eth_dev_default_mac_addr_set(MAC2);
>
> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 
> 'MAC2' duplicated.
>
> Will this cause any problem for the application to receive the packets
> with 'MAC2' address?
> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
> without any other impact to the application?
I think it's just a waste of space.
>
>> This patch adds the logic of removing MAC addresses for this scenario.
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>> v2:
>> * fixed commit log.
>> ---
>>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 028907bc4b..7faff17d9a 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -4340,6 +4340,7 @@ int
>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>> rte_ether_addr *addr)
>>   {
>>       struct rte_eth_dev *dev;
>> +    int index;
>>       int ret;
>>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t 
>> port_id, struct rte_ether_addr *addr)
>>       if (ret < 0)
>>           return ret;
>>   +    /*
>> +     * If the address has been added as a non-default MAC address by
>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>> +     * dev->data->mac_addrs[].
>> +     */
>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>> +    if (index > 0) {
>> +        /* remove address in NIC data structure */
>> +        rte_ether_addr_copy(&null_mac_addr,
>> +                    &dev->data->mac_addrs[index]);
>> +        /* reset pool bitmap */
>> +        dev->data->mac_pool_sel[index] = 0;
>> +    }
>> +
>
> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
> driver removes similar duplication itself, but I am not sure if this is
> valid for all drivers.
>
> If driver is not removing the duplicate in the HW configuration, the 
> driver
> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
The same MAC address does not occupy two HW entries, which is also a
waste for HW. After all, HW entry resources are also limited.
The PMD should also take this into account.
So, I think, we don't have to think about it here.
>
> What about following logic to be sure HW configuration and
> 'dev->data->mac_addrs[]' is same:
>
>   index = eth_dev_get_mac_addr_index(port_id, addr);
>   if (index > 0)
> rte_eth_dev_mac_addr_remove(port_id, addr);
>   (*dev->dev_ops->mac_addr_set)(dev, addr);
The logic above seems to be good. But if .mac_addr_set() failed to
execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'.
It's not good.

Hope for your reply.  Thanks.
>>       /* Update default address in NIC data structure */
>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>
>
> .

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-20  6:49     ` lihuisong (C)
@ 2021-10-20  7:41       ` Ferruh Yigit
  2021-10-20 10:15         ` Kevin Traynor
  0 siblings, 1 reply; 40+ messages in thread
From: Ferruh Yigit @ 2021-10-20  7:41 UTC (permalink / raw)
  To: lihuisong (C), Min Hu (Connor), dev; +Cc: thomas

On 10/20/2021 7:49 AM, lihuisong (C) wrote:
> Hi Ferruh
> 
> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>> applications modify the default MAC address by
>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>> MAC address has been added as a non-default MAC address by
>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>
>>
>> Hi Connor,
>>
>> I see the problem, but can you please clarify what is the impact to the end user?
>>
>> If application does as following:
>>   rte_eth_dev_mac_addr_add(MAC1);
>>   rte_eth_dev_mac_addr_add(MAC2);
>>   rte_eth_dev_mac_addr_add(MAC3);
>>   rte_eth_dev_default_mac_addr_set(MAC2);
>>
>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.
>>
>> Will this cause any problem for the application to receive the packets
>> with 'MAC2' address?
>> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
>> without any other impact to the application?
> I think it's just a waste of space.

True, it is a waste. But if there is no other visible user impact, we can
handle the issue with lower priority and clarifying the impact in commit log
helps to others.

>>
>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>> v2:
>>> * fixed commit log.
>>> ---
>>>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index 028907bc4b..7faff17d9a 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -4340,6 +4340,7 @@ int
>>>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>   {
>>>       struct rte_eth_dev *dev;
>>> +    int index;
>>>       int ret;
>>>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>       if (ret < 0)
>>>           return ret;
>>>   +    /*
>>> +     * If the address has been added as a non-default MAC address by
>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>> +     * dev->data->mac_addrs[].
>>> +     */
>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>> +    if (index > 0) {
>>> +        /* remove address in NIC data structure */
>>> +        rte_ether_addr_copy(&null_mac_addr,
>>> +                    &dev->data->mac_addrs[index]);
>>> +        /* reset pool bitmap */
>>> +        dev->data->mac_pool_sel[index] = 0;
>>> +    }
>>> +
>>
>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>> driver removes similar duplication itself, but I am not sure if this is
>> valid for all drivers.
>>
>> If driver is not removing the duplicate in the HW configuration, the driver
>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
> The same MAC address does not occupy two HW entries, which is also a
> waste for HW. After all, HW entry resources are also limited.
> The PMD should also take this into account.
> So, I think, we don't have to think about it here.

I am not sure all PMD take this into account, I briefly checked the ixgbe
code and I am not sure if it handles this.

Also it is possible to think that this responsibility is pushed to the
application, like application should remove a MAC address before setting
it as default MAC...

>>
>> What about following logic to be sure HW configuration and
>> 'dev->data->mac_addrs[]' is same:
>>
>>   index = eth_dev_get_mac_addr_index(port_id, addr);
>>   if (index > 0)
>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>   (*dev->dev_ops->mac_addr_set)(dev, addr);
> The logic above seems to be good. But if .mac_addr_set() failed to
> execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'.
> It's not good.
> 

Agree. So may need to get a copy of 'addr' and add it back on failure.

The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
'dev_ops->mac_addr_set()' is, it may result different behavior on
different PMDs.
For the PMDs that clean the redundant MAC address via 'dev_ops->mac_addr_set()'
may try to remove (although it will fail) the new set default MAC address.
That is why first remove the MAC and later add it back as default
seems safer to me.

> Hope for your reply.  Thanks.
>>>       /* Update default address in NIC data structure */
>>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>
>>
>> .


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-20  7:41       ` Ferruh Yigit
@ 2021-10-20 10:15         ` Kevin Traynor
  2021-10-20 16:32           ` Ferruh Yigit
  0 siblings, 1 reply; 40+ messages in thread
From: Kevin Traynor @ 2021-10-20 10:15 UTC (permalink / raw)
  To: Ferruh Yigit, lihuisong (C), Min Hu (Connor), dev; +Cc: thomas

On 20/10/2021 08:41, Ferruh Yigit wrote:
> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>> Hi Ferruh
>>
>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>> applications modify the default MAC address by
>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>> MAC address has been added as a non-default MAC address by
>>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>
>>>
>>> Hi Connor,
>>>
>>> I see the problem, but can you please clarify what is the impact to the end user?
>>>
>>> If application does as following:
>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>
>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.
>>>
>>> Will this cause any problem for the application to receive the packets
>>> with 'MAC2' address?
>>> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
>>> without any other impact to the application?
>> I think it's just a waste of space.
> 
> True, it is a waste. But if there is no other visible user impact, we can
> handle the issue with lower priority and clarifying the impact in commit log
> helps to others.
> 
>>>
>>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>> v2:
>>>> * fixed commit log.
>>>> ---
>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index 028907bc4b..7faff17d9a 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -4340,6 +4340,7 @@ int
>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>    {
>>>>        struct rte_eth_dev *dev;
>>>> +    int index;
>>>>        int ret;
>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>        if (ret < 0)
>>>>            return ret;
>>>>    +    /*
>>>> +     * If the address has been added as a non-default MAC address by
>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>> +     * dev->data->mac_addrs[].
>>>> +     */
>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>> +    if (index > 0) {
>>>> +        /* remove address in NIC data structure */
>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>> +                    &dev->data->mac_addrs[index]);
>>>> +        /* reset pool bitmap */
>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>> +    }
>>>> +
>>>
>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>> driver removes similar duplication itself, but I am not sure if this is
>>> valid for all drivers.
>>>
>>> If driver is not removing the duplicate in the HW configuration, the driver
>>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
>> The same MAC address does not occupy two HW entries, which is also a
>> waste for HW. After all, HW entry resources are also limited.
>> The PMD should also take this into account.
>> So, I think, we don't have to think about it here.
> 
> I am not sure all PMD take this into account, I briefly checked the ixgbe
> code and I am not sure if it handles this.
> 
> Also it is possible to think that this responsibility is pushed to the
> application, like application should remove a MAC address before setting
> it as default MAC...
> 

Yes, the API view is more important than saving one entry in an array. 
 From API perspective with this patch,

set_default(MAC1)
add(MAC2)
add(MAC3)
add(MAC4)
default=MAC1, Filters=MAC2, MAC3, MAC4

set_default(MAC2)
default=MAC2, Filters= MAC3, MAC4

set_default(MAC3)
default=MAC3, Filters= MAC4

set_default(MAC4)
default=MAC4, Filters=

set_default(MAC5)
default=MAC5, Filters=

Did I get it right? If so, it seems wrong to silently remove the 
filters. In which case, it would be easier to just not remove them in 
the first place (current behaviour).

If they really need to be removed from the filter list when they are 
set_default(), then perhaps they should be restored to it when there is 
a new set_default().

>>>
>>> What about following logic to be sure HW configuration and
>>> 'dev->data->mac_addrs[]' is same:
>>>
>>>    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>    if (index > 0)
>>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>>    (*dev->dev_ops->mac_addr_set)(dev, addr);
>> The logic above seems to be good. But if .mac_addr_set() failed to
>> execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'.
>> It's not good.
>>
> 
> Agree. So may need to get a copy of 'addr' and add it back on failure.
> 
> The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
> 'dev_ops->mac_addr_set()' is, it may result different behavior on
> different PMDs.
> For the PMDs that clean the redundant MAC address via 'dev_ops->mac_addr_set()'
> may try to remove (although it will fail) the new set default MAC address.
> That is why first remove the MAC and later add it back as default
> seems safer to me.
> 
>> Hope for your reply.  Thanks.
>>>>        /* Update default address in NIC data structure */
>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>
>>>
>>> .
> 


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-20 10:15         ` Kevin Traynor
@ 2021-10-20 16:32           ` Ferruh Yigit
  2021-10-21  2:05             ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Ferruh Yigit @ 2021-10-20 16:32 UTC (permalink / raw)
  To: Kevin Traynor, lihuisong (C), Min Hu (Connor), dev; +Cc: thomas

On 10/20/2021 11:15 AM, Kevin Traynor wrote:
> On 20/10/2021 08:41, Ferruh Yigit wrote:
>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>> Hi Ferruh
>>>
>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>
>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>>> applications modify the default MAC address by
>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>>> MAC address has been added as a non-default MAC address by
>>>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>
>>>>
>>>> Hi Connor,
>>>>
>>>> I see the problem, but can you please clarify what is the impact to the end user?
>>>>
>>>> If application does as following:
>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>
>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.
>>>>
>>>> Will this cause any problem for the application to receive the packets
>>>> with 'MAC2' address?
>>>> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
>>>> without any other impact to the application?
>>> I think it's just a waste of space.
>>
>> True, it is a waste. But if there is no other visible user impact, we can
>> handle the issue with lower priority and clarifying the impact in commit log
>> helps to others.
>>
>>>>
>>>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>>>
>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> ---
>>>>> v2:
>>>>> * fixed commit log.
>>>>> ---
>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>    1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>> index 028907bc4b..7faff17d9a 100644
>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>    {
>>>>>        struct rte_eth_dev *dev;
>>>>> +    int index;
>>>>>        int ret;
>>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>        if (ret < 0)
>>>>>            return ret;
>>>>>    +    /*
>>>>> +     * If the address has been added as a non-default MAC address by
>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>> +     * dev->data->mac_addrs[].
>>>>> +     */
>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>> +    if (index > 0) {
>>>>> +        /* remove address in NIC data structure */
>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>> +                    &dev->data->mac_addrs[index]);
>>>>> +        /* reset pool bitmap */
>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>> +    }
>>>>> +
>>>>
>>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>>> driver removes similar duplication itself, but I am not sure if this is
>>>> valid for all drivers.
>>>>
>>>> If driver is not removing the duplicate in the HW configuration, the driver
>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
>>> The same MAC address does not occupy two HW entries, which is also a
>>> waste for HW. After all, HW entry resources are also limited.
>>> The PMD should also take this into account.
>>> So, I think, we don't have to think about it here.
>>
>> I am not sure all PMD take this into account, I briefly checked the ixgbe
>> code and I am not sure if it handles this.
>>
>> Also it is possible to think that this responsibility is pushed to the
>> application, like application should remove a MAC address before setting
>> it as default MAC...
>>
> 
> Yes, the API view is more important than saving one entry in an array. From API perspective with this patch,
> 
> set_default(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> default=MAC1, Filters=MAC2, MAC3, MAC4
> 
> set_default(MAC2)
> default=MAC2, Filters= MAC3, MAC4
> 
> set_default(MAC3)
> default=MAC3, Filters= MAC4
> 
> set_default(MAC4)
> default=MAC4, Filters=
> 
> set_default(MAC5)
> default=MAC5, Filters=
> 
> Did I get it right? If so, it seems wrong to silently remove the filters. In which case, it would be easier to just not remove them in the first place (current behaviour).
> 

Yep, this is the updated behavior. And agree it looks wrong when you
show like this. (btw, this is only ethdev record of MAC filters, what
is updated in this patch, HW still may be keeping all filters.)

> If they really need to be removed from the filter list when they are set_default(), then perhaps they should be restored to it when there is a new set_default().
> 

I am for keeping current behavior. Application always can explicitly remove a
MAC filter before setting it default if required.


>>>>
>>>> What about following logic to be sure HW configuration and
>>>> 'dev->data->mac_addrs[]' is same:
>>>>
>>>>    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>    if (index > 0)
>>>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>>>    (*dev->dev_ops->mac_addr_set)(dev, addr);
>>> The logic above seems to be good. But if .mac_addr_set() failed to
>>> execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'.
>>> It's not good.
>>>
>>
>> Agree. So may need to get a copy of 'addr' and add it back on failure.
>>
>> The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
>> 'dev_ops->mac_addr_set()' is, it may result different behavior on
>> different PMDs.
>> For the PMDs that clean the redundant MAC address via 'dev_ops->mac_addr_set()'
>> may try to remove (although it will fail) the new set default MAC address.
>> That is why first remove the MAC and later add it back as default
>> seems safer to me.
>>
>>> Hope for your reply.  Thanks.
>>>>>        /* Update default address in NIC data structure */
>>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>>
>>>>
>>>> .
>>
> 


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-20 16:32           ` Ferruh Yigit
@ 2021-10-21  2:05             ` lihuisong (C)
  2021-10-21  8:30               ` Ferruh Yigit
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2021-10-21  2:05 UTC (permalink / raw)
  To: Ferruh Yigit, Kevin Traynor, Min Hu (Connor), dev; +Cc: thomas


在 2021/10/21 0:32, Ferruh Yigit 写道:
> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>> Hi Ferruh
>>>>
>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address 
>>>>>> when
>>>>>> applications modify the default MAC address by
>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>>>> MAC address has been added as a non-default MAC address by
>>>>>> rte_eth_dev_mac_addr_add() API, the 
>>>>>> rte_eth_dev_default_mac_addr_set()
>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, 
>>>>>> one MAC
>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>
>>>>>
>>>>> Hi Connor,
>>>>>
>>>>> I see the problem, but can you please clarify what is the impact 
>>>>> to the end user?
>>>>>
>>>>> If application does as following:
>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>
>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 
>>>>> 'MAC2' duplicated.
>>>>>
>>>>> Will this cause any problem for the application to receive the 
>>>>> packets
>>>>> with 'MAC2' address?
>>>>> Or is the only problem one extra space used in 
>>>>> 'dev->data->mac_addrs[]'
>>>>> without any other impact to the application?
>>>> I think it's just a waste of space.
>>>
>>> True, it is a waste. But if there is no other visible user impact, 
>>> we can
>>> handle the issue with lower priority and clarifying the impact in 
>>> commit log
>>> helps to others.
>>>
>>>>>
>>>>>> This patch adds the logic of removing MAC addresses for this 
>>>>>> scenario.
>>>>>>
>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>> ---
>>>>>> v2:
>>>>>> * fixed commit log.
>>>>>> ---
>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>    1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>> rte_ether_addr *addr)
>>>>>>    {
>>>>>>        struct rte_eth_dev *dev;
>>>>>> +    int index;
>>>>>>        int ret;
>>>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t 
>>>>>> port_id, struct rte_ether_addr *addr)
>>>>>>        if (ret < 0)
>>>>>>            return ret;
>>>>>>    +    /*
>>>>>> +     * If the address has been added as a non-default MAC 
>>>>>> address by
>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>> +     * dev->data->mac_addrs[].
>>>>>> +     */
>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>> +    if (index > 0) {
>>>>>> +        /* remove address in NIC data structure */
>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>> + &dev->data->mac_addrs[index]);
>>>>>> +        /* reset pool bitmap */
>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>> +    }
>>>>>> +
>>>>>
>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>>>> driver removes similar duplication itself, but I am not sure if 
>>>>> this is
>>>>> valid for all drivers.
>>>>>
>>>>> If driver is not removing the duplicate in the HW configuration, 
>>>>> the driver
>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
>>>> The same MAC address does not occupy two HW entries, which is also a
>>>> waste for HW. After all, HW entry resources are also limited.
>>>> The PMD should also take this into account.
>>>> So, I think, we don't have to think about it here.
>>>
>>> I am not sure all PMD take this into account, I briefly checked the 
>>> ixgbe
>>> code and I am not sure if it handles this.
>>>
>>> Also it is possible to think that this responsibility is pushed to the
>>> application, like application should remove a MAC address before 
>>> setting
>>> it as default MAC...
>>>
>>
>> Yes, the API view is more important than saving one entry in an 
>> array. From API perspective with this patch,
>>
>> set_default(MAC1)
>> add(MAC2)
>> add(MAC3)
>> add(MAC4)
>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>
>> set_default(MAC2)
>> default=MAC2, Filters= MAC3, MAC4
>>
>> set_default(MAC3)
>> default=MAC3, Filters= MAC4
>>
>> set_default(MAC4)
>> default=MAC4, Filters=
>>
>> set_default(MAC5)
>> default=MAC5, Filters=
>>
>> Did I get it right? If so, it seems wrong to silently remove the 
>> filters. In which case, it would be easier to just not remove them in 
>> the first place (current behaviour).
>>
>
> Yep, this is the updated behavior. And agree it looks wrong when you
> show like this. (btw, this is only ethdev record of MAC filters, what
> is updated in this patch, HW still may be keeping all filters.)

Whether HW saves all filters depends on the implementation of the 
set_default()

in the driver. According to the implementation of this API of all PMDs, 
some drivers

will first remove the old default MAC in HW and then add the new one 
when calling

the set_default(). I am not sure if the HW that didn't do this would 
remove the old

default MAC. If not, we may need to standardize this API in the ethdev 
layer.

>
>> If they really need to be removed from the filter list when they are 
>> set_default(), then perhaps they should be restored to it when there 
>> is a new set_default().
>>
>
> I am for keeping current behavior. Application always can explicitly 
> remove a
> MAC filter before setting it default if required.

But application can not remove the duplicate MAC if the MAC is the 
current default

MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed to 
remove.

>
>
>>>>>
>>>>> What about following logic to be sure HW configuration and
>>>>> 'dev->data->mac_addrs[]' is same:
>>>>>
>>>>>    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>    if (index > 0)
>>>>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>>>>    (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>> The logic above seems to be good. But if .mac_addr_set() failed to
>>>> execute, the addr has been removed from HW and 
>>>> 'dev->data->mac_addrs[]'.
>>>> It's not good.
>>>>
>>>
>>> Agree. So may need to get a copy of 'addr' and add it back on failure.
>>>
>>> The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
>>> 'dev_ops->mac_addr_set()' is, it may result different behavior on
>>> different PMDs.
>>> For the PMDs that clean the redundant MAC address via 
>>> 'dev_ops->mac_addr_set()'
>>> may try to remove (although it will fail) the new set default MAC 
>>> address.
>>> That is why first remove the MAC and later add it back as default
>>> seems safer to me.
>>>
>>>> Hope for your reply.  Thanks.
>>>>>>        /* Update default address in NIC data structure */
>>>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>>>
>>>>>
>>>>> .
>>>
>>
>
> .

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-21  2:05             ` lihuisong (C)
@ 2021-10-21  8:30               ` Ferruh Yigit
  2021-10-22  2:04                 ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Ferruh Yigit @ 2021-10-21  8:30 UTC (permalink / raw)
  To: lihuisong (C), Kevin Traynor, Min Hu (Connor), dev; +Cc: thomas

On 10/21/2021 3:05 AM, lihuisong (C) wrote:
> 
> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>> Hi Ferruh
>>>>>
>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>>
>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>>>>> applications modify the default MAC address by
>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>>>>> MAC address has been added as a non-default MAC address by
>>>>>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>>
>>>>>>
>>>>>> Hi Connor,
>>>>>>
>>>>>> I see the problem, but can you please clarify what is the impact to the end user?
>>>>>>
>>>>>> If application does as following:
>>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>>
>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.
>>>>>>
>>>>>> Will this cause any problem for the application to receive the packets
>>>>>> with 'MAC2' address?
>>>>>> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
>>>>>> without any other impact to the application?
>>>>> I think it's just a waste of space.
>>>>
>>>> True, it is a waste. But if there is no other visible user impact, we can
>>>> handle the issue with lower priority and clarifying the impact in commit log
>>>> helps to others.
>>>>
>>>>>>
>>>>>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>>>>>
>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>> ---
>>>>>>> v2:
>>>>>>> * fixed commit log.
>>>>>>> ---
>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>>    1 file changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>>>    {
>>>>>>>        struct rte_eth_dev *dev;
>>>>>>> +    int index;
>>>>>>>        int ret;
>>>>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>>>        if (ret < 0)
>>>>>>>            return ret;
>>>>>>>    +    /*
>>>>>>> +     * If the address has been added as a non-default MAC address by
>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>>> +     * dev->data->mac_addrs[].
>>>>>>> +     */
>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>> +    if (index > 0) {
>>>>>>> +        /* remove address in NIC data structure */
>>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>>> + &dev->data->mac_addrs[index]);
>>>>>>> +        /* reset pool bitmap */
>>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>>> +    }
>>>>>>> +
>>>>>>
>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>>>>> driver removes similar duplication itself, but I am not sure if this is
>>>>>> valid for all drivers.
>>>>>>
>>>>>> If driver is not removing the duplicate in the HW configuration, the driver
>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
>>>>> The same MAC address does not occupy two HW entries, which is also a
>>>>> waste for HW. After all, HW entry resources are also limited.
>>>>> The PMD should also take this into account.
>>>>> So, I think, we don't have to think about it here.
>>>>
>>>> I am not sure all PMD take this into account, I briefly checked the ixgbe
>>>> code and I am not sure if it handles this.
>>>>
>>>> Also it is possible to think that this responsibility is pushed to the
>>>> application, like application should remove a MAC address before setting
>>>> it as default MAC...
>>>>
>>>
>>> Yes, the API view is more important than saving one entry in an array. From API perspective with this patch,
>>>
>>> set_default(MAC1)
>>> add(MAC2)
>>> add(MAC3)
>>> add(MAC4)
>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>
>>> set_default(MAC2)
>>> default=MAC2, Filters= MAC3, MAC4
>>>
>>> set_default(MAC3)
>>> default=MAC3, Filters= MAC4
>>>
>>> set_default(MAC4)
>>> default=MAC4, Filters=
>>>
>>> set_default(MAC5)
>>> default=MAC5, Filters=
>>>
>>> Did I get it right? If so, it seems wrong to silently remove the filters. In which case, it would be easier to just not remove them in the first place (current behaviour).
>>>
>>
>> Yep, this is the updated behavior. And agree it looks wrong when you
>> show like this. (btw, this is only ethdev record of MAC filters, what
>> is updated in this patch, HW still may be keeping all filters.)
> 
> Whether HW saves all filters depends on the implementation of the set_default()
> 
> in the driver. According to the implementation of this API of all PMDs, some drivers
> 
> will first remove the old default MAC in HW and then add the new one when calling
> 
> the set_default(). I am not sure if the HW that didn't do this would remove the old
> 
> default MAC. If not, we may need to standardize this API in the ethdev layer.
> 
>>
>>> If they really need to be removed from the filter list when they are set_default(), then perhaps they should be restored to it when there is a new set_default().
>>>
>>
>> I am for keeping current behavior. Application always can explicitly remove a
>> MAC filter before setting it default if required.
> 
> But application can not remove the duplicate MAC if the MAC is the current default
> 
> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed to remove.
> 

But can do other-way around, first remove (the non default one), later
'rte_eth_dev_default_mac_addr_set()'.

>>
>>
>>>>>>
>>>>>> What about following logic to be sure HW configuration and
>>>>>> 'dev->data->mac_addrs[]' is same:
>>>>>>
>>>>>>    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>    if (index > 0)
>>>>>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>>>>>    (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>>> The logic above seems to be good. But if .mac_addr_set() failed to
>>>>> execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'.
>>>>> It's not good.
>>>>>
>>>>
>>>> Agree. So may need to get a copy of 'addr' and add it back on failure.
>>>>
>>>> The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
>>>> 'dev_ops->mac_addr_set()' is, it may result different behavior on
>>>> different PMDs.
>>>> For the PMDs that clean the redundant MAC address via 'dev_ops->mac_addr_set()'
>>>> may try to remove (although it will fail) the new set default MAC address.
>>>> That is why first remove the MAC and later add it back as default
>>>> seems safer to me.
>>>>
>>>>> Hope for your reply.  Thanks.
>>>>>>>        /* Update default address in NIC data structure */
>>>>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>>>>
>>>>>>
>>>>>> .
>>>>
>>>
>>
>> .


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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-21  8:30               ` Ferruh Yigit
@ 2021-10-22  2:04                 ` lihuisong (C)
  2021-10-26 10:21                   ` Ferruh Yigit
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2021-10-22  2:04 UTC (permalink / raw)
  To: Ferruh Yigit, Kevin Traynor, Min Hu (Connor), dev; +Cc: thomas


在 2021/10/21 16:30, Ferruh Yigit 写道:
> On 10/21/2021 3:05 AM, lihuisong (C) wrote:
>>
>> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>>> Hi Ferruh
>>>>>>
>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>>>
>>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC 
>>>>>>>> address when
>>>>>>>> applications modify the default MAC address by
>>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new 
>>>>>>>> default
>>>>>>>> MAC address has been added as a non-default MAC address by
>>>>>>>> rte_eth_dev_mac_addr_add() API, the 
>>>>>>>> rte_eth_dev_default_mac_addr_set()
>>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, 
>>>>>>>> one MAC
>>>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>>>
>>>>>>>
>>>>>>> Hi Connor,
>>>>>>>
>>>>>>> I see the problem, but can you please clarify what is the impact 
>>>>>>> to the end user?
>>>>>>>
>>>>>>> If application does as following:
>>>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>>>
>>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which 
>>>>>>> has 'MAC2' duplicated.
>>>>>>>
>>>>>>> Will this cause any problem for the application to receive the 
>>>>>>> packets
>>>>>>> with 'MAC2' address?
>>>>>>> Or is the only problem one extra space used in 
>>>>>>> 'dev->data->mac_addrs[]'
>>>>>>> without any other impact to the application?
>>>>>> I think it's just a waste of space.
>>>>>
>>>>> True, it is a waste. But if there is no other visible user impact, 
>>>>> we can
>>>>> handle the issue with lower priority and clarifying the impact in 
>>>>> commit log
>>>>> helps to others.
>>>>>
>>>>>>>
>>>>>>>> This patch adds the logic of removing MAC addresses for this 
>>>>>>>> scenario.
>>>>>>>>
>>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>> * fixed commit log.
>>>>>>>> ---
>>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>>>    1 file changed, 15 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>> rte_ether_addr *addr)
>>>>>>>>    {
>>>>>>>>        struct rte_eth_dev *dev;
>>>>>>>> +    int index;
>>>>>>>>        int ret;
>>>>>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>>>> @@ -4361,6 +4362,20 @@ 
>>>>>>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>> rte_ether_addr *addr)
>>>>>>>>        if (ret < 0)
>>>>>>>>            return ret;
>>>>>>>>    +    /*
>>>>>>>> +     * If the address has been added as a non-default MAC 
>>>>>>>> address by
>>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>>>> +     * dev->data->mac_addrs[].
>>>>>>>> +     */
>>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>>> +    if (index > 0) {
>>>>>>>> +        /* remove address in NIC data structure */
>>>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>>>> + &dev->data->mac_addrs[index]);
>>>>>>>> +        /* reset pool bitmap */
>>>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>
>>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>>>>>> driver removes similar duplication itself, but I am not sure if 
>>>>>>> this is
>>>>>>> valid for all drivers.
>>>>>>>
>>>>>>> If driver is not removing the duplicate in the HW configuration, 
>>>>>>> the driver
>>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not 
>>>>>>> good.
>>>>>> The same MAC address does not occupy two HW entries, which is also a
>>>>>> waste for HW. After all, HW entry resources are also limited.
>>>>>> The PMD should also take this into account.
>>>>>> So, I think, we don't have to think about it here.
>>>>>
>>>>> I am not sure all PMD take this into account, I briefly checked 
>>>>> the ixgbe
>>>>> code and I am not sure if it handles this.
>>>>>
>>>>> Also it is possible to think that this responsibility is pushed to 
>>>>> the
>>>>> application, like application should remove a MAC address before 
>>>>> setting
>>>>> it as default MAC...
>>>>>
>>>>
>>>> Yes, the API view is more important than saving one entry in an 
>>>> array. From API perspective with this patch,
>>>>
>>>> set_default(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>>
>>>> set_default(MAC2)
>>>> default=MAC2, Filters= MAC3, MAC4
>>>>
>>>> set_default(MAC3)
>>>> default=MAC3, Filters= MAC4
>>>>
>>>> set_default(MAC4)
>>>> default=MAC4, Filters=
>>>>
>>>> set_default(MAC5)
>>>> default=MAC5, Filters=
>>>>
>>>> Did I get it right? If so, it seems wrong to silently remove the 
>>>> filters. In which case, it would be easier to just not remove them 
>>>> in the first place (current behaviour).
>>>>
>>>
>>> Yep, this is the updated behavior. And agree it looks wrong when you
>>> show like this. (btw, this is only ethdev record of MAC filters, what
>>> is updated in this patch, HW still may be keeping all filters.)
>>
>> Whether HW saves all filters depends on the implementation of the 
>> set_default()
>>
>> in the driver. According to the implementation of this API of all 
>> PMDs, some drivers
>>
>> will first remove the old default MAC in HW and then add the new one 
>> when calling
>>
>> the set_default(). I am not sure if the HW that didn't do this would 
>> remove the old
>>
>> default MAC. If not, we may need to standardize this API in the 
>> ethdev layer.
>>
>>>
>>>> If they really need to be removed from the filter list when they 
>>>> are set_default(), then perhaps they should be restored to it when 
>>>> there is a new set_default().
>>>>
>>>
>>> I am for keeping current behavior. Application always can explicitly 
>>> remove a
>>> MAC filter before setting it default if required.
>>
>> But application can not remove the duplicate MAC if the MAC is the 
>> current default
>>
>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed to 
>> remove.
>>
>
> But can do other-way around, first remove (the non default one), later
> 'rte_eth_dev_default_mac_addr_set()'.

This introduces a usage dependency on the user. We don't have a 
statement in some place

for the dependency. What's more, if the user does not follow this 
dependency, the user

will no longer be able to remove the MAC.

So it may be more appropriate to deal with problem in ethdev layer.


**Scheme A:**

The decision is left to the user, but there are usage dependency and 
irremovable possibility.**
**

*Scheme B:*

index = eth_dev_get_mac_addr_index(port_id, addr);
if (index > 0) {
     mac_pool_sel_bk = dev->data->mac_pool_sel[index];
     rte_eth_dev_mac_addr_remove(port_id, addr);
}
ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
if (ret < 0) {
     if (index > 0) {
         rte_eth_dev_mac_addr_add(port_id, addr, 0);
         dev->data->mac_pool_sel[index] = mac_pool_sel_bk;
     }

     return ret;
}

* Scheme C:*

  Use the method in this patch. It assumes that the driver has only one 
HW entry for a MAC.

What do you think we should do?

>
>>>
>>>
>>>>>>>
>>>>>>> What about following logic to be sure HW configuration and
>>>>>>> 'dev->data->mac_addrs[]' is same:
>>>>>>>
>>>>>>>    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>>    if (index > 0)
>>>>>>> rte_eth_dev_mac_addr_remove(port_id, addr);
>>>>>>>    (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>>>> The logic above seems to be good. But if .mac_addr_set() failed to
>>>>>> execute, the addr has been removed from HW and 
>>>>>> 'dev->data->mac_addrs[]'.
>>>>>> It's not good.
>>>>>>
>>>>>
>>>>> Agree. So may need to get a copy of 'addr' and add it back on 
>>>>> failure.
>>>>>
>>>>> The concern I have to call 'rte_eth_dev_mac_addr_remove()' after
>>>>> 'dev_ops->mac_addr_set()' is, it may result different behavior on
>>>>> different PMDs.
>>>>> For the PMDs that clean the redundant MAC address via 
>>>>> 'dev_ops->mac_addr_set()'
>>>>> may try to remove (although it will fail) the new set default MAC 
>>>>> address.
>>>>> That is why first remove the MAC and later add it back as default
>>>>> seems safer to me.
>>>>>
>>>>>> Hope for your reply.  Thanks.
>>>>>>>>        /* Update default address in NIC data structure */
>>>>>>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>>>>>
>>>>>>>
>>>>>>> .
>>>>>
>>>>
>>>
>>> .
>
> .

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-22  2:04                 ` lihuisong (C)
@ 2021-10-26 10:21                   ` Ferruh Yigit
  2021-11-08  6:55                     ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Ferruh Yigit @ 2021-10-26 10:21 UTC (permalink / raw)
  To: lihuisong (C), Kevin Traynor, Min Hu (Connor), dev; +Cc: thomas

On 10/22/2021 3:04 AM, lihuisong (C) wrote:
> 
> 在 2021/10/21 16:30, Ferruh Yigit 写道:
>> On 10/21/2021 3:05 AM, lihuisong (C) wrote:
>>>
>>> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>>>> Hi Ferruh
>>>>>>>
>>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>>>>
>>>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>>>>>>> applications modify the default MAC address by
>>>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new default
>>>>>>>>> MAC address has been added as a non-default MAC address by
>>>>>>>>> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set()
>>>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC
>>>>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Connor,
>>>>>>>>
>>>>>>>> I see the problem, but can you please clarify what is the impact to the end user?
>>>>>>>>
>>>>>>>> If application does as following:
>>>>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>>>>
>>>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has 'MAC2' duplicated.
>>>>>>>>
>>>>>>>> Will this cause any problem for the application to receive the packets
>>>>>>>> with 'MAC2' address?
>>>>>>>> Or is the only problem one extra space used in 'dev->data->mac_addrs[]'
>>>>>>>> without any other impact to the application?
>>>>>>> I think it's just a waste of space.
>>>>>>
>>>>>> True, it is a waste. But if there is no other visible user impact, we can
>>>>>> handle the issue with lower priority and clarifying the impact in commit log
>>>>>> helps to others.
>>>>>>
>>>>>>>>
>>>>>>>>> This patch adds the logic of removing MAC addresses for this scenario.
>>>>>>>>>
>>>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>
>>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>>> ---
>>>>>>>>> v2:
>>>>>>>>> * fixed commit log.
>>>>>>>>> ---
>>>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>>>>    1 file changed, 15 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>>>>>    {
>>>>>>>>>        struct rte_eth_dev *dev;
>>>>>>>>> +    int index;
>>>>>>>>>        int ret;
>>>>>>>>>          RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>>>>> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>>>>>>>>>        if (ret < 0)
>>>>>>>>>            return ret;
>>>>>>>>>    +    /*
>>>>>>>>> +     * If the address has been added as a non-default MAC address by
>>>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>>>>> +     * dev->data->mac_addrs[].
>>>>>>>>> +     */
>>>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>>>> +    if (index > 0) {
>>>>>>>>> +        /* remove address in NIC data structure */
>>>>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>>>>> + &dev->data->mac_addrs[index]);
>>>>>>>>> +        /* reset pool bitmap */
>>>>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it assumes
>>>>>>>> driver removes similar duplication itself, but I am not sure if this is
>>>>>>>> valid for all drivers.
>>>>>>>>
>>>>>>>> If driver is not removing the duplicate in the HW configuration, the driver
>>>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not good.
>>>>>>> The same MAC address does not occupy two HW entries, which is also a
>>>>>>> waste for HW. After all, HW entry resources are also limited.
>>>>>>> The PMD should also take this into account.
>>>>>>> So, I think, we don't have to think about it here.
>>>>>>
>>>>>> I am not sure all PMD take this into account, I briefly checked the ixgbe
>>>>>> code and I am not sure if it handles this.
>>>>>>
>>>>>> Also it is possible to think that this responsibility is pushed to the
>>>>>> application, like application should remove a MAC address before setting
>>>>>> it as default MAC...
>>>>>>
>>>>>
>>>>> Yes, the API view is more important than saving one entry in an array. From API perspective with this patch,
>>>>>
>>>>> set_default(MAC1)
>>>>> add(MAC2)
>>>>> add(MAC3)
>>>>> add(MAC4)
>>>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>>>
>>>>> set_default(MAC2)
>>>>> default=MAC2, Filters= MAC3, MAC4
>>>>>
>>>>> set_default(MAC3)
>>>>> default=MAC3, Filters= MAC4
>>>>>
>>>>> set_default(MAC4)
>>>>> default=MAC4, Filters=
>>>>>
>>>>> set_default(MAC5)
>>>>> default=MAC5, Filters=
>>>>>
>>>>> Did I get it right? If so, it seems wrong to silently remove the filters. In which case, it would be easier to just not remove them in the first place (current behaviour).
>>>>>
>>>>
>>>> Yep, this is the updated behavior. And agree it looks wrong when you
>>>> show like this. (btw, this is only ethdev record of MAC filters, what
>>>> is updated in this patch, HW still may be keeping all filters.)
>>>
>>> Whether HW saves all filters depends on the implementation of the set_default()
>>>
>>> in the driver. According to the implementation of this API of all PMDs, some drivers
>>>
>>> will first remove the old default MAC in HW and then add the new one when calling
>>>
>>> the set_default(). I am not sure if the HW that didn't do this would remove the old
>>>
>>> default MAC. If not, we may need to standardize this API in the ethdev layer.
>>>
>>>>
>>>>> If they really need to be removed from the filter list when they are set_default(), then perhaps they should be restored to it when there is a new set_default().
>>>>>
>>>>
>>>> I am for keeping current behavior. Application always can explicitly remove a
>>>> MAC filter before setting it default if required.
>>>
>>> But application can not remove the duplicate MAC if the MAC is the current default
>>>
>>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed to remove.
>>>
>>
>> But can do other-way around, first remove (the non default one), later
>> 'rte_eth_dev_default_mac_addr_set()'.
> 
> This introduces a usage dependency on the user. We don't have a statement in some place
> 
> for the dependency. What's more, if the user does not follow this dependency, the user
> 
> will no longer be able to remove the MAC.
> 
> So it may be more appropriate to deal with problem in ethdev layer.
> 
> 
> **Scheme A:**
> 
> The decision is left to the user, but there are usage dependency and irremovable possibility.**
> **
> 
> *Scheme B:*
> 
> index = eth_dev_get_mac_addr_index(port_id, addr);
> if (index > 0) {
>      mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>      rte_eth_dev_mac_addr_remove(port_id, addr);
> }
> ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
> if (ret < 0) {
>      if (index > 0) {
>          rte_eth_dev_mac_addr_add(port_id, addr, 0);
>          dev->data->mac_pool_sel[index] = mac_pool_sel_bk;
>      }
> 
>      return ret;
> }
> 
> * Scheme C:*
> 
>   Use the method in this patch. It assumes that the driver has only one HW entry for a MAC.
> 
> What do you think we should do?
> 

If the impact is only loosing one entry in the array without any functional
effect, I am OK to keep behavior as it is.

If there is more motivation for fix, I would prefer option B to be sure all
drivers behave same by explicit remove.

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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-10-26 10:21                   ` Ferruh Yigit
@ 2021-11-08  6:55                     ` lihuisong (C)
  2022-04-25  6:42                       ` Min Hu (Connor)
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2021-11-08  6:55 UTC (permalink / raw)
  To: Ferruh Yigit, Kevin Traynor, Min Hu (Connor), dev; +Cc: thomas


在 2021/10/26 18:21, Ferruh Yigit 写道:
> On 10/22/2021 3:04 AM, lihuisong (C) wrote:
>>
>> 在 2021/10/21 16:30, Ferruh Yigit 写道:
>>> On 10/21/2021 3:05 AM, lihuisong (C) wrote:
>>>>
>>>> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>>>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>>>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>>>>> Hi Ferruh
>>>>>>>>
>>>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>>>>>
>>>>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC 
>>>>>>>>>> address when
>>>>>>>>>> applications modify the default MAC address by
>>>>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new 
>>>>>>>>>> default
>>>>>>>>>> MAC address has been added as a non-default MAC address by
>>>>>>>>>> rte_eth_dev_mac_addr_add() API, the 
>>>>>>>>>> rte_eth_dev_default_mac_addr_set()
>>>>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a 
>>>>>>>>>> result, one MAC
>>>>>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Connor,
>>>>>>>>>
>>>>>>>>> I see the problem, but can you please clarify what is the 
>>>>>>>>> impact to the end user?
>>>>>>>>>
>>>>>>>>> If application does as following:
>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>>>>>
>>>>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which 
>>>>>>>>> has 'MAC2' duplicated.
>>>>>>>>>
>>>>>>>>> Will this cause any problem for the application to receive the 
>>>>>>>>> packets
>>>>>>>>> with 'MAC2' address?
>>>>>>>>> Or is the only problem one extra space used in 
>>>>>>>>> 'dev->data->mac_addrs[]'
>>>>>>>>> without any other impact to the application?
>>>>>>>> I think it's just a waste of space.
>>>>>>>
>>>>>>> True, it is a waste. But if there is no other visible user 
>>>>>>> impact, we can
>>>>>>> handle the issue with lower priority and clarifying the impact 
>>>>>>> in commit log
>>>>>>> helps to others.
>>>>>>>
>>>>>>>>>
>>>>>>>>>> This patch adds the logic of removing MAC addresses for this 
>>>>>>>>>> scenario.
>>>>>>>>>>
>>>>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>> v2:
>>>>>>>>>> * fixed commit log.
>>>>>>>>>> ---
>>>>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>>>>>    1 file changed, 15 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>>>> rte_ether_addr *addr)
>>>>>>>>>>    {
>>>>>>>>>>        struct rte_eth_dev *dev;
>>>>>>>>>> +    int index;
>>>>>>>>>>        int ret;
>>>>>>>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>>>>>> @@ -4361,6 +4362,20 @@ 
>>>>>>>>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>>>> rte_ether_addr *addr)
>>>>>>>>>>        if (ret < 0)
>>>>>>>>>>            return ret;
>>>>>>>>>>    +    /*
>>>>>>>>>> +     * If the address has been added as a non-default MAC 
>>>>>>>>>> address by
>>>>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>>>>>> +     * dev->data->mac_addrs[].
>>>>>>>>>> +     */
>>>>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>>>>> +    if (index > 0) {
>>>>>>>>>> +        /* remove address in NIC data structure */
>>>>>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>>>>>> + &dev->data->mac_addrs[index]);
>>>>>>>>>> +        /* reset pool bitmap */
>>>>>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it 
>>>>>>>>> assumes
>>>>>>>>> driver removes similar duplication itself, but I am not sure 
>>>>>>>>> if this is
>>>>>>>>> valid for all drivers.
>>>>>>>>>
>>>>>>>>> If driver is not removing the duplicate in the HW 
>>>>>>>>> configuration, the driver
>>>>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not 
>>>>>>>>> good.
>>>>>>>> The same MAC address does not occupy two HW entries, which is 
>>>>>>>> also a
>>>>>>>> waste for HW. After all, HW entry resources are also limited.
>>>>>>>> The PMD should also take this into account.
>>>>>>>> So, I think, we don't have to think about it here.
>>>>>>>
>>>>>>> I am not sure all PMD take this into account, I briefly checked 
>>>>>>> the ixgbe
>>>>>>> code and I am not sure if it handles this.
>>>>>>>
>>>>>>> Also it is possible to think that this responsibility is pushed 
>>>>>>> to the
>>>>>>> application, like application should remove a MAC address before 
>>>>>>> setting
>>>>>>> it as default MAC...
>>>>>>>
>>>>>>
>>>>>> Yes, the API view is more important than saving one entry in an 
>>>>>> array. From API perspective with this patch,
>>>>>>
>>>>>> set_default(MAC1)
>>>>>> add(MAC2)
>>>>>> add(MAC3)
>>>>>> add(MAC4)
>>>>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>>>>
>>>>>> set_default(MAC2)
>>>>>> default=MAC2, Filters= MAC3, MAC4
>>>>>>
>>>>>> set_default(MAC3)
>>>>>> default=MAC3, Filters= MAC4
>>>>>>
>>>>>> set_default(MAC4)
>>>>>> default=MAC4, Filters=
>>>>>>
>>>>>> set_default(MAC5)
>>>>>> default=MAC5, Filters=
>>>>>>
>>>>>> Did I get it right? If so, it seems wrong to silently remove the 
>>>>>> filters. In which case, it would be easier to just not remove 
>>>>>> them in the first place (current behaviour).
>>>>>>
>>>>>
>>>>> Yep, this is the updated behavior. And agree it looks wrong when you
>>>>> show like this. (btw, this is only ethdev record of MAC filters, what
>>>>> is updated in this patch, HW still may be keeping all filters.)
>>>>
>>>> Whether HW saves all filters depends on the implementation of the 
>>>> set_default()
>>>>
>>>> in the driver. According to the implementation of this API of all 
>>>> PMDs, some drivers
>>>>
>>>> will first remove the old default MAC in HW and then add the new 
>>>> one when calling
>>>>
>>>> the set_default(). I am not sure if the HW that didn't do this 
>>>> would remove the old
>>>>
>>>> default MAC. If not, we may need to standardize this API in the 
>>>> ethdev layer.
>>>>
>>>>>
>>>>>> If they really need to be removed from the filter list when they 
>>>>>> are set_default(), then perhaps they should be restored to it 
>>>>>> when there is a new set_default().
>>>>>>
>>>>>
>>>>> I am for keeping current behavior. Application always can 
>>>>> explicitly remove a
>>>>> MAC filter before setting it default if required.
>>>>
>>>> But application can not remove the duplicate MAC if the MAC is the 
>>>> current default
>>>>
>>>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed 
>>>> to remove.
>>>>
>>>
>>> But can do other-way around, first remove (the non default one), later
>>> 'rte_eth_dev_default_mac_addr_set()'.
>>
>> This introduces a usage dependency on the user. We don't have a 
>> statement in some place
>>
>> for the dependency. What's more, if the user does not follow this 
>> dependency, the user
>>
>> will no longer be able to remove the MAC.
>>
>> So it may be more appropriate to deal with problem in ethdev layer.
>>
>>
>> **Scheme A:**
>>
>> The decision is left to the user, but there are usage dependency and 
>> irremovable possibility.**
>> **
>>
>> *Scheme B:*
>>
>> index = eth_dev_get_mac_addr_index(port_id, addr);
>> if (index > 0) {
>>      mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>      rte_eth_dev_mac_addr_remove(port_id, addr);
>> }
>> ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>> if (ret < 0) {
>>      if (index > 0) {
>>          rte_eth_dev_mac_addr_add(port_id, addr, 0);
>>          dev->data->mac_pool_sel[index] = mac_pool_sel_bk;
>>      }
>>
>>      return ret;
>> }
>>
>> * Scheme C:*
>>
>>   Use the method in this patch. It assumes that the driver has only 
>> one HW entry for a MAC.
>>
>> What do you think we should do?
>>
>
> If the impact is only loosing one entry in the array without any 
> functional
> effect, I am OK to keep behavior as it is.
>
> If there is more motivation for fix, I would prefer option B to be 
> sure all
> drivers behave same by explicit remove.
> .

Hi, Ferruh

There may be a functional problem in one scenario if we don't fix this.

Testpmd does the following steps:

1) add MAC1/2/3

mac_addr add 0 MAC1

mac_addr add 0 MAC2

mac_addr add 0 MAC3

2) then set new default MAC3

mac_addr set 0 MAC3

3)show mac_addrs list

MAC3

MAC1

MAC2

MAC3

4)then set new default MAC4

mac_addr set 0 MAC4

5) now, the mac_addrs list is as follows:

MAC4

MAC1

MAC2

MAC3


As a result, the network engine cannot receive packets from MAC3.

But application can see the MAC3 in mac_addrs list.

Btw, rte_eth_dev_default_mac_addr_set() will replace the old default MAC 
with

the new one, and the old one will be deleted.


So, we should fix it. What do you think? Thanks.








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

* Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
  2021-11-08  6:55                     ` lihuisong (C)
@ 2022-04-25  6:42                       ` Min Hu (Connor)
  0 siblings, 0 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-04-25  6:42 UTC (permalink / raw)
  To: lihuisong (C), Ferruh Yigit, Kevin Traynor, dev; +Cc: thomas, ferruh.yigit

Hi, Ferruh,
	what do you think of this patch?


在 2021/11/8 14:55, lihuisong (C) 写道:
> 
> 在 2021/10/26 18:21, Ferruh Yigit 写道:
>> On 10/22/2021 3:04 AM, lihuisong (C) wrote:
>>>
>>> 在 2021/10/21 16:30, Ferruh Yigit 写道:
>>>> On 10/21/2021 3:05 AM, lihuisong (C) wrote:
>>>>>
>>>>> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>>>>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>>>>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>>>>>> Hi Ferruh
>>>>>>>>>
>>>>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>>>>>>
>>>>>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC 
>>>>>>>>>>> address when
>>>>>>>>>>> applications modify the default MAC address by
>>>>>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new 
>>>>>>>>>>> default
>>>>>>>>>>> MAC address has been added as a non-default MAC address by
>>>>>>>>>>> rte_eth_dev_mac_addr_add() API, the 
>>>>>>>>>>> rte_eth_dev_default_mac_addr_set()
>>>>>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a 
>>>>>>>>>>> result, one MAC
>>>>>>>>>>> address occupies two index capacities in dev->data->mac_addrs[].
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Connor,
>>>>>>>>>>
>>>>>>>>>> I see the problem, but can you please clarify what is the 
>>>>>>>>>> impact to the end user?
>>>>>>>>>>
>>>>>>>>>> If application does as following:
>>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC1);
>>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC2);
>>>>>>>>>>    rte_eth_dev_mac_addr_add(MAC3);
>>>>>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2);
>>>>>>>>>>
>>>>>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which 
>>>>>>>>>> has 'MAC2' duplicated.
>>>>>>>>>>
>>>>>>>>>> Will this cause any problem for the application to receive the 
>>>>>>>>>> packets
>>>>>>>>>> with 'MAC2' address?
>>>>>>>>>> Or is the only problem one extra space used in 
>>>>>>>>>> 'dev->data->mac_addrs[]'
>>>>>>>>>> without any other impact to the application?
>>>>>>>>> I think it's just a waste of space.
>>>>>>>>
>>>>>>>> True, it is a waste. But if there is no other visible user 
>>>>>>>> impact, we can
>>>>>>>> handle the issue with lower priority and clarifying the impact 
>>>>>>>> in commit log
>>>>>>>> helps to others.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> This patch adds the logic of removing MAC addresses for this 
>>>>>>>>>>> scenario.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>>>>> ---
>>>>>>>>>>> v2:
>>>>>>>>>>> * fixed commit log.
>>>>>>>>>>> ---
>>>>>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>>>>>>>>>>>    1 file changed, 15 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>>>>>>>>> index 028907bc4b..7faff17d9a 100644
>>>>>>>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>>>>>>>> @@ -4340,6 +4340,7 @@ int
>>>>>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>>>>> rte_ether_addr *addr)
>>>>>>>>>>>    {
>>>>>>>>>>>        struct rte_eth_dev *dev;
>>>>>>>>>>> +    int index;
>>>>>>>>>>>        int ret;
>>>>>>>>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>>>>>>> @@ -4361,6 +4362,20 @@ 
>>>>>>>>>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct 
>>>>>>>>>>> rte_ether_addr *addr)
>>>>>>>>>>>        if (ret < 0)
>>>>>>>>>>>            return ret;
>>>>>>>>>>>    +    /*
>>>>>>>>>>> +     * If the address has been added as a non-default MAC 
>>>>>>>>>>> address by
>>>>>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>>>>>>>>> +     * dev->data->mac_addrs[].
>>>>>>>>>>> +     */
>>>>>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>>>>>>>>> +    if (index > 0) {
>>>>>>>>>>> +        /* remove address in NIC data structure */
>>>>>>>>>>> +        rte_ether_addr_copy(&null_mac_addr,
>>>>>>>>>>> + &dev->data->mac_addrs[index]);
>>>>>>>>>>> +        /* reset pool bitmap */
>>>>>>>>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it 
>>>>>>>>>> assumes
>>>>>>>>>> driver removes similar duplication itself, but I am not sure 
>>>>>>>>>> if this is
>>>>>>>>>> valid for all drivers.
>>>>>>>>>>
>>>>>>>>>> If driver is not removing the duplicate in the HW 
>>>>>>>>>> configuration, the driver
>>>>>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not 
>>>>>>>>>> good.
>>>>>>>>> The same MAC address does not occupy two HW entries, which is 
>>>>>>>>> also a
>>>>>>>>> waste for HW. After all, HW entry resources are also limited.
>>>>>>>>> The PMD should also take this into account.
>>>>>>>>> So, I think, we don't have to think about it here.
>>>>>>>>
>>>>>>>> I am not sure all PMD take this into account, I briefly checked 
>>>>>>>> the ixgbe
>>>>>>>> code and I am not sure if it handles this.
>>>>>>>>
>>>>>>>> Also it is possible to think that this responsibility is pushed 
>>>>>>>> to the
>>>>>>>> application, like application should remove a MAC address before 
>>>>>>>> setting
>>>>>>>> it as default MAC...
>>>>>>>>
>>>>>>>
>>>>>>> Yes, the API view is more important than saving one entry in an 
>>>>>>> array. From API perspective with this patch,
>>>>>>>
>>>>>>> set_default(MAC1)
>>>>>>> add(MAC2)
>>>>>>> add(MAC3)
>>>>>>> add(MAC4)
>>>>>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>>>>>
>>>>>>> set_default(MAC2)
>>>>>>> default=MAC2, Filters= MAC3, MAC4
>>>>>>>
>>>>>>> set_default(MAC3)
>>>>>>> default=MAC3, Filters= MAC4
>>>>>>>
>>>>>>> set_default(MAC4)
>>>>>>> default=MAC4, Filters=
>>>>>>>
>>>>>>> set_default(MAC5)
>>>>>>> default=MAC5, Filters=
>>>>>>>
>>>>>>> Did I get it right? If so, it seems wrong to silently remove the 
>>>>>>> filters. In which case, it would be easier to just not remove 
>>>>>>> them in the first place (current behaviour).
>>>>>>>
>>>>>>
>>>>>> Yep, this is the updated behavior. And agree it looks wrong when you
>>>>>> show like this. (btw, this is only ethdev record of MAC filters, what
>>>>>> is updated in this patch, HW still may be keeping all filters.)
>>>>>
>>>>> Whether HW saves all filters depends on the implementation of the 
>>>>> set_default()
>>>>>
>>>>> in the driver. According to the implementation of this API of all 
>>>>> PMDs, some drivers
>>>>>
>>>>> will first remove the old default MAC in HW and then add the new 
>>>>> one when calling
>>>>>
>>>>> the set_default(). I am not sure if the HW that didn't do this 
>>>>> would remove the old
>>>>>
>>>>> default MAC. If not, we may need to standardize this API in the 
>>>>> ethdev layer.
>>>>>
>>>>>>
>>>>>>> If they really need to be removed from the filter list when they 
>>>>>>> are set_default(), then perhaps they should be restored to it 
>>>>>>> when there is a new set_default().
>>>>>>>
>>>>>>
>>>>>> I am for keeping current behavior. Application always can 
>>>>>> explicitly remove a
>>>>>> MAC filter before setting it default if required.
>>>>>
>>>>> But application can not remove the duplicate MAC if the MAC is the 
>>>>> current default
>>>>>
>>>>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed 
>>>>> to remove.
>>>>>
>>>>
>>>> But can do other-way around, first remove (the non default one), later
>>>> 'rte_eth_dev_default_mac_addr_set()'.
>>>
>>> This introduces a usage dependency on the user. We don't have a 
>>> statement in some place
>>>
>>> for the dependency. What's more, if the user does not follow this 
>>> dependency, the user
>>>
>>> will no longer be able to remove the MAC.
>>>
>>> So it may be more appropriate to deal with problem in ethdev layer.
>>>
>>>
>>> **Scheme A:**
>>>
>>> The decision is left to the user, but there are usage dependency and 
>>> irremovable possibility.**
>>> **
>>>
>>> *Scheme B:*
>>>
>>> index = eth_dev_get_mac_addr_index(port_id, addr);
>>> if (index > 0) {
>>>      mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>>      rte_eth_dev_mac_addr_remove(port_id, addr);
>>> }
>>> ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>> if (ret < 0) {
>>>      if (index > 0) {
>>>          rte_eth_dev_mac_addr_add(port_id, addr, 0);
>>>          dev->data->mac_pool_sel[index] = mac_pool_sel_bk;
>>>      }
>>>
>>>      return ret;
>>> }
>>>
>>> * Scheme C:*
>>>
>>>   Use the method in this patch. It assumes that the driver has only 
>>> one HW entry for a MAC.
>>>
>>> What do you think we should do?
>>>
>>
>> If the impact is only loosing one entry in the array without any 
>> functional
>> effect, I am OK to keep behavior as it is.
>>
>> If there is more motivation for fix, I would prefer option B to be 
>> sure all
>> drivers behave same by explicit remove.
>> .
> 
> Hi, Ferruh
> 
> There may be a functional problem in one scenario if we don't fix this.
> 
> Testpmd does the following steps:
> 
> 1) add MAC1/2/3
> 
> mac_addr add 0 MAC1
> 
> mac_addr add 0 MAC2
> 
> mac_addr add 0 MAC3
> 
> 2) then set new default MAC3
> 
> mac_addr set 0 MAC3
> 
> 3)show mac_addrs list
> 
> MAC3
> 
> MAC1
> 
> MAC2
> 
> MAC3
> 
> 4)then set new default MAC4
> 
> mac_addr set 0 MAC4
> 
> 5) now, the mac_addrs list is as follows:
> 
> MAC4
> 
> MAC1
> 
> MAC2
> 
> MAC3
> 
> 
> As a result, the network engine cannot receive packets from MAC3.
> 
> But application can see the MAC3 in mac_addrs list.
> 
> Btw, rte_eth_dev_default_mac_addr_set() will replace the old default MAC 
> with
> 
> the new one, and the old one will be deleted.
> 
> 
> So, we should fix it. What do you think? Thanks.
> 
> 
> 
> 
> 
> 
> 
> .

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

* [PATCH V3 0/2] ethdev: fix MAC addrs list
  2021-09-22  3:36 [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs Min Hu (Connor)
                   ` (2 preceding siblings ...)
  2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
@ 2022-05-14  2:00 ` Min Hu (Connor)
  2022-05-14  2:00   ` [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
                     ` (3 more replies)
  3 siblings, 4 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-05-14  2:00 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko, ktraynor, thomas

From: Huisong Li <lihuisong@huawei.com>

The index zero of rte_eth_dev_data::mac_addrs array is as the default MAC  
index, and other indexes can't be the same as the address corresponding to
index 0. If we break it, may cause following problems:                     
1) waste of MAC address spaces.                                            
2) a fake MAC address in the MAC list, isn't in hardware MAC entries.      
3) a MAC address is assigned to diffent pool.

Huisong Li (2):
  ethdev: fix one address occupies two indexes in MAC addrs
  ethdev: document default and non-default MAC address

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

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

-- 
2.33.0


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

* [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
@ 2022-05-14  2:00   ` Min Hu (Connor)
  2022-05-14  2:00   ` [PATCH V3 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-05-14  2:00 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko, ktraynor, thomas

From: Huisong Li <lihuisong@huawei.com>

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

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

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

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu <humin29@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 29a3d80466..ab4a16487f 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4248,7 +4248,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);
@@ -4266,16 +4269,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
 
+	/*
+	 * If the address has been added as a non-default MAC address by
+	 * rte_eth_dev_mac_addr_add API, it should be removed from
+	 * dev->data->mac_addrs[].
+	 */
+	index = eth_dev_get_mac_addr_index(port_id, addr);
+	if (index > 0) {
+		/* remove address in NIC data structure */
+		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
+		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
+		if (ret < 0) {
+			RTE_ETHDEV_LOG(ERR,
+			"Delete MAC address from the MAC list of ethdev port %u.\n",
+			port_id);
+			return ret;
+		}
+		/* reset pool bitmap */
+		dev->data->mac_pool_sel[index] = 0;
+	}
+
 	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
 	if (ret < 0)
-		return ret;
+		goto back;
 
 	/* Update default address in NIC data structure */
 	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
 
 	return 0;
-}
 
+back:
+	if (index > 0) {
+		pool = 0;
+		do {
+			if (mac_pool_sel_bk & UINT64_C(1))
+				rte_eth_dev_mac_addr_add(port_id, addr, pool);
+			mac_pool_sel_bk >>= 1;
+			pool++;
+		} while (mac_pool_sel_bk);
+	}
+
+	return ret;
+}
 
 /*
  * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
-- 
2.33.0


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

* [PATCH V3 2/2] ethdev: document default and non-default MAC address
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
  2022-05-14  2:00   ` [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
@ 2022-05-14  2:00   ` Min Hu (Connor)
  2022-05-31 15:22   ` [PATCH V3 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
  2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
  3 siblings, 0 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-05-14  2:00 UTC (permalink / raw)
  To: dev; +Cc: ferruh.yigit, andrew.rybchenko, ktraynor, thomas

From: Huisong Li <lihuisong@huawei.com>

The rte_eth_dev_data::mac_addrs is a MAC address array. The index zero of
this array is as the default address index, and other indexes can't be the
same as the address corresponding to index 0. If we break it, may cause
following problems:
1) waste of MAC address spaces.
2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
3) a MAC address is assigned to diffent pool.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu <humin29@huawei.com>
---
 lib/ethdev/ethdev_driver.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 69d9dc21d8..d49e9138c6 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -115,7 +115,12 @@ struct rte_eth_dev_data {
 
 	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
 
-	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
+	/**
+	 * Device Ethernet link address. The index zero of the array is as the
+	 * index of the default address, and other indexes can't be the same
+	 * as the address corresponding to index 0.
+	 * @see rte_eth_dev_release_port()
+	 */
 	struct rte_ether_addr *mac_addrs;
 	/** Bitmap associating MAC addresses to pools */
 	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
-- 
2.33.0


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

* Re: [PATCH V3 0/2] ethdev: fix MAC addrs list
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
  2022-05-14  2:00   ` [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
  2022-05-14  2:00   ` [PATCH V3 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
@ 2022-05-31 15:22   ` Andrew Rybchenko
  2022-06-01  6:43     ` Min Hu (Connor)
  2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
  3 siblings, 1 reply; 40+ messages in thread
From: Andrew Rybchenko @ 2022-05-31 15:22 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: ferruh.yigit, ktraynor, thomas

On 5/14/22 05:00, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The index zero of rte_eth_dev_data::mac_addrs array is as the default MAC
> index, and other indexes can't be the same as the address corresponding to
> index 0. If we break it, may cause following problems:
> 1) waste of MAC address spaces.
> 2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
> 3) a MAC address is assigned to diffent pool.

The series looks broken in the patchwork. As the result unit tests
are run in a strange way. Please, format patches once again and send v4.


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

* [PATCH v4 0/2] ethdev: fix MAC addrs list
  2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
                     ` (2 preceding siblings ...)
  2022-05-31 15:22   ` [PATCH V3 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
@ 2022-06-01  6:39   ` Min Hu (Connor)
  2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
                       ` (2 more replies)
  3 siblings, 3 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-06-01  6:39 UTC (permalink / raw)
  To: dev

The index zero of rte_eth_dev_data::mac_addrs array is as the default MAC
index, and other indexes can't be the same as the address corresponding to
index 0. If we break it, may cause following problems:
1) waste of MAC address spaces.
2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
3) a MAC address is assigned to diffent pool.

Huisong Li (2):
  ethdev: fix one address occupies two indexes in MAC addrs
  ethdev: document default and non-default MAC address
---
v4:
  - fix broken in the patchwork

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

v2:
  - fixed commit log.

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

-- 
2.33.0


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

* [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
@ 2022-06-01  6:39     ` Min Hu (Connor)
  2022-06-01 17:49       ` Andrew Rybchenko
  2022-06-01  6:39     ` [PATCH v4 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
  2022-06-01 17:49     ` [PATCH v4 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
  2 siblings, 1 reply; 40+ messages in thread
From: Min Hu (Connor) @ 2022-06-01  6:39 UTC (permalink / raw)
  To: dev

From: Huisong Li <lihuisong@huawei.com>

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

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

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

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu <humin29@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 46c088dc88..fc9ca8d6fd 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4260,7 +4260,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);
@@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
 
+	/*
+	 * If the address has been added as a non-default MAC address by
+	 * rte_eth_dev_mac_addr_add API, it should be removed from
+	 * dev->data->mac_addrs[].
+	 */
+	index = eth_dev_get_mac_addr_index(port_id, addr);
+	if (index > 0) {
+		/* remove address in NIC data structure */
+		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
+		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
+		if (ret < 0) {
+			RTE_ETHDEV_LOG(ERR,
+			"Delete MAC address from the MAC list of ethdev port %u.\n",
+			port_id);
+			return ret;
+		}
+		/* reset pool bitmap */
+		dev->data->mac_pool_sel[index] = 0;
+	}
+
 	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
 	if (ret < 0)
-		return ret;
+		goto back;
 
 	/* Update default address in NIC data structure */
 	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
 
 	return 0;
-}
 
+back:
+	if (index > 0) {
+		pool = 0;
+		do {
+			if (mac_pool_sel_bk & UINT64_C(1))
+				rte_eth_dev_mac_addr_add(port_id, addr, pool);
+			mac_pool_sel_bk >>= 1;
+			pool++;
+		} while (mac_pool_sel_bk);
+	}
+
+	return ret;
+}
 
 /*
  * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find
-- 
2.33.0


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

* [PATCH v4 2/2] ethdev: document default and non-default MAC address
  2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
  2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
@ 2022-06-01  6:39     ` Min Hu (Connor)
  2022-06-01 17:49       ` Andrew Rybchenko
  2022-06-01 17:49     ` [PATCH v4 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
  2 siblings, 1 reply; 40+ messages in thread
From: Min Hu (Connor) @ 2022-06-01  6:39 UTC (permalink / raw)
  To: dev

From: Huisong Li <lihuisong@huawei.com>

The rte_eth_dev_data::mac_addrs is a MAC address array. The index zero of
this array is as the default address index, and other indexes can't be the
same as the address corresponding to index 0. If we break it, may cause
following problems:
1) waste of MAC address spaces.
2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
3) a MAC address is assigned to diffent pool.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu <humin29@huawei.com>
---
 lib/ethdev/ethdev_driver.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 69d9dc21d8..d49e9138c6 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -115,7 +115,12 @@ struct rte_eth_dev_data {
 
 	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
 
-	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
+	/**
+	 * Device Ethernet link address. The index zero of the array is as the
+	 * index of the default address, and other indexes can't be the same
+	 * as the address corresponding to index 0.
+	 * @see rte_eth_dev_release_port()
+	 */
 	struct rte_ether_addr *mac_addrs;
 	/** Bitmap associating MAC addresses to pools */
 	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
-- 
2.33.0


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

* Re: [PATCH V3 0/2] ethdev: fix MAC addrs list
  2022-05-31 15:22   ` [PATCH V3 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
@ 2022-06-01  6:43     ` Min Hu (Connor)
  0 siblings, 0 replies; 40+ messages in thread
From: Min Hu (Connor) @ 2022-06-01  6:43 UTC (permalink / raw)
  To: Andrew Rybchenko, dev; +Cc: ferruh.yigit, ktraynor, thomas

Hi, Andrew,
	v4 has been sent. Thanks.

在 2022/5/31 23:22, Andrew Rybchenko 写道:
> On 5/14/22 05:00, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The index zero of rte_eth_dev_data::mac_addrs array is as the default MAC
>> index, and other indexes can't be the same as the address 
>> corresponding to
>> index 0. If we break it, may cause following problems:
>> 1) waste of MAC address spaces.
>> 2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
>> 3) a MAC address is assigned to diffent pool.
> 
> The series looks broken in the patchwork. As the result unit tests
> are run in a strange way. Please, format patches once again and send v4.
> 
> .

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

* Re: [PATCH v4 0/2] ethdev: fix MAC addrs list
  2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
  2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
  2022-06-01  6:39     ` [PATCH v4 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
@ 2022-06-01 17:49     ` Andrew Rybchenko
  2 siblings, 0 replies; 40+ messages in thread
From: Andrew Rybchenko @ 2022-06-01 17:49 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: Thomas Monjalon, Ferruh Yigit

On 6/1/22 09:39, Min Hu (Connor) wrote:
> The index zero of rte_eth_dev_data::mac_addrs array is as the default MAC
> index, and other indexes can't be the same as the address corresponding to
> index 0. If we break it, may cause following problems:
> 1) waste of MAC address spaces.
> 2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
> 3) a MAC address is assigned to diffent pool.
> 
> Huisong Li (2):
>    ethdev: fix one address occupies two indexes in MAC addrs
>    ethdev: document default and non-default MAC address
> ---
> v4:
>    - fix broken in the patchwork
> 
> v3:
>    - first explicitly remove the non-default MAC, then set default one.
>    - document default and non-default MAC address
> 
> v2:
>    - fixed commit log.
> 
>   lib/ethdev/ethdev_driver.h |  7 ++++++-
>   lib/ethdev/rte_ethdev.c    | 39 ++++++++++++++++++++++++++++++++++++--
>   2 files changed, 43 insertions(+), 3 deletions(-)
> 

Please, don't forget to add maintainers in Cc when you send patches.
   --cc-cmd ./devtools/get-maintainers.sh
or
   --to-cmd ./devtools/get-maintainers.sh
(I'd prefer To and have dev@dpdk.org in Cc)

Don't worry about it right now. I'll add Thomas and Ferrh in my replies.

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

* Re: [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
@ 2022-06-01 17:49       ` Andrew Rybchenko
  2022-06-02  3:16         ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Andrew Rybchenko @ 2022-06-01 17:49 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: Thomas Monjalon, Ferruh Yigit

On 6/1/22 09:39, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The dev->data->mac_addrs[0] will be changed to a new MAC address when
> applications modify the default MAC address by
> rte_eth_dev_default_mac_addr_set(). However, if the new default one has
> been added as a non-default MAC address by rte_eth_dev_mac_addr_add(), the
> the rte_eth_dev_default_mac_addr_set() doesn't remove it from the mac_addrs
> list. As a result, one MAC address occupies two indexes in the list. Like:
> add(MAC1)
> add(MAC2)
> add(MAC3)
> add(MAC4)
> set_default(MAC3)
> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
> 
> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> old default MAC when set default MAC. If user continues to do
> set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> MAC2, MAC3, MAC4). At this moment, user can still view MAC3 from the list,
> but packets with MAC3 aren't actually received by the PMD.

IMHO, the main problem is inconsistency which exists right now.
rte_eth_dev_mac_addr_add() checks for duplicate MAC addition
including the default one (index zero) and extends the entry
pool mask (including zero entry case).

However, the patch above does not extend zero entry pool mask.
So, the result will depend on order which is bad:
A. Set default to A, add MAC A with pool 2 => pool mask has 2
B. Add MAC A with pool 2, set default to A => pool mask is empty

Am I missing something in the code?
What is the right/intended behaviour?

> 
> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu <humin29@huawei.com>
> ---
>   lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 46c088dc88..fc9ca8d6fd 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4260,7 +4260,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);
> @@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr)
>   
>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>   
> +	/*
> +	 * If the address has been added as a non-default MAC address by
> +	 * rte_eth_dev_mac_addr_add API, it should be removed from
> +	 * dev->data->mac_addrs[].
> +	 */
> +	index = eth_dev_get_mac_addr_index(port_id, addr);
> +	if (index > 0) {
> +		/* remove address in NIC data structure */
> +		mac_pool_sel_bk = dev->data->mac_pool_sel[index];
> +		ret = rte_eth_dev_mac_addr_remove(port_id, addr);
> +		if (ret < 0) {
> +			RTE_ETHDEV_LOG(ERR,
> +			"Delete MAC address from the MAC list of ethdev port %u.\n",
> +			port_id);
> +			return ret;
> +		}
> +		/* reset pool bitmap */
> +		dev->data->mac_pool_sel[index] = 0;
> +	}
> +
>   	ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>   	if (ret < 0)
> -		return ret;
> +		goto back;
>   
>   	/* Update default address in NIC data structure */
>   	rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>   
>   	return 0;
> -}
>   
> +back:
> +	if (index > 0) {
> +		pool = 0;
> +		do {
> +			if (mac_pool_sel_bk & UINT64_C(1))
> +				rte_eth_dev_mac_addr_add(port_id, addr, pool);

Don't we want to have at least error logs in the case of rollback
failure here?

> +			mac_pool_sel_bk >>= 1;
> +			pool++;
> +		} while (mac_pool_sel_bk);

Please, compare vs 0 explicitly.

> +	}
> +
> +	return ret;
> +}
>   
>   /*
>    * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find


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

* Re: [PATCH v4 2/2] ethdev: document default and non-default MAC address
  2022-06-01  6:39     ` [PATCH v4 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
@ 2022-06-01 17:49       ` Andrew Rybchenko
  0 siblings, 0 replies; 40+ messages in thread
From: Andrew Rybchenko @ 2022-06-01 17:49 UTC (permalink / raw)
  To: Min Hu (Connor), dev; +Cc: Thomas Monjalon, Ferruh Yigit

On 6/1/22 09:39, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> The rte_eth_dev_data::mac_addrs is a MAC address array. The index zero of
> this array is as the default address index, and other indexes can't be the
> same as the address corresponding to index 0. If we break it, may cause
> following problems:
> 1) waste of MAC address spaces.
> 2) a fake MAC address in the MAC list, isn't in hardware MAC entries.
> 3) a MAC address is assigned to diffent pool.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu <humin29@huawei.com>
> ---
>   lib/ethdev/ethdev_driver.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 69d9dc21d8..d49e9138c6 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -115,7 +115,12 @@ struct rte_eth_dev_data {
>   
>   	uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */
>   
> -	/** Device Ethernet link address. @see rte_eth_dev_release_port() */
> +	/**
> +	 * Device Ethernet link address. The index zero of the array is as the
> +	 * index of the default address, and other indexes can't be the same
> +	 * as the address corresponding to index 0.
> +	 * @see rte_eth_dev_release_port()
> +	 */
>   	struct rte_ether_addr *mac_addrs;
>   	/** Bitmap associating MAC addresses to pools */
>   	uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];

The patch itself does not make sense since it documents what happens
in the first patch of the series. So, it should be a part of the first
patch.

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

* Re: [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-06-01 17:49       ` Andrew Rybchenko
@ 2022-06-02  3:16         ` lihuisong (C)
  2022-06-02 13:54           ` Andrew Rybchenko
  0 siblings, 1 reply; 40+ messages in thread
From: lihuisong (C) @ 2022-06-02  3:16 UTC (permalink / raw)
  To: Andrew Rybchenko, Min Hu (Connor), dev; +Cc: Thomas Monjalon, Ferruh Yigit


在 2022/6/2 1:49, Andrew Rybchenko 写道:
> On 6/1/22 09:39, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>> applications modify the default MAC address by
>> rte_eth_dev_default_mac_addr_set(). However, if the new default one has
>> been added as a non-default MAC address by 
>> rte_eth_dev_mac_addr_add(), the
>> the rte_eth_dev_default_mac_addr_set() doesn't remove it from the 
>> mac_addrs
>> list. As a result, one MAC address occupies two indexes in the list. 
>> Like:
>> add(MAC1)
>> add(MAC2)
>> add(MAC3)
>> add(MAC4)
>> set_default(MAC3)
>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>>
>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>> old default MAC when set default MAC. If user continues to do
>> set_default(MAC5), and the mac_addrs list is default=MAC5, 
>> filters=(MAC1,
>> MAC2, MAC3, MAC4). At this moment, user can still view MAC3 from the 
>> list,
>> but packets with MAC3 aren't actually received by the PMD.
>
> IMHO, the main problem is inconsistency which exists right now.
> rte_eth_dev_mac_addr_add() checks for duplicate MAC addition
> including the default one (index zero) and extends the entry
> pool mask (including zero entry case).
>
> However, the patch above does not extend zero entry pool mask.
> So, the result will depend on order which is bad:
> A. Set default to A, add MAC A with pool 2 => pool mask has 2
> B. Add MAC A with pool 2, set default to A => pool mask is empty
>
I don't know how this MAC pool works in which driver.
However, the 'eth_dev_mac_restore' API show that 1) the default MAC has 
only pool zero
if set it by the 'mac_addr_add', 2) the default one hasn't pool 
information if set it
by 'default_mac_addr_set'.

Do you mean we should inherit its pool mask in this case?
> Am I missing something in the code?
> What is the right/intended behaviour?
>
>>
>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu <humin29@huawei.com>
>> ---
>>   lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>> index 46c088dc88..fc9ca8d6fd 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -4260,7 +4260,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);
>> @@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t 
>> port_id, struct rte_ether_addr *addr)
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>>   +    /*
>> +     * If the address has been added as a non-default MAC address by
>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>> +     * dev->data->mac_addrs[].
>> +     */
>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>> +    if (index > 0) {
>> +        /* remove address in NIC data structure */
>> +        mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>> +        ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>> +        if (ret < 0) {
>> +            RTE_ETHDEV_LOG(ERR,
>> +            "Delete MAC address from the MAC list of ethdev port 
>> %u.\n",
>> +            port_id);
>> +            return ret;
>> +        }
>> +        /* reset pool bitmap */
>> +        dev->data->mac_pool_sel[index] = 0;
>> +    }
>> +
>>       ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>       if (ret < 0)
>> -        return ret;
>> +        goto back;
>>         /* Update default address in NIC data structure */
>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>         return 0;
>> -}
>>   +back:
>> +    if (index > 0) {
>> +        pool = 0;
>> +        do {
>> +            if (mac_pool_sel_bk & UINT64_C(1))
>> +                rte_eth_dev_mac_addr_add(port_id, addr, pool);
>
> Don't we want to have at least error logs in the case of rollback
> failure here?
It doesn't feel necessary. It may trigger the printing of a large number 
of error logs
in abnormal scenarios.
>
>> +            mac_pool_sel_bk >>= 1;
>> +            pool++;
>> +        } while (mac_pool_sel_bk);
>
> Please, compare vs 0 explicitly.
Ack
>
>> +    }
>> +
>> +    return ret;
>> +}
>>     /*
>>    * Returns index into MAC address array of addr. Use 
>> 00:00:00:00:00:00 to find
>
> .

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

* Re: [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-06-02  3:16         ` lihuisong (C)
@ 2022-06-02 13:54           ` Andrew Rybchenko
  2022-06-11  9:04             ` lihuisong (C)
  0 siblings, 1 reply; 40+ messages in thread
From: Andrew Rybchenko @ 2022-06-02 13:54 UTC (permalink / raw)
  To: lihuisong (C), Min Hu (Connor), dev
  Cc: Thomas Monjalon, Ferruh Yigit, Shepard Siegel, Ed Czeck,
	John Miller, Rasesh Mody, Shahed Shaikh, Ajit Khaparde,
	Somnath Kotur, Simei Su, Wenjun Wu, Qi Zhang, Xiao Wang,
	Yuying Zhang, Beilei Xing, Qiming Yang, Jiawen Wu, Jiawen Wu,
	Jian Wang

Cc more driver maintainers

On 6/2/22 06:16, lihuisong (C) wrote:
> 
> 在 2022/6/2 1:49, Andrew Rybchenko 写道:
>> On 6/1/22 09:39, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>> applications modify the default MAC address by
>>> rte_eth_dev_default_mac_addr_set(). However, if the new default one has
>>> been added as a non-default MAC address by 
>>> rte_eth_dev_mac_addr_add(), the
>>> the rte_eth_dev_default_mac_addr_set() doesn't remove it from the 
>>> mac_addrs
>>> list. As a result, one MAC address occupies two indexes in the list. 
>>> Like:
>>> add(MAC1)
>>> add(MAC2)
>>> add(MAC3)
>>> add(MAC4)
>>> set_default(MAC3)
>>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>>>
>>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
>>> old default MAC when set default MAC. If user continues to do
>>> set_default(MAC5), and the mac_addrs list is default=MAC5, 
>>> filters=(MAC1,
>>> MAC2, MAC3, MAC4). At this moment, user can still view MAC3 from the 
>>> list,
>>> but packets with MAC3 aren't actually received by the PMD.
>>
>> IMHO, the main problem is inconsistency which exists right now.
>> rte_eth_dev_mac_addr_add() checks for duplicate MAC addition
>> including the default one (index zero) and extends the entry
>> pool mask (including zero entry case).
>>
>> However, the patch above does not extend zero entry pool mask.
>> So, the result will depend on order which is bad:
>> A. Set default to A, add MAC A with pool 2 => pool mask has 2
>> B. Add MAC A with pool 2, set default to A => pool mask is empty
>>
> I don't know how this MAC pool works in which driver.

Me too

> However, the 'eth_dev_mac_restore' API show that 1) the default MAC has 
> only pool zero
> if set it by the 'mac_addr_add', 2) the default one hasn't pool 
> information if set it
> by 'default_mac_addr_set'.
> 
> Do you mean we should inherit its pool mask in this case?

I simply want to make it consistent

>> Am I missing something in the code?
>> What is the right/intended behaviour?
>>
>>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu <humin29@huawei.com>
>>> ---
>>>   lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index 46c088dc88..fc9ca8d6fd 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -4260,7 +4260,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);
>>> @@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t 
>>> port_id, struct rte_ether_addr *addr)
>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>>>   +    /*
>>> +     * If the address has been added as a non-default MAC address by
>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>> +     * dev->data->mac_addrs[].
>>> +     */
>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>> +    if (index > 0) {
>>> +        /* remove address in NIC data structure */
>>> +        mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>> +        ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>>> +        if (ret < 0) {
>>> +            RTE_ETHDEV_LOG(ERR,
>>> +            "Delete MAC address from the MAC list of ethdev port 
>>> %u.\n",
>>> +            port_id);
>>> +            return ret;
>>> +        }
>>> +        /* reset pool bitmap */
>>> +        dev->data->mac_pool_sel[index] = 0;
>>> +    }
>>> +
>>>       ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>       if (ret < 0)
>>> -        return ret;
>>> +        goto back;
>>>         /* Update default address in NIC data structure */
>>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>         return 0;
>>> -}
>>>   +back:
>>> +    if (index > 0) {
>>> +        pool = 0;
>>> +        do {
>>> +            if (mac_pool_sel_bk & UINT64_C(1))
>>> +                rte_eth_dev_mac_addr_add(port_id, addr, pool);
>>
>> Don't we want to have at least error logs in the case of rollback
>> failure here?
> It doesn't feel necessary. It may trigger the printing of a large number 
> of error logs
> in abnormal scenarios.

Otherwise how will user know that rollback failed and configuration
is inconsistent?

>>
>>> +            mac_pool_sel_bk >>= 1;
>>> +            pool++;
>>> +        } while (mac_pool_sel_bk);
>>
>> Please, compare vs 0 explicitly.
> Ack
>>
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>>     /*
>>>    * Returns index into MAC address array of addr. Use 
>>> 00:00:00:00:00:00 to find
>>
>> .


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

* Re: [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs
  2022-06-02 13:54           ` Andrew Rybchenko
@ 2022-06-11  9:04             ` lihuisong (C)
  0 siblings, 0 replies; 40+ messages in thread
From: lihuisong (C) @ 2022-06-11  9:04 UTC (permalink / raw)
  To: Andrew Rybchenko, Min Hu (Connor), dev
  Cc: Thomas Monjalon, Ferruh Yigit, Shepard Siegel, Ed Czeck,
	John Miller, Rasesh Mody, Shahed Shaikh, Ajit Khaparde,
	Somnath Kotur, Simei Su, Wenjun Wu, Qi Zhang, Xiao Wang,
	Yuying Zhang, Beilei Xing, Qiming Yang, Jiawen Wu, Jian Wang


在 2022/6/2 21:54, Andrew Rybchenko 写道:
> Cc more driver maintainers
>
> On 6/2/22 06:16, lihuisong (C) wrote:
>>
>> 在 2022/6/2 1:49, Andrew Rybchenko 写道:
>>> On 6/1/22 09:39, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>> applications modify the default MAC address by
>>>> rte_eth_dev_default_mac_addr_set(). However, if the new default one 
>>>> has
>>>> been added as a non-default MAC address by 
>>>> rte_eth_dev_mac_addr_add(), the
>>>> the rte_eth_dev_default_mac_addr_set() doesn't remove it from the 
>>>> mac_addrs
>>>> list. As a result, one MAC address occupies two indexes in the 
>>>> list. Like:
>>>> add(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> set_default(MAC3)
>>>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4
>>>>
>>>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do 
>>>> remove the
>>>> old default MAC when set default MAC. If user continues to do
>>>> set_default(MAC5), and the mac_addrs list is default=MAC5, 
>>>> filters=(MAC1,
>>>> MAC2, MAC3, MAC4). At this moment, user can still view MAC3 from 
>>>> the list,
>>>> but packets with MAC3 aren't actually received by the PMD.
>>>
>>> IMHO, the main problem is inconsistency which exists right now.
>>> rte_eth_dev_mac_addr_add() checks for duplicate MAC addition
>>> including the default one (index zero) and extends the entry
>>> pool mask (including zero entry case).
>>>
>>> However, the patch above does not extend zero entry pool mask.
>>> So, the result will depend on order which is bad:
>>> A. Set default to A, add MAC A with pool 2 => pool mask has 2
>>> B. Add MAC A with pool 2, set default to A => pool mask is empty
>>>
>> I don't know how this MAC pool works in which driver.
>
> Me too
>
>> However, the 'eth_dev_mac_restore' API show that 1) the default MAC 
>> has only pool zero
>> if set it by the 'mac_addr_add', 2) the default one hasn't pool 
>> information if set it
>> by 'default_mac_addr_set'.
>>
>> Do you mean we should inherit its pool mask in this case?
>
> I simply want to make it consistent
I know what you mean. However, from the implementations mentioned above, 
it seems that
drivers used pool mask don't care the pool mask of the default MAC.
Do we need to set pool mask for the default MAC?
>
>>> Am I missing something in the code?
>>> What is the right/intended behaviour?
>>>
>>>>
>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu <humin29@huawei.com>
>>>> ---
>>>>   lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index 46c088dc88..fc9ca8d6fd 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -4260,7 +4260,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);
>>>> @@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t 
>>>> port_id, struct rte_ether_addr *addr)
>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>>>>   +    /*
>>>> +     * If the address has been added as a non-default MAC address by
>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from
>>>> +     * dev->data->mac_addrs[].
>>>> +     */
>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>>> +    if (index > 0) {
>>>> +        /* remove address in NIC data structure */
>>>> +        mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>>> +        ret = rte_eth_dev_mac_addr_remove(port_id, addr);
>>>> +        if (ret < 0) {
>>>> +            RTE_ETHDEV_LOG(ERR,
>>>> +            "Delete MAC address from the MAC list of ethdev port 
>>>> %u.\n",
>>>> +            port_id);
>>>> +            return ret;
>>>> +        }
>>>> +        /* reset pool bitmap */
>>>> +        dev->data->mac_pool_sel[index] = 0;
>>>> +    }
>>>> +
>>>>       ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>>       if (ret < 0)
>>>> -        return ret;
>>>> +        goto back;
>>>>         /* Update default address in NIC data structure */
>>>>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>>         return 0;
>>>> -}
>>>>   +back:
>>>> +    if (index > 0) {
>>>> +        pool = 0;
>>>> +        do {
>>>> +            if (mac_pool_sel_bk & UINT64_C(1))
>>>> +                rte_eth_dev_mac_addr_add(port_id, addr, pool);
>>>
>>> Don't we want to have at least error logs in the case of rollback
>>> failure here?
>> It doesn't feel necessary. It may trigger the printing of a large 
>> number of error logs
>> in abnormal scenarios.
>
> Otherwise how will user know that rollback failed and configuration
> is inconsistent?
Ack.
>
>>>
>>>> +            mac_pool_sel_bk >>= 1;
>>>> +            pool++;
>>>> +        } while (mac_pool_sel_bk);
>>>
>>> Please, compare vs 0 explicitly.
>> Ack
>>>
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>>     /*
>>>>    * Returns index into MAC address array of addr. Use 
>>>> 00:00:00:00:00:00 to find
>>>
>>> .
>
> .

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

end of thread, other threads:[~2022-06-11  9:05 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22  3:36 [dpdk-dev] [PATCH] ethdev: fix one MAC address occupies two index in mac addrs Min Hu (Connor)
2021-09-22  6:39 ` Andrew Rybchenko
2021-09-22  7:43   ` Huisong Li
2021-09-22  8:02     ` Andrew Rybchenko
2021-09-22  9:48       ` Huisong Li
2021-10-05 19:21 ` Thomas Monjalon
2021-10-08  7:02   ` Min Hu (Connor)
2021-10-08 10:04     ` Thomas Monjalon
2021-10-09  9:53       ` Min Hu (Connor)
2021-10-11  9:02         ` Thomas Monjalon
2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-10-11 10:35   ` Thomas Monjalon
2021-10-12  2:58     ` lihuisong (C)
2021-10-12  7:14       ` Thomas Monjalon
2021-10-15  2:00         ` lihuisong (C)
2021-10-19 17:45   ` Ferruh Yigit
2021-10-20  6:49     ` lihuisong (C)
2021-10-20  7:41       ` Ferruh Yigit
2021-10-20 10:15         ` Kevin Traynor
2021-10-20 16:32           ` Ferruh Yigit
2021-10-21  2:05             ` lihuisong (C)
2021-10-21  8:30               ` Ferruh Yigit
2021-10-22  2:04                 ` lihuisong (C)
2021-10-26 10:21                   ` Ferruh Yigit
2021-11-08  6:55                     ` lihuisong (C)
2022-04-25  6:42                       ` Min Hu (Connor)
2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
2022-05-14  2:00   ` [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
2022-05-14  2:00   ` [PATCH V3 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
2022-05-31 15:22   ` [PATCH V3 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
2022-06-01  6:43     ` Min Hu (Connor)
2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
2022-06-01 17:49       ` Andrew Rybchenko
2022-06-02  3:16         ` lihuisong (C)
2022-06-02 13:54           ` Andrew Rybchenko
2022-06-11  9:04             ` lihuisong (C)
2022-06-01  6:39     ` [PATCH v4 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
2022-06-01 17:49       ` Andrew Rybchenko
2022-06-01 17:49     ` [PATCH v4 0/2] ethdev: fix MAC addrs list Andrew Rybchenko

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git