[PATCH] failsafe: fix segfault on hotplug event

[PATCH] failsafe: fix segfault on hotplug event

[Luc Pelletier]

When the failsafe PMD encounters a hotplug event, it switches its rx/tx
functions to "safe" ones that validate the sub-device's rx/tx functions
before calling them. It switches the rx/tx functions by changing the
function pointers in the rte_eth_dev structure.

Following commit 7a0935239b, the rx/tx functions of PMDs are no longer
called through the function pointers in the rte_eth_dev structure. They
are rather called through a flat array named rte_eth_fp_ops. The
function pointers in that array are initialized when the devices start
and are initialized.

When a hotplug event occurs, the function pointers in rte_eth_fp_ops
still point to the "unsafe" rx/tx functions in the failsafe PMD since
they haven't been updated. This results in a segmentation fault because
it ends up using the "unsafe" functions, when the "safe" functions
should have been used.

To fix the problem, the failsafe PMD code was changed to update the
function pointers in the rte_eth_fp_ops array when a hotplug event
occurs.

Fixes: 7a0935239b ("ethdev: make fast-path functions to use new flat array")
Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_rxtx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index fe67293299..34d59dfbb1 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -5,6 +5,7 @@
 
 #include <rte_atomic.h>
 #include <rte_debug.h>
+#include <rte_ethdev.h>
 #include <rte_mbuf.h>
 #include <ethdev_driver.h>
 
@@ -44,9 +45,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 		DEBUG("Using safe RX bursts%s",
 		      (force_safe ? " (forced)" : ""));
 		dev->rx_pkt_burst = &failsafe_rx_burst;
+		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
+			&failsafe_rx_burst;
 	} else if (!need_safe && safe_set) {
 		DEBUG("Using fast RX bursts");
 		dev->rx_pkt_burst = &failsafe_rx_burst_fast;
+		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
+			&failsafe_rx_burst_fast;
 	}
 	need_safe = force_safe || fs_tx_unsafe(TX_SUBDEV(dev));
 	safe_set = (dev->tx_pkt_burst == &failsafe_tx_burst);
@@ -54,9 +59,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 		DEBUG("Using safe TX bursts%s",
 		      (force_safe ? " (forced)" : ""));
 		dev->tx_pkt_burst = &failsafe_tx_burst;
+		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
+			&failsafe_tx_burst;
 	} else if (!need_safe && safe_set) {
 		DEBUG("Using fast TX bursts");
 		dev->tx_pkt_burst = &failsafe_tx_burst_fast;
+		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
+			&failsafe_tx_burst_fast;
 	}
 	rte_wmb();
 }
-- 
2.25.1

[PATCH v2] failsafe: fix segfault on hotplug event

[Luc Pelletier]

When the failsafe PMD encounters a hotplug event, it switches its rx/tx
functions to "safe" ones that validate the sub-device's rx/tx functions
before calling them. It switches the rx/tx functions by changing the
function pointers in the rte_eth_dev structure.

Following commit 7a0935239b9e, the rx/tx functions of PMDs are no longer
called through the function pointers in the rte_eth_dev structure. They
are rather called through a flat array named rte_eth_fp_ops. The
function pointers in that array are initialized when the devices start
and are initialized.

When a hotplug event occurs, the function pointers in rte_eth_fp_ops
still point to the "unsafe" rx/tx functions in the failsafe PMD since
they haven't been updated. This results in a segmentation fault because
it ends up using the "unsafe" functions, when the "safe" functions
should have been used.

To fix the problem, the failsafe PMD code was changed to update the
function pointers in the rte_eth_fp_ops array when a hotplug event
occurs.

Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---

v2:
* fixed git commit hashes in commit message

 drivers/net/failsafe/failsafe_rxtx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index fe67293299..34d59dfbb1 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -5,6 +5,7 @@
 
 #include <rte_atomic.h>
 #include <rte_debug.h>
+#include <rte_ethdev.h>
 #include <rte_mbuf.h>
 #include <ethdev_driver.h>
 
@@ -44,9 +45,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 		DEBUG("Using safe RX bursts%s",
 		      (force_safe ? " (forced)" : ""));
 		dev->rx_pkt_burst = &failsafe_rx_burst;
+		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
+			&failsafe_rx_burst;
 	} else if (!need_safe && safe_set) {
 		DEBUG("Using fast RX bursts");
 		dev->rx_pkt_burst = &failsafe_rx_burst_fast;
+		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
+			&failsafe_rx_burst_fast;
 	}
 	need_safe = force_safe || fs_tx_unsafe(TX_SUBDEV(dev));
 	safe_set = (dev->tx_pkt_burst == &failsafe_tx_burst);
@@ -54,9 +59,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 		DEBUG("Using safe TX bursts%s",
 		      (force_safe ? " (forced)" : ""));
 		dev->tx_pkt_burst = &failsafe_tx_burst;
+		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
+			&failsafe_tx_burst;
 	} else if (!need_safe && safe_set) {
 		DEBUG("Using fast TX bursts");
 		dev->tx_pkt_burst = &failsafe_tx_burst_fast;
+		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
+			&failsafe_tx_burst_fast;
 	}
 	rte_wmb();
 }
-- 
2.25.1

Re: [PATCH v2] failsafe: fix segfault on hotplug event

[Stephen Hemminger]

On Thu, 10 Nov 2022 12:42:43 -0500
Luc Pelletier <lucp.at.work@gmail.com> wrote:

> When the failsafe PMD encounters a hotplug event, it switches its rx/tx
> functions to "safe" ones that validate the sub-device's rx/tx functions
> before calling them. It switches the rx/tx functions by changing the
> function pointers in the rte_eth_dev structure.
> 
> Following commit 7a0935239b9e, the rx/tx functions of PMDs are no longer
> called through the function pointers in the rte_eth_dev structure. They
> are rather called through a flat array named rte_eth_fp_ops. The
> function pointers in that array are initialized when the devices start
> and are initialized.
> 
> When a hotplug event occurs, the function pointers in rte_eth_fp_ops
> still point to the "unsafe" rx/tx functions in the failsafe PMD since
> they haven't been updated. This results in a segmentation fault because
> it ends up using the "unsafe" functions, when the "safe" functions
> should have been used.
> 
> To fix the problem, the failsafe PMD code was changed to update the
> function pointers in the rte_eth_fp_ops array when a hotplug event
> occurs.

Have it in both places might be breaking other drivers as well.
Shouldn't there be a ethdev function when changing rx/tx burst.

Also, changing a variable used by another thread needs to be
using __atomic_store and __atomic_load to guarantee that CPU
or compiler will no that it changed.

