Re: MAC address set requires decision

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

On 2/23/2023 4:32 AM, Hemant Agrawal wrote:
> 
> On 22-Feb-23 11:25 PM, Honnappa Nagarahalli wrote:
>> -----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
> 
> Both of the above option seems good.  The option #1 above is safe, where
> you are making the mac address set as independent of mac filtering. Also
> making sure that mac filter are not messed up. However, the application
> needs to add error handling now to delete and set.
> 
> In the option #2,  I  assume, it will provide full backward
> compatibility i.e. the ethernet library can take care of the logic and
> application need not to implement anything extra ? If that is the case,
> it seems to be best.
> 

I think #2 is not fully backward compatible,

Previously set() replaces MAC in list[0], so multiple set() commands end
up with single MAC address. So device will filter only one MAC.

With #2, after first set(), application will need to add() first, later
set() and del() old MAC, if I understand correctly.

prev:
set(MAC1)
set(MAC2)
set(MAC3)

becomes:
set(MAC1)
add(MAC2)
set(MAC2)
del(MAC1)
add(MAC3)
set(MAC3)
del(MAC2)


Hence I think this complicates application that wants to just update
default MAC.


>  
> 
> Regards
> 
> Hemant 
> 
> 
>>>> -----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