DPDK patches and discussions
 help / color / mirror / Atom feed
From: "lihuisong (C)" <lihuisong@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Kevin Traynor <ktraynor@redhat.com>,
	"Min Hu (Connor)" <humin29@huawei.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
Date: Mon, 8 Nov 2021 14:55:01 +0800
Message-ID: <4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com> (raw)
In-Reply-To: <1aed24f9-c2e2-5ffd-cb08-4ce7b7a8588a@intel.com>


在 2021/10/26 18:21, Ferruh Yigit 写道:
> On 10/22/2021 3:04 AM, lihuisong (C) wrote:
>>
>> 在 2021/10/21 16:30, Ferruh Yigit 写道:
>>> On 10/21/2021 3:05 AM, lihuisong (C) wrote:
>>>>
>>>> 在 2021/10/21 0:32, Ferruh Yigit 写道:
>>>>> On 10/20/2021 11:15 AM, Kevin Traynor wrote:
>>>>>> On 20/10/2021 08:41, Ferruh Yigit wrote:
>>>>>>> On 10/20/2021 7:49 AM, lihuisong (C) wrote:
>>>>>>>> Hi Ferruh
>>>>>>>>
>>>>>>>> 在 2021/10/20 1:45, Ferruh Yigit 写道:
>>>>>>>>> On 10/11/2021 10:28 AM, Min Hu (Connor) wrote:
>>>>>>>>>> From: Huisong Li <lihuisong@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() 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.
>>>>>>>
>>>>>>> True, it is a waste. But if there is no other visible user 
>>>>>>> impact, we can
>>>>>>> handle the issue with lower priority and clarifying the impact 
>>>>>>> in commit log
>>>>>>> helps to others.
>>>>>>>
>>>>>>>>>
>>>>>>>>>> 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 <lihuisong@huawei.com>
>>>>>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>> 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.
>>>>>>>
>>>>>>> I am not sure all PMD take this into account, I briefly checked 
>>>>>>> the ixgbe
>>>>>>> code and I am not sure if it handles this.
>>>>>>>
>>>>>>> Also it is possible to think that this responsibility is pushed 
>>>>>>> to the
>>>>>>> application, like application should remove a MAC address before 
>>>>>>> setting
>>>>>>> it as default MAC...
>>>>>>>
>>>>>>
>>>>>> Yes, the API view is more important than saving one entry in an 
>>>>>> array. From API perspective with this patch,
>>>>>>
>>>>>> set_default(MAC1)
>>>>>> add(MAC2)
>>>>>> add(MAC3)
>>>>>> add(MAC4)
>>>>>> default=MAC1, Filters=MAC2, MAC3, MAC4
>>>>>>
>>>>>> set_default(MAC2)
>>>>>> default=MAC2, Filters= MAC3, MAC4
>>>>>>
>>>>>> set_default(MAC3)
>>>>>> default=MAC3, Filters= MAC4
>>>>>>
>>>>>> set_default(MAC4)
>>>>>> default=MAC4, Filters=
>>>>>>
>>>>>> set_default(MAC5)
>>>>>> default=MAC5, Filters=
>>>>>>
>>>>>> Did I get it right? If so, it seems wrong to silently remove the 
>>>>>> filters. In which case, it would be easier to just not remove 
>>>>>> them in the first place (current behaviour).
>>>>>>
>>>>>
>>>>> Yep, this is the updated behavior. And agree it looks wrong when you
>>>>> show like this. (btw, this is only ethdev record of MAC filters, what
>>>>> is updated in this patch, HW still may be keeping all filters.)
>>>>
>>>> Whether HW saves all filters depends on the implementation of the 
>>>> set_default()
>>>>
>>>> in the driver. According to the implementation of this API of all 
>>>> PMDs, some drivers
>>>>
>>>> will first remove the old default MAC in HW and then add the new 
>>>> one when calling
>>>>
>>>> the set_default(). I am not sure if the HW that didn't do this 
>>>> would remove the old
>>>>
>>>> default MAC. If not, we may need to standardize this API in the 
>>>> ethdev layer.
>>>>
>>>>>
>>>>>> If they really need to be removed from the filter list when they 
>>>>>> are set_default(), then perhaps they should be restored to it 
>>>>>> when there is a new set_default().
>>>>>>
>>>>>
>>>>> I am for keeping current behavior. Application always can 
>>>>> explicitly remove a
>>>>> MAC filter before setting it default if required.
>>>>
>>>> But application can not remove the duplicate MAC if the MAC is the 
>>>> current default
>>>>
>>>> MAC by rte_eth_dev_mac_addr_remove(). In this case, it will failed 
>>>> to remove.
>>>>
>>>
>>> But can do other-way around, first remove (the non default one), later
>>> 'rte_eth_dev_default_mac_addr_set()'.
>>
>> This introduces a usage dependency on the user. We don't have a 
>> statement in some place
>>
>> for the dependency. What's more, if the user does not follow this 
>> dependency, the user
>>
>> will no longer be able to remove the MAC.
>>
>> So it may be more appropriate to deal with problem in ethdev layer.
>>
>>
>> **Scheme A:**
>>
>> The decision is left to the user, but there are usage dependency and 
>> irremovable possibility.**
>> **
>>
>> *Scheme B:*
>>
>> index = eth_dev_get_mac_addr_index(port_id, addr);
>> if (index > 0) {
>>      mac_pool_sel_bk = dev->data->mac_pool_sel[index];
>>      rte_eth_dev_mac_addr_remove(port_id, addr);
>> }
>> ret = (*dev->dev_ops->mac_addr_set)(dev, addr);
>> if (ret < 0) {
>>      if (index > 0) {
>>          rte_eth_dev_mac_addr_add(port_id, addr, 0);
>>          dev->data->mac_pool_sel[index] = mac_pool_sel_bk;
>>      }
>>
>>      return ret;
>> }
>>
>> * Scheme C:*
>>
>>   Use the method in this patch. It assumes that the driver has only 
>> one HW entry for a MAC.
>>
>> What do you think we should do?
>>
>
> If the impact is only loosing one entry in the array without any 
> functional
> effect, I am OK to keep behavior as it is.
>
> If there is more motivation for fix, I would prefer option B to be 
> sure all
> drivers behave same by explicit remove.
> .

