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 8395C42417; Thu, 19 Jan 2023 15:38:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 77E2C410EE; Thu, 19 Jan 2023 15:38:20 +0100 (CET) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by mails.dpdk.org (Postfix) with ESMTP id B979B400D5 for ; Thu, 19 Jan 2023 15:38:19 +0100 (CET) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 50C2F5C0121; Thu, 19 Jan 2023 09:38:19 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Thu, 19 Jan 2023 09:38:19 -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=1674139099; x= 1674225499; bh=JjMRYbdBP4+Zau8fKZn051ukb7eVveUz89WcaBHs66Q=; b=U Z0HP6M2u+9R4ciCkBhpMpcssDz6++A7igIIKDy1vl8tbOZNGarx5pkeOx42DKkhR U1wD3mk4EOfrT/iqUZUULyI0aNIcDj3aRj8UeOM0moTENKTRjDkBH6XKKdcf9u3q LjmMGuTBm11zxJQtk507kDRqMXA0g0VoQLOTfsJDwsslmjQZQnX0Pjg4iMqa7fD7 JQ8VZk7xttaCg1D8vIn421fq12EEDtSQVUhg51lCt0cxrzaedjZ5f2ix02SHeeVr OMwjPH34wyAAYCibvAhbfgG7X1ECDeTc8GCS06ESkh+hJW9u+paxwcy4IaRDXLJS XxYZTNBrqHfhh8CbYo2Pw== 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=1674139099; x= 1674225499; bh=JjMRYbdBP4+Zau8fKZn051ukb7eVveUz89WcaBHs66Q=; b=V Z60cTBKzZGt/yyYYHSjuMGPvA8925fMfRjrSwfT7AU2CdS8YZ2b0gc6/+GoZ6u21 Ys4VT3r3/S6FC++VpdRWb4A+SebdISSt6b08RKJq72/HQjarrHGaARsPhlBpq1P2 6/1BPE+8KEXUDCAkEU90u6gai4vec/eqwYS3bVBWeJhg0y//hZc89GxjL6zsEews EsYxvKQqrM+4Xbiq08sawA6n1Dcfu/15Y+3R507OR9LWvW4V5tJWRwlOoZevoUQa W00Lzr7QveQri8XRqdWOwmnaNtpWu8oM0ZgZS8xUI3NloYSRwRwB9tSNLZMa58jR rMuMfKKkb1QiQkLfzUpUQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedruddutddgieejucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeegtddtleejjeegffekkeektdejvedtheevtdekiedvueeuvdei uddvleevjeeujeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 19 Jan 2023 09:38:18 -0500 (EST) From: Thomas Monjalon To: "lihuisong (C)" 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: Thu, 19 Jan 2023 15:38:17 +0100 Message-ID: <18892721.fAMKPKieAE@thomas> In-Reply-To: <12cc8e2a-fa7a-44a6-981a-ee91c50faafc@huawei.com> References: <20211011092811.55172-1-humin29@huawei.com> <2944156.uZKlY2gecq@thomas> <12cc8e2a-fa7a-44a6-981a-ee91c50faafc@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" 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, You missed some questions and comments below. 19/01/2023 10:57, lihuisong (C): > =E5=9C=A8 2023/1/18 16:26, Thomas Monjalon =E5=86=99=E9=81=93: > > 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(). Howeve= r, > >> 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_ad= drs > >> list. As a result, one MAC address occupies two indexes in the list. L= ike: > >> add(MAC1) > >> add(MAC2) > >> add(MAC3) > >> add(MAC4) > >> set_default(MAC3) > >> default=3DMAC3, filters=3DMAC1, 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 t= he > >> old default MAC when set default MAC. If user continues to do > >> set_default(MAC5), and the mac_addrs list is default=3DMAC5, filters= =3D(MAC1, > >> MAC2, MAC3, MAC4). At this moment, user can still see MAC3 from the li= st, > >> 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 r= est 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 { > >> =20 > >> uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation failure= s */ > >> =20 > >> - /** Device Ethernet link address. @see rte_eth_dev_release_port() */ > >> + /** > >> + * Device Ethernet link address. The index zero of the array is as t= he > > 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, s= truct 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 =3D 0; > >> struct rte_eth_dev *dev; > >> + uint32_t pool; > >> + int index; > >> int ret; > >> =20 > >> 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 =3D=3D NULL) > >> return -ENOTSUP; > >> =20 > >> + /* > >> + * 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 =3D eth_dev_get_mac_addr_index(port_id, addr); > >> + if (index > 0) { > >> + /* Remove address in dev data structure */ > >> + mac_pool_sel_bk =3D dev->data->mac_pool_sel[index]; > >> + ret =3D rte_eth_dev_mac_addr_remove(port_id, addr); > >> + if (ret < 0) { > >> + RTE_ETHDEV_LOG(ERR, "Delete MAC address from the MAC list of ethde= v 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] =3D 0; > > mac_pool_sel[index] is already reset in rte_eth_dev_mac_addr_remove(). > Yes, this can be deleted. > > > >> + } > >> ret =3D (*dev->dev_ops->mac_addr_set)(dev, addr); > >> if (ret < 0) > >> - return ret; > >> + goto out; > >> =20 > >> /* Update default address in NIC data structure */ > >> rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]); > >> =20 > >> return 0; > >> -} > >> =20 > >> +out: > >> + if (index > 0) { > >> + pool =3D 0; > >> + do { > >> + if (mac_pool_sel_bk & UINT64_C(1)) { > >> + if (rte_eth_dev_mac_addr_add(port_id, addr, > >> + pool) !=3D 0) > >> + RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool id(%u) in port %= u.\n", > >> + pool, port_id); > >> + } > >> + mac_pool_sel_bk >>=3D 1; > >> + pool++; > >> + } while (mac_pool_sel_bk !=3D 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.