* [PATCH] ethdev: optimize how the values of the flag variables are assigned @ 2025-05-08 2:33 Sunyang Wu 2025-05-08 5:34 ` Stephen Hemminger 0 siblings, 1 reply; 4+ messages in thread From: Sunyang Wu @ 2025-05-08 2:33 UTC (permalink / raw) To: dev; +Cc: thomas, ferruh.yigit, andrew.rybchenko 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ethdev: optimize how the values of the flag variables are assigned 2025-05-08 2:33 [PATCH] ethdev: optimize how the values of the flag variables are assigned Sunyang Wu @ 2025-05-08 5:34 ` Stephen Hemminger 2025-05-08 6:26 ` 回复: " Sunyang Wu 0 siblings, 1 reply; 4+ messages in thread From: Stephen Hemminger @ 2025-05-08 5:34 UTC (permalink / raw) To: Sunyang Wu; +Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko [-- Attachment #1: Type: text/plain, Size: 1473 bytes --] 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 > > [-- Attachment #2: Type: text/html, Size: 2162 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* 回复: [PATCH] ethdev: optimize how the values of the flag variables are assigned 2025-05-08 5:34 ` Stephen Hemminger @ 2025-05-08 6:26 ` Sunyang Wu 2025-05-08 8:17 ` Morten Brørup 0 siblings, 1 reply; 4+ messages in thread From: Sunyang Wu @ 2025-05-08 6:26 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko [-- Attachment #1: Type: text/plain, Size: 2604 bytes --] 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(-) 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 [-- Attachment #2: Type: text/html, Size: 6819 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] ethdev: optimize how the values of the flag variables are assigned 2025-05-08 6:26 ` 回复: " Sunyang Wu @ 2025-05-08 8:17 ` Morten Brørup 0 siblings, 0 replies; 4+ messages in thread From: Morten Brørup @ 2025-05-08 8:17 UTC (permalink / raw) To: Sunyang Wu, Stephen Hemminger Cc: dev, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-08 8:17 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-05-08 2:33 [PATCH] ethdev: optimize how the values of the flag variables are assigned Sunyang Wu 2025-05-08 5:34 ` Stephen Hemminger 2025-05-08 6:26 ` 回复: " Sunyang Wu 2025-05-08 8:17 ` Morten Brørup
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).