RE: [PATCH] failsafe: fix segfault on hotplug event

[Konstantin Ananyev]

 
> When the failsafe PMD encounters a hotplug event, it switches its rx/tx
> functions to "safe" ones that validate the sub-device's rx/tx functions
> before calling them. It switches the rx/tx functions by changing the
> function pointers in the rte_eth_dev structure.
> 
> Following commit 7a0935239b, the rx/tx functions of PMDs are no longer
> called through the function pointers in the rte_eth_dev structure. They
> are rather called through a flat array named rte_eth_fp_ops. The
> function pointers in that array are initialized when the devices start
> and are initialized.
> 
> When a hotplug event occurs, the function pointers in rte_eth_fp_ops
> still point to the "unsafe" rx/tx functions in the failsafe PMD since
> they haven't been updated. This results in a segmentation fault because
> it ends up using the "unsafe" functions, when the "safe" functions
> should have been used.
> 
> To fix the problem, the failsafe PMD code was changed to update the
> function pointers in the rte_eth_fp_ops array when a hotplug event
> occurs.

 
It is not recommended way to update rte_eth_fp_ops[] contents directly.
There are eth_dev_fp_ops_setup()/ eth_dev_fp_ops_reset() that supposed
to be used for that.
About the fix itself - while it might help till some extent,
I think it will not remove the problem completely.
There still remain a race-condition between rte_eth_rx_burst() and failsafe_eth_rmv_event_callback().
Right now DPDK doesn't support switching PMD fast-ops functions (or updating rxq/txq data)
on the fly.
  
> Fixes: 7a0935239b ("ethdev: make fast-path functions to use new flat array")
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> index fe67293299..34d59dfbb1 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -5,6 +5,7 @@
> 
>  #include <rte_atomic.h>
>  #include <rte_debug.h>
> +#include <rte_ethdev.h>
>  #include <rte_mbuf.h>
>  #include <ethdev_driver.h>
> 
> @@ -44,9 +45,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  		DEBUG("Using safe RX bursts%s",
>  		      (force_safe ? " (forced)" : ""));
>  		dev->rx_pkt_burst = &failsafe_rx_burst;
> +		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
> +			&failsafe_rx_burst;
>  	} else if (!need_safe && safe_set) {
>  		DEBUG("Using fast RX bursts");
>  		dev->rx_pkt_burst = &failsafe_rx_burst_fast;
> +		rte_eth_fp_ops[dev->data->port_id].rx_pkt_burst =
> +			&failsafe_rx_burst_fast;
>  	}
>  	need_safe = force_safe || fs_tx_unsafe(TX_SUBDEV(dev));
>  	safe_set = (dev->tx_pkt_burst == &failsafe_tx_burst);
> @@ -54,9 +59,13 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  		DEBUG("Using safe TX bursts%s",
>  		      (force_safe ? " (forced)" : ""));
>  		dev->tx_pkt_burst = &failsafe_tx_burst;
> +		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
> +			&failsafe_tx_burst;
>  	} else if (!need_safe && safe_set) {
>  		DEBUG("Using fast TX bursts");
>  		dev->tx_pkt_burst = &failsafe_tx_burst_fast;
> +		rte_eth_fp_ops[dev->data->port_id].tx_pkt_burst =
> +			&failsafe_tx_burst_fast;
>  	}
>  	rte_wmb();
>  }
> --
> 2.25.1

Re: [PATCH] failsafe: fix segfault on hotplug event

[Luc Pelletier]

Hi Konstantin,

> It is not recommended way to update rte_eth_fp_ops[] contents directly.
> There are eth_dev_fp_ops_setup()/ eth_dev_fp_ops_reset() that supposed
> to be used for that.

Good to know. I see another fix that was made in a different PMD that
does exactly the same thing:

https://github.com/DPDK/dpdk/commit/bcd68b68415172815e55fc67cf3947c0433baf74

CC'ing the authors for awareness.

> About the fix itself - while it might help till some extent,
> I think it will not remove the problem completely.
> There still remain a race-condition between rte_eth_rx_burst() and failsafe_eth_rmv_event_callback().
> Right now DPDK doesn't support switching PMD fast-ops functions (or updating rxq/txq data)
> on the fly.

Thanks for the information. This is very helpful.

Are you saying that the previous code also had that same race
condition? It was only updating the rte_eth_dev structure, but I
assume the problem would have been the same since rte_eth_rx_burst()
in DPDK versions <=20 use the function pointers in rte_eth_dev, not
rte_eth_fp_ops.

Can you think of a possible solution to this problem? I'm happy to
provide a patch to properly fix the problem. Having your guidance
would be extremely helpful.

Thanks!

Re: [PATCH] failsafe: fix segfault on hotplug event

[Stephen Hemminger]

On Wed, 16 Nov 2022 16:51:59 -0500
Luc Pelletier <lucp.at.work@gmail.com> wrote:

> Hi Konstantin,
> 
> > It is not recommended way to update rte_eth_fp_ops[] contents directly.
> > There are eth_dev_fp_ops_setup()/ eth_dev_fp_ops_reset() that supposed
> > to be used for that.  
> 
> Good to know. I see another fix that was made in a different PMD that
> does exactly the same thing:
> 
> https://github.com/DPDK/dpdk/commit/bcd68b68415172815e55fc67cf3947c0433baf74
> 
> CC'ing the authors for awareness.
> 
> > About the fix itself - while it might help till some extent,
> > I think it will not remove the problem completely.
> > There still remain a race-condition between rte_eth_rx_burst() and failsafe_eth_rmv_event_callback().
> > Right now DPDK doesn't support switching PMD fast-ops functions (or updating rxq/txq data)
> > on the fly.  
> 
> Thanks for the information. This is very helpful.
> 
> Are you saying that the previous code also had that same race
> condition? It was only updating the rte_eth_dev structure, but I
> assume the problem would have been the same since rte_eth_rx_burst()
> in DPDK versions <=20 use the function pointers in rte_eth_dev, not
> rte_eth_fp_ops.
> 
> Can you think of a possible solution to this problem? I'm happy to
> provide a patch to properly fix the problem. Having your guidance
> would be extremely helpful.
> 
> Thanks!

Changing burst mode on a running device is not safe because
of lack of locking and/or memory barriers.

Would have been better to not to do this optimization.
Just have one rx_burst/tx_burst function and look at what
ever conditions are present there.

RE: [PATCH] failsafe: fix segfault on hotplug event

[Konstantin Ananyev]

Hi Luc,
 
