From: "Min Hu (Connor)" <humin29@huawei.com>
To: "lihuisong (C)" <lihuisong@huawei.com>,
Ferruh Yigit <ferruh.yigit@intel.com>,
Kevin Traynor <ktraynor@redhat.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: <thomas@monjalon.net>, <ferruh.yigit@xilinx.com>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix one MAC address occupies two index in mac addrs
Date: Mon, 25 Apr 2022 14:42:50 +0800 [thread overview]
Message-ID: <b63b54dd-2009-af1f-065a-8cd3db567012@huawei.com> (raw)
In-Reply-To: <4ac36702-59f3-efa0-fedc-991b765a0eb7@huawei.com>
Hi, Ferruh,
what do you think of this patch?
在 2021/11/8 14:55, lihuisong (C) 写道:
>
> 在 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.
>
>
>
>
>
>
>
> .
next prev parent reply other threads:[~2022-04-25 6:42 UTC|newest]
Thread overview: 82+ 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)
2022-04-25 6:42 ` Min Hu (Connor) [this message]
2022-10-20 9:31 ` [PATCH V5] ethdev: fix one address occupies two indexes in MAC addrs Huisong Li
2022-11-16 7:37 ` lihuisong (C)
2022-12-06 8:08 ` lihuisong (C)
2023-01-10 1:00 ` fengchengwen
2023-01-18 8:26 ` Thomas Monjalon
2023-01-18 8:38 ` Thomas Monjalon
2023-01-19 10:09 ` lihuisong (C)
2023-01-19 9:57 ` lihuisong (C)
2023-01-19 14:38 ` Thomas Monjalon
2023-01-28 1:38 ` lihuisong (C)
2023-01-31 6:41 ` [PATCH V6] ethdev: fix one address occupies two entries " Huisong Li
2023-02-01 10:42 ` Thomas Monjalon
2023-02-01 12:26 ` lihuisong (C)
2023-02-01 13:15 ` [PATCH V7] " Huisong Li
2023-02-01 16:37 ` Thomas Monjalon
2023-02-02 1:11 ` lihuisong (C)
2023-02-02 11:50 ` Thomas Monjalon
2023-02-02 12:19 ` lihuisong (C)
2023-02-02 12:36 ` [PATCH V8] " Huisong Li
2023-02-02 13:11 ` Thomas Monjalon
2023-02-02 18:09 ` Ferruh Yigit
2023-02-02 21:10 ` Thomas Monjalon
2023-02-02 21:50 ` Morten Brørup
2023-02-03 1:56 ` lihuisong (C)
2023-02-03 12:58 ` Ferruh Yigit
2023-02-04 2:57 ` lihuisong (C)
2023-02-09 8:32 ` lihuisong (C)
2023-02-09 12:45 ` Ferruh Yigit
2023-02-10 9:54 ` lihuisong (C)
2023-02-10 12:27 ` Ferruh Yigit
2023-02-10 13:20 ` lihuisong (C)
2023-05-16 11:47 ` lihuisong (C)
2023-05-16 14:13 ` Ferruh Yigit
2023-05-17 7:45 ` lihuisong (C)
2023-05-17 8:53 ` Ferruh Yigit
2023-05-17 11:46 ` lihuisong (C)
2023-05-17 13:43 ` Ferruh Yigit
2023-05-19 3:00 ` [PATCH V9] " Huisong Li
2023-05-19 8:42 ` Ferruh Yigit
2023-05-19 9:21 ` lihuisong (C)
2023-05-19 9:31 ` [PATCH V10] " Huisong Li
2023-05-19 10:45 ` Ferruh Yigit
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=b63b54dd-2009-af1f-065a-8cd3db567012@huawei.com \
--to=humin29@huawei.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=ferruh.yigit@xilinx.com \
--cc=ktraynor@redhat.com \
--cc=lihuisong@huawei.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).