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 8A0E741BB4; Fri, 3 Feb 2023 02:56:11 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1FAF74014F; Fri, 3 Feb 2023 02:56:11 +0100 (CET) Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by mails.dpdk.org (Postfix) with ESMTP id 464BE40141 for ; Fri, 3 Feb 2023 02:56:08 +0100 (CET) Received: from kwepemm600004.china.huawei.com (unknown [172.30.72.57]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4P7JY21MqBzyNG9; Fri, 3 Feb 2023 09:51:34 +0800 (CST) Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Fri, 3 Feb 2023 09:56:04 +0800 Message-ID: <4344ec91-666f-135c-e342-7b99f6b89ed9@huawei.com> Date: Fri, 3 Feb 2023 09:56:04 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs To: Thomas Monjalon , Ferruh Yigit CC: , , , , , , , , , =?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> From: "lihuisong (C)" In-Reply-To: <6754145.G0QQBjFxQf@thomas> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm600004.china.huawei.com (7.193.23.242) X-CFilter-Loop: Reflected 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 在 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 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. 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 > > > .