> > Hi Konstantin,
> >
> > > It is not recommended way to update rte_eth_fp_ops[] contents directly.
> > > There are eth_dev_fp_ops_setup()/ eth_dev_fp_ops_reset() that supposed
> > > to be used for that.
> >
> > Good to know. I see another fix that was made in a different PMD that
> > does exactly the same thing:
> >
> > https://github.com/DPDK/dpdk/commit/bcd68b68415172815e55fc67cf3947c0433baf74
> >
> > CC'ing the authors for awareness.
> >
> > > About the fix itself - while it might help till some extent,
> > > I think it will not remove the problem completely.
> > > There still remain a race-condition between rte_eth_rx_burst() and failsafe_eth_rmv_event_callback().
> > > Right now DPDK doesn't support switching PMD fast-ops functions (or updating rxq/txq data)
> > > on the fly.
> >
> > Thanks for the information. This is very helpful.
> >
> > Are you saying that the previous code also had that same race
> > condition?

Yes, I believe so. 

> It was only updating the rte_eth_dev structure, but I
> > assume the problem would have been the same since rte_eth_rx_burst()
> > in DPDK versions <=20 use the function pointers in rte_eth_dev, not
> > rte_eth_fp_ops.
> >
> > Can you think of a possible solution to this problem? I'm happy to
> > provide a patch to properly fix the problem. Having your guidance
> > would be extremely helpful.
> >
> > Thanks!
> 
> Changing burst mode on a running device is not safe because
> of lack of locking and/or memory barriers.
> 
> Would have been better to not to do this optimization.
> Just have one rx_burst/tx_burst function and look at what
> ever conditions are present there.

I think Stephen is right - within DPDK it is just not possible to switch RX/TX
function on the fly (without some external synchronization).
So the safe way is to always use safe version of RX/TX call.
I personally don't think such few extra checks will affect performance that much.

As another nit: inside failsafe rx_burst functions it is probably better not to access dev->data->rx_queues directly,
but call rte_eth_rx_burst(sdev->sdev_port_id, ...); instead.
Same for TX.

Thanks
Konstantin

 

[PATCH v3 0/5] Failsafe bug fixes and improvements

[Luc Pelletier]

Following the comments on v1 and v2 of the patch, I've tried to
address all issues that were mentioned. I've also fixed other issues
discovered during testing. I've split the changes into several
commits. Please note that only the bug fix commits have been marked
for back-porting to stable.

Luc Pelletier (5):
  failsafe: fix segfault on hotplug event
  failsafe: use public rx/tx burst API
  failsafe: fix double release of port
  failsafe: use public APIs in fs_link_update
  failsafe: make sub-device remove flag thread-safe

 drivers/net/failsafe/failsafe_ether.c   | 12 ++--
 drivers/net/failsafe/failsafe_ops.c     | 38 +++++-----
 drivers/net/failsafe/failsafe_private.h | 12 +---
 drivers/net/failsafe/failsafe_rxtx.c    | 94 ++-----------------------
 4 files changed, 28 insertions(+), 128 deletions(-)

-- 
2.38.1

[PATCH v3 1/5] failsafe: fix segfault on hotplug event

[Luc Pelletier]

When the failsafe PMD encounters a hotplug event, it switches its rx/tx
functions to "safe" ones that validate the sub-device's rx/tx functions
before calling them. It switches the rx/tx functions by changing the
function pointers in the rte_eth_dev structure.

Following commit 7a0935239b9e, the rx/tx functions of PMDs are no longer
called through the function pointers in the rte_eth_dev structure. They
are rather called through a flat array named rte_eth_fp_ops. The
function pointers in that array are initialized when the devices start
and are initialized.

When a hotplug event occurs, the function pointers in rte_eth_fp_ops
still point to the "unsafe" rx/tx functions in the failsafe PMD since
they haven't been updated. This results in a segmentation fault because
it ends up using the "unsafe" functions, when the "safe" functions
should have been used.

To fix the problem, the "unsafe" rx/tx functions were completely
removed. The "safe" functions are now always used. Modifying the rx/tx
functions on-the-fly is not supported by DPDK, so this is the correct
approach and should have very minimal impact on performance.

Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c   |  2 -
 drivers/net/failsafe/failsafe_private.h |  8 ---
 drivers/net/failsafe/failsafe_rxtx.c    | 83 +------------------------
 3 files changed, 1 insertion(+), 92 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 10b90fd837..517126565f 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -595,8 +595,6 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused,
 	fs_lock(fs_dev(sdev), 0);
 	/* Switch as soon as possible tx_dev. */
 	fs_switch_dev(fs_dev(sdev), sdev);
-	/* Use safe bursts in any case. */
-	failsafe_set_burst_fn(fs_dev(sdev), 1);
 	/*
 	 * Async removal, the sub-PMD will try to unregister
 	 * the callback at the source of the current thread context.
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 53a451c1b1..3865f2fc34 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -208,18 +208,11 @@ int failsafe_hotplug_alarm_cancel(struct rte_eth_dev *dev);
 
 /* RX / TX */
 
