[PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs

[lihuisong (C)]

在 2022/6/2 1:49, Andrew Rybchenko 写道:
> On 6/1/22 09:39, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong at huawei.com>
>>
>> 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 at dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong at huawei.com>
>> Signed-off-by: Min Hu <humin29 at huawei.com>
>> ---
>>   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
>
> .