Hi, Ferruh

There may be a functional problem in one scenario if we don't fix this.

Testpmd does the following steps:

1) add MAC1/2/3

mac_addr add 0 MAC1

mac_addr add 0 MAC2

mac_addr add 0 MAC3

2) then set new default MAC3

mac_addr set 0 MAC3

3)show mac_addrs list

MAC3

MAC1

MAC2

MAC3

4)then set new default MAC4

mac_addr set 0 MAC4

5) now, the mac_addrs list is as follows:

MAC4

MAC1

MAC2

MAC3


As a result, the network engine cannot receive packets from MAC3.

But application can see the MAC3 in mac_addrs list.

Btw, rte_eth_dev_default_mac_addr_set() will replace the old default MAC 
with

the new one, and the old one will be deleted.


So, we should fix it. What do you think? Thanks.








  reply	other threads:[~2021-11-08  6:55 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22  3:36 [dpdk-dev] [PATCH] " Min Hu (Connor)
2021-09-22  6:39 ` Andrew Rybchenko
2021-09-22  7:43   ` Huisong Li
2021-09-22  8:02     ` Andrew Rybchenko
2021-09-22  9:48       ` Huisong Li
2021-10-05 19:21 ` Thomas Monjalon
2021-10-08  7:02   ` Min Hu (Connor)
2021-10-08 10:04     ` Thomas Monjalon
2021-10-09  9:53       ` Min Hu (Connor)
2021-10-11  9:02         ` Thomas Monjalon
2021-10-11  9:28 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-10-11 10:35   ` Thomas Monjalon
2021-10-12  2:58     ` lihuisong (C)
2021-10-12  7:14       ` Thomas Monjalon
2021-10-15  2:00         ` lihuisong (C)
2021-10-19 17:45   ` Ferruh Yigit
2021-10-20  6:49     ` lihuisong (C)
2021-10-20  7:41       ` Ferruh Yigit
2021-10-20 10:15         ` Kevin Traynor
2021-10-20 16:32           ` Ferruh Yigit
2021-10-21  2:05             ` lihuisong (C)
2021-10-21  8:30               ` Ferruh Yigit
2021-10-22  2:04                 ` lihuisong (C)
2021-10-26 10:21                   ` Ferruh Yigit
2021-11-08  6:55                     ` lihuisong (C) [this message]
2022-04-25  6:42                       ` Min Hu (Connor)
2022-05-14  2:00 ` [PATCH V3 0/2] ethdev: fix MAC addrs list Min Hu (Connor)
2022-05-14  2:00   ` [PATCH V3 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
2022-05-14  2:00   ` [PATCH V3 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
2022-05-31 15:22   ` [PATCH V3 0/2] ethdev: fix MAC addrs list Andrew Rybchenko
2022-06-01  6:43     ` Min Hu (Connor)
2022-06-01  6:39   ` [PATCH v4 " Min Hu (Connor)
2022-06-01  6:39     ` [PATCH v4 1/2] ethdev: fix one address occupies two indexes in MAC addrs Min Hu (Connor)
2022-06-01 17:49       ` Andrew Rybchenko
2022-06-02  3:16         ` lihuisong (C)
2022-06-02 13:54           ` Andrew Rybchenko
2022-06-11  9:04             ` lihuisong (C)
2022-06-01  6:39     ` [PATCH v4 2/2] ethdev: document default and non-default MAC address Min Hu (Connor)
2022-06-01 17:49       ` Andrew Rybchenko
2022-06-01 17:49     ` [PATCH v4 0/2] ethdev: fix MAC addrs list Andrew Rybchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com \
    --to=lihuisong@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=humin29@huawei.com \
    --cc=ktraynor@redhat.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git