-void failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe);
-
 uint16_t failsafe_rx_burst(void *rxq,
 		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
 uint16_t failsafe_tx_burst(void *txq,
 		struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
 
-uint16_t failsafe_rx_burst_fast(void *rxq,
-		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
-uint16_t failsafe_tx_burst_fast(void *txq,
-		struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
-
 /* ARGS */
 
 int failsafe_args_parse(struct rte_eth_dev *dev, const char *params);
@@ -487,7 +480,6 @@ fs_switch_dev(struct rte_eth_dev *dev,
 	} else {
 		return;
 	}
-	failsafe_set_burst_fn(dev, 0);
 	rte_wmb();
 }
 
diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index fe67293299..707fe60a36 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -28,39 +28,6 @@ fs_tx_unsafe(struct sub_device *sdev)
 		(sdev->state != DEV_STARTED);
 }
 
-void
-failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
-{
-	struct sub_device *sdev;
-	uint8_t i;
-	int need_safe;
-	int safe_set;
-
-	need_safe = force_safe;
-	FOREACH_SUBDEV(sdev, i, dev)
-		need_safe |= fs_rx_unsafe(sdev);
-	safe_set = (dev->rx_pkt_burst == &failsafe_rx_burst);
-	if (need_safe && !safe_set) {
-		DEBUG("Using safe RX bursts%s",
-		      (force_safe ? " (forced)" : ""));
-		dev->rx_pkt_burst = &failsafe_rx_burst;
-	} else if (!need_safe && safe_set) {
-		DEBUG("Using fast RX bursts");
-		dev->rx_pkt_burst = &failsafe_rx_burst_fast;
-	}
-	need_safe = force_safe || fs_tx_unsafe(TX_SUBDEV(dev));
-	safe_set = (dev->tx_pkt_burst == &failsafe_tx_burst);
-	if (need_safe && !safe_set) {
-		DEBUG("Using safe TX bursts%s",
-		      (force_safe ? " (forced)" : ""));
-		dev->tx_pkt_burst = &failsafe_tx_burst;
-	} else if (!need_safe && safe_set) {
-		DEBUG("Using fast TX bursts");
-		dev->tx_pkt_burst = &failsafe_tx_burst_fast;
-	}
-	rte_wmb();
-}
-
 /*
  * Override source port in Rx packets.
  *
@@ -89,7 +56,7 @@ failsafe_rx_burst(void *queue,
 	rxq = queue;
 	sdev = rxq->sdev;
 	do {
-		if (fs_rx_unsafe(sdev)) {
+		if (unlikely(fs_rx_unsafe(sdev))) {
 			nb_rx = 0;
 			sdev = sdev->next;
 			continue;
@@ -108,34 +75,6 @@ failsafe_rx_burst(void *queue,
 	return nb_rx;
 }
 
-uint16_t
-failsafe_rx_burst_fast(void *queue,
-			 struct rte_mbuf **rx_pkts,
-			 uint16_t nb_pkts)
-{
-	struct sub_device *sdev;
-	struct rxq *rxq;
-	void *sub_rxq;
-	uint16_t nb_rx;
-
-	rxq = queue;
-	sdev = rxq->sdev;
-	do {
-		RTE_ASSERT(!fs_rx_unsafe(sdev));
-		sub_rxq = ETH(sdev)->data->rx_queues[rxq->qid];
-		FS_ATOMIC_P(rxq->refcnt[sdev->sid]);
-		nb_rx = ETH(sdev)->
-			rx_pkt_burst(sub_rxq, rx_pkts, nb_pkts);
-		FS_ATOMIC_V(rxq->refcnt[sdev->sid]);
-		sdev = sdev->next;
-	} while (nb_rx == 0 && sdev != rxq->sdev);
-	rxq->sdev = sdev;
-	if (nb_rx)
-		failsafe_rx_set_port(rx_pkts, nb_rx,
-				     rxq->priv->data->port_id);
-	return nb_rx;
-}
-
 uint16_t
 failsafe_tx_burst(void *queue,
 		  struct rte_mbuf **tx_pkts,
@@ -156,23 +95,3 @@ failsafe_tx_burst(void *queue,
 	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
 	return nb_tx;
 }
-
-uint16_t
-failsafe_tx_burst_fast(void *queue,
-			 struct rte_mbuf **tx_pkts,
-			 uint16_t nb_pkts)
-{
-	struct sub_device *sdev;
-	struct txq *txq;
-	void *sub_txq;
-	uint16_t nb_tx;
-
-	txq = queue;
-	sdev = TX_SUBDEV(&rte_eth_devices[txq->priv->data->port_id]);
-	RTE_ASSERT(!fs_tx_unsafe(sdev));
-	sub_txq = ETH(sdev)->data->tx_queues[txq->qid];
-	FS_ATOMIC_P(txq->refcnt[sdev->sid]);
-	nb_tx = ETH(sdev)->tx_pkt_burst(sub_txq, tx_pkts, nb_pkts);
-	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
-	return nb_tx;
-}
-- 
2.38.1

[PATCH v3 2/5] failsafe: use public rx/tx burst API

[Luc Pelletier]

The failsafe rx/tx functions are calling the rx/tx functions of
sub-devices via the function pointers stored in the rte_eth_dev
structure rather than use the public APIs (rte_eth_rx/tx_burst).

Use the public API rather than access structure internals which could
change in the future.

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_rxtx.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 707fe60a36..6d8ab0a6e7 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -50,7 +50,6 @@ failsafe_rx_burst(void *queue,
 {
 	struct sub_device *sdev;
 	struct rxq *rxq;
-	void *sub_rxq;
 	uint16_t nb_rx;
 
 	rxq = queue;
@@ -61,10 +60,8 @@ failsafe_rx_burst(void *queue,
 			sdev = sdev->next;
 			continue;
 		}
-		sub_rxq = ETH(sdev)->data->rx_queues[rxq->qid];
 		FS_ATOMIC_P(rxq->refcnt[sdev->sid]);
-		nb_rx = ETH(sdev)->
-			rx_pkt_burst(sub_rxq, rx_pkts, nb_pkts);
+		nb_rx = rte_eth_rx_burst(PORT_ID(sdev), rxq->qid, rx_pkts, nb_pkts);
 		FS_ATOMIC_V(rxq->refcnt[sdev->sid]);
 		sdev = sdev->next;
 	} while (nb_rx == 0 && sdev != rxq->sdev);
@@ -82,16 +79,14 @@ failsafe_tx_burst(void *queue,
 {
 	struct sub_device *sdev;
 	struct txq *txq;
-	void *sub_txq;
 	uint16_t nb_tx;
 
 	txq = queue;
 	sdev = TX_SUBDEV(&rte_eth_devices[txq->priv->data->port_id]);
 	if (unlikely(fs_tx_unsafe(sdev)))
 		return 0;
-	sub_txq = ETH(sdev)->data->tx_queues[txq->qid];
 	FS_ATOMIC_P(txq->refcnt[sdev->sid]);
-	nb_tx = ETH(sdev)->tx_pkt_burst(sub_txq, tx_pkts, nb_pkts);
+	nb_tx = rte_eth_tx_burst(PORT_ID(sdev), txq->qid, tx_pkts, nb_pkts);
 	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
 	return nb_tx;
 }
-- 
2.38.1

[PATCH v3 3/5] failsafe: fix double release of port

[Luc Pelletier]

When a sub-device is hot-unplugged, rte_eth_dev_release_port is being
called twice. Once indirectly via rte_eth_dev_close and once explicitly.

Changed the code to remove the explicit call since it's not required and
results in mistakenly releasing port 0 (due to zero'ing of memory being
done in rte_eth_dev_release_port).

Fixes: fac0ae546e5f ("net/failsafe: free port by dedicated function")
Cc: Matan Azrad <matan@nvidia.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 517126565f..3bfc446638 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -301,8 +301,6 @@ fs_dev_remove(struct sub_device *sdev)
 		if (ret < 0) {
 			ERROR("Bus detach failed for sub_device %u",
 			      SUB_ID(sdev));
-		} else {
-			rte_eth_dev_release_port(ETH(sdev));
 		}
 		sdev->state = DEV_PARSED;
 		/* fallthrough */
-- 
2.38.1

[PATCH v3 4/5] failsafe: use public APIs in fs_link_update

[Luc Pelletier]

The link_update function pointer is called directly on a sub-device, via
its rte_eth_dev structure. In addition, it assumes that if link_update
returns a value other than 0 or -1, an error occurred. That's not
accurate. For example, the mlx5 PMD returns a positive value to indicate
that the link status was updated. This results in fs_link_update failing
when it really should have succeeded.

The code now uses the public APIs rte_eth_link_get and
rte_eth_link_get_nowait to query the link status of each sub-device. It
also uses rte_eth_linkstatus_set to set the link status of the failsafe
device.

Fixes: ae80146c5a1b ("net/failsafe: fix removed device handling")
Cc: Matan Azrad <matan@nvidia.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ops.c | 38 ++++++++++++++---------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index d357e1bc83..7bd223af37 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -820,34 +820,32 @@ fs_link_update(struct rte_eth_dev *dev,
 {
 	struct sub_device *sdev;
 	uint8_t i;
-	int ret;
+	int ret = -1;
+	int sdev_link_ret;
+	struct rte_eth_link link_info;
 
 	fs_lock(dev, 0);
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
-		DEBUG("Calling link_update on sub_device %d", i);
-		ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete);
-		if (ret && ret != -1 && sdev->remove == 0 &&
-		    rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
-			ERROR("Link update failed for sub_device %d with error %d",
-			      i, ret);
-			fs_unlock(dev, 0);
-			return ret;
+		DEBUG("Calling rte_eth_link_get on sub_device %d", i);
+		if (wait_to_complete) {
+			sdev_link_ret = rte_eth_link_get(PORT_ID(sdev), &link_info);
+		} else {
+			sdev_link_ret = rte_eth_link_get_nowait(PORT_ID(sdev),
+				&link_info);
 		}
-	}
-	if (TX_SUBDEV(dev)) {
-		struct rte_eth_link *l1;
-		struct rte_eth_link *l2;
 
-		l1 = &dev->data->dev_link;
-		l2 = &ETH(TX_SUBDEV(dev))->data->dev_link;
-		if (memcmp(l1, l2, sizeof(*l1))) {
-			*l1 = *l2;
-			fs_unlock(dev, 0);
-			return 0;
+		if (likely(sdev_link_ret == 0)) {
+			if (TX_SUBDEV(dev) == sdev)
+				ret = rte_eth_linkstatus_set(dev, &link_info);
+		}
+		else if (sdev->remove == 0 &&
+				rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
+			ERROR("Link get failed for sub_device %d with error %d",
+				i, sdev_link_ret);
 		}
 	}
 	fs_unlock(dev, 0);
-	return -1;
+	return ret;
 }
 
 static int
-- 
2.38.1

[PATCH v3 5/5] failsafe: make sub-device remove flag thread-safe

[Luc Pelletier]

For each sub-device, there's a remove flag that's used to indicate if
the sub-device has been removed or not. This flag is currently a
volatile bit field. Even if it's marked as volatile, a bit field is not
thread-safe according to the C standard.

The remove flag is now an unsigned int rather than a bit field. We also
now use the atomic built-ins to read/write the value of the remove flag
to ensure thread-safety.

Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Cc: Gaetan Rivet <grive@u256.net>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c   | 8 ++++----
 drivers/net/failsafe/failsafe_ops.c     | 2 +-
 drivers/net/failsafe/failsafe_private.h | 4 ++--
 drivers/net/failsafe/failsafe_rxtx.c    | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 3bfc446638..9014777e52 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -311,7 +311,7 @@ fs_dev_remove(struct sub_device *sdev)
 		/* the end */
 		break;
 	}
-	sdev->remove = 0;
+	__atomic_store_n(&sdev->remove, 0, __ATOMIC_RELEASE);
 	failsafe_hotplug_alarm_install(fs_dev(sdev));
 }
 
@@ -388,7 +388,7 @@ failsafe_dev_remove(struct rte_eth_dev *dev)
 	uint8_t i;
 
 	FOREACH_SUBDEV(sdev, i, dev) {
-		if (!sdev->remove)
+		if (!__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE))
 			continue;
 
 		/* Active devices must have finished their burst and
@@ -556,7 +556,7 @@ failsafe_eth_dev_state_sync(struct rte_eth_dev *dev)
 err_remove:
 	FOREACH_SUBDEV(sdev, i, dev)
 		if (sdev->state != PRIV(dev)->state)
-			sdev->remove = 1;
+			__atomic_store_n(&sdev->remove, 1, __ATOMIC_RELEASE);
 	return ret;
 }
 
@@ -597,7 +597,7 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused,
 	 * Async removal, the sub-PMD will try to unregister
 	 * the callback at the source of the current thread context.
 	 */
-	sdev->remove = 1;
+	__atomic_store_n(&sdev->remove, 1, __ATOMIC_RELEASE);
 	fs_unlock(fs_dev(sdev), 0);
 	return 0;
 }
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index 7bd223af37..86d97970bc 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -838,7 +838,7 @@ fs_link_update(struct rte_eth_dev *dev,
 			if (TX_SUBDEV(dev) == sdev)
 				ret = rte_eth_linkstatus_set(dev, &link_info);
 		}
