> From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Thursday, 2 February 2023 22.11 > > 02/02/2023 19:09, Ferruh Yigit: > > On 2/2/2023 12:36 PM, Huisong Li wrote: > > > The dev->data->mac_addrs[0] will be changed to a new MAC address > when > > > applications modify the default MAC address by .mac_addr_set(). > However, > > > if the new default one has been added as a non-default MAC address > by > > > .mac_addr_add(), the .mac_addr_set() doesn't remove it from the > mac_addrs > > > list. As a result, one MAC address occupies two entries in the > list. Like: > > > add(MAC1) > > > add(MAC2) > > > add(MAC3) > > > add(MAC4) > > > set_default(MAC3) > > > default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4 > > > Note: MAC3 occupies two entries. > > > > > > In addition, some PMDs, such as i40e, ice, hns3 and so on, do > remove the > > > old default MAC when set default MAC. If user continues to do > > > set_default(MAC5), and the mac_addrs list is default=MAC5, > filters=(MAC1, > > > MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the > list, > > > but packets with MAC3 aren't actually received by the PMD. > > > > > > So need to ensure that the new default address is removed from the > rest of > > > the list if the address was already in the list. > > > > > > > Same comment from past seems already valid, I am not looking to the > set > > for a while, sorry if this is already discussed and decided, > > if not, I am referring to the side effect that setting MAC addresses > > cause to remove MAC addresses, think following case: > > > > add(MAC1) -> MAC1 > > add(MAC2) -> MAC1, MAC2 > > add(MAC3) -> MAC1, MAC2, MAC3 > > add(MAC4) -> MAC1, MAC2, MAC3, MAC4 > > set(MAC3) -> MAC3, MAC2, MAC4 > > set(MAC4) -> MAC4, MAC2 > > set(MAC2) -> MAC2 > > > > I am not exactly clear what is the intention with set(), > > That's the problem, nobody is clear with the current behavior. > The doc says "Set the default MAC address." and nothing else. > > > if there is > > single MAC I guess intention is to replace it with new one, but if > there > > are multiple MACs and one of them are already in the list intention > may > > be just to change the default MAC. > > The assumption in this patch is that "Set" means "Replace", not "Swap". > So this patch takes the approach 1/ Replace and keep Unique. > > > If above assumption is correct, what about following: > > > > set(MAC) { > > if only_default_mac_exist > > replace_default_mac > > > > if MAC exists in list > > swap MAC and list[0] > > else > > replace_default_mac > > } > > This approach 2/ is a mix of Swap and Replace. > The old default MAC destiny depends on whether > we have added the new MAC as "secondary" before setting as new default. > > > This swap prevents removing MAC side affect, does it make sense? > > Another approach would be 3/ to do an "Always Swap" > even if the new MAC didn't exist before, > you keep the old default MAC as a secondary MAC. > > And the current approach 0/ is to Replace default MAC address > without touching the secondary addresses at all. > > So we have 4 choices. > We could vote, roll a dice, or find a strong argument?
If there is only one MAC, MAC1, I would certainly expect Set(MAC2) to replace MAC1 with MAC2. If there is a list of MACs, I might expect Set() to select one in the list. And, if calling Set() with a MAC not in the list, we could either 1. return an error, or 2. implicitly add it to the list and then set it. If we implicitly add it to the list (i.e. follow option 2 described above), it should not be implicitly be removed from the list if another MAC is Set(). This behavior seems weird, which speaks for returning an error if setting a MAC not in the list (i.e. option 1). Disclaimer: I have no experience configuring NICs with multiple MACs - we promiscuous mode or one MAC address. So my input regarding multiple MACs is purely theoretical.
