diff mbox series

回复: [PATCH] ethdev: optimize how the values of the flag variables are assigned

Message ID KL1PR0601MB55678B1F0A4D7AC99B21B5F6908BA@KL1PR0601MB5567.apcprd06.prod.outlook.com (mailing list archive)
State Not Applicable
Delegated to: Stephen Hemminger
Headers
Series 回复: [PATCH] ethdev: optimize how the values of the flag variables are assigned |

Checks

Context Check Description
ci/loongarch-compilation warning apply patch failure
ci/Intel-compilation warning apply issues
ci/iol-testing warning apply patch failure

Commit Message

Sunyang Wu May 8, 2025, 6:26 a.m. UTC 
Thank you for your reply. Personally, I think that when disabling promiscuous/all-multicast mode, the corresponding flag should be set based on the return value. This is because, at the driver implementation level, the driver may check the flag to determine whether the corresponding disable operation needs to be executed. If the flag is set before the operation is completed, the driver will not execute the operation when it checks the flag, as it will find that the flag has already been set.

发件人: Stephen Hemminger <stephen@networkplumber.org>
发送时间: 2025年5月8日 13:35
收件人: Sunyang Wu <sunyang.wu@jaguarmicro.com>
抄送: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
主题: Re: [PATCH] ethdev: optimize how the values of the flag variables are assigned

External Mail: This email originated from OUTSIDE of the organization!
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe.

Why bother? This is not critical path.
Original code looked fine

On Thu, May 8, 2025, 11:34 Sunyang Wu <sunyang.wu@jaguarmicro.com<mailto:sunyang.wu@jaguarmicro.com>> wrote:
Set the values of the promiscuous and all_multicast variables
according to the return value.

Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com<mailto:sunyang.wu@jaguarmicro.com>>
---
 lib/ethdev/rte_ethdev.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

--
2.19.0.rc0.windows.1

Comments

Morten Brørup May 8, 2025, 8:17 a.m. UTC | #1 
Wouldn't the correct fix assume the change has no effect if the operation failed, like the _enable_ functions do, i.e.:

int
rte_eth_promiscuous_disable(uint16_t port_id)
{
	struct rte_eth_dev *dev;
	int diag = 0;

	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
	dev = &rte_eth_devices[port_id];

	if (dev->data->promiscuous == 0)
		return 0;

	if (dev->dev_ops->promiscuous_disable == NULL)
		return -ENOTSUP;

-	dev->data->promiscuous = 0;
	diag = dev->dev_ops->promiscuous_disable(dev);
-	if (diag != 0)
-		dev->data->promiscuous = 1;
+	if (diag == 0)
+		dev->data->promiscuous = 0;
	diag = eth_err(port_id, diag);

	rte_eth_trace_promiscuous_disable(port_id, dev->data->promiscuous,
					  diag);

	return diag;
}


From: Sunyang Wu [mailto:sunyang.wu@jaguarmicro.com] 
Sent: Thursday, 8 May 2025 08.27

Thank you for your reply. Personally, I think that when disabling promiscuous/all-multicast mode, the corresponding flag should be set based on the return value. This is because, at the driver implementation level, the driver may check the flag to determine whether the corresponding disable operation needs to be executed. If the flag is set before the operation is completed, the driver will not execute the operation when it checks the flag, as it will find that the flag has already been set.

发件人: Stephen Hemminger <stephen@networkplumber.org> 
发送时间: 2025年5月8日 13:35
收件人: Sunyang Wu <sunyang.wu@jaguarmicro.com>
抄送: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
主题: Re: [PATCH] ethdev: optimize how the values of the flag variables are assigned


External Mail: This email originated from OUTSIDE of the organization! 
Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe. 

Why bother? This is not critical path. 
Original code looked fine 

On Thu, May 8, 2025, 11:34 Sunyang Wu <sunyang.wu@jaguarmicro.com> wrote:
Set the values of the promiscuous and all_multicast variables
according to the return value.

Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
---
 lib/ethdev/rte_ethdev.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d4197322a0..b1f593edc4 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3044,10 +3044,8 @@ rte_eth_promiscuous_disable(uint16_t port_id)
        if (dev->dev_ops->promiscuous_disable == NULL)
                return -ENOTSUP;

-       dev->data->promiscuous = 0;
        diag = dev->dev_ops->promiscuous_disable(dev);
-       if (diag != 0)
-               dev->data->promiscuous = 1;
+       dev->data->promiscuous = (diag == 0) ? 0 : 1;

        diag = eth_err(port_id, diag);

@@ -3112,10 +3110,9 @@ rte_eth_allmulticast_disable(uint16_t port_id)

        if (dev->dev_ops->allmulticast_disable == NULL)
                return -ENOTSUP;
-       dev->data->all_multicast = 0;
+
        diag = dev->dev_ops->allmulticast_disable(dev);
-       if (diag != 0)
-               dev->data->all_multicast = 1;
+       dev->data->all_multicast = (diag == 0) ? 0 : 1;

        diag = eth_err(port_id, diag);