-		else if (sdev->remove == 0 &&
+		else if (__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) == 0 &&
 				rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
 			ERROR("Link get failed for sub_device %d with error %d",
 				i, sdev_link_ret);
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 3865f2fc34..0577a62ab2 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -131,7 +131,7 @@ struct sub_device {
 	/* sub device port id*/
 	uint16_t sdev_port_id; /* shared between processes */
 	/* flag calling for recollection */
-	volatile unsigned int remove:1;
+	unsigned int remove;
 	/* flow isolation state */
 	int flow_isolated:1;
 	/* RMV callback registration state */
@@ -490,7 +490,7 @@ static inline int
 fs_err(struct sub_device *sdev, int err)
 {
 	/* A device removal shouldn't be reported as an error. */
-	if (sdev->remove == 1 || err == -EIO)
+	if (__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) == 1 || err == -EIO)
 		return rte_errno = 0;
 	return err;
 }
diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 6d8ab0a6e7..c5caab4204 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -16,7 +16,7 @@ fs_rx_unsafe(struct sub_device *sdev)
 	return (ETH(sdev) == NULL) ||
 		(ETH(sdev)->rx_pkt_burst == NULL) ||
 		(sdev->state != DEV_STARTED) ||
-		(sdev->remove != 0);
+		(__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) != 0);
 }
 
 static inline int
-- 
2.38.1

[PATCH v4 0/5] Failsafe bug fixes and improvements

[Luc Pelletier]

v2:
* fixed git commit hashes in commit message
v3:
* address comments / split into several commits
v4:
* fix checkpatch warnings

