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 7B36C41BBC; Fri, 3 Feb 2023 13:59:25 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 080F64067B; Fri, 3 Feb 2023 13:59:25 +0100 (CET) Received: from NAM10-DM6-obe.outbound.protection.outlook.com (mail-dm6nam10on2088.outbound.protection.outlook.com [40.107.93.88]) by mails.dpdk.org (Postfix) with ESMTP id 13E964021E for ; Fri, 3 Feb 2023 13:59:23 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RWxiZHgcexYqAyY8875I2/crRtdLEC87IY6/b0L9S2ybnKdZFfp2lu4LlGaJNXIwLI/ug+JwRcM4o2yQC92qsLKFcJj0ai67yxPiVorOPJrWM3fsdBPmt0z0pjeHWTqq+oggptqFTyDTdz46k+nqATmhdX34oFnZLP16WCXncKOekvZw8CuqPO+1ga3eTBIOvhVHDYlvajZ5H0u7H5bmR5/DkwW2y42ZsAiXi9Y3pDGh/6NYMRyhLX5oQ9/N3nfYKgIDBlj9gqPx+JQrR/XCRq0jrzSB+lA7ZAYEkUFnwaMnBaV3jg2ysb26D5iCTU/ElRn02YZcHHlTzXUBdKNV8A== 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=Y0pkviKyHYvm+f9juovOmhJA2r7Ju4CBbIM25pvDqR4=; b=PVLV8WivJu9rdf1Xg3d67i5LCyAQYBTA+d/agBjnBOJWYTxVfF0ql1buOONtY0yRg0ek10NcTgolILYrHrwhw0hpdYnUlxXJVMPoTdXZkLFwjwbc6sjzm/C0eB/W1/1X1IPUzUooGTpW3+HIn1zImXES7yr+UC8yV/usocxXWNGS6+iqHWVenD/Ut359XgvJSCKVKMA3TllnH2sgKH/jb2kyMQg7mo4YSfPQ8R15APgk70lyMbU4T4MHFoMmBJ8lRf5b7/jhWhwatlBhlK+gnzbtyfuYshQ8lA4GpBF5NhN19Z33uSodwdmrdlFfkmznv1oyXQUwwaZaWMYNEZ4U8A== 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=Y0pkviKyHYvm+f9juovOmhJA2r7Ju4CBbIM25pvDqR4=; b=KYkR60q1Qn2+m8nouP8Nkp0lLfN8zfYYbFPXdy4JBpto10vz5GsXIY8M72dY/SbevUEjMBOYW6BybTSahoWkpbCMPnpRHiMAqhSaWjrGgbc9VtCFCulsN5kOcWG05ZAaFXganR5/q5mxsU5bnrVHoZczs7gg43evdplpizxZOVo= 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 CH0PR12MB5251.namprd12.prod.outlook.com (2603:10b6:610:d2::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6064.25; Fri, 3 Feb 2023 12:59:21 +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.031; Fri, 3 Feb 2023 12:59:14 +0000 Message-ID: Date: Fri, 3 Feb 2023 12:58:14 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 To: "lihuisong (C)" , Thomas Monjalon Cc: dev@dpdk.org, andrew.rybchenko@oktetlabs.ru, liudongdong3@huawei.com, huangdaode@huawei.com, fengchengwen@huawei.com, jerinj@marvell.com, bruce.richardson@intel.com, maxime.coquelin@redhat.com, david.marchand@redhat.com, =?UTF-8?Q?Morten_Br=c3=b8rup?= References: <20221020093102.20679-1-lihuisong@huawei.com> <20230202123625.14975-1-lihuisong@huawei.com> <9b816855-2b5e-4296-d954-aa23cbd97a4c@amd.com> <6754145.G0QQBjFxQf@thomas> <4344ec91-666f-135c-e342-7b99f6b89ed9@huawei.com> Content-Language: en-US From: Ferruh Yigit Subject: Re: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs In-Reply-To: <4344ec91-666f-135c-e342-7b99f6b89ed9@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-ClientProxiedBy: LO6P265CA0013.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:339::19) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|CH0PR12MB5251:EE_ X-MS-Office365-Filtering-Correlation-Id: f0b89fa0-a680-4bd6-187d-08db05e6734a X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: SnSwa3Ac4BVDtzxDgFFTR+N8W7eu0MRP2ZgxIAO2wpOpPU66n1jIjdcroEiL8RHLm8Kk07Db6ZLypeVDulE6/pgll/2rEY3eplmCzgqhcunSeiP2YzLAo5PrVcGDynyVNuhExqV8imuvxWg23nYINduAlQeg9A1jKT0vH7PEyM1Rdqzq0ub9K9oGvYrrzL1tonEJCmndiunZwNJMutrqKtPqxk7u4O/pT18rbfUU6193QgTbM1piZ5jy3bsjBxFtY4A3kq1xh6TpWVt3GH/sILgpcgQ4usq+oHgr9coD58hejgIcqtPZBMpzjOojvtLkT4FG9q5QHBJVnQEWBtnq2vjTd/XhA73hLs4NGatw/NnRQuH/O2AAQSneh9FNBPUmhbEMOq8H2tfrgge8LP5iVdNsWZ2zTneozyb16dBtjxOwmYNnr8pnObIsRhTDKfiBf63ddGiRgL+z5z81B/peY4zlqgxKqVgpPR4Ouc17YSUDngjILyrZkfzu7ulD2aZMzSFu88dH8EbwcMiZ9ccZhQEz2p3JJ/HJ8akJq+gV6JfDyKoOOJOws0+nvtPYaukbZS85pFU8hP00sTYiSjKU7+7oq2rf4cuqc63P1w1T2Ld1pAMVFqs/6dWJq7aZF55rsbN+qNiE7MmmTva9bRvSCR4YhDFN2TxA9pkBKqx62g0annoAiSUHBRfQy/x9Vpepb4RTzO2ofecVK78D9PtbwSm0UCXJ4wY/bj9ThQUaMJw= 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)(346002)(366004)(376002)(39860400002)(396003)(136003)(451199018)(36756003)(2906002)(38100700002)(44832011)(83380400001)(2616005)(316002)(31696002)(6512007)(186003)(110136005)(26005)(6486002)(6506007)(478600001)(66476007)(66556008)(66946007)(8676002)(5660300002)(41300700001)(86362001)(4326008)(8936002)(7416002)(53546011)(31686004)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aWVYNnJjTzdlWGFaK1VaRTR2SEd2cjBiRlBjL05QUG9vVEcrbEduMjVkWkFL?= =?utf-8?B?UytxZS8rTGFjR1ZQYVpLZjRwTTVLcGlhbmpwVFp1M0hHbTgwRHBBazMzdzVy?= =?utf-8?B?YTVRNUJwdWJMSnNEdU4yU2ZBVGVJSHp5Nks5ekh2ZFNXWFBRenVEUy9KdEYy?= =?utf-8?B?K0FKbDlWZzZ3R1ZSMWduSHhsWGNXaC9NaDhIR3RjYmtjV0tNSE9aR251c1pC?= =?utf-8?B?RE5ob28rTzl4YitLRGNxRlRFR0hzMVlDR0xTcXFWSXJITC9OTEtnMXJXaDdQ?= =?utf-8?B?QWZlblRINHk5SWZPeU5wY3pzMDh5UWFKd2lldE1EYThuYTJab3VJbkV0K1hk?= =?utf-8?B?Q3AxaC9MTC8raXR5VThHNE1vNmo3TVVjSzUweVVSL2tuWnd3aEhXaVBaOTN4?= =?utf-8?B?U2NOY1c3TndHeXRITFphTkJjQWY5QzNpQ0w3TWVxeGZQMjdzZnBacmZadDFx?= =?utf-8?B?ZUY3eVROcWp0RFp3QmhiZUhpRjcwMCt6aU1ib1VUb3ZoSGtpTzJoNkFFUUM5?= =?utf-8?B?V055cXRxWW9KZ3hCdmUzYkdqTWxjSFZMSUdUTWVsQUFjNmxhN1RoYk1OdVZn?= =?utf-8?B?T29zeGZ1S2lVVi9yR2tGRzM3S2NlZVdaMTFYMVhlTVJwMVJJL1B4dmw5cFdK?= =?utf-8?B?UGE1V2tXcjBORCt3RmorUGtMSm5JY0twOExGZDFXcms1QVh0WE12aWRRTGFu?= =?utf-8?B?ODZ5VUl6TkxUUCt5bjNkYnBHSlJHbmFmUGF1SkNnL3NVOUNzK2dkUDliNWVo?= =?utf-8?B?UVJscXRrYkZkeVJnMUFBTlRxRWp3UkdPZk4rS2V0UWFvUnZzSDNPNmhsWDNk?= =?utf-8?B?dk1mTDB4b1hZeXo1dUFFNGNjdHhUakpaTmN2K3N6TG9mZjlMdEpkZzU5NjRT?= =?utf-8?B?VStCK2NKc1VXb1lDaWl3YTJrTnhFTlFwMG81bTNnU2xYLzFJMkJSczMwbjE4?= =?utf-8?B?T2dka0hSR2xhTnExQVhud0Q3TVhaQWZoam8rbHFRMTNObExLV05TbDk0SUJ2?= =?utf-8?B?QndWWDRiQjdFdU9HQ3MvZjdIY1ZPSVBjWmYxeFBuS1lmV3RhclB4Um5uZDlx?= =?utf-8?B?T25tUEtaWmpNdHJBRlJmekhRUjVKZnhIMDFpbGFKemJQd3hKaWsrdWcxV0dk?= =?utf-8?B?eGRmY2pUS2dIVHlGd3BScmNUYkdTQ24xMGJnaFIya05pOHhteGFMZ2VyUCsv?= =?utf-8?B?R2FoTU9MODRvRWt4SjNLMnpoT1FacTdxVlQvanBZRjcxRG5zQ3dtZEJjcVpn?= =?utf-8?B?VkwzcnltVVFJRzBGYlV6cjRNM3JqUnViQTFkWm8zODlaQ2t6WWJvR1VGblpP?= =?utf-8?B?TDBoWUdmQWZ3djVSdVpRS1NsQXNhNGNxdEF4Q2kyNzdHNVVQd3NVbGovbTlC?= =?utf-8?B?bnpCNSttQXlhVWpMU0FUbERKOTI2c3NOV2ZHOEZBaHViK1I2bWtrWVZQN2kz?= =?utf-8?B?bS8vLzJYdmVXNFNBYUJwRWEyay9oT0tzVGVWWEhHazloTHNDVDV0ZjZtWEZT?= =?utf-8?B?M1BxZ08rS2M5bm5aclhMS2FiVmJKRllmWlh0NFhTM2pXTjMvbHlpdVc0MEtn?= =?utf-8?B?Z05JV3VHUE1rYldyOGhpUmtDUmVOZWVaaThiVTdjNXFwaGtFa0xDMlNDeG52?= =?utf-8?B?dTV0MVNHb1hyOXRkYlJDZFdqQk5LVVI4RGxSSHAybkFIQ1h6VnpNcU1IUTZj?= =?utf-8?B?eUJMNGd1czQ4TUdyNFNVcXY1MW0rZFNkMVhEdEJFajdob0tGbXg5YXRQOWM4?= =?utf-8?B?a3liTnA1YnVCdWI0dUNGNUczOG9idlMxM0xQcDVUOU5MUVNRamVJdlJ3bUZE?= =?utf-8?B?WmJaNmZqVVY3dnRXWWJlbUtSRHdGeko5MUdMN01hVHF5WDUzdk5CVlZqVVYy?= =?utf-8?B?d1VqVm1KMnRxdFVoajkwSk5qS3JKc3ZTRmZxNVZNN1NEUzNKQjJ3cjlkaVhK?= =?utf-8?B?d1p2d20zZUtSSlZkQ2ZBZ001cGZXY0FLaStMQ2lRV1NoVUtrT25mV2xTSG9T?= =?utf-8?B?ZStVYjh0K3RKSmV3VE9CWVljV1QwbXJvUGlKVTkvZUYzMTdzeU1QV25CaXJF?= =?utf-8?B?ZkcvOGh6a3dxNFZIZFFPVWNuMWM4MkdHb3NDdUtzdTIvektZZkhGcE8rVzhm?= =?utf-8?Q?Zi4MGW52WOWnPeHZ+4hJXqYjY?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: f0b89fa0-a680-4bd6-187d-08db05e6734a X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Feb 2023 12:59:14.0878 (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: FOhUB5j8tGvyqfGjP1DgqoO9mkHdzyznzVAlH5yYI6ieBwkNECDro6RM25zf6o7J X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5251 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/3/2023 1:56 AM, lihuisong (C) wrote: > > 在 2023/2/3 5:10, Thomas Monjalon 写道: >> 02/02/2023 19:09, Ferruh Yigit: >>> 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(), >> That's the problem, nobody is clear with the current behavior. >> The doc says "Set the default MAC address." and nothing else. > Indeed. But we can see the following information. > From the ethdev layer, this set() API always replaces the old default > address (index 0) without adding the old one. > From the PMD layer, set() interface of some PMDs, such as i40e, ice, > hns3 and so on (as far as I know), > also do remove the hardware entry of the old default address. If we define behavior clearly, I think we can adapt PMD implementation according it, unless there is HW limitation. >> >>> 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. >> The assumption in this patch is that "Set" means "Replace", not "Swap". >> So this patch takes the approach 1/ Replace and keep Unique. >> >>> 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 approach 2/ is a mix of Swap and Replace. >> The old default MAC destiny depends on whether >> we have added the new MAC as "secondary" before setting as new default. >> >>> This swap prevents removing MAC side affect, does it make sense? >> Another approach would be 3/ to do an "Always Swap" >> even if the new MAC didn't exist before, >> you keep the old default MAC as a secondary MAC. >> >> And the current approach 0/ is to Replace default MAC address >> without touching the secondary addresses at all. >> >> So we have 4 choices. >> We could vote, roll a dice, or find a strong argument? > According to the implement of set() in ethdev and PMD layer, it always > use "Replace", not "Swap". > If we use "Swap" now, the behavior of this API will be changed. > I'm not sure if the application can accept this change or has other > effects. > This patch is also changing behavior, because of implied remove address, same concern is valid with this patch. As I checked again current implementation may have one more problem (this from reading code, I did not test this): add(MAC1) -> MAC1 add(MAC2) -> MAC1, MAC2 set(MAC2) -> MAC2, MAC2 del(MAC2) -> FAILS This fails because `rte_eth_dev_mac_addr_remove()` can't remove default MAC, and it only tries to remove first address it finds, it can't find and remove second 'MAC2'. I wasn't too much bothered with wasting one MAC address slot, so wasn't sure if a change is required at all, but if above analysis is correct I think this is more serious problem to justify the change. I don't think always swap (option /3) is good idea, specially for single MAC address exists case, and current case has (option 0/) has mentioned problems. Remaining ones are mix of swap and replace (option 2/) and this patch (option /1). I think mix of swap and replace (option 2/ above) has some benefits: - It always replaces default MAC - Prevents duplication MAC address in the list - Doesn't implicitly remove address from list BUT, if the agreement is this patch (option 1/) I am OK with that too, I just want to make sure that it is discussed. > BTW, it seems that the ethernet port in kernel also replaces the old > address if we modify the one. > Use the test command: ifconfig eth0 hw ether new_mac For default MAC address it is more clear that intention is to replace it, but question is about what to do with the list of MAC addresses.