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 CFF1AA0C45; Wed, 20 Oct 2021 08:50:01 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 590AF40150; Wed, 20 Oct 2021 08:50:01 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 50B4B40142 for ; Wed, 20 Oct 2021 08:49:59 +0200 (CEST) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.55]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4HZ1MR3vqQzbn7f; Wed, 20 Oct 2021 14:45:23 +0800 (CST) Received: from dggema767-chm.china.huawei.com (10.1.198.209) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2308.15; Wed, 20 Oct 2021 14:49:56 +0800 Received: from [10.67.103.231] (10.67.103.231) by dggema767-chm.china.huawei.com (10.1.198.209) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.15; Wed, 20 Oct 2021 14:49:56 +0800 Message-ID: Date: Wed, 20 Oct 2021 14:49:56 +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 To: Ferruh Yigit , "Min Hu (Connor)" , CC: References: <20210922033630.41130-1-humin29@huawei.com> <20211011092811.55172-1-humin29@huawei.com> From: "lihuisong (C)" In-Reply-To: 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 dggema767-chm.china.huawei.com (10.1.198.209) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs 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 Sender: "dev" Hi Ferruh 在 2021/10/20 1:45, Ferruh Yigit 写道: > On 10/11/2021 10:28 AM, Min Hu (Connor) wrote: >> From: Huisong Li >> >> The dev->data->mac_addrs[0] will be changed to a new MAC address when >> applications modify the default MAC address by >> rte_eth_dev_default_mac_addr_set() API. However, If the new default >> MAC address has been added as a non-default MAC address by >> rte_eth_dev_mac_addr_add() API, the rte_eth_dev_default_mac_addr_set() >> API doesn't remove it from dev->data->mac_addrs[]. As a result, one MAC >> address occupies two index capacities in dev->data->mac_addrs[]. >> > > Hi Connor, > > I see the problem, but can you please clarify what is the impact to > the end user? > > If application does as following: >   rte_eth_dev_mac_addr_add(MAC1); >   rte_eth_dev_mac_addr_add(MAC2); >   rte_eth_dev_mac_addr_add(MAC3); >   rte_eth_dev_default_mac_addr_set(MAC2); > > The 'dev->data->mac_addrs[]' will have: "MAC2,MAC2,MAC3" which has > 'MAC2' duplicated. > > Will this cause any problem for the application to receive the packets > with 'MAC2' address? > Or is the only problem one extra space used in 'dev->data->mac_addrs[]' > without any other impact to the application? I think it's just a waste of space. > >> This patch adds the logic of removing MAC addresses for this scenario. >> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> Signed-off-by: Min Hu (Connor) >> --- >> v2: >> * fixed commit log. >> --- >>   lib/ethdev/rte_ethdev.c | 15 +++++++++++++++ >>   1 file changed, 15 insertions(+) >> >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index 028907bc4b..7faff17d9a 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -4340,6 +4340,7 @@ int >>   rte_eth_dev_default_mac_addr_set(uint16_t port_id, struct >> rte_ether_addr *addr) >>   { >>       struct rte_eth_dev *dev; >> +    int index; >>       int ret; >>         RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> @@ -4361,6 +4362,20 @@ rte_eth_dev_default_mac_addr_set(uint16_t >> port_id, struct rte_ether_addr *addr) >>       if (ret < 0) >>           return ret; >>   +    /* >> +     * 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[]. >> +     */ >> +    index = eth_dev_get_mac_addr_index(port_id, addr); >> +    if (index > 0) { >> +        /* remove address in NIC data structure */ >> +        rte_ether_addr_copy(&null_mac_addr, >> +                    &dev->data->mac_addrs[index]); >> +        /* reset pool bitmap */ >> +        dev->data->mac_pool_sel[index] = 0; >> +    } >> + > > Here only 'dev->data->mac_addrs[]' array is updated and it assumes > driver removes similar duplication itself, but I am not sure if this is > valid for all drivers. > > If driver is not removing the duplicate in the HW configuration, the > driver > config and 'dev->data->mac_addrs[]' will diverge, which is not good. The same MAC address does not occupy two HW entries, which is also a waste for HW. After all, HW entry resources are also limited. The PMD should also take this into account. So, I think, we don't have to think about it here. > > What about following logic to be sure HW configuration and > 'dev->data->mac_addrs[]' is same: > >   index = eth_dev_get_mac_addr_index(port_id, addr); >   if (index > 0) > rte_eth_dev_mac_addr_remove(port_id, addr); >   (*dev->dev_ops->mac_addr_set)(dev, addr); The logic above seems to be good. But if .mac_addr_set() failed to execute, the addr has been removed from HW and 'dev->data->mac_addrs[]'. It's not good. Hope for your reply.  Thanks. >>       /* Update default address in NIC data structure */ >>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]); >> > > .