Luc Pelletier (5):
  failsafe: fix segfault on hotplug event
  failsafe: use public rx/tx burst API
  failsafe: fix double release of port
  failsafe: use public APIs in fs_link_update
  failsafe: make sub-device remove flag thread-safe

 drivers/net/failsafe/failsafe_ether.c   | 12 ++--
 drivers/net/failsafe/failsafe_ops.c     | 37 +++++-----
 drivers/net/failsafe/failsafe_private.h | 12 +---
 drivers/net/failsafe/failsafe_rxtx.c    | 94 ++-----------------------
 4 files changed, 27 insertions(+), 128 deletions(-)

-- 
2.38.1

[PATCH v4 1/5] failsafe: fix segfault on hotplug event

[Luc Pelletier]

When the failsafe PMD encounters a hotplug event, it switches its rx/tx
functions to "safe" ones that validate the sub-device's rx/tx functions
before calling them. It switches the rx/tx functions by changing the
function pointers in the rte_eth_dev structure.

Following commit 7a0935239b9e, the rx/tx functions of PMDs are no longer
called through the function pointers in the rte_eth_dev structure. They
are rather called through a flat array named rte_eth_fp_ops. The
function pointers in that array are initialized when the devices start
and are initialized.

When a hotplug event occurs, the function pointers in rte_eth_fp_ops
still point to the "unsafe" rx/tx functions in the failsafe PMD since
they haven't been updated. This results in a segmentation fault because
it ends up using the "unsafe" functions, when the "safe" functions
should have been used.

To fix the problem, the "unsafe" rx/tx functions were completely
removed. The "safe" functions are now always used. Modifying the rx/tx
functions on-the-fly is not supported by DPDK, so this is the correct
approach and should have very minimal impact on performance.

Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c   |  2 -
 drivers/net/failsafe/failsafe_private.h |  8 ---
 drivers/net/failsafe/failsafe_rxtx.c    | 83 +------------------------
 3 files changed, 1 insertion(+), 92 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 10b90fd837..517126565f 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -595,8 +595,6 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused,
 	fs_lock(fs_dev(sdev), 0);
 	/* Switch as soon as possible tx_dev. */
 	fs_switch_dev(fs_dev(sdev), sdev);
-	/* Use safe bursts in any case. */
-	failsafe_set_burst_fn(fs_dev(sdev), 1);
 	/*
 	 * Async removal, the sub-PMD will try to unregister
 	 * the callback at the source of the current thread context.
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 53a451c1b1..3865f2fc34 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -208,18 +208,11 @@ int failsafe_hotplug_alarm_cancel(struct rte_eth_dev *dev);
 
 /* RX / TX */
 
