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 6017441BB3; Thu, 2 Feb 2023 22:50:27 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4291440EDC; Thu, 2 Feb 2023 22:50:27 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id AA43940693 for ; Thu, 2 Feb 2023 22:50:26 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs Date: Thu, 2 Feb 2023 22:50:23 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D876F9@smartserver.smartshare.dk> In-Reply-To: <6754145.G0QQBjFxQf@thomas> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs Thread-Index: Adk3StcyRgPsi7b8Q0OvEqNEwJlhpQAAx/hg References: <20221020093102.20679-1-lihuisong@huawei.com> <20230202123625.14975-1-lihuisong@huawei.com> <9b816855-2b5e-4296-d954-aa23cbd97a4c@amd.com> <6754145.G0QQBjFxQf@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Thomas Monjalon" , "Huisong Li" , "Ferruh Yigit" Cc: , , , , , , , , 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 > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, 2 February 2023 22.11 >=20 > 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=3DMAC3, the rest of the list=3DMAC1, 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=3DMAC5, > filters=3D(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(), >=20 > That's the problem, nobody is clear with the current behavior. > The doc says "Set the default MAC address." and nothing else. >=20 > > 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. >=20 > The assumption in this patch is that "Set" means "Replace", not = "Swap". > So this patch takes the approach 1/ Replace and keep Unique. >=20 > > 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 > > } >=20 > 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. >=20 > > This swap prevents removing MAC side affect, does it make sense? >=20 > 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. >=20 > And the current approach 0/ is to Replace default MAC address > without touching the secondary addresses at all. >=20 > So we have 4 choices. > We could vote, roll a dice, or find a strong argument? If there is only one MAC, MAC1, I would certainly expect Set(MAC2) to = replace MAC1 with MAC2. If there is a list of MACs, I might expect Set() to select one in the = list. And, if calling Set() with a MAC not in the list, we could either = 1. return an error, or 2. implicitly add it to the list and then set it. If we implicitly add it to the list (i.e. follow option 2 described = above), it should not be implicitly be removed from the list if another = MAC is Set(). This behavior seems weird, which speaks for returning an = error if setting a MAC not in the list (i.e. option 1). Disclaimer: I have no experience configuring NICs with multiple MACs - = we promiscuous mode or one MAC address. So my input regarding multiple = MACs is purely theoretical.