RE: MAC address set requires decision

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

Hello,
	Moving this discussion to the dev mailing list as per the request in Techboard meeting today. I could not find a single email with all the responses from Techboard members. So, some of the comments need to be repeated. But this is the base response.

Thanks,
Honnappa

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, February 16, 2023 9:05 AM
> To: Ferruh Yigit <ferruh.yigit@amd.com>; techboard@dpdk.org
> Cc: Huisong Li <lihuisong@huawei.com>; Chengwen Feng
> <fengchengwen@huawei.com>
> Subject: RE: MAC address set requires decision
> 
> Alternative suggestions:
> 
> 1. Don't allow "set" of mac address to value already in the list. The user must
> delete the entry manually first before adding it. Similarly, "add" fails if no
> default mac address is set. This ensures consistency by enforcing strict
> separation between the default mac address and the extra mac addresses.
> You can't have extra addresses without a default, and you can't have
> duplicates.
> 
> 2. Always enforce overlap between the two lists - once default mac address is
> set (automatically adding it to the mac addresses list), you can only replace
> the default mac address by using an already-added one to the list. In this
> case, the default address is only really an index into the address list, and no
> deletion ever occurs.
> 
> All the solutions below seem rather mixed to me, I'd rather see either strict
> overlap, or strict non-overlap. Both these cases make it that you need more
> calls to do certain tasks, e.g. with #2 to just replace mac address, you need to
> add, set, then delete, but we can always add new, clearly named APIs, to do
> these compound ops. On the plus side, with #2 we could make things doubly
> clear by changing the parameter type of "set" to be an index, rather than
> explicit mac, to make it clear what is happening, that you are choosing a
> default mac from a list of pre-configured options.
> 
> Regards,
> /Bruce
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@amd.com>
> > Sent: Thursday, February 16, 2023 2:44 PM
> > To: techboard@dpdk.org
> > Cc: Huisong Li <lihuisong@huawei.com>; Chengwen Feng
> > <fengchengwen@huawei.com>
> > Subject: MAC address set requires decision
> >
> > Hi Board,
> >
> > We need a decision on how MAC address set works in DPDK, is it
> > possible to vote offline so we can proceed with the patch for this release?
> >
> >
> > Can you please select one of:
> > a) Keep current implementation
> > b) Proposal 1
> > c) Proposal 2
> >
> > Details below, @Huisong feel free to add/correct if needed.
> >
> >
> >
> > Background:
> > DPDK supports multiple MAC address for MAC filtering. MAC addresses
> > are kept in a list, and index 0 is default MAC address.
> >
> > `rte_eth_dev_default_mac_addr_set()` -> sets default MAC [ set() ]
> > `rte_eth_dev_mac_addr_add()` -> adds MAC to list, if no default MAC
> > set this adds to index 0 [ add() ] `rte_eth_dev_mac_addr_remove()` ->
> > remove MAC from list [ del() ]
> >
> >
> > Problem:
> > When a MAC address is already in the list, if set() called, what will
> > be the behavior? Like:
> >
> > add(MAC1) => MAC1
> > add(MAC2) => MAC1, MAC2
> > add(MAC3) => MAC1, MAC2, MAC3
> > set(MAC2) => ???
> >
> >
> >
> > Current code behavior:
> > add(MAC1) => MAC1
> > add(MAC2) => MAC1, MAC2
> > add(MAC3) => MAC1, MAC2, MAC3
> > set(MAC2) => MAC2, MAC2, MAC3
> >
> > Problem with current behavior:
> > - A MAC address is duplicated in list (MAC2), and this leads different
> > implementation for different PMDs. Some removes MAC2 filter some not.
> > - Can't delete duplicate, because del() tries to delete first MAC it
> > finds and since it first finds default MAC address, fails to delete.
> > (We can fix del() if desicion to keep this implementation.)
> >
> >
> >
> > Proposal 1 (in the patchwork):
> > https://patches.dpdk.org/project/dpdk/patch/20230202123625.14975-1-
> > lihuisong@huawei.com/
> >
> > set(MAC) deletes MAC if it is in the list:
> >
> > add(MAC1) => MAC1
> > add(MAC2) => MAC1, MAC2
> > add(MAC3) => MAC1, MAC2, MAC3
> > set(MAC2) => MAC2, MAC3
> > set(MAC3) => MAC3
> >
> >
> > Disagreement on this proposal:
> > - It causes implicit delete of MAC addresses in the list, so MAC list
> > may shrink with multiple set() calls, this may be confusing
> >
> >
> >
> > Proposal 2 (suggested alternative):
> > set(MAC) {
> >     if only_default_mac_exist
> >         replace_default_mac
> >
> >     if MAC exists in list
> > 	swap MAC and list[0]
> >     else
> > 	replace_default_mac
> > }
> >
> > Intention here is to prevent implicit delete, swap is just a way to
> > keep MAC address in the list, like:
> > add(MAC1) => MAC1
> > add(MAC2) => MAC1, MAC2
> > add(MAC3) => MAC1, MAC2, MAC3
> > set(MAC2) => MAC2, MAC1, MAC3
> > set(MAC3) => MAC3, MAC1, MAC2
> >
> > Disagreement on this proposal:
> > - It is not clear user expects to keep swapped MAC address.
> >
> >
> > Thanks,
> > Ferruh