-void failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe);
-
 uint16_t failsafe_rx_burst(void *rxq,
 		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
 uint16_t failsafe_tx_burst(void *txq,
 		struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
 
-uint16_t failsafe_rx_burst_fast(void *rxq,
-		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
-uint16_t failsafe_tx_burst_fast(void *txq,
-		struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
-
 /* ARGS */
 
 int failsafe_args_parse(struct rte_eth_dev *dev, const char *params);
@@ -487,7 +480,6 @@ fs_switch_dev(struct rte_eth_dev *dev,
 	} else {
 		return;
 	}
-	failsafe_set_burst_fn(dev, 0);
 	rte_wmb();
 }
 
diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index fe67293299..707fe60a36 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -28,39 +28,6 @@ fs_tx_unsafe(struct sub_device *sdev)
 		(sdev->state != DEV_STARTED);
 }
 
-void
-failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
-{
-	struct sub_device *sdev;
-	uint8_t i;
-	int need_safe;
-	int safe_set;
-
-	need_safe = force_safe;
-	FOREACH_SUBDEV(sdev, i, dev)
-		need_safe |= fs_rx_unsafe(sdev);
-	safe_set = (dev->rx_pkt_burst == &failsafe_rx_burst);
-	if (need_safe && !safe_set) {
-		DEBUG("Using safe RX bursts%s",
-		      (force_safe ? " (forced)" : ""));
-		dev->rx_pkt_burst = &failsafe_rx_burst;
-	} else if (!need_safe && safe_set) {
-		DEBUG("Using fast RX bursts");
-		dev->rx_pkt_burst = &failsafe_rx_burst_fast;
-	}
-	need_safe = force_safe || fs_tx_unsafe(TX_SUBDEV(dev));
-	safe_set = (dev->tx_pkt_burst == &failsafe_tx_burst);
-	if (need_safe && !safe_set) {
-		DEBUG("Using safe TX bursts%s",
-		      (force_safe ? " (forced)" : ""));
-		dev->tx_pkt_burst = &failsafe_tx_burst;
-	} else if (!need_safe && safe_set) {
-		DEBUG("Using fast TX bursts");
-		dev->tx_pkt_burst = &failsafe_tx_burst_fast;
-	}
-	rte_wmb();
-}
-
 /*
  * Override source port in Rx packets.
  *
@@ -89,7 +56,7 @@ failsafe_rx_burst(void *queue,
 	rxq = queue;
 	sdev = rxq->sdev;
 	do {
-		if (fs_rx_unsafe(sdev)) {
+		if (unlikely(fs_rx_unsafe(sdev))) {
 			nb_rx = 0;
 			sdev = sdev->next;
 			continue;
@@ -108,34 +75,6 @@ failsafe_rx_burst(void *queue,
 	return nb_rx;
 }
 
-uint16_t
-failsafe_rx_burst_fast(void *queue,
-			 struct rte_mbuf **rx_pkts,
-			 uint16_t nb_pkts)
-{
-	struct sub_device *sdev;
-	struct rxq *rxq;
-	void *sub_rxq;
-	uint16_t nb_rx;
-
-	rxq = queue;
-	sdev = rxq->sdev;
-	do {
-		RTE_ASSERT(!fs_rx_unsafe(sdev));
-		sub_rxq = ETH(sdev)->data->rx_queues[rxq->qid];
-		FS_ATOMIC_P(rxq->refcnt[sdev->sid]);
-		nb_rx = ETH(sdev)->
-			rx_pkt_burst(sub_rxq, rx_pkts, nb_pkts);
-		FS_ATOMIC_V(rxq->refcnt[sdev->sid]);
-		sdev = sdev->next;
-	} while (nb_rx == 0 && sdev != rxq->sdev);
-	rxq->sdev = sdev;
-	if (nb_rx)
-		failsafe_rx_set_port(rx_pkts, nb_rx,
-				     rxq->priv->data->port_id);
-	return nb_rx;
-}
-
 uint16_t
 failsafe_tx_burst(void *queue,
 		  struct rte_mbuf **tx_pkts,
@@ -156,23 +95,3 @@ failsafe_tx_burst(void *queue,
 	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
 	return nb_tx;
 }
-
-uint16_t
-failsafe_tx_burst_fast(void *queue,
-			 struct rte_mbuf **tx_pkts,
-			 uint16_t nb_pkts)
-{
-	struct sub_device *sdev;
-	struct txq *txq;
-	void *sub_txq;
-	uint16_t nb_tx;
-
-	txq = queue;
-	sdev = TX_SUBDEV(&rte_eth_devices[txq->priv->data->port_id]);
-	RTE_ASSERT(!fs_tx_unsafe(sdev));
-	sub_txq = ETH(sdev)->data->tx_queues[txq->qid];
-	FS_ATOMIC_P(txq->refcnt[sdev->sid]);
-	nb_tx = ETH(sdev)->tx_pkt_burst(sub_txq, tx_pkts, nb_pkts);
-	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
-	return nb_tx;
-}
-- 
2.38.1

[PATCH v4 2/5] failsafe: use public rx/tx burst API

[Luc Pelletier]

The failsafe rx/tx functions are calling the rx/tx functions of
sub-devices via the function pointers stored in the rte_eth_dev
structure rather than use the public APIs (rte_eth_rx/tx_burst).

Use the public API rather than access structure internals which could
change in the future.

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_rxtx.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 707fe60a36..6d8ab0a6e7 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -50,7 +50,6 @@ failsafe_rx_burst(void *queue,
 {
 	struct sub_device *sdev;
 	struct rxq *rxq;
-	void *sub_rxq;
 	uint16_t nb_rx;
 
 	rxq = queue;
@@ -61,10 +60,8 @@ failsafe_rx_burst(void *queue,
 			sdev = sdev->next;
 			continue;
 		}
-		sub_rxq = ETH(sdev)->data->rx_queues[rxq->qid];
 		FS_ATOMIC_P(rxq->refcnt[sdev->sid]);
-		nb_rx = ETH(sdev)->
-			rx_pkt_burst(sub_rxq, rx_pkts, nb_pkts);
+		nb_rx = rte_eth_rx_burst(PORT_ID(sdev), rxq->qid, rx_pkts, nb_pkts);
 		FS_ATOMIC_V(rxq->refcnt[sdev->sid]);
 		sdev = sdev->next;
 	} while (nb_rx == 0 && sdev != rxq->sdev);
@@ -82,16 +79,14 @@ failsafe_tx_burst(void *queue,
 {
 	struct sub_device *sdev;
 	struct txq *txq;
-	void *sub_txq;
 	uint16_t nb_tx;
 
 	txq = queue;
 	sdev = TX_SUBDEV(&rte_eth_devices[txq->priv->data->port_id]);
 	if (unlikely(fs_tx_unsafe(sdev)))
 		return 0;
-	sub_txq = ETH(sdev)->data->tx_queues[txq->qid];
 	FS_ATOMIC_P(txq->refcnt[sdev->sid]);
-	nb_tx = ETH(sdev)->tx_pkt_burst(sub_txq, tx_pkts, nb_pkts);
+	nb_tx = rte_eth_tx_burst(PORT_ID(sdev), txq->qid, tx_pkts, nb_pkts);
 	FS_ATOMIC_V(txq->refcnt[sdev->sid]);
 	return nb_tx;
 }
-- 
2.38.1

[PATCH v4 3/5] failsafe: fix double release of port

[Luc Pelletier]

When a sub-device is hot-unplugged, rte_eth_dev_release_port is being
called twice. Once indirectly via rte_eth_dev_close and once explicitly.

Changed the code to remove the explicit call since it's not required and
results in mistakenly releasing port 0 (due to zero'ing of memory being
done in rte_eth_dev_release_port).

Fixes: fac0ae546e5f ("net/failsafe: free port by dedicated function")
Cc: Matan Azrad <matan@nvidia.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 517126565f..3bfc446638 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -301,8 +301,6 @@ fs_dev_remove(struct sub_device *sdev)
 		if (ret < 0) {
 			ERROR("Bus detach failed for sub_device %u",
 			      SUB_ID(sdev));
-		} else {
-			rte_eth_dev_release_port(ETH(sdev));
 		}
 		sdev->state = DEV_PARSED;
 		/* fallthrough */
-- 
2.38.1

[PATCH v4 4/5] failsafe: use public APIs in fs_link_update

[Luc Pelletier]

The link_update function pointer is called directly on a sub-device, via
its rte_eth_dev structure. In addition, it assumes that if link_update
returns a value other than 0 or -1, an error occurred. That's not
accurate. For example, the mlx5 PMD returns a positive value to indicate
that the link status was updated. This results in fs_link_update failing
when it really should have succeeded.

The code now uses the public APIs rte_eth_link_get and
rte_eth_link_get_nowait to query the link status of each sub-device. It
also uses rte_eth_linkstatus_set to set the link status of the failsafe
device.

Fixes: ae80146c5a1b ("net/failsafe: fix removed device handling")
Cc: Matan Azrad <matan@nvidia.com>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ops.c | 37 +++++++++++++----------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index d357e1bc83..b66dfa269b 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -820,34 +820,31 @@ fs_link_update(struct rte_eth_dev *dev,
 {
 	struct sub_device *sdev;
 	uint8_t i;
-	int ret;
+	int ret = -1;
+	int sdev_link_ret;
+	struct rte_eth_link link_info;
 
 	fs_lock(dev, 0);
 	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
-		DEBUG("Calling link_update on sub_device %d", i);
-		ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete);
-		if (ret && ret != -1 && sdev->remove == 0 &&
-		    rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
-			ERROR("Link update failed for sub_device %d with error %d",
-			      i, ret);
-			fs_unlock(dev, 0);
-			return ret;
+		DEBUG("Calling rte_eth_link_get on sub_device %d", i);
+		if (wait_to_complete) {
+			sdev_link_ret = rte_eth_link_get(PORT_ID(sdev), &link_info);
+		} else {
+			sdev_link_ret = rte_eth_link_get_nowait(PORT_ID(sdev),
+				&link_info);
 		}
-	}
-	if (TX_SUBDEV(dev)) {
-		struct rte_eth_link *l1;
-		struct rte_eth_link *l2;
 
-		l1 = &dev->data->dev_link;
-		l2 = &ETH(TX_SUBDEV(dev))->data->dev_link;
-		if (memcmp(l1, l2, sizeof(*l1))) {
-			*l1 = *l2;
-			fs_unlock(dev, 0);
-			return 0;
+		if (likely(sdev_link_ret == 0)) {
+			if (TX_SUBDEV(dev) == sdev)
+				ret = rte_eth_linkstatus_set(dev, &link_info);
+		} else if (sdev->remove == 0 &&
+				rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
+			ERROR("Link get failed for sub_device %d with error %d",
+				i, sdev_link_ret);
 		}
 	}
 	fs_unlock(dev, 0);
-	return -1;
+	return ret;
 }
 
 static int
-- 
2.38.1

[PATCH v4 5/5] failsafe: make sub-device remove flag thread-safe

[Luc Pelletier]

For each sub-device, there's a remove flag that's used to indicate if
the sub-device has been removed or not. This flag is currently a
volatile bit field. Even if it's marked as volatile, a bit field is not
thread-safe according to the C standard.

The remove flag is now an unsigned int rather than a bit field. We also
now use the atomic built-ins to read/write the value of the remove flag
to ensure thread-safety.

Fixes: 598fb8aec6f6 ("net/failsafe: support device removal")
Cc: Gaetan Rivet <grive@u256.net>
Cc: stable@dpdk.org

Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>
---
 drivers/net/failsafe/failsafe_ether.c   | 8 ++++----
 drivers/net/failsafe/failsafe_ops.c     | 2 +-
 drivers/net/failsafe/failsafe_private.h | 4 ++--
 drivers/net/failsafe/failsafe_rxtx.c    | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
index 3bfc446638..9014777e52 100644
--- a/drivers/net/failsafe/failsafe_ether.c
+++ b/drivers/net/failsafe/failsafe_ether.c
@@ -311,7 +311,7 @@ fs_dev_remove(struct sub_device *sdev)
 		/* the end */
 		break;
 	}
-	sdev->remove = 0;
+	__atomic_store_n(&sdev->remove, 0, __ATOMIC_RELEASE);
 	failsafe_hotplug_alarm_install(fs_dev(sdev));
 }
 
@@ -388,7 +388,7 @@ failsafe_dev_remove(struct rte_eth_dev *dev)
 	uint8_t i;
 
 	FOREACH_SUBDEV(sdev, i, dev) {
-		if (!sdev->remove)
+		if (!__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE))
 			continue;
 
 		/* Active devices must have finished their burst and
@@ -556,7 +556,7 @@ failsafe_eth_dev_state_sync(struct rte_eth_dev *dev)
 err_remove:
 	FOREACH_SUBDEV(sdev, i, dev)
 		if (sdev->state != PRIV(dev)->state)
-			sdev->remove = 1;
+			__atomic_store_n(&sdev->remove, 1, __ATOMIC_RELEASE);
 	return ret;
 }
 
@@ -597,7 +597,7 @@ failsafe_eth_rmv_event_callback(uint16_t port_id __rte_unused,
 	 * Async removal, the sub-PMD will try to unregister
 	 * the callback at the source of the current thread context.
 	 */
-	sdev->remove = 1;
+	__atomic_store_n(&sdev->remove, 1, __ATOMIC_RELEASE);
 	fs_unlock(fs_dev(sdev), 0);
 	return 0;
 }
diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index b66dfa269b..e65c8a261a 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -837,7 +837,7 @@ fs_link_update(struct rte_eth_dev *dev,
 		if (likely(sdev_link_ret == 0)) {
 			if (TX_SUBDEV(dev) == sdev)
 				ret = rte_eth_linkstatus_set(dev, &link_info);
-		} else if (sdev->remove == 0 &&
+		} else if (__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) == 0 &&
 				rte_eth_dev_is_removed(PORT_ID(sdev)) == 0) {
 			ERROR("Link get failed for sub_device %d with error %d",
 				i, sdev_link_ret);
diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
index 3865f2fc34..0577a62ab2 100644
--- a/drivers/net/failsafe/failsafe_private.h
+++ b/drivers/net/failsafe/failsafe_private.h
@@ -131,7 +131,7 @@ struct sub_device {
 	/* sub device port id*/
 	uint16_t sdev_port_id; /* shared between processes */
 	/* flag calling for recollection */
-	volatile unsigned int remove:1;
+	unsigned int remove;
 	/* flow isolation state */
 	int flow_isolated:1;
 	/* RMV callback registration state */
@@ -490,7 +490,7 @@ static inline int
 fs_err(struct sub_device *sdev, int err)
 {
 	/* A device removal shouldn't be reported as an error. */
-	if (sdev->remove == 1 || err == -EIO)
+	if (__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) == 1 || err == -EIO)
 		return rte_errno = 0;
 	return err;
 }
diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 6d8ab0a6e7..c5caab4204 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -16,7 +16,7 @@ fs_rx_unsafe(struct sub_device *sdev)
 	return (ETH(sdev) == NULL) ||
 		(ETH(sdev)->rx_pkt_burst == NULL) ||
 		(sdev->state != DEV_STARTED) ||
-		(sdev->remove != 0);
+		(__atomic_load_n(&sdev->remove, __ATOMIC_ACQUIRE) != 0);
 }
 
 static inline int
-- 
2.38.1

Re: [PATCH v3 1/5] failsafe: fix segfault on hotplug event

[Stephen Hemminger]

On Tue, 29 Nov 2022 09:48:29 -0500
Luc Pelletier <lucp.at.work@gmail.com> wrote:

> When the failsafe PMD encounters a hotplug event, it switches its rx/tx
> functions to "safe" ones that validate the sub-device's rx/tx functions
> before calling them. It switches the rx/tx functions by changing the
> function pointers in the rte_eth_dev structure.
> 
> Following commit 7a0935239b9e, the rx/tx functions of PMDs are no longer
> called through the function pointers in the rte_eth_dev structure. They
> are rather called through a flat array named rte_eth_fp_ops. The
> function pointers in that array are initialized when the devices start
> and are initialized.
> 
> When a hotplug event occurs, the function pointers in rte_eth_fp_ops
> still point to the "unsafe" rx/tx functions in the failsafe PMD since
> they haven't been updated. This results in a segmentation fault because
> it ends up using the "unsafe" functions, when the "safe" functions
> should have been used.
> 
> To fix the problem, the "unsafe" rx/tx functions were completely
> removed. The "safe" functions are now always used. Modifying the rx/tx
> functions on-the-fly is not supported by DPDK, so this is the correct
> approach and should have very minimal impact on performance.
> 
> Fixes: 7a0935239b9e ("ethdev: make fast-path functions to use new flat array")
> Cc: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Luc Pelletier <lucp.at.work@gmail.com>


I don't use or have way to test failsafe anymore.
But wanted to give a try at reviewing this.
This version does not apply to current code base.
If still interested, please rebase and resubmit.