在 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 <lihuis...@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: sta...@dpdk.org
Signed-off-by: Huisong Li <lihuis...@huawei.com>
Signed-off-by: Min Hu (Connor) <humi...@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.
