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 DA1BAA00BE; Mon, 25 Apr 2022 08:42:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 78E6641132; Mon, 25 Apr 2022 08:42:57 +0200 (CEST) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id 25593410E6 for ; Mon, 25 Apr 2022 08:42:54 +0200 (CEST) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.57]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4KmwM164vMzCsMR; Mon, 25 Apr 2022 14:38:21 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Mon, 25 Apr 2022 14:42:51 +0800 Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs To: "lihuisong (C)" , Ferruh Yigit , Kevin Traynor , "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> <4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com> From: "Min Hu (Connor)" Message-ID: Date: Mon, 25 Apr 2022 14:42:50 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: <4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To kwepemi500012.china.huawei.com (7.221.188.12) 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 Hi, Ferruh, what do you think of this patch? 在 2021/11/8 14:55, lihuisong (C) 写道: > > 在 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. > > > > > > > > .