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 B12054240B; Wed, 18 Jan 2023 09:38:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4B9FD410DD; Wed, 18 Jan 2023 09:38:45 +0100 (CET) Received: from wout4-smtp.messagingengine.com (wout4-smtp.messagingengine.com [64.147.123.20]) by mails.dpdk.org (Postfix) with ESMTP id BCF294003F for ; Wed, 18 Jan 2023 09:38:42 +0100 (CET) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.west.internal (Postfix) with ESMTP id 999903200313; Wed, 18 Jan 2023 03:38:40 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Wed, 18 Jan 2023 03:38:41 -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=1674031120; x= 1674117520; bh=u4aKfXmJ32LLyO1rZhnemNh8/MYuJ+C6bpWU6JXaZDw=; b=k jyhvvwGoZv6NGNH03Iwa263MELNnxdgwEA+vPjbjrlhEZhDlm41LSg8KI9PGdxCK +zvLDfQi3O3LT6bPRVGXhflbguIglYI0jQDZ+/GTQf7eKlKyUKuarYPnbw/4K8d1 kcqKEwOaN6Ys8oPp+HNRCmjUuqyMJzBXJIZcJeuSfqLktiEsKgOqddmF66IEfmFN SGY9cP14N4LEcHC9XGoOwRXnmrbQh5Rte6T83eqf/vsijW8+05G27ML3j/8A094N sxGleAWjU4RCiYsvRosaaUiTf4zPZbJncp8lD8ZWdz4LzwDk3M+tCthJIQhGxebs svxsWr5PrUecAIpIBhatw== 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=1674031120; x= 1674117520; bh=u4aKfXmJ32LLyO1rZhnemNh8/MYuJ+C6bpWU6JXaZDw=; b=L Wblt6p2TwHHFPaJDZkYumblf4jAsfU1kammr1A5mHuYBm3z9IXukM3yiIH5q9Jbz nlC4O6V6oIwccvn0WYb7gtt9xdPhBUZNyJoL+HMpSiru0ySSkw4cBy6OIc41zwLK s/f2JqgK9P35sTc4ujT7VUqUf0cIce/k4Bn/qEo1j2IjmQGjQ8eGSNkXLseZWV9+ ZkX7WCm9sUHJdFZjPCNV4BcxJlXTEU/5+c31Gi/7mwedVPkfIiI41WA602SgfuDD wbOduODfnj0OCDDU2iFnIIWIPIcY+2ymlV5Qta6uOrVzuXPTONymRa4gWbcJ5oWl rzPfm1OFKwVaKu11029lQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedruddtjedguddvhecutefuodetggdotefrod 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:38:38 -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, jerinj@marvell.com, bruce.richardson@intel.com, david.marchand@redhat.com Subject: Re: [PATCH V5] ethdev: fix one address occupies two indexes in MAC addrs Date: Wed, 18 Jan 2023 09:38:37 +0100 Message-ID: <7970217.gsGJI6kyIV@thomas> In-Reply-To: <2944156.uZKlY2gecq@thomas> References: <20211011092811.55172-1-humin29@huawei.com> <20221020093102.20679-1-lihuisong@huawei.com> <2944156.uZKlY2gecq@thomas> 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 18/01/2023 09: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 may 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. Sorry for the confusion. I mean the opposite. With the new behavior, if we want the old default address to be kept in the list, we need to add it explicitly with rte_eth_dev_mac_addr_add. This is a behavior change and must be highlighted in the release notes. > > 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?