-- 
2.19.0.rc0.windows.1
Stephen Hemminger May 20, 2025, 3:25 p.m. UTC | #2 
On Thu, 8 May 2025 10:17:44 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> Wouldn't the correct fix assume the change has no effect if the operation failed, like the _enable_ functions do, i.e.:
> 
> int
> rte_eth_promiscuous_disable(uint16_t port_id)
> {
> 	struct rte_eth_dev *dev;
> 	int diag = 0;
> 
> 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 	dev = &rte_eth_devices[port_id];
> 
> 	if (dev->data->promiscuous == 0)
> 		return 0;
> 
> 	if (dev->dev_ops->promiscuous_disable == NULL)
> 		return -ENOTSUP;
> 
> -	dev->data->promiscuous = 0;
> 	diag = dev->dev_ops->promiscuous_disable(dev);
> -	if (diag != 0)
> -		dev->data->promiscuous = 1;
> +	if (diag == 0)
> +		dev->data->promiscuous = 0;
> 	diag = eth_err(port_id, diag);
> 
> 	rte_eth_trace_promiscuous_disable(port_id, dev->data->promiscuous,
> 					  diag);
> 
> 	return diag;
> }
> 
> 
> From: Sunyang Wu [mailto:sunyang.wu@jaguarmicro.com] 
> Sent: Thursday, 8 May 2025 08.27
> 
> Thank you for your reply. Personally, I think that when disabling promiscuous/all-multicast mode, the corresponding flag should be set based on the return value. This is because, at the driver implementation level, the driver may check the flag to determine whether the corresponding disable operation needs to be executed. If the flag is set before the operation is completed, the driver will not execute the operation when it checks the flag, as it will find that the flag has already been set.
> 
> 发件人: Stephen Hemminger <stephen@networkplumber.org> 
> 发送时间: 2025年5月8日 13:35
> 收件人: Sunyang Wu <sunyang.wu@jaguarmicro.com>
> 抄送: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> 主题: Re: [PATCH] ethdev: optimize how the values of the flag variables are assigned
> 
> 
> External Mail: This email originated from OUTSIDE of the organization! 
> Do not click links, open attachments or provide ANY information unless you recognize the sender and know the content is safe. 
> 
> Why bother? This is not critical path. 
> Original code looked fine 
> 
> On Thu, May 8, 2025, 11:34 Sunyang Wu <sunyang.wu@jaguarmicro.com> wrote:
> Set the values of the promiscuous and all_multicast variables
> according to the return value.
> 
> Signed-off-by: Sunyang Wu <sunyang.wu@jaguarmicro.com>
> ---
>  lib/ethdev/rte_ethdev.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index d4197322a0..b1f593edc4 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -3044,10 +3044,8 @@ rte_eth_promiscuous_disable(uint16_t port_id)
>         if (dev->dev_ops->promiscuous_disable == NULL)
>                 return -ENOTSUP;
> 
> -       dev->data->promiscuous = 0;
>         diag = dev->dev_ops->promiscuous_disable(dev);
> -       if (diag != 0)
> -               dev->data->promiscuous = 1;
> +       dev->data->promiscuous = (diag == 0) ? 0 : 1;
> 
>         diag = eth_err(port_id, diag);
> 
> @@ -3112,10 +3110,9 @@ rte_eth_allmulticast_disable(uint16_t port_id)
> 
>         if (dev->dev_ops->allmulticast_disable == NULL)
>                 return -ENOTSUP;
> -       dev->data->all_multicast = 0;
> +
>         diag = dev->dev_ops->allmulticast_disable(dev);
> -       if (diag != 0)
> -               dev->data->all_multicast = 1;
> +       dev->data->all_multicast = (diag == 0) ? 0 : 1;
> 
>         diag = eth_err(port_id, diag);
> 

Agree.
The concept is good, but it is not an optimization.
There is a bug here. The flag should not be set if driver returns an error.
The error should be directly returned to application and state should not change.
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index d4197322a0..b1f593edc4 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3044,10 +3044,8 @@  rte_eth_promiscuous_disable(uint16_t port_id)
        if (dev->dev_ops->promiscuous_disable == NULL)
                return -ENOTSUP;

-       dev->data->promiscuous = 0;
        diag = dev->dev_ops->promiscuous_disable(dev);
-       if (diag != 0)
-               dev->data->promiscuous = 1;
+       dev->data->promiscuous = (diag == 0) ? 0 : 1;

        diag = eth_err(port_id, diag);

@@ -3112,10 +3110,9 @@  rte_eth_allmulticast_disable(uint16_t port_id)

        if (dev->dev_ops->allmulticast_disable == NULL)
                return -ENOTSUP;
-       dev->data->all_multicast = 0;
+
        diag = dev->dev_ops->allmulticast_disable(dev);
-       if (diag != 0)
-               dev->data->all_multicast = 1;
+       dev->data->all_multicast = (diag == 0) ? 0 : 1;

        diag = eth_err(port_id, diag);