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 A94764240B; Wed, 18 Jan 2023 09:26:54 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4FBA940DFD; Wed, 18 Jan 2023 09:26:54 +0100 (CET) Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by mails.dpdk.org (Postfix) with ESMTP id A84244003F for ; Wed, 18 Jan 2023 09:26:51 +0100 (CET) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.west.internal (Postfix) with ESMTP id 1B5DC3200754; Wed, 18 Jan 2023 03:26:50 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Wed, 18 Jan 2023 03:26:50 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm3; t=1674030409; x= 1674116809; bh=NsFZwlb6hBqjF36O8YqlltH8mNYCNBQ06Ga4kCGgHsA=; b=w lAUqsgkrwd/fZs6orojWkVMFxJ/Jw5Lio7fyx8eJ8JzXgbY2UOBMcwTez+QZ9iBM DKlDmGKbG2Nmd13t2hO58Yd8x1+Qpcdo9DWe5bDrxEPOlRrpBwjmlGlmnaSJzxlp Q8CQVYlx6FNP69HH3E8FB5zPrmAtc4GQihi76zD4l2lRQ3FOkuRYGqEkLg1PNeQP jVP8H7SHPPhtHWq8L0oxHLgFUcO/wfeYhxcArQ3W55mThYy4eNViprRlfvZqTaFo qCxcpe8eLH7Zp66tgWUEbpBQ9HANsmFUDzoXZ1dkJjnla+PS5GOkFpUb3xPBA9SM bzXmBLUi8j62q2q1lTbzA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm3; t=1674030409; x= 1674116809; bh=NsFZwlb6hBqjF36O8YqlltH8mNYCNBQ06Ga4kCGgHsA=; b=Z 7mFVHx3IYNqcl6OTZE65R3iX2YCpvADP1PEGtSiKViZT5ZDwCJeiLinDdJABeOS+ 6CEtT4QItk3Z2uDgYzufycJ+xnOTQnW6fDm+KEYYsXmHu13Fs90S//eG84qymNGP g0UdkpIOEvBUE12S56NolfltQxmNNL8n88SeG34x9vC3f/1YDk1Cgc2mxHOsHE5R qJI66azPvBfvUto3ek+gsjFgPc399oUSjDgCC7Ix+3K45G8VoXy0tUfPgRTC2d8D F5AapkLEw+c5VsO6K/9T6D6oJJm/8ROL4d93H0mgetTNjZLUXLnKTvdNTQEhiprB Exrkp0/sc25Joff/eiV3A== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedruddtjedguddvvdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvfevufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhho mhgrshcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqne cuggftrfgrthhtvghrnheptdejieeifeehtdffgfdvleetueeffeehueejgfeuteeftddt ieekgfekudehtdfgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilh hfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 18 Jan 2023 03:26:48 -0500 (EST) From: Thomas Monjalon To: Huisong Li Cc: dev@dpdk.org, andrew.rybchenko@oktetlabs.ru, ferruh.yigit@amd.com, liudongdong3@huawei.com, huangdaode@huawei.com Subject: Re: [PATCH V5] ethdev: fix one address occupies two indexes in MAC addrs Date: Wed, 18 Jan 2023 09:26:47 +0100 Message-ID: <2944156.uZKlY2gecq@thomas> In-Reply-To: <20221020093102.20679-1-lihuisong@huawei.com> References: <20211011092811.55172-1-humin29@huawei.com> <20221020093102.20679-1-lihuisong@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 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[]." > + 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. > + 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(). > + } > 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?