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 2DA9742415; Thu, 19 Jan 2023 10:58:00 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C2AD3410DD; Thu, 19 Jan 2023 10:57:59 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 6E5244068E for ; Thu, 19 Jan 2023 10:57:58 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4NyJ0y3BM8zRrGc; Thu, 19 Jan 2023 17:56:02 +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, 19 Jan 2023 17:57:55 +0800 Message-ID: <12cc8e2a-fa7a-44a6-981a-ee91c50faafc@huawei.com> Date: Thu, 19 Jan 2023 17:57:55 +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> <20221020093102.20679-1-lihuisong@huawei.com> <2944156.uZKlY2gecq@thomas> From: "lihuisong (C)" In-Reply-To: <2944156.uZKlY2gecq@thomas> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) 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/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. > >> 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. > >> 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. > >> + * @see rte_eth_dev_release_port() > Why referencing this function here? > >> + */ >> --- 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. > > > > .