From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7101041C49; Thu, 9 Feb 2023 09:32:41 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5ED6A40DF8; Thu, 9 Feb 2023 09:32:41 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id EF1084067B for ; Thu, 9 Feb 2023 09:32:38 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4PC96C5Mp2zRry5; Thu, 9 Feb 2023 16:30:11 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Thu, 9 Feb 2023 16:32:26 +0800 Message-ID: <246d2d27-d898-48b5-f708-62293dd281da@huawei.com> Date: Thu, 9 Feb 2023 16:32:26 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs From: "lihuisong (C)" To: Ferruh Yigit , Thomas Monjalon CC: , , , , , , , , , =?UTF-8?Q?Morten_Br=c3=b8rup?= References: <20221020093102.20679-1-lihuisong@huawei.com> <20230202123625.14975-1-lihuisong@huawei.com> <9b816855-2b5e-4296-d954-aa23cbd97a4c@amd.com> <6754145.G0QQBjFxQf@thomas> <4344ec91-666f-135c-e342-7b99f6b89ed9@huawei.com> <9acbcaaa-c38e-347c-13da-b433a42c8be8@huawei.com> In-Reply-To: <9acbcaaa-c38e-347c-13da-b433a42c8be8@huawei.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 在 2023/2/4 10:57, lihuisong (C) 写道: > > 在 2023/2/3 20:58, Ferruh Yigit 写道: >> On 2/3/2023 1:56 AM, lihuisong (C) wrote: >>> 在 2023/2/3 5:10, Thomas Monjalon 写道: >>>> 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. >>> Indeed. But we can see the following information. >>>  From the ethdev layer, this set() API always replaces the old default >>> address (index 0) without adding the old one. >>>  From the PMD layer, set() interface of some PMDs, such as i40e, ice, >>> hns3 and so on (as far as I know), >>> also do remove the hardware entry of the old default address. >> If we define behavior clearly, I think we can adapt PMD implementation >> according it, unless there is HW limitation. > Right. I think this is another point (issue 2/) to be discussed. > Namely, whether the old default address should be removed when set new > default one. > If we want to explicitly unify the behavior of all PMDs in ethdev > layer as described above, > there may be no problem if do the following: > 1) In the ethdev layer, remove the old default address if the old one > is exist. > 2) For PMD i40e, ice and hns3, remvoe the code of deleting the old > default address before adding the new one. >    For other PMDs, we probably don't need to do anything because they > have supported remove_addr() API. >    (Without explicitly removing the old default address, I don't know > if their hardware or firmware >     removes the old one when set a new address. But, we explicitly > remove the old one in ethdev layer now, >     I'm not sure if this has an effect on these PMDs.) >>>>> 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? >>> According to the implement of set() in ethdev and PMD layer, it always >>> use "Replace", not "Swap". >>> If we use "Swap" now, the behavior of this API will be changed. >>> I'm not sure if the application can accept this change or has other >>> effects. >>> >> This patch is also changing behavior, because of implied remove address, >> same concern is valid with this patch. > Indeed, it changes the behavior. > But this patch only resolves the problem (issue 1/) that the entries > of the MAC address list possibly are not uniques. > Fixing it may be little impact on the application. >> >> >> As I checked again current implementation may have one more problem >> (this from reading code, I did not test this): >> add(MAC1) -> MAC1 >> add(MAC2) -> MAC1, MAC2 >> set(MAC2) -> MAC2, MAC2 >> del(MAC2) -> FAILS >> >> This fails because `rte_eth_dev_mac_addr_remove()` can't remove default >> MAC, and it only tries to remove first address it finds, it can't find >> and remove second 'MAC2'. >> I wasn't too much bothered with wasting one MAC address slot, so wasn't >> sure if a change is required at all, but if above analysis is correct I >> think this is more serious problem to justify the change. > Your analysis is fully correct. >> >> >> I don't think always swap (option /3) is good idea, specially for single >> MAC address exists case, and current case has (option 0/) has mentioned >> problems. > +1 >> Remaining ones are mix of swap and replace (option 2/) and this patch >> (option /1). >> >> I think mix of swap and replace (option 2/ above) has some benefits: >> - It always replaces default MAC >> - Prevents duplication MAC address in the list >> - Doesn't implicitly remove address from list > As far as I know, the first entry (index 0) always be the default > address in all PMDs, > but it's not documented. (So this patch did it, that's what was > discussed earlier). > The 'Swap' may be inappropriate. It may need to be discussed. >> >> BUT, if the agreement is this patch (option 1/) I am OK with that too, I >> just want to make sure that it is discussed. >> >> >>> BTW, it seems that the ethernet port in kernel also replaces the old >>> address if we modify the one. >>> Use the test command: ifconfig eth0 hw ether new_mac >> For default MAC address it is more clear that intention is to replace >> it, but question is about what to do with the list of MAC addresses. > Hi Ferruh and Thomas, > > As mentioned above, they are actually two problems (issue /1 and issue > /2). > Can we deal with them separately? > #1 For issue /1, it's really a problem. This patch is responsible for it. > #2 For issue /2, I will send a RFC to discuss as described above. >      It may require the participation of all PMD maintainers. > > What do you think? > > Hi Ferruh and Thomas, What do you think of the above proposal? Looking forward to your reply. /Huisong >> >> . > .