* [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start
@ 2018-07-20 10:02 Radu Nicolau
2018-07-24 14:34 ` Chas Williams
2018-07-25 9:39 ` [dpdk-dev] [PATCH v2] net/bonding: fix race condition Radu Nicolau
0 siblings, 2 replies; 6+ messages in thread
From: Radu Nicolau @ 2018-07-20 10:02 UTC (permalink / raw)
To: dev; +Cc: declan.doherty, chas3, Radu Nicolau, stable
Race condition can appear in the bond_mode_8023ad_periodic_cb()
callback when bonding port is stopped, reconfigured and restarted.
Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index fc4d4fd..6f66743 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2087,8 +2087,6 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
}
eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
- eth_dev->data->dev_started = 1;
-
internals = eth_dev->data->dev_private;
if (internals->slave_count == 0) {
@@ -2167,6 +2165,7 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
internals->mode == BONDING_MODE_ALB)
bond_tlb_enable(internals);
+ eth_dev->data->dev_started = 1;
return 0;
out_err:
--
2.7.5
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start
2018-07-20 10:02 [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start Radu Nicolau
@ 2018-07-24 14:34 ` Chas Williams
2018-07-25 8:53 ` Radu Nicolau
2018-07-25 9:39 ` [dpdk-dev] [PATCH v2] net/bonding: fix race condition Radu Nicolau
1 sibling, 1 reply; 6+ messages in thread
From: Chas Williams @ 2018-07-24 14:34 UTC (permalink / raw)
To: Radu Nicolau; +Cc: dev, Declan Doherty, Chas Williams, stable
I think this adds another race. In
bond_ethdev_slave_link_status_change_monitor(), it checks to see if bonding
is started, if not it exits. So you need to have dev_started = 1 set
before you enable this callback.
On Fri, Jul 20, 2018 at 6:09 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
> Race condition can appear in the bond_mode_8023ad_periodic_cb()
> callback when bonding port is stopped, reconfigured and restarted.
>
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index fc4d4fd..6f66743 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2087,8 +2087,6 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> }
>
> eth_dev->data->dev_link.link_status = ETH_LINK_DOWN;
> - eth_dev->data->dev_started = 1;
> -
> internals = eth_dev->data->dev_private;
>
> if (internals->slave_count == 0) {
> @@ -2167,6 +2165,7 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> internals->mode == BONDING_MODE_ALB)
> bond_tlb_enable(internals);
>
> + eth_dev->data->dev_started = 1;
> return 0;
>
> out_err:
> --
> 2.7.5
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start
2018-07-24 14:34 ` Chas Williams
@ 2018-07-25 8:53 ` Radu Nicolau
0 siblings, 0 replies; 6+ messages in thread
From: Radu Nicolau @ 2018-07-25 8:53 UTC (permalink / raw)
To: Chas Williams; +Cc: dev, Declan Doherty, Chas Williams, stable
On 7/24/2018 3:34 PM, Chas Williams wrote:
> I think this adds another race. In
> bond_ethdev_slave_link_status_change_monitor(), it checks to see if
> bonding is started, if not it exits. So you need to have dev_started
> = 1 set before you enable this callback.
>
Indeed, I will have another look at this. Thanks for reviewing!
Can you also have a look at this one? https://patches.dpdk.org/patch/43187/
I think I addressed all your previous feedback.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH v2] net/bonding: fix race condition
2018-07-20 10:02 [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start Radu Nicolau
2018-07-24 14:34 ` Chas Williams
@ 2018-07-25 9:39 ` Radu Nicolau
2018-07-27 10:55 ` Chas Williams
1 sibling, 1 reply; 6+ messages in thread
From: Radu Nicolau @ 2018-07-25 9:39 UTC (permalink / raw)
To: dev; +Cc: declan.doherty, chas3, ferruh.yigit, Radu Nicolau, stable
Race condition can appear in the bond_mode_8023ad_periodic_cb()
callback when bonding port is stopped, reconfigured and restarted.
Re-ordered calls in bond_ethdev_start() to have callback alarm set
after slave ports are reconfigured.
Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
v2: reworked patch
drivers/net/bonding/rte_eth_bond_pmd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index fc4d4fd..70ec728 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2113,10 +2113,6 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
}
}
- /* Update all slave devices MACs*/
- if (mac_address_slaves_update(eth_dev) != 0)
- goto out_err;
-
/* If bonded device is configure in promiscuous mode then re-apply config */
if (internals->promiscuous_en)
bond_ethdev_promiscuous_enable(eth_dev);
@@ -2157,6 +2153,10 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
(void *)&rte_eth_devices[internals->port_id]);
}
+ /* Update all slave devices MACs*/
+ if (mac_address_slaves_update(eth_dev) != 0)
+ goto out_err;
+
if (internals->user_defined_primary_port)
bond_ethdev_primary_set(internals, internals->primary_port);
--
2.7.5
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/bonding: fix race condition
2018-07-25 9:39 ` [dpdk-dev] [PATCH v2] net/bonding: fix race condition Radu Nicolau
@ 2018-07-27 10:55 ` Chas Williams
2018-08-05 0:09 ` Thomas Monjalon
0 siblings, 1 reply; 6+ messages in thread
From: Chas Williams @ 2018-07-27 10:55 UTC (permalink / raw)
To: Radu Nicolau; +Cc: dev, Declan Doherty, Chas Williams, Ferruh Yigit, stable
On Wed, Jul 25, 2018 at 5:46 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
> Race condition can appear in the bond_mode_8023ad_periodic_cb()
> callback when bonding port is stopped, reconfigured and restarted.
>
> Re-ordered calls in bond_ethdev_start() to have callback alarm set
> after slave ports are reconfigured.
>
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>
Acked-by: Chas Williams <chas3@att.com>
> ---
> v2: reworked patch
>
> drivers/net/bonding/rte_eth_bond_pmd.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index fc4d4fd..70ec728 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2113,10 +2113,6 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> }
> }
>
> - /* Update all slave devices MACs*/
> - if (mac_address_slaves_update(eth_dev) != 0)
> - goto out_err;
> -
> /* If bonded device is configure in promiscuous mode then re-apply
> config */
> if (internals->promiscuous_en)
> bond_ethdev_promiscuous_enable(eth_dev);
> @@ -2157,6 +2153,10 @@ bond_ethdev_start(struct rte_eth_dev *eth_dev)
> (void *)&rte_eth_devices[internals->port_id]);
> }
>
> + /* Update all slave devices MACs*/
> + if (mac_address_slaves_update(eth_dev) != 0)
> + goto out_err;
> +
> if (internals->user_defined_primary_port)
> bond_ethdev_primary_set(internals,
> internals->primary_port);
>
> --
> 2.7.5
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [dpdk-dev] [PATCH v2] net/bonding: fix race condition
2018-07-27 10:55 ` Chas Williams
@ 2018-08-05 0:09 ` Thomas Monjalon
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Monjalon @ 2018-08-05 0:09 UTC (permalink / raw)
To: Radu Nicolau
Cc: dev, Chas Williams, Declan Doherty, Chas Williams, Ferruh Yigit, stable
27/07/2018 12:55, Chas Williams:
> On Wed, Jul 25, 2018 at 5:46 AM Radu Nicolau <radu.nicolau@intel.com> wrote:
>
> > Race condition can appear in the bond_mode_8023ad_periodic_cb()
> > callback when bonding port is stopped, reconfigured and restarted.
> >
> > Re-ordered calls in bond_ethdev_start() to have callback alarm set
> > after slave ports are reconfigured.
> >
> > Fixes: 2efb58cbab6e ("bond: new link bonding library")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> >
>
> Acked-by: Chas Williams <chas3@att.com>
Applied, thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-05 0:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20 10:02 [dpdk-dev] [PATCH] net/bonding: set started flag at the end of dev start Radu Nicolau
2018-07-24 14:34 ` Chas Williams
2018-07-25 8:53 ` Radu Nicolau
2018-07-25 9:39 ` [dpdk-dev] [PATCH v2] net/bonding: fix race condition Radu Nicolau
2018-07-27 10:55 ` Chas Williams
2018-08-05 0:09 ` Thomas Monjalon
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).