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 80E35A0548; Thu, 2 Jun 2022 15:54:16 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5F5BA40691; Thu, 2 Jun 2022 15:54:16 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 422EF4021E for ; Thu, 2 Jun 2022 15:54:15 +0200 (CEST) Received: from [192.168.1.126] (unknown [188.242.181.57]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 7AE55C8; Thu, 2 Jun 2022 16:54:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 7AE55C8 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1654178054; bh=xjpY/wUGOpwsg4yqc0QJ26tdz/AWyWrW+8NlRz1pJa8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=oZ/RMnR5Qi09IpZ1fO1WmXM3rmaDtnjKsjoYElNdWHwbGbb/JgOQDvvlSpbDRJqJG Mzfce3FbI+eOhtXnFKvdZZhIDhCnrXW2LHtsqEckXoNDSCC1Z+07/HqWaxwm2qqURR 377cAD+nKhNjeYmOxNZ+ATB0JkPHtgIYPgB/taUg= Message-ID: <816b0738-f986-b448-79c6-4d4be23be87f@oktetlabs.ru> Date: Thu, 2 Jun 2022 16:54:13 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Content-Language: en-US To: "lihuisong (C)" , "Min Hu (Connor)" , dev@dpdk.org Cc: Thomas Monjalon , Ferruh Yigit , Shepard Siegel , Ed Czeck , John Miller , Rasesh Mody , Shahed Shaikh , Ajit Khaparde , Somnath Kotur , Simei Su , Wenjun Wu , Qi Zhang , Xiao Wang , Yuying Zhang , Beilei Xing , Qiming Yang , Jiawen Wu , Jiawen Wu , Jian Wang 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> <1ddd57dd-2a97-712b-916b-022d3b4fb172@huawei.com> From: Andrew Rybchenko In-Reply-To: <1ddd57dd-2a97-712b-916b-022d3b4fb172@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 Cc more driver maintainers On 6/2/22 06:16, lihuisong (C) wrote: > > 在 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. Me too > 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? I simply want to make it consistent >> 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. Otherwise how will user know that rollback failed and configuration is inconsistent? >> >>> +            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 >> >> .