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 A93E7A0C4B; Mon, 8 Nov 2021 07:55:40 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30EE940151; Mon, 8 Nov 2021 07:55:40 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 7431F40040 for ; Mon, 8 Nov 2021 07:55:10 +0100 (CET) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.54]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4HnhZH6RN4zcbBR; Mon, 8 Nov 2021 14:50:15 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.15; Mon, 8 Nov 2021 14:55:02 +0800 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.2308.15; Mon, 8 Nov 2021 14:55:01 +0800 Message-ID: <4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com> Date: Mon, 8 Nov 2021 14:55:01 +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 To: Ferruh Yigit , Kevin Traynor , "Min Hu (Connor)" , "dev@dpdk.org" CC: References: <20210922033630.41130-1-humin29@huawei.com> <20211011092811.55172-1-humin29@huawei.com> <8decd3d4-4370-2f89-44d5-3e0dd848a628@intel.com> <31cd92b1-d388-bd50-963a-8a85a924c0a2@redhat.com> <82e8ba2a-d9bb-5f73-d878-c9cf0801acef@intel.com> <792354b1-bd2d-3946-72c8-2ab4fc8d07bf@huawei.com> <1aed24f9-c2e2-5ffd-cb08-4ce7b7a8588a@intel.com> From: "lihuisong (C)" In-Reply-To: <1aed24f9-c2e2-5ffd-cb08-4ce7b7a8588a@intel.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 Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs 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 Sender: "dev" 在 2021/10/26 18:21, Ferruh Yigit 写道: > On 10/22/2021 3:04 AM, lihuisong (C) wrote: >> >> 在 2021/10/21 16:30, Ferruh Yigit 写道: >>> On 10/21/2021 3:05 AM, lihuisong (C) wrote: >>>> >>>> 在 2021/10/21 0:32, Ferruh Yigit 写道: >>>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote: >>>>>> On 20/10/2021 08:41, Ferruh Yigit wrote: >>>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote: >>>>>>>> Hi Ferruh >>>>>>>> >>>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道: >>>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote: >>>>>>>>>> From: Huisong Li >>>>>>>>>> >>>>>>>>>> The dev->data->mac_addrs[0] will be changed to a new MAC >>>>>>>>>> address when >>>>>>>>>> applications modify the default MAC address by >>>>>>>>>> rte_eth_dev_default_mac_addr_set() API. However, If the new >>>>>>>>>> default >>>>>>>>>> MAC address has been added as a non-default MAC address by >>>>>>>>>> rte_eth_dev_mac_addr_add() API, the >>>>>>>>>> rte_eth_dev_default_mac_addr_set() >>>>>>>>>> API doesn't remove it from dev->data->mac_addrs[]. As a >>>>>>>>>> result, one MAC >>>>>>>>>> address occupies two index capacities in dev->data->mac_addrs[]. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Connor, >>>>>>>>> >>>>>>>>> I see the problem, but can you please clarify what is the >>>>>>>>> impact to the end user? >>>>>>>>> >>>>>>>>> If application does as following: >>>>>>>>>    rte_eth_dev_mac_addr_add(MAC1); >>>>>>>>>    rte_eth_dev_mac_addr_add(MAC2); >>>>>>>>>    rte_eth_dev_mac_addr_add(MAC3); >>>>>>>>>    rte_eth_dev_default_mac_addr_set(MAC2); >>>>>>>>> >>>>>>>>> The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which >>>>>>>>> has 'MAC2' duplicated. >>>>>>>>> >>>>>>>>> Will this cause any problem for the application to receive the >>>>>>>>> packets >>>>>>>>> with 'MAC2' address? >>>>>>>>> Or is the only problem one extra space used in >>>>>>>>> 'dev->data->mac_addrs[]' >>>>>>>>> without any other impact to the application? >>>>>>>> I think it's just a waste of space. >>>>>>> >>>>>>> True, it is a waste. But if there is no other visible user >>>>>>> impact, we can >>>>>>> handle the issue with lower priority and clarifying the impact >>>>>>> in commit log >>>>>>> helps to others. >>>>>>> >>>>>>>>> >>>>>>>>>> This patch adds the logic of removing MAC addresses for this >>>>>>>>>> scenario. >>>>>>>>>> >>>>>>>>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") >>>>>>>>>> Cc: stable@dpdk.org >>>>>>>>>> >>>>>>>>>> Signed-off-by: Huisong Li >>>>>>>>>> Signed-off-by: Min Hu (Connor) >>>>>>>>>> --- >>>>>>>>>> v2: >>>>>>>>>> * fixed commit log. >>>>>>>>>> --- >>>>>>>>>>    lib/ethdev/rte_ethdev.c | 15 +++++++++++++++ >>>>>>>>>>    1 file changed, 15 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >>>>>>>>>> index 028907bc4b..7faff17d9a 100644 >>>>>>>>>> --- a/lib/ethdev/rte_ethdev.c >>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.c >>>>>>>>>> @@ -4340,6 +4340,7 @@ int >>>>>>>>>>    rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct >>>>>>>>>> rte_ether_addr *addr) >>>>>>>>>>    { >>>>>>>>>>        struct rte_eth_dev *dev; >>>>>>>>>> +    int index; >>>>>>>>>>        int ret; >>>>>>>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>>>>>>>>> @@ -4361,6 +4362,20 @@ >>>>>>>>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct >>>>>>>>>> rte_ether_addr *addr) >>>>>>>>>>        if (ret < 0) >>>>>>>>>>            return ret; >>>>>>>>>>    +    /* >>>>>>>>>> +     * If the address has been added as a non-default MAC >>>>>>>>>> address by >>>>>>>>>> +     * rte_eth_dev_mac_addr_add API, it should be removed from >>>>>>>>>> +     * dev->data->mac_addrs[]. >>>>>>>>>> +     */ >>>>>>>>>> +    index = eth_dev_get_mac_addr_index(port_id, addr); >>>>>>>>>> +    if (index > 0) { >>>>>>>>>> +        /* remove address in NIC data structure */ >>>>>>>>>> +        rte_ether_addr_copy(&null_mac_addr, >>>>>>>>>> + &dev->data->mac_addrs[index]); >>>>>>>>>> +        /* reset pool bitmap */ >>>>>>>>>> +        dev->data->mac_pool_sel[index] = 0; >>>>>>>>>> +    } >>>>>>>>>> + >>>>>>>>> >>>>>>>>> Here only 'dev->data->mac_addrs[]' array is updated and it >>>>>>>>> assumes >>>>>>>>> driver removes similar duplication itself, but I am not sure >>>>>>>>> if this is >>>>>>>>> valid for all drivers. >>>>>>>>> >>>>>>>>> If driver is not removing the duplicate in the HW >>>>>>>>> configuration, the driver >>>>>>>>> config and 'dev->data->mac_addrs[]' will diverge, which is not >>>>>>>>> good. >>>>>>>> The same MAC address does not occupy two HW entries, which is >>>>>>>> also a >>>>>>>> waste for HW. After all, HW entry resources are also limited. >>>>>>>> The PMD should also take this into account. >>>>>>>> So, I think, we don't have to think about it here. >>>>>>> >>>>>>> I am not sure all PMD take this into account, I briefly checked >>>>>>> the ixgbe >>>>>>> code and I am not sure if it handles this. >>>>>>> >>>>>>> Also it is possible to think that this responsibility is pushed >>>>>>> to the >>>>>>> application, like application should remove a MAC address before >>>>>>> setting >>>>>>> it as default MAC... >>>>>>> >>>>>> >>>>>> Yes, the API view is more important than saving one entry in an >>>>>> array. From API perspective with this patch, >>>>>> >>>>>> set_default(MAC1) >>>>>> add(MAC2) >>>>>> add(MAC3) >>>>>> add(MAC4) >>>>>> default=MAC1, Filters=MAC2, MAC3, MAC4 >>>>>> >>>>>> set_default(MAC2) >>>>>> default=MAC2, Filters= MAC3, MAC4 >>>>>> >>>>>> set_default(MAC3) >>>>>> default=MAC3, Filters= MAC4 >>>>>> >>>>>> set_default(MAC4) >>>>>> default=MAC4, Filters= >>>>>> >>>>>> set_default(MAC5) >>>>>> default=MAC5, Filters= >>>>>> >>>>>> Did I get it right? If so, it seems wrong to silently remove the >>>>>> filters. In which case, it would be easier to just not remove >>>>>> them in the first place (current behaviour). >>>>>> >>>>> >>>>> Yep, this is the updated behavior. And agree it looks wrong when you >>>>> show like this. (btw, this is only ethdev record of MAC filters, what >>>>> is updated in this patch, HW still may be keeping all filters.) >>>> >>>> Whether HW saves all filters depends on the implementation of the >>>> set_default() >>>> >>>> in the driver. According to the implementation of this API of all >>>> PMDs, some drivers >>>> >>>> will first remove the old default MAC in HW and then add the new >>>> one when calling >>>> >>>> the set_default(). I am not sure if the HW that didn't do this >>>> would remove the old >>>> >>>> default MAC. If not, we may need to standardize this API in the >>>> ethdev layer. >>>> >>>>> >>>>>> If they really need to be removed from the filter list when they >>>>>> are set_default(), then perhaps they should be restored to it >>>>>> when there is a new set_default(). >>>>>> >>>>> >>>>> I am for keeping current behavior. Application always can >>>>> explicitly remove a >>>>> MAC filter before setting it default if required. >>>> >>>> But application can not remove the duplicate MAC if the MAC is the >>>> current default >>>> >>>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed >>>> to remove. >>>> >>> >>> But can do other-way around, first remove (the non default one), later >>> 'rte_eth_dev_default_mac_addr_set()'. >> >> This introduces a usage dependency on the user. We don't have a >> statement in some place >> >> for the dependency. What's more, if the user does not follow this >> dependency, the user >> >> will no longer be able to remove the MAC. >> >> So it may be more appropriate to deal with problem in ethdev layer. >> >> >> **Scheme A:** >> >> The decision is left to the user, but there are usage dependency and >> irremovable possibility.** >> ** >> >> *Scheme B:* >> >> index = eth_dev_get_mac_addr_index(port_id, addr); >> if (index > 0) { >>      mac_pool_sel_bk = dev->data->mac_pool_sel[index]; >>      rte_eth_dev_mac_addr_remove(port_id, addr); >> } >> ret = (*dev->dev_ops->mac_addr_set)(dev, addr); >> if (ret < 0) { >>      if (index > 0) { >>          rte_eth_dev_mac_addr_add(port_id, addr, 0); >>          dev->data->mac_pool_sel[index] = mac_pool_sel_bk; >>      } >> >>      return ret; >> } >> >> * Scheme C:* >> >>   Use the method in this patch. It assumes that the driver has only >> one HW entry for a MAC. >> >> What do you think we should do? >> > > If the impact is only loosing one entry in the array without any > functional > effect, I am OK to keep behavior as it is. > > If there is more motivation for fix, I would prefer option B to be > sure all > drivers behave same by explicit remove. > . Hi, Ferruh There may be a functional problem in one scenario if we don't fix this. Testpmd does the following steps: 1) add MAC1/2/3 mac_addr add 0 MAC1 mac_addr add 0 MAC2 mac_addr add 0 MAC3 2) then set new default MAC3 mac_addr set 0 MAC3 3）show mac_addrs list MAC3 MAC1 MAC2 MAC3 4）then set new default MAC4 mac_addr set 0 MAC4 5) now, the mac_addrs list is as follows: MAC4 MAC1 MAC2 MAC3 As a result, the network engine cannot receive packets from MAC3. But application can see the MAC3 in mac_addrs list. Btw, rte_eth_dev_default_mac_addr_set() will replace the old default MAC with the new one, and the old one will be deleted. So, we should fix it. What do you think? Thanks.