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 27CC1A0548; Thu, 2 Jun 2022 05:16:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C571C40694; Thu, 2 Jun 2022 05:16:55 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id E08C84021E for ; Thu, 2 Jun 2022 05:16:53 +0200 (CEST) Received: from kwepemi100021.china.huawei.com (unknown [172.30.72.54]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4LDB3c0mYGzjXQr; Thu, 2 Jun 2022 11:15:40 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by kwepemi100021.china.huawei.com (7.221.188.223) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 2 Jun 2022 11:16:51 +0800 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.24; Thu, 2 Jun 2022 11:16:50 +0800 Message-ID: <1ddd57dd-2a97-712b-916b-022d3b4fb172@huawei.com> Date: Thu, 2 Jun 2022 11:16:50 +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 v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs To: Andrew Rybchenko , "Min Hu (Connor)" , CC: Thomas Monjalon , Ferruh Yigit References: <20220514020049.57294-1-humin29@huawei.com> <20220601063949.43202-1-humin29@huawei.com> <20220601063949.43202-2-humin29@huawei.com> <121cb226-a634-5a9a-81dc-3a23c1901619@oktetlabs.ru> From: "lihuisong (C)" In-Reply-To: <121cb226-a634-5a9a-81dc-3a23c1901619@oktetlabs.ru> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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 在 2022/6/2 1:49, Andrew Rybchenko 写道: > On 6/1/22 09:39, 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(). However, if the new default one has >> been added as a non-default MAC address by >> rte_eth_dev_mac_addr_add(), the >> the rte_eth_dev_default_mac_addr_set() doesn't remove it from the >> mac_addrs >> list. As a result, one MAC address occupies two indexes in the list. >> Like: >> add(MAC1) >> add(MAC2) >> add(MAC3) >> add(MAC4) >> set_default(MAC3) >> default=MAC3, filters=MAC1, MAC2, MAC3, MAC4 >> >> 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 view MAC3 from the >> list, >> but packets with MAC3 aren't actually received by the PMD. > > IMHO, the main problem is inconsistency which exists right now. > rte_eth_dev_mac_addr_add() checks for duplicate MAC addition > including the default one (index zero) and extends the entry > pool mask (including zero entry case). > > However, the patch above does not extend zero entry pool mask. > So, the result will depend on order which is bad: > A. Set default to A, add MAC A with pool 2 => pool mask has 2 > B. Add MAC A with pool 2, set default to A => pool mask is empty > I don't know how this MAC pool works in which driver. However, the 'eth_dev_mac_restore' API show that 1) the default MAC has only pool zero if set it by the 'mac_addr_add', 2) the default one hasn't pool information if set it by 'default_mac_addr_set'. Do you mean we should inherit its pool mask in this case? > Am I missing something in the code? > What is the right/intended behaviour? > >> >> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> Signed-off-by: Min Hu >> --- >>   lib/ethdev/rte_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++-- >>   1 file changed, 37 insertions(+), 2 deletions(-) >> >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index 46c088dc88..fc9ca8d6fd 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -4260,7 +4260,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); >> @@ -4278,16 +4281,48 @@ rte_eth_dev_default_mac_addr_set(uint16_t >> port_id, struct rte_ether_addr *addr) >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP); >>   +    /* >> +     * 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 */ >> +        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, >> +            "Delete MAC address from the MAC list of ethdev port >> %u.\n", >> +            port_id); >> +            return ret; >> +        } >> +        /* reset pool bitmap */ >> +        dev->data->mac_pool_sel[index] = 0; >> +    } >> + >>       ret = (*dev->dev_ops->mac_addr_set)(dev, addr); >>       if (ret < 0) >> -        return ret; >> +        goto back; >>         /* Update default address in NIC data structure */ >>       rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]); >>         return 0; >> -} >>   +back: >> +    if (index > 0) { >> +        pool = 0; >> +        do { >> +            if (mac_pool_sel_bk & UINT64_C(1)) >> +                rte_eth_dev_mac_addr_add(port_id, addr, pool); > > Don't we want to have at least error logs in the case of rollback > failure here? It doesn't feel necessary. It may trigger the printing of a large number of error logs in abnormal scenarios. > >> +            mac_pool_sel_bk >>= 1; >> +            pool++; >> +        } while (mac_pool_sel_bk); > > Please, compare vs 0 explicitly. Ack > >> +    } >> + >> +    return ret; >> +} >>     /* >>    * Returns index into MAC address array of addr. Use >> 00:00:00:00:00:00 to find > > .