[dpdk-dev] [PATCH v5 3/3] net/failsafe: fix calling device during RMV events

Gaëtan Rivet gaetan.rivet at 6wind.com
Thu Feb 8 19:11:03 CET 2018


On Thu, Feb 08, 2018 at 04:34:13PM +0000, Matan Azrad wrote:
> Following are the failure steps:
> 1. The physical device is removed due to change in one of PF parameters
> (e.g. MTU) 2. The interrupt thread flags the device 3. Within 2 seconds
> Interrupt thread initializes the actual device removal, then every 2
> seconds it tries to re-sync (plug in) the device. The trials fail as
> long as VF parameter mismatches the PF parameter.
> 4. A control thread initiates a control operation on failsafe which
> initiates this operation on the device.
> 5. A race condition occurs between the control thread and interrupt
> thread when accessing the device data structures.
> 
> This patch mitigates the race condition in step 5.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Matan Azrad <matan at mellanox.com>
> ---
>  drivers/net/failsafe/failsafe.c         |  2 +-
>  drivers/net/failsafe/failsafe_eal.c     |  2 +-
>  drivers/net/failsafe/failsafe_ether.c   |  2 +-
>  drivers/net/failsafe/failsafe_ops.c     | 26 +++++++++++++++++--------
>  drivers/net/failsafe/failsafe_private.h | 34 ++++++++++++++++++++++++++-------
>  5 files changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 7b2cdbb..6cdefd0 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -187,7 +187,7 @@
>  		 * If MAC address was provided as a parameter,
>  		 * apply to all probed slaves.
>  		 */
> -		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> +		FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {

No need for the UNSAFE here. The ports should have been just
initialized, and sdev->remove should be 0.

If sdev->remove is 1, then it means it has been set already by a plugout
event, meaning that rte_eth_dev_default_mac_addr_set should not even be
called on it.

>  			ret = rte_eth_dev_default_mac_addr_set(PORT_ID(sdev),
>  							       mac);
>  			if (ret) {
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index c3d6731..b3b9c32 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -126,7 +126,7 @@
>  	int sdev_ret;
>  	int ret = 0;
>  
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {
>  		sdev_ret = rte_eal_hotplug_remove(sdev->bus->name,
>  							sdev->dev->name);
>  		if (sdev_ret) {
> diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c
> index ca42376..f2a52c9 100644
> --- a/drivers/net/failsafe/failsafe_ether.c
> +++ b/drivers/net/failsafe/failsafe_ether.c
> @@ -325,7 +325,7 @@
>  	struct sub_device *sdev;
>  	uint8_t i;
>  
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)
>  		if (sdev->remove && fs_rxtx_clean(sdev)) {
>  			fs_dev_stats_save(sdev);
>  			fs_dev_remove(sdev);
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index a7c2dba..3d2cb32 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -181,6 +181,9 @@
>  	FOREACH_SUBDEV(sdev, i, dev) {
>  		if (sdev->state != DEV_ACTIVE)
>  			continue;
> +		if (sdev->remove == 1 && PRIV(dev)->state < DEV_STARTED)
> +			/* Application shouldn't start removed sub-devices. */
> +			continue;

FOREACH_SUBDEV should already have avoided sub-devices which remove flag
is 1.

If not, then the fs_err(sdev, ret) stanza right after will let the loop
continue, and the port should be handled by the next slave cleanup.

>  		DEBUG("Starting sub_device %d", i);
>  		ret = rte_eth_dev_start(PORT_ID(sdev));
>  		if (ret) {
> @@ -265,10 +268,17 @@
>  	uint8_t i;
>  
>  	failsafe_hotplug_alarm_cancel(dev);
> -	if (PRIV(dev)->state == DEV_STARTED)
> +	if (PRIV(dev)->state == DEV_STARTED) {
> +		/*
> +		 * Clean remove flags to allow stop for all sub-devices because
> +		 * there is not hot-plug alarm to stop the removed sub-devices.
> +		 */
> +		FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_STARTED)
> +			sdev->remove = 0;

Why make this conditional to state == DEV_STARTED?

>  		dev->dev_ops->dev_stop(dev);
> +	}
>  	PRIV(dev)->state = DEV_ACTIVE - 1;
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {
>  		DEBUG("Closing sub_device %d", i);
>  		rte_eth_dev_close(PORT_ID(sdev));
>  		sdev->state = DEV_ACTIVE - 1;

-->

	/*
	 * Clean remove flags to allow stop for all sub-devices because
	 * there is no hot-plug alarm to clean the removed sub-devices.
	 */
	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)
		sdev->remove = 0;
	if (PRIV(dev)->state == DEV_STARTED)
		dev->dev_ops->dev_stop(dev);
	PRIV(dev)->state = DEV_ACTIVE - 1;
	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
		DEBUG("Closing sub_device %d", i);
		rte_eth_dev_close(PORT_ID(sdev));
		sdev->state = DEV_ACTIVE - 1;

> @@ -309,7 +319,7 @@
>  	if (rxq->event_fd > 0)
>  		close(rxq->event_fd);
>  	dev = rxq->priv->dev;
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)

No need here, as you would have reset sdev->remove if the port was
closing, or it would be dealt with by fs_dev_remove if the alarm is
still running.

>  		SUBOPS(sdev, rx_queue_release)
>  			(ETH(sdev)->data->rx_queues[rxq->qid]);
>  	dev->data->rx_queues[rxq->qid] = NULL;
> @@ -376,7 +386,7 @@

you really should update your git, it is difficult to verify these
changes without the function contexts.

>  		return ret;
>  	rxq->event_fd = intr_handle.efds[0];
>  	dev->data->rx_queues[rx_queue_id] = rxq;
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {

Why should we setup queues on ports marked for removal?

>  		ret = rte_eth_rx_queue_setup(PORT_ID(sdev),
>  				rx_queue_id,
>  				nb_rx_desc, socket_id,
> @@ -493,7 +503,7 @@
>  		return;
>  	txq = queue;
>  	dev = txq->priv->dev;
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE)
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE)

Same as for rx_queue_release: either the device is closing, and the flag
has been reset, or the alarm is still running and will take care of
this.

>  		SUBOPS(sdev, tx_queue_release)
>  			(ETH(sdev)->data->tx_queues[txq->qid]);

Actually, now that I think about it, there seems to be an issue with
queues not released on plugout?

In fs_dev_remove, we only do the general dev_stop and dev_close on the
sub_device.

shouldn't we call tx_queue_release as well before?

>  	dev->data->tx_queues[txq->qid] = NULL;
> @@ -548,7 +558,7 @@
>  	txq->info.nb_desc = nb_tx_desc;
>  	txq->priv = PRIV(dev);
>  	dev->data->tx_queues[tx_queue_id] = txq;
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {

Why using the UNSAFE operator for a setup operation? (Same as for
rx_queue_setup.)

>  		ret = rte_eth_tx_queue_setup(PORT_ID(sdev),
>  				tx_queue_id,
>  				nb_tx_desc, socket_id,
> @@ -663,7 +673,7 @@
>  	int ret;
>  
>  	rte_memcpy(stats, &PRIV(dev)->stats_accumulator, sizeof(*stats));
> -	FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) {
> +	FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_ACTIVE) {

Why do you want to attempt a stat read on port bound for removal?

>  		struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats;
>  		uint64_t *timestamp = &sdev->stats_snapshot.timestamp;
>  
> @@ -746,7 +756,7 @@
>  
>  		rx_offload_capa = default_infos.rx_offload_capa;
>  		rxq_offload_capa = default_infos.rx_queue_offload_capa;
> -		FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_PROBED) {
> +		FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, DEV_PROBED) {

same here.

>  			rte_eth_dev_info_get(PORT_ID(sdev),
>  					&PRIV(dev)->infos);
>  			rx_offload_capa &= PRIV(dev)->infos.rx_offload_capa;
> diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h
> index f3be152..7ddd63a 100644
> --- a/drivers/net/failsafe/failsafe_private.h
> +++ b/drivers/net/failsafe/failsafe_private.h
> @@ -244,16 +244,31 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
>  	((sdev)->sid)
>  
>  /**
> - * Stateful iterator construct over fail-safe sub-devices:
> + * Stateful iterator construct over fail-safe sub-devices,
> + * including the removed sub-devices:

"including sub-devices marked for removal" is more correct here, as the
device is not actually removed yet, only scheduled for.

> + * s:     (struct sub_device *), iterator
> + * i:     (uint8_t), increment
> + * dev:   (struct rte_eth_dev *), fail-safe ethdev
> + * state: (enum dev_state), minimum acceptable device state
> + */
> +

Here the same documentation as for other macros: parameters type, quick
description of what it does.

> +#define FOREACH_SUBDEV_STATE_UNSAFE(s, i, dev, state)	\
> +	for (s = fs_find_next((dev), 0, state, 0, &i);	\
> +	     s != NULL;					\
> +	     s = fs_find_next((dev), i + 1, state, 0, &i))
> +
> +/**
> + * Stateful iterator construct over fail-safe sub-devices,
> + * except the removed sub-devices:
>   * s:     (struct sub_device *), iterator
>   * i:     (uint8_t), increment
>   * dev:   (struct rte_eth_dev *), fail-safe ethdev
>   * state: (enum dev_state), minimum acceptable device state
>   */
>  #define FOREACH_SUBDEV_STATE(s, i, dev, state)		\
> -	for (s = fs_find_next((dev), 0, state, &i);	\
> +	for (s = fs_find_next((dev), 0, state, 1, &i);	\
>  	     s != NULL;					\
> -	     s = fs_find_next((dev), i + 1, state, &i))
> +	     s = fs_find_next((dev), i + 1, state, 1, &i))
>  
>  /**
>   * Iterator construct over fail-safe sub-devices:
> @@ -262,7 +277,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
>   * dev: (struct rte_eth_dev *), fail-safe ethdev
>   */
>  #define FOREACH_SUBDEV(s, i, dev)			\
> -	FOREACH_SUBDEV_STATE(s, i, dev, DEV_UNDEFINED)
> +	FOREACH_SUBDEV_STATE_UNSAFE(s, i, dev, DEV_UNDEFINED)

No actually, the default case should be using the "SAFE" iterator, so no
change needed here.

>  
>  /* dev: (struct rte_eth_dev *) fail-safe device */
>  #define PREFERRED_SUBDEV(dev) \
> @@ -328,6 +343,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
>  fs_find_next(struct rte_eth_dev *dev,
>  	     uint8_t sid,
>  	     enum dev_state min_state,
> +	     uint8_t check_remove,

skip_remove? Seems more descriptive.

>  	     uint8_t *sid_out)
>  {
>  	struct sub_device *subs;
> @@ -336,8 +352,12 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
>  	subs = PRIV(dev)->subs;
>  	tail = PRIV(dev)->subs_tail;
>  	while (sid < tail) {
> -		if (subs[sid].state >= min_state)
> -			break;
> +		if (subs[sid].state >= min_state) {
> +			if (check_remove == 0)
> +				break;
> +			if (PRIV(dev)->subs[sid].remove == 0)
> +				break;
> +		}
>  		sid++;
>  	}
>  	*sid_out = sid;
> @@ -376,7 +396,7 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id,
>  		uint8_t i;
>  
>  		/* Using acceptable device */
> -		FOREACH_SUBDEV_STATE(sdev, i, dev, req_state) {
> +		FOREACH_SUBDEV_STATE_UNSAFE(sdev, i, dev, req_state) {

Why should we switch emitting device to one marked for removal?

>  			if (sdev == banned)
>  				continue;
>  			DEBUG("Switching tx_dev to sub_device %d",
> -- 
> 1.8.3.1
> 

Regards,
-- 
Gaëtan Rivet
6WIND


More information about the dev mailing list