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 AFC3A424A5; Sat, 28 Jan 2023 02:38:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4C8F140150; Sat, 28 Jan 2023 02:38:39 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id C166B40146 for ; Sat, 28 Jan 2023 02:38:37 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.55]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4P3cRj0zDMzJqDK; Sat, 28 Jan 2023 09:34:09 +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; Sat, 28 Jan 2023 09:38:33 +0800 Message-ID: <13dcf41f-ff78-d215-1410-a2ebbe6ba6c3@huawei.com> Date: Sat, 28 Jan 2023 09:38:04 +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 V5] ethdev: fix one address occupies two indexes in MAC addrs To: Thomas Monjalon CC: , , , , References: <20211011092811.55172-1-humin29@huawei.com> <2944156.uZKlY2gecq@thomas> <12cc8e2a-fa7a-44a6-981a-ee91c50faafc@huawei.com> <18892721.fAMKPKieAE@thomas> From: "lihuisong (C)" In-Reply-To: <18892721.fAMKPKieAE@thomas> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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/1/19 22:38, Thomas Monjalon 写道: > Hi, > > You missed some questions and comments below. Sorry for my later reply. I just got back from vacation. Please take a look again. > > 19/01/2023 10:57, lihuisong (C): >> 在 2023/1/18 16:26, Thomas Monjalon 写道: >>> 20/10/2022 11:31, Huisong Li: >>>> 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 indexes in the list. Like: >>>> add(MAC1) >>>> add(MAC2) >>>> add(MAC3) >>>> add(MAC4) >>>> set_default(MAC3) >>>> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4 >>> I agree it would be simpler to ensure that the addresses are uniques. >>> >>>> 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. >>> If MAC3 is not removed with rte_eth_dev_mac_addr_remove() by the app, >>> MAC3 packets should be received. >>> The MAC address should not be removed by the PMD. Please review another thread you pulled. >>> >>>> So this patch adds a remove operation in set default MAC API documents >>>> this behavior. >>> Let's be clear here: only the new default address is removed from the rest of the list. ok. Your expression is more explicit. >>> >>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") >>>> Cc: stable@dpdk.org >>>> --- a/lib/ethdev/ethdev_driver.h >>>> +++ b/lib/ethdev/ethdev_driver.h >>>> @@ -116,7 +116,12 @@ struct rte_eth_dev_data { >>>> >>>> uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failures */ >>>> >>>> - /** Device Ethernet link address. @see rte_eth_dev_release_port() */ >>>> + /** >>>> + * Device Ethernet link address. The index zero of the array is as the >>> It should be "addresses" as there can be multiple. >>> >>> What means "as" above? >>> Can we say the first entry (index zero) is the default address? >>> >>>> + * index of the default address, and other indexes can't be the same >>> You can split the sentence in 2 instead of ", and". >>> indexes -> entries >>> can't -> cannot >>> >>>> + * as the address corresponding to index 0. >>> simpler: as the default address. Above all will be fixed. Thank you for your suggestion. >>> >>>> + * @see rte_eth_dev_release_port() >>> Why referencing this function here? It was here before, and I didn't care too much, but I think it is redundant. I will remove it. >>> >>>> + */ >>>> --- a/lib/ethdev/rte_ethdev.c >>>> +++ b/lib/ethdev/rte_ethdev.c >>>> @@ -4498,7 +4498,10 @@ rte_eth_dev_mac_addr_remove(uint16_t port_id, struct rte_ether_addr *addr) >>>> int >>>> rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr) >>>> { >>>> + uint64_t mac_pool_sel_bk = 0; >>>> struct rte_eth_dev *dev; >>>> + uint32_t pool; >>>> + int index; >>>> int ret; >>>> >>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>>> @@ -4517,16 +4520,50 @@ rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct rte_ether_addr *addr) >>>> if (*dev->dev_ops->mac_addr_set == NULL) >>>> return -ENOTSUP; >>>> >>>> + /* >>>> + * 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[]. >>>> + */ >>> Make is simpler: >>> "Keep address unique in dev->data->mac_addrs[]." >> Ack >>>> + index = eth_dev_get_mac_addr_index(port_id, addr); >>>> + if (index > 0) { >>>> + /* Remove address in dev data structure */ >>>> + mac_pool_sel_bk = dev->data->mac_pool_sel[index]; >>>> + ret = rte_eth_dev_mac_addr_remove(port_id, addr); >>>> + if (ret < 0) { >>>> + RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethdev port %u.\n", >>>> + port_id); >>> It is not clear with this log that it failed. >> Adjust as follows? >> "Delete old address entry from the MAC list of ethdev port %u.\n" >>>> + return ret; >>>> + } >>>> + /* Reset pool bitmap */ >>>> + dev->data->mac_pool_sel[index] = 0; >>> mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove(). >> Yes, this can be deleted. >>>> + } >>>> ret = (*dev->dev_ops->mac_addr_set)(dev, addr); >>>> if (ret < 0) >>>> - return ret; >>>> + goto out; >>>> >>>> /* Update default address in NIC data structure */ >>>> rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]); >>>> >>>> return 0; >>>> -} >>>> >>>> +out: >>>> + if (index > 0) { >>>> + pool = 0; >>>> + do { >>>> + if (mac_pool_sel_bk & UINT64_C(1)) { >>>> + if (rte_eth_dev_mac_addr_add(port_id, addr, >>>> + pool) != 0) >>>> + RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %u.\n", >>>> + pool, port_id); >>>> + } >>>> + mac_pool_sel_bk >>= 1; >>>> + pool++; >>>> + } while (mac_pool_sel_bk != 0); >>>> + } >>>> + >>>> + return ret; >>>> +} >>> Can we avoid this rollback by removing the address after mac_addr_set() succeed? >> No, the new default address will be removed if we do that. >> we need to remove it first if the new default address has been added as a >> non-default MAC address to the MAC list. > OK yes. > > > .