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

[Thomas Monjalon <thomas@monjalon.net>]

02/02/2023 19:09, Ferruh Yigit:
> On 2/2/2023 12:36 PM, Huisong Li wrote:
> > The dev->data->mac_addrs[0] will be changed to a new MAC address when
> > applications modify the default MAC address by .mac_addr_set(). However,
> > if the new default one has been added as a non-default MAC address by
> > .mac_addr_add(), the .mac_addr_set() doesn't remove it from the mac_addrs
> > list. As a result, one MAC address occupies two entries in the list. Like:
> > add(MAC1)
> > add(MAC2)
> > add(MAC3)
> > add(MAC4)
> > set_default(MAC3)
> > default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4
> > Note: MAC3 occupies two entries.
> > 
> > In addition, some PMDs, such as i40e, ice, hns3 and so on, do remove the
> > old default MAC when set default MAC. If user continues to do
> > set_default(MAC5), and the mac_addrs list is default=MAC5, filters=(MAC1,
> > MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the list,
> > but packets with MAC3 aren't actually received by the PMD.
> > 
> > So need to ensure that the new default address is removed from the rest of
> > the list if the address was already in the list.
> > 
> 
> Same comment from past seems already valid, I am not looking to the set
> for a while, sorry if this is already discussed and decided,
> if not, I am referring to the side effect that setting MAC addresses
> cause to remove MAC addresses, think following case:
> 
> add(MAC1) -> MAC1
> add(MAC2) -> MAC1, MAC2
> add(MAC3) -> MAC1, MAC2, MAC3
> add(MAC4) -> MAC1, MAC2, MAC3, MAC4
> set(MAC3) -> MAC3, MAC2, MAC4
> set(MAC4) -> MAC4, MAC2
> set(MAC2) -> MAC2
> 
> I am not exactly clear what is the intention with set(),

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

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

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

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

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

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

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

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

So we have 4 choices.
We could vote, roll a dice, or find a strong argument?