Re: [PATCH V8] ethdev: fix one address occupies two entries in MAC addrs

["lihuisong (C)" <lihuisong@huawei.com>]

在 2023/5/16 22:13, Ferruh Yigit 写道:
> On 5/16/2023 12:47 PM, lihuisong (C) wrote:
>> Hi Ferruh,
>>
>> There is no result on techboard.
>> How to deal with this problem next?
> +techboard for comment.
>
>
> Btw, what was your positioning to Bruce's suggestion,
> when a MAC address is in the list, fail to set it as default and enforce
> user do the corrective action (delete MAC explicitly etc...).
If a MAC address is in the list, rte_eth_dev_default_mac_addr_set 
returns failure?
> If you are OK with it, that is good for me too, unless techboard objects
> we can proceed with that one.
>
>
>> /Huisong
>>
>> 在 2023/2/2 20:36, Huisong Li 写道:
>>> 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.
>>>
>>> Fixes: 854d8ad4ef68 ("ethdev: add default mac address modifier")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
>>> ---
>>> v8: fix some comments.
>>> v7: add announcement in the release notes and document this behavior.
>>> v6: fix commit log and some code comments.
>>> v5:
>>>    - merge the second patch into the first patch.
>>>    - add error log when rollback failed.
>>> v4:
>>>     - fix broken in the patchwork
>>> v3:
>>>     - first explicitly remove the non-default MAC, then set default one.
>>>     - document default and non-default MAC address
>>> v2:
>>>     - fixed commit log.
>>> ---
>>>    doc/guides/rel_notes/release_23_03.rst |  6 +++++
>>>    lib/ethdev/ethdev_driver.h             |  6 ++++-
>>>    lib/ethdev/rte_ethdev.c                | 35 ++++++++++++++++++++++++--
>>>    lib/ethdev/rte_ethdev.h                |  3 +++
>>>    4 files changed, 47 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/release_23_03.rst
>>> b/doc/guides/rel_notes/release_23_03.rst
>>> index 84b112a8b1..1c9b9912c2 100644
>>> --- a/doc/guides/rel_notes/release_23_03.rst
>>> +++ b/doc/guides/rel_notes/release_23_03.rst
>>> @@ -105,6 +105,12 @@ API Changes
>>>       Also, make sure to start the actual text at the margin.
>>>       =======================================================
>>>    +* ethdev: ensured all entries in MAC address list are uniques.
>>> +  When setting a default MAC address with the function
>>> +  ``rte_eth_dev_default_mac_addr_set``,
>>> +  the address is now removed from the rest of the address list
>>> +  in order to ensure it is only at index 0 of the list.
>>> +
>>>      ABI Changes
>>>    -----------
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index dde3ec84ef..3994c61b86 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -117,7 +117,11 @@ struct rte_eth_dev_data {
>>>          uint64_t rx_mbuf_alloc_failed; /**< Rx ring mbuf allocation
>>> failures */
>>>    -    /** Device Ethernet link address. @see
>>> rte_eth_dev_release_port() */
>>> +    /**
>>> +     * Device Ethernet link addresses.
>>> +     * All entries are unique.
>>> +     * The first entry (index zero) is the default address.
>>> +     */
>>>        struct rte_ether_addr *mac_addrs;
>>>        /** Bitmap associating MAC addresses to pools */
>>>        uint64_t mac_pool_sel[RTE_ETH_NUM_RECEIVE_MAC_ADDR];
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index 86ca303ab5..de25183619 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -4498,7 +4498,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);
>>> @@ -4517,16 +4520,44 @@ rte_eth_dev_default_mac_addr_set(uint16_t
>>> port_id, struct rte_ether_addr *addr)
>>>        if (*dev->dev_ops->mac_addr_set == NULL)
>>>            return -ENOTSUP;
>>>    +    /* Keep address unique in dev->data->mac_addrs[]. */
>>> +    index = eth_dev_get_mac_addr_index(port_id, addr);
>>> +    if (index > 0) {
>>> +        /* Remove address in dev 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, "Cannot remove the port %u address
>>> from the rest of list.\n",
>>> +                       port_id);
>>> +            return ret;
>>> +        }
>>> +    }
>>>        ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>>>        if (ret < 0)
>>> -        return ret;
>>> +        goto out;
>>>          /* Update default address in NIC data structure */
>>>        rte_ether_addr_copy(addr, &dev->data->mac_addrs[0]);
>>>          return 0;
>>> -}
>>>    +out:
>>> +    if (index > 0) {
>>> +        pool = 0;
>>> +        do {
>>> +            if (mac_pool_sel_bk & UINT64_C(1)) {
>>> +                if (rte_eth_dev_mac_addr_add(port_id, addr,
>>> +                                 pool) != 0)
>>> +                    RTE_ETHDEV_LOG(ERR, "failed to restore MAC pool
>>> id(%u) in port %u.\n",
>>> +                               pool, port_id);
>>> +            }
>>> +            mac_pool_sel_bk >>= 1;
>>> +            pool++;
>>> +        } while (mac_pool_sel_bk != 0);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>>      /*
>>>     * Returns index into MAC address array of addr. Use
>>> 00:00:00:00:00:00 to find
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index d22de196db..2456153457 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -4356,6 +4356,9 @@ int rte_eth_dev_mac_addr_remove(uint16_t port_id,
>>>      /**
>>>     * Set the default MAC address.
>>> + * It replaces the address at index 0 of the MAC address list.
>>> + * If the address was already in the MAC address list,
>>> + * it is removed from the rest of the list.
>>>     *
>>>     * @param port_id
>>>     *   The port identifier of the Ethernet device.
> .