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.richard...@intel.com> >>> Sent: Thursday, February 16, 2023 9:05 AM >>> To: Ferruh Yigit <ferruh.yi...@amd.com>; techbo...@dpdk.org >>> Cc: Huisong Li <lihuis...@huawei.com>; Chengwen Feng >>> <fengcheng...@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.yi...@amd.com> >>>> Sent: Thursday, February 16, 2023 2:44 PM >>>> To: techbo...@dpdk.org >>>> Cc: Huisong Li <lihuis...@huawei.com>; Chengwen Feng >>>> <fengcheng...@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- >>>> lihuis...@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
