On Thu, 8 May 2025 10:17:44 +0200
Morten Brørup <[email protected]> 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:[email protected]]
> 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 <[email protected]>
> 发送时间: 2025年5月8日 13:35
> 收件人: Sunyang Wu <[email protected]>
> 抄送: dev <[email protected]>; Thomas Monjalon <[email protected]>; Ferruh Yigit
> <[email protected]>; Andrew Rybchenko <[email protected]>
> 主题: 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 <[email protected]> wrote:
> Set the values of the promiscuous and all_multicast variables
> according to the return value.
>
> Signed-off-by: Sunyang Wu <[email protected]>
> ---
> 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.
