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 B747B41BB1; Thu, 2 Feb 2023 19:09:29 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A2F9B410D7; Thu, 2 Feb 2023 19:09:29 +0100 (CET) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2082.outbound.protection.outlook.com [40.107.223.82]) by mails.dpdk.org (Postfix) with ESMTP id 7386240693 for ; Thu, 2 Feb 2023 19:09:28 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ocniOFw4Jr/Ebq6uRpvT6nDCR2VmqMaMqPb7wMhPMKrzzh/WbMgsvX11BNXvrxP/9k6U0f2xE9QNjDyAXd/48U+UB609M92AjL18NlK3WJIPk4A5lYx3aFzaLA7yRn79bSvoW2mmvWVtbf+TW9jm7yHKBBGoQ2Q6wgHWBEDdSPh/OnV0BOrdesu7SNJFviIkh/jViswpRQ/4Xe32B8LUzA6GiLOeRTwmMUQDLSJibzt/L46TPnZS+6xm+3VcWBRxwGhhz1bgH1p6uj18EvDkiAtMDECqgs1IR3Fm7JSuiJatYzUUVOrVAgb3kGmF4CbEyIYjV9JKRmyJyxwWlUq7ZA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=1EHqSe/gv9MS4HwT1mY10xmUlGQAQKMne94ZXjMtH+w=; b=dow/rRjgz5qVzbtCwF+gRkCtdqOyygDwJaIci+Z70sDSM0wWdEk1pb2+eFOzlESOHrp0lLknfyJWRK0qn3Ph8wWATp1Lrv9QJNybSecxlBo7FSSUHxnipJmghY7CKsx+a52ML0lBd9/qoxKVQyitr9Jt6iQkxJLTYbWTplajjmXrqvUdYUb8T8u1CVjSGPzX1ARFd/wSPgJnOh6NZCSjcKFN6gF6YyS2GBcmH4nwGUEKKDLgXgexhVqYn9V13Qfd1tzklLhYcD7uMi2fIfVMxFawuVXwoctKeJSPKQAsSqs96o5O0m7BrOv59SY0rGTyrVVtP5Iv70tbUFr4wsQImg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1EHqSe/gv9MS4HwT1mY10xmUlGQAQKMne94ZXjMtH+w=; b=IBnGoMOQys82mN3gEkN1s0zJ2OY4rhSOz8F9Mmc/JBWf7XI/JPTrbQiF++8GeZ+rLATDLoudDCqR2DROnmyFq0PDai6myQjz31uxbQy0QpawY1+KpjxRhS76kM8zAUjCvu9pRsIkc5ctewnC5dP91DmdlKvWHhEGJLXjEbzuKT4= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; Received: from CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) by SA0PR12MB4576.namprd12.prod.outlook.com (2603:10b6:806:93::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6064.25; Thu, 2 Feb 2023 18:09:26 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::4807:1f44:5e04:e05a]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::4807:1f44:5e04:e05a%8]) with mapi id 15.20.6064.024; Thu, 2 Feb 2023 18:09:26 +0000 Message-ID: <9b816855-2b5e-4296-d954-aa23cbd97a4c@amd.com> Date: Thu, 2 Feb 2023 18:09:20 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Content-Language: en-US To: Huisong Li , dev@dpdk.org Cc: thomas@monjalon.net, andrew.rybchenko@oktetlabs.ru, liudongdong3@huawei.com, huangdaode@huawei.com, fengchengwen@huawei.com References: <20221020093102.20679-1-lihuisong@huawei.com> <20230202123625.14975-1-lihuisong@huawei.com> From: Ferruh Yigit Subject: Re: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs In-Reply-To: <20230202123625.14975-1-lihuisong@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: PR3P250CA0005.EURP250.PROD.OUTLOOK.COM (2603:10a6:102:57::10) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|SA0PR12MB4576:EE_ X-MS-Office365-Filtering-Correlation-Id: d0d40f96-ced3-444c-34e4-08db05489ea6 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 3QP1Ereunl0tur89Ql4V2xKCpMImBG3FcE7/kQL/YB3kWacPcnmtG0FSkEFfsxO2Zcr2RNjbUja/PHR4LxxRzB1mNkfzMtU8b6UYOLenrENdoGL3xV9GpnqYJLUx/fcvdMhr1zoB9uv6+xrAA6lT6tbW1roIkkMlInHuoff269GX16aS1NOqdm91m/hSo2Ltn+RryMdGjH8wBvPQp4uqm87zl6xDCfhQ8jsQe1rrufbw/tViiN7UkOG4kNAksbJ4xgx4f5nLzpWKahuBTO2QyaZsPH4DgqPRB72bL7aKVApGNk0E/nO4SoYgEBmTgigFgtNTSE4xTUW7UnIHgOO2Xh8ljSsi4Vvgjt68k+teWJs5pECrAyDhkOFKYGtAxwt2mlJQN6I7r0bpjamMnxAS31MKTjnOuyhVqKNRPLHFZxvYh8vA8uWvfWFT63Ndg13BVJzSpfLvTNSfSUOEIxbi6wg5oCJ+tk9Lt/Nx+tidGyB/7Bcpp8nz6G9QZ5GFK5YNWihXWWNksEWJRmn70F1IFlVB9P5UgATnSd66nZsxWwAX92quiWLQ9GvRvO8KDU3L682zJBpJwnelWx62SW0cBxVnAUq10aPkV2vIPcADiC4tcsU/ZGk3g4pe6NN06eEG/wFccLIBR+wcDZCy4Luo4K2q30kae1C6MWtIuzSGPbr87M1stAcuju1lYE/A6nAvEhrtgGmgcKb7TcIHdbaCE3NPM+5UFEl3QP77shpEvSo= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH2PR12MB4294.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230025)(4636009)(376002)(136003)(346002)(39860400002)(396003)(366004)(451199018)(5660300002)(31686004)(44832011)(41300700001)(8936002)(66556008)(478600001)(4326008)(31696002)(66476007)(66946007)(8676002)(6512007)(53546011)(6506007)(83380400001)(36756003)(6666004)(186003)(2616005)(26005)(2906002)(38100700002)(86362001)(316002)(6486002)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NWdQUGVGdXlOdXg3VVowWTd6bHlpTzZlVFZZNFREUDNmbjU0Q0NJa09yNFUv?= =?utf-8?B?bSsyZW1FeUN6c0w0dnZPRjVSQkdJM3JJVWJkaEswdy9wZ3phWWpaSG45Y2hr?= =?utf-8?B?NjlOcWQyV1BLbnFqN2dYckFvdTM4M2l2UjI1ajhOZzRSL1VpNmNZVUpDOGR2?= =?utf-8?B?WkI3b0dZRkFFZWl0MFh2c0owYXMycmMyOG9xd3JMZ0RHZVVEMUY0ZjV2azZK?= =?utf-8?B?azUzTjRMRXgzMnYxbXgxczFsQllzVFFxQ3dHYVlkM1ZEejRmd2ZZV0tCSlk1?= =?utf-8?B?LzhpVldXRm5UK2cvcVVsMGMwSkg5aHJYWEFFd0I1bFoyY1prU3FMbk9aN3pW?= =?utf-8?B?d2dDUDJId2Jtai9IUExlNnFYMkFQQ1hTN3NhV3I1aTRtWHcyaVVkcHRiSmow?= =?utf-8?B?bHRoeGYzWDZjZVZremVUNUR6SFJYRHZaNjFoVWR0Y21tcDR4MzRhUitwVHdK?= =?utf-8?B?ay9INjV5Y0FOQ2hFUGFCZGVVMHltUTIzUzROeHVkb3pGUGRhc2VzYWRmaU9I?= =?utf-8?B?djBrRTNvc3E5WEphRVU1WDBOM0ZOQkFOcVdFOHBrTiszd0swakpvUzNiOEtK?= =?utf-8?B?T1FBdWlBZFR2NDVkaEF3OFNUR1NrVmtQOVhZbGpCRlZ5U2pNNDhvbjgwUnpy?= =?utf-8?B?Tm9VTFk0YmxEYXNsQkJ5RXZBWXJSRDlodFRCR0hrZkFsU1RUTDNmR1krSWY2?= =?utf-8?B?S3BVbjhsanVSVERPODR6OEt0VU83ZnRUejhoVktIS0dmZHBzTlUwWk5Uck50?= =?utf-8?B?bmVtb2VHNXRDSTZ4Q2Q5SEtPenNHbnRpSkM1eElOU2hDUFIrUGxmRjNUTUpq?= =?utf-8?B?Ykl3S1RDUzlnaUFMbHJIbkJqWHBZbGZnNUtlSVN2aGI5bXBwcHFRbU0wNENO?= =?utf-8?B?UDJLOTNwSjFIUEFQYXBqUy9NTlBSZ3k1bFV6eXRoRDg0V1VEVDRETWtCWjhV?= =?utf-8?B?aWRGdmY5eXRPYVpSRi9BaWhEVVR1dEQxSjFTOHpNZkFmQ2tsY3dQekY2OUJ6?= =?utf-8?B?bVVOV0tQWWpUb1RwQ1dTamozZjBxS1BEL2x3ODhXejgrQk5ZZDk2R0hucGI3?= =?utf-8?B?OTQ2Mm95cU9IVzhnMDJQdVRDTGYzdVo2QlZ3b05YT3dQTG1ubEJKVE1YME1q?= =?utf-8?B?MzUrb29Kd3FoRTA2OHBEUy9XbnY0KytIajhLTldmaXlxbDZsemZXTkFKSC9G?= =?utf-8?B?SjdGYm9ValIrQVZCSVhGcEkySXpGbEx2Vm5vUGZ3b0R2bEswRnZSSWZnVE5D?= =?utf-8?B?WHpBbWh3bXJBT3ZiajYvMnNPMWp2YXQrOUU3RWtFa2lDdkFDamhYNzUwTWl6?= =?utf-8?B?TENYYy9zT1dmWGo3WmF5STR6RlVFQ3Q3eUx0VjQ2cHFHc1lsb2FGY0hXTGVZ?= =?utf-8?B?eUMxKzFKQk9TUFNKd2MvNENGTDhlbUhVa054WUxZbG9YRlJPcUNmZHhVMmd5?= =?utf-8?B?NlNERXlaRDNLWk92RTRKa3BqdXBSWm14WW5NaDI5aFQyT3FjZERhWjliQmRl?= =?utf-8?B?YkFVekRybDR5ODVQdSs3VTI5dTZWU2RWbVh5VnkwU2gvbVEzeldONWgvWkJ3?= =?utf-8?B?Wjk5bmhtNUxFMnM2QmpsRXpmWi9FVGpqRjUyWE53TkZwNGcwQ1h5U3Rlb205?= =?utf-8?B?bm1uZWlRM0JwcGF6MnlVbXhRMFlYVVEvWm85S0R2UVNFUGdFbXVJWWpWQ0ti?= =?utf-8?B?bGhEWllseU1objErQ1JIclJRd21DQWhBSkNETkkwaEJTYjE3N2sxM3phcXJU?= =?utf-8?B?eGN1Q1dSMzFJNEZZNFdaVGd3eFQrRS9VQ3dsRDV6VFR4NWVKZldGOHJRZ0F4?= =?utf-8?B?YnJ2ZEVBMUdJVmM2SWJnZUt2L0xDbnZCRVdzU05ORHcwQ212bi9oU1l1VXNO?= =?utf-8?B?d1lxMVZ4dTdPazFsY2ZkZmRWVkdtMThpQ3Uwb05HQ0Zzdko0QVkrSDFSVU84?= =?utf-8?B?Z2Y5L2FsYWcyVkhIeVVBdGQ0STdkMG9tOGkyL1ZhZHpVOGxLTTA3cTJ6dDZG?= =?utf-8?B?NlhiWktISmlmaFFoZUdrcW82S1FQZi9xdVpTL3lGSGZjSVNQMm8zemJKcVc4?= =?utf-8?B?U01WZFhrVm1TVGtCNXV0Rzh4K2RnSmxHdUlXUE53T1pnNTBGek1YbVphNnNI?= =?utf-8?Q?4CVPyPtyEPLgde7NscgZrXvbw?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: d0d40f96-ced3-444c-34e4-08db05489ea6 X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2023 18:09:26.3502 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: MjkzNXapR7zoz9e6OCkGWh7TTTflkXYMAC8eA8VODLQnvQtHKYmJLW8VySfw2LHD X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4576 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 On 2/2/2023 12:36 PM, Huisong Li wrote: > 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 entries in the list. Like: > add(MAC1) > add(MAC2) > add(MAC3) > add(MAC4) > set_default(MAC3) > default=MAC3, the rest of the list=MAC1, MAC2, MAC3, MAC4 > Note: MAC3 occupies two entries. > > 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. > > So need to ensure that the new default address is removed from the rest of > the list if the address was already in the list. > Same comment from past seems already valid, I am not looking to the set for a while, sorry if this is already discussed and decided, if not, I am referring to the side effect that setting MAC addresses cause to remove MAC addresses, think following case: add(MAC1) -> MAC1 add(MAC2) -> MAC1, MAC2 add(MAC3) -> MAC1, MAC2, MAC3 add(MAC4) -> MAC1, MAC2, MAC3, MAC4 set(MAC3) -> MAC3, MAC2, MAC4 set(MAC4) -> MAC4, MAC2 set(MAC2) -> MAC2 I am not exactly clear what is the intention with set(), if there is single MAC I guess intention is to replace it with new one, but if there are multiple MACs and one of them are already in the list intention may be just to change the default MAC. If above assumption is correct, what about following: set(MAC) { if only_default_mac_exist replace_default_mac if MAC exists in list swap MAC and list[0] else replace_default_mac } This swap prevents removing MAC side affect, does it make sense? > Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") > Cc: stable@dpdk.org > > Signed-off-by: Huisong Li > Acked-by: Chengwen Feng > --- > v8: fix some comments. > v7: add announcement in the release notes and document this behavior. > v6: fix commit log and some code comments. > v5: > - merge the second patch into the first patch. > - add error log when rollback failed. > v4: > - fix broken in the patchwork > v3: > - first explicitly remove the non-default MAC, then set default one. > - document default and non-default MAC address > v2: > - fixed commit log. > --- > doc/guides/rel_notes/release_23_03.rst | 6 +++++ > lib/ethdev/ethdev_driver.h | 6 ++++- > lib/ethdev/rte_ethdev.c | 35 ++++++++++++++++++++++++-- > lib/ethdev/rte_ethdev.h | 3 +++ > 4 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst > index 84b112a8b1..1c9b9912c2 100644 > --- a/doc/guides/rel_notes/release_23_03.rst > +++ b/doc/guides/rel_notes/release_23_03.rst > @@ -105,6 +105,12 @@ API Changes > Also, make sure to start the actual text at the margin. > ======================================================= > > +* ethdev: ensured all entries in MAC address list are uniques. > + When setting a default MAC address with the function > + ``rte_eth_dev_default_mac_addr_set``, > + the address is now removed from the rest of the address list > + in order to ensure it is only at index 0 of the list. > + > > ABI Changes > ----------- > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h > index dde3ec84ef..3994c61b86 100644 > --- a/lib/ethdev/ethdev_driver.h > +++ b/lib/ethdev/ethdev_driver.h > @@ -117,7 +117,11 @@ 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 addresses. > + * All entries are unique. > + * The first entry (index zero) is the default address. > + */ > struct rte_ether_addr *mac_addrs; > /** Bitmap associating MAC addresses to pools */ > uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR]; > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c > index 86ca303ab5..de25183619 100644 > --- 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,44 @@ 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; > > + /* 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, "Cannot remove the port %u address from the rest of list.\n", > + port_id); > + return ret; > + } > + } > 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; > +} > > /* > * Returns index into MAC address array of addr. Use 00:00:00:00:00:00 to find > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h > index d22de196db..2456153457 100644 > --- a/lib/ethdev/rte_ethdev.h > +++ b/lib/ethdev/rte_ethdev.h > @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id, > > /** > * Set the default MAC address. > + * It replaces the address at index 0 of the MAC address list. > + * If the address was already in the MAC address list, > + * it is removed from the rest of the list. > * > * @param port_id > * The port identifier of the Ethernet device.