[V7] ethdev: fix one address occupies two entries in MAC addrs
Checks
Commit Message
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 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>
---
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(-)
Comments
01/02/2023 14:15, 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 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>
[...]
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -101,10 +101,16 @@ API Changes
> Use fixed width quotes for ``function_names`` or ``struct_names``.
> Use the past tense.
>
> +
useless line?
> This section is a comment. Do not overwrite or remove it.
> Also, make sure to start the actual text at the margin.
> =======================================================
Please check the comment: "make sure to start the actual text at the margin."
> + * ethdev: ensured all entries in MAC address list is unique.
is -> are uniques
> + The function ``rte_eth_dev_default_mac_addr_set`` replaces the address
> + at index 0 of the address list. If the address was already in the
> + address list, it is removed from the rest of the list.
You need to highlight what changed:
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.
[...]
> --- 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.
> + */
You remember I asked to split lines after the dot?
> /**
> * 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.
Here you can split after the comma.
在 2023/2/2 0:37, Thomas Monjalon 写道:
> 01/02/2023 14:15, 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 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>
> [...]
>> --- a/doc/guides/rel_notes/release_23_03.rst
>> +++ b/doc/guides/rel_notes/release_23_03.rst
>> @@ -101,10 +101,16 @@ API Changes
>> Use fixed width quotes for ``function_names`` or ``struct_names``.
>> Use the past tense.
>>
>> +
> useless line?
will remove.
>
>> This section is a comment. Do not overwrite or remove it.
>> Also, make sure to start the actual text at the margin.
>> =======================================================
> Please check the comment: "make sure to start the actual text at the margin."
It means to start the actual text at the beginning of the line, right?
>
>> + * ethdev: ensured all entries in MAC address list is unique.
> is -> are uniques
All right.
>
>> + The function ``rte_eth_dev_default_mac_addr_set`` replaces the address
>> + at index 0 of the address list. If the address was already in the
>> + address list, it is removed from the rest of the list.
> You need to highlight what changed:
>
> 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.
ok, highlight it.
>
> [...]
>> --- 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.
>> + */
> You remember I asked to split lines after the dot?
Sorry for my misunderstanding. Need to occupy one line for each sentence
here?
Do we generally use this style to make comments? or are we just trying
to highlight it here?
>
>> /**
>> * 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.
> Here you can split after the comma.
Ack
>
>
>
> .
02/02/2023 02:11, lihuisong (C):
> 在 2023/2/2 0:37, Thomas Monjalon 写道:
> > 01/02/2023 14:15, Huisong Li:
> >> This section is a comment. Do not overwrite or remove it.
> >> Also, make sure to start the actual text at the margin.
> >> =======================================================
> > Please check the comment: "make sure to start the actual text at the margin."
>
> It means to start the actual text at the beginning of the line, right?
It means no extra space at the beginning of the line.
> >> - /** 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.
> >> + */
> > You remember I asked to split lines after the dot?
> Sorry for my misunderstanding. Need to occupy one line for each sentence
> here?
A sentence may be longer than one line,
but it is better split lines logically (after dots, commas, etc).
> Do we generally use this style to make comments? or are we just trying
> to highlight it here?
That's a recommendation when writing text (comments and rst doc).
Having lines split logically may help reading,
and will make simpler patches when updating in future,
because in general we change a single sentence.
在 2023/2/2 19:50, Thomas Monjalon 写道:
> 02/02/2023 02:11, lihuisong (C):
>> 在 2023/2/2 0:37, Thomas Monjalon 写道:
>>> 01/02/2023 14:15, Huisong Li:
>>>> This section is a comment. Do not overwrite or remove it.
>>>> Also, make sure to start the actual text at the margin.
>>>> =======================================================
>>> Please check the comment: "make sure to start the actual text at the margin."
>> It means to start the actual text at the beginning of the line, right?
> It means no extra space at the beginning of the line.
>
>>>> - /** 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.
>>>> + */
>>> You remember I asked to split lines after the dot?
>> Sorry for my misunderstanding. Need to occupy one line for each sentence
>> here?
> A sentence may be longer than one line,
> but it is better split lines logically (after dots, commas, etc).
>
>> Do we generally use this style to make comments? or are we just trying
>> to highlight it here?
> That's a recommendation when writing text (comments and rst doc).
> Having lines split logically may help reading,
> and will make simpler patches when updating in future,
> because in general we change a single sentence.
All right, got it. The next version will be out soon.
>
>
> .
@@ -101,10 +101,16 @@ API Changes
Use fixed width quotes for ``function_names`` or ``struct_names``.
Use the past tense.
+
This section is a comment. Do not overwrite or remove it.
Also, make sure to start the actual text at the margin.
=======================================================
+ * ethdev: ensured all entries in MAC address list is unique.
+ The function ``rte_eth_dev_default_mac_addr_set`` replaces the address
+ at index 0 of the address list. If the address was already in the
+ address list, it is removed from the rest of the list.
+
ABI Changes
-----------
@@ -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];
@@ -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
@@ -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.