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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/3/2023 1:56 AM, lihuisong (C) wrote:
> 
> 在 2023/2/3 5:10, Thomas Monjalon 写道:
>> 02/02/2023 19:09, Ferruh Yigit:
>>> On 2/2/2023 12:36 PM, Huisong Li wrote:
>>>> The dev->data->mac_addrs[0] will be changed to a new MAC address when
>>>> applications modify the default MAC address by .mac_addr_set().
>>>> However,
>>>> if the new default one has been added as a non-default MAC address by
>>>> .mac_addr_add(), the .mac_addr_set() doesn't remove it from the
>>>> mac_addrs
>>>> list. As a result, one MAC address occupies two entries in the list.
>>>> Like:
>>>> add(MAC1)
>>>> add(MAC2)
>>>> add(MAC3)
>>>> add(MAC4)
>>>> set_default(MAC3)
>>>> default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
>>>> Note: MAC3 occupies two entries.
>>>>
>>>> In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove
>>>> the
>>>> old default MAC when set default MAC. If user continues to do
>>>> set_default(MAC5), and the mac_addrs list is default=MAC5,
>>>> filters=(MAC1,
>>>> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the
>>>> list,
>>>> but packets with MAC3 aren't actually received by the PMD.
>>>>
>>>> So need to ensure that the new default address is removed from the
>>>> rest of
>>>> the list if the address was already in the list.
>>>>
>>> Same comment from past seems already valid, I am not looking to the set
>>> for a while, sorry if this is already discussed and decided,
>>> if not, I am referring to the side effect that setting MAC addresses
>>> cause to remove MAC addresses, think following case:
>>>
>>> add(MAC1) -> MAC1
>>> add(MAC2) -> MAC1, MAC2
>>> add(MAC3) -> MAC1, MAC2, MAC3
>>> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
>>> set(MAC3) -> MAC3, MAC2, MAC4
>>> set(MAC4) -> MAC4, MAC2
>>> set(MAC2) -> MAC2
>>>
>>> I am not exactly clear what is the intention with set(),
>> That's the problem, nobody is clear with the current behavior.
>> The doc says "Set the default MAC address." and nothing else.
> Indeed. But we can see the following information.
> From the ethdev layer, this set() API always replaces the old default
> address (index 0) without adding the old one.
> From the PMD layer, set() interface of some PMDs, such as i40e, ice,
> hns3 and so on (as far as I know),
> also do remove the hardware entry of the old default address.

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

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

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


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

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


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

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

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


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


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

For default MAC address it is more clear that intention is to replace
it, but question is about what to do with the list of MAC addresses.