[dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..e78624127 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * slaves. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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->dev->data->port_id);
 	return nb_rx;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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->dev->data->port_id);
 	return nb_rx;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..e78624127 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * slaves. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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->dev->data->port_id);
 	return nb_rx;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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->dev->data->port_id);
 	return nb_rx;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[David Marchand]

Hey Adrien,

On Thu, Apr 18, 2019 at 3:12 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
wrote:

> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
>
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
>
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
>

Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c
> b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int
> force_safe)
>         rte_wmb();
>  }
>
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
> uint16_t port)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i != nb_pkts; ++i)
> +               rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>                   struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>                 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->dev->data->port_id);
>         return nb_rx;
>  }
>
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>                 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->dev->data->port_id);
>         return nb_rx;
>  }
>
> --
> 2.11.0
>

Not a big fan of those duplicated rx_burst functions...
Reviewed-by: David Marchand <david.marchand@redhat.com>

I suppose the bonding pmd has the same issue.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[David Marchand]

Hey Adrien,

On Thu, Apr 18, 2019 at 3:12 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
wrote:

> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
>
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
>
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
>

Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c
> b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int
> force_safe)
>         rte_wmb();
>  }
>
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts,
> uint16_t port)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i != nb_pkts; ++i)
> +               rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>                   struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>                 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->dev->data->port_id);
>         return nb_rx;
>  }
>
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>                 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->dev->data->port_id);
>         return nb_rx;
>  }
>
> --
> 2.11.0
>

Not a big fan of those duplicated rx_burst functions...
Reviewed-by: David Marchand <david.marchand@redhat.com>

I suppose the bonding pmd has the same issue.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Gaëtan Rivet]

On Thu, Apr 18, 2019 at 03:11:26PM +0200, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Missing a Cc: stable@dpdk.org no? Given that no one saw that until a
slightly heavier application was used with the fail-safe, I guess few
people will care, but still.

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  	rte_wmb();
>  }
>  
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> -- 
> 2.11.0

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Gaëtan Rivet]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 2276 bytes --]

On Thu, Apr 18, 2019 at 03:11:26PM +0200, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Missing a Cc: stable@dpdk.org no? Given that no one saw that until a
slightly heavier application was used with the fail-safe, I guess few
people will care, but still.

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  	rte_wmb();
>  }
>  
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> -- 
> 2.11.0

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Ferruh Yigit]

On 4/18/2019 2:11 PM, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  	rte_wmb();
>  }
>  
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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->dev->data->port_id);

error: struct "fs_priv" has no field "dev"

I guess intention is: "rxq->priv->data->port_id"

>  	return nb_rx;
>  }
>  
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> 

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Ferruh Yigit]

On 4/18/2019 2:11 PM, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
>  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> index 231c83291..e78624127 100644
> --- a/drivers/net/failsafe/failsafe_rxtx.c
> +++ b/drivers/net/failsafe/failsafe_rxtx.c
> @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
>  	rte_wmb();
>  }
>  
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.
> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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->dev->data->port_id);

error: struct "fs_priv" has no field "dev"

I guess intention is: "rxq->priv->data->port_id"

>  	return nb_rx;
>  }
>  
> @@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
>  		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->dev->data->port_id);
>  	return nb_rx;
>  }
>  
> 

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 03:42:24PM +0100, Ferruh Yigit wrote:
> On 4/18/2019 2:11 PM, Adrien Mazarguil wrote:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> > 
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> > index 231c83291..e78624127 100644
> > --- a/drivers/net/failsafe/failsafe_rxtx.c
> > +++ b/drivers/net/failsafe/failsafe_rxtx.c
> > @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
> >  	rte_wmb();
> >  }
> >  
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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->dev->data->port_id);
> 
> error: struct "fs_priv" has no field "dev"
> 
> I guess intention is: "rxq->priv->data->port_id"

Indeed, I validated this patch as is against v18.11 and overlooked a
compilation check against master. I'll send an updated version with
Cc stable and all, thanks!

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 03:42:24PM +0100, Ferruh Yigit wrote:
> On 4/18/2019 2:11 PM, Adrien Mazarguil wrote:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> > 
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
> > index 231c83291..e78624127 100644
> > --- a/drivers/net/failsafe/failsafe_rxtx.c
> > +++ b/drivers/net/failsafe/failsafe_rxtx.c
> > @@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
> >  	rte_wmb();
> >  }
> >  
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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->dev->data->port_id);
> 
> error: struct "fs_priv" has no field "dev"
> 
> I guess intention is: "rxq->priv->data->port_id"

Indeed, I validated this patch as is against v18.11 and overlooked a
compilation check against master. I'll send an updated version with
Cc stable and all, thanks!

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
--
v2 changes:

Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
"rxq->priv->data->port_id" (since v19.05) and checked compilation against
master this time.

Given the limited scope of that change, reviewed-by/acked-by lines were
kept.
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..b9cddec78 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * slaves. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
--
v2 changes:

Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
"rxq->priv->data->port_id" (since v19.05) and checked compilation against
master this time.

Given the limited scope of that change, reviewed-by/acked-by lines were
kept.
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..b9cddec78 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * slaves. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 17:32, Adrien Mazarguil:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> --
> v2 changes:
> 
> Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
> "rxq->priv->data->port_id" (since v19.05) and checked compilation against
> master this time.
> 
> Given the limited scope of that change, reviewed-by/acked-by lines were
> kept.
> ---
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.

"slave" is a wording from bonding.
In failsafe, it is sub-device, isn't it?

> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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;
>  }

I'm afraid the performance drop to be hard.
How the port id in mbuf is used exactly? What crash are you seeing?

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 17:32, Adrien Mazarguil:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> --
> v2 changes:
> 
> Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
> "rxq->priv->data->port_id" (since v19.05) and checked compilation against
> master this time.
> 
> Given the limited scope of that change, reviewed-by/acked-by lines were
> kept.
> ---
> +/*
> + * Override source port in Rx packets.
> + *
> + * Make Rx packets originate from this PMD instance instead of one of its
> + * slaves. This is mandatory to avoid breaking applications.

"slave" is a wording from bonding.
In failsafe, it is sub-device, isn't it?

> + */
> +static void
> +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i != nb_pkts; ++i)
> +		rx_pkts[i]->port = port;
> +}
> +
>  uint16_t
>  failsafe_rx_burst(void *queue,
>  		  struct rte_mbuf **rx_pkts,
> @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
>  		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;
>  }

I'm afraid the performance drop to be hard.
How the port id in mbuf is used exactly? What crash are you seeing?

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 17:39, Thomas Monjalon:
> 18/04/2019 17:32, Adrien Mazarguil:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
[...]
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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;
> >  }
> 
> I'm afraid the performance drop to be hard.
> How the port id in mbuf is used exactly? What crash are you seeing?

Another way to fix it without performance drop would be to add
a new driver op to set the top-level port id.
This top-level id would be stored in the private structure of the port,
initialized with the port id of the port itself, and used to fill mbufs.

Thoughts?

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 17:39, Thomas Monjalon:
> 18/04/2019 17:32, Adrien Mazarguil:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
[...]
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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;
> >  }
> 
> I'm afraid the performance drop to be hard.
> How the port id in mbuf is used exactly? What crash are you seeing?

Another way to fix it without performance drop would be to add
a new driver op to set the top-level port id.
This top-level id would be stored in the private structure of the port,
initialized with the port id of the port itself, and used to fill mbufs.

Thoughts?

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Gaëtan Rivet]

On Thu, Apr 18, 2019 at 05:39:37PM +0200, Thomas Monjalon wrote:
> 18/04/2019 17:32, Adrien Mazarguil:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> > 
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > --
> > v2 changes:
> > 
> > Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
> > "rxq->priv->data->port_id" (since v19.05) and checked compilation against
> > master this time.
> > 
> > Given the limited scope of that change, reviewed-by/acked-by lines were
> > kept.
> > ---
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> 
> "slave" is a wording from bonding.
> In failsafe, it is sub-device, isn't it?
> 

Yes, there is however one other comment in failsafe code refering to a
sub-device as a slave.

I'm not really up-to-par with the LSF CoC[1] and whether it is aligned
with the Contributor Covenant also adopted by Linux[2]. I guess you were
only referring to using the proper nomenclature and not this subject,
but I can't pass on an opportunity to out-nitpick :D .

This can be changed on merge as sub-device is more correct. Overall personally
I don't really care either way.

[1]: https://www.linuxfoundation.org/code-of-conduct/
[2]: https://www.kernel.org/doc/html/latest/process/code-of-conduct.html

> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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;
> >  }
> 
> I'm afraid the performance drop to be hard.
> How the port id in mbuf is used exactly? What crash are you seeing?
> 
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Gaëtan Rivet]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 2674 bytes --]

On Thu, Apr 18, 2019 at 05:39:37PM +0200, Thomas Monjalon wrote:
> 18/04/2019 17:32, Adrien Mazarguil:
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> > 
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> > 
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > --
> > v2 changes:
> > 
> > Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
> > "rxq->priv->data->port_id" (since v19.05) and checked compilation against
> > master this time.
> > 
> > Given the limited scope of that change, reviewed-by/acked-by lines were
> > kept.
> > ---
> > +/*
> > + * Override source port in Rx packets.
> > + *
> > + * Make Rx packets originate from this PMD instance instead of one of its
> > + * slaves. This is mandatory to avoid breaking applications.
> 
> "slave" is a wording from bonding.
> In failsafe, it is sub-device, isn't it?
> 

Yes, there is however one other comment in failsafe code refering to a
sub-device as a slave.

I'm not really up-to-par with the LSF CoC[1] and whether it is aligned
with the Contributor Covenant also adopted by Linux[2]. I guess you were
only referring to using the proper nomenclature and not this subject,
but I can't pass on an opportunity to out-nitpick :D .

This can be changed on merge as sub-device is more correct. Overall personally
I don't really care either way.

[1]: https://www.linuxfoundation.org/code-of-conduct/
[2]: https://www.kernel.org/doc/html/latest/process/code-of-conduct.html

> > + */
> > +static void
> > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i != nb_pkts; ++i)
> > +		rx_pkts[i]->port = port;
> > +}
> > +
> >  uint16_t
> >  failsafe_rx_burst(void *queue,
> >  		  struct rte_mbuf **rx_pkts,
> > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> >  		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;
> >  }
> 
> I'm afraid the performance drop to be hard.
> How the port id in mbuf is used exactly? What crash are you seeing?
> 
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 05:51:18PM +0200, Thomas Monjalon wrote:
> 18/04/2019 17:39, Thomas Monjalon:
> > 18/04/2019 17:32, Adrien Mazarguil:
> > > When passed to the application, Rx packets retain the port ID value
> > > originally set by slave devices. Unfortunately these IDs have no meaning to
> > > applications, which are typically unaware of their existence.
> > > 
> > > This confuses those caring about the source port field in mbufs (m->port)
> > > which experience issues ranging from traffic drop to crashes.
> [...]
> > > +/*
> > > + * Override source port in Rx packets.
> > > + *
> > > + * Make Rx packets originate from this PMD instance instead of one of its
> > > + * slaves. This is mandatory to avoid breaking applications.
> > > + */
<snip>
> > "slave" is a wording from bonding.
> > In failsafe, it is sub-device, isn't it?

I don't mind, although grep shows a couple of comments talking about slaves
already. Either way I think it fits as those are failsafe's pets, as in
failsafe does whatever it wants to them and they don't have a say :)

Does it warrant a v3?

> > > +static void
> > > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i != nb_pkts; ++i)
> > > +		rx_pkts[i]->port = port;
> > > +}
> > > +
> > >  uint16_t
> > >  failsafe_rx_burst(void *queue,
> > >  		  struct rte_mbuf **rx_pkts,
> > > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> > >  		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;
> > >  }
> > 
> > I'm afraid the performance drop to be hard.

Mbufs are still hot from the oven at this stage, so it's not *that*
expensive. I don't see a more efficient approach.

> > How the port id in mbuf is used exactly?

Applications that dissociate Rx itself from packet processing, or whenever a
networking stack is involved. Basically every time some code wonders where a
packet comes from due to lack of context and looks at m->port for the
answer (e.g. checking that a packet arrives on the right port given its
destination address).

> > What crash are you seeing?

None, thankfully. In my specific use case, 6WINDGate's stack simply drops
traffic coming from unknown ports.

However nothing prevents applications from using m->port as an index of some
array they allocated to quickly retrieve port context without looking it
up. They wouldn't expect indices they do not know about in there; assuming
it will result in a crash is not far fetched.

> Another way to fix it without performance drop would be to add
> a new driver op to set the top-level port id.
> This top-level id would be stored in the private structure of the port,
> initialized with the port id of the port itself, and used to fill mbufs.
> 
> Thoughts?

Adding a new devop as a fix would be a problem for stable releases, so this
patch is definitely needed, at least as a first step.

I'm not against a new API, however would it be worth the trouble? Especially
considering it would only be used by failsafe-like drivers with something to
hide from applications which is not the main use case.

For some PMDs, this operation could only be done at init time before port ID
is stored in private Rx queue data for fast retrieval. Retrieving it through
a pointer so it can be updated anytime would make it more expensive than
necessary for them.

It's understood that having failsafe in the dataplane has a cost, but even
with the proposed fix, that cost is dwarfed by the amount of work done by a
true PMD (and the application) for Rx processing.

My suggestion is to wait for someone to complain about the performance
compared to what they had before that fix, only then see what we can do.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 05:51:18PM +0200, Thomas Monjalon wrote:
> 18/04/2019 17:39, Thomas Monjalon:
> > 18/04/2019 17:32, Adrien Mazarguil:
> > > When passed to the application, Rx packets retain the port ID value
> > > originally set by slave devices. Unfortunately these IDs have no meaning to
> > > applications, which are typically unaware of their existence.
> > > 
> > > This confuses those caring about the source port field in mbufs (m->port)
> > > which experience issues ranging from traffic drop to crashes.
> [...]
> > > +/*
> > > + * Override source port in Rx packets.
> > > + *
> > > + * Make Rx packets originate from this PMD instance instead of one of its
> > > + * slaves. This is mandatory to avoid breaking applications.
> > > + */
<snip>
> > "slave" is a wording from bonding.
> > In failsafe, it is sub-device, isn't it?

I don't mind, although grep shows a couple of comments talking about slaves
already. Either way I think it fits as those are failsafe's pets, as in
failsafe does whatever it wants to them and they don't have a say :)

Does it warrant a v3?

> > > +static void
> > > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; i != nb_pkts; ++i)
> > > +		rx_pkts[i]->port = port;
> > > +}
> > > +
> > >  uint16_t
> > >  failsafe_rx_burst(void *queue,
> > >  		  struct rte_mbuf **rx_pkts,
> > > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> > >  		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;
> > >  }
> > 
> > I'm afraid the performance drop to be hard.

Mbufs are still hot from the oven at this stage, so it's not *that*
expensive. I don't see a more efficient approach.

> > How the port id in mbuf is used exactly?

Applications that dissociate Rx itself from packet processing, or whenever a
networking stack is involved. Basically every time some code wonders where a
packet comes from due to lack of context and looks at m->port for the
answer (e.g. checking that a packet arrives on the right port given its
destination address).

> > What crash are you seeing?

None, thankfully. In my specific use case, 6WINDGate's stack simply drops
traffic coming from unknown ports.

However nothing prevents applications from using m->port as an index of some
array they allocated to quickly retrieve port context without looking it
up. They wouldn't expect indices they do not know about in there; assuming
it will result in a crash is not far fetched.

> Another way to fix it without performance drop would be to add
> a new driver op to set the top-level port id.
> This top-level id would be stored in the private structure of the port,
> initialized with the port id of the port itself, and used to fill mbufs.
> 
> Thoughts?

Adding a new devop as a fix would be a problem for stable releases, so this
patch is definitely needed, at least as a first step.

I'm not against a new API, however would it be worth the trouble? Especially
considering it would only be used by failsafe-like drivers with something to
hide from applications which is not the main use case.

For some PMDs, this operation could only be done at init time before port ID
is stored in private Rx queue data for fast retrieval. Retrieving it through
a pointer so it can be updated anytime would make it more expensive than
necessary for them.

It's understood that having failsafe in the dataplane has a cost, but even
with the proposed fix, that cost is dwarfed by the amount of work done by a
true PMD (and the application) for Rx processing.

My suggestion is to wait for someone to complain about the performance
compared to what they had before that fix, only then see what we can do.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 18:46, Adrien Mazarguil:
> On Thu, Apr 18, 2019 at 05:51:18PM +0200, Thomas Monjalon wrote:
> > 18/04/2019 17:39, Thomas Monjalon:
> > > 18/04/2019 17:32, Adrien Mazarguil:
> > > > When passed to the application, Rx packets retain the port ID value
> > > > originally set by slave devices. Unfortunately these IDs have no meaning to
> > > > applications, which are typically unaware of their existence.
> > > > 
> > > > This confuses those caring about the source port field in mbufs (m->port)
> > > > which experience issues ranging from traffic drop to crashes.
> > [...]
> > > > +/*
> > > > + * Override source port in Rx packets.
> > > > + *
> > > > + * Make Rx packets originate from this PMD instance instead of one of its
> > > > + * slaves. This is mandatory to avoid breaking applications.
> > > > + */
> <snip>
> > > "slave" is a wording from bonding.
> > > In failsafe, it is sub-device, isn't it?
> 
> I don't mind, although grep shows a couple of comments talking about slaves
> already. Either way I think it fits as those are failsafe's pets, as in
> failsafe does whatever it wants to them and they don't have a say :)
> 
> Does it warrant a v3?

Yes please, except if Ferruh is already doing the change on apply.

> > > > +static void
> > > > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i != nb_pkts; ++i)
> > > > +		rx_pkts[i]->port = port;
> > > > +}
> > > > +
> > > >  uint16_t
> > > >  failsafe_rx_burst(void *queue,
> > > >  		  struct rte_mbuf **rx_pkts,
> > > > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> > > >  		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;
> > > >  }
> > > 
> > > I'm afraid the performance drop to be hard.
> 
> Mbufs are still hot from the oven at this stage, so it's not *that*
> expensive. I don't see a more efficient approach.

Yes, Ali did some quick tests showing no perf drop.

> > > How the port id in mbuf is used exactly?
> 
> Applications that dissociate Rx itself from packet processing, or whenever a
> networking stack is involved. Basically every time some code wonders where a
> packet comes from due to lack of context and looks at m->port for the
> answer (e.g. checking that a packet arrives on the right port given its
> destination address).
> 
> > > What crash are you seeing?
> 
> None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> traffic coming from unknown ports.
> 
> However nothing prevents applications from using m->port as an index of some
> array they allocated to quickly retrieve port context without looking it
> up. They wouldn't expect indices they do not know about in there; assuming
> it will result in a crash is not far fetched.
> 
> > Another way to fix it without performance drop would be to add
> > a new driver op to set the top-level port id.
> > This top-level id would be stored in the private structure of the port,
> > initialized with the port id of the port itself, and used to fill mbufs.
> > 
> > Thoughts?
> 
> Adding a new devop as a fix would be a problem for stable releases, so this
> patch is definitely needed, at least as a first step.
> 
> I'm not against a new API, however would it be worth the trouble? Especially
> considering it would only be used by failsafe-like drivers with something to
> hide from applications which is not the main use case.
> 
> For some PMDs, this operation could only be done at init time before port ID
> is stored in private Rx queue data for fast retrieval. Retrieving it through
> a pointer so it can be updated anytime would make it more expensive than
> necessary for them.

I don't understand this comment.
The port id is currently retrieved via some pointers already.
I suggest to look at private structure, it is not different.

> It's understood that having failsafe in the dataplane has a cost, but even
> with the proposed fix, that cost is dwarfed by the amount of work done by a
> true PMD (and the application) for Rx processing.
> 
> My suggestion is to wait for someone to complain about the performance
> compared to what they had before that fix, only then see what we can do.

OK

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 18:46, Adrien Mazarguil:
> On Thu, Apr 18, 2019 at 05:51:18PM +0200, Thomas Monjalon wrote:
> > 18/04/2019 17:39, Thomas Monjalon:
> > > 18/04/2019 17:32, Adrien Mazarguil:
> > > > When passed to the application, Rx packets retain the port ID value
> > > > originally set by slave devices. Unfortunately these IDs have no meaning to
> > > > applications, which are typically unaware of their existence.
> > > > 
> > > > This confuses those caring about the source port field in mbufs (m->port)
> > > > which experience issues ranging from traffic drop to crashes.
> > [...]
> > > > +/*
> > > > + * Override source port in Rx packets.
> > > > + *
> > > > + * Make Rx packets originate from this PMD instance instead of one of its
> > > > + * slaves. This is mandatory to avoid breaking applications.
> > > > + */
> <snip>
> > > "slave" is a wording from bonding.
> > > In failsafe, it is sub-device, isn't it?
> 
> I don't mind, although grep shows a couple of comments talking about slaves
> already. Either way I think it fits as those are failsafe's pets, as in
> failsafe does whatever it wants to them and they don't have a say :)
> 
> Does it warrant a v3?

Yes please, except if Ferruh is already doing the change on apply.

> > > > +static void
> > > > +failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	for (i = 0; i != nb_pkts; ++i)
> > > > +		rx_pkts[i]->port = port;
> > > > +}
> > > > +
> > > >  uint16_t
> > > >  failsafe_rx_burst(void *queue,
> > > >  		  struct rte_mbuf **rx_pkts,
> > > > @@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
> > > >  		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;
> > > >  }
> > > 
> > > I'm afraid the performance drop to be hard.
> 
> Mbufs are still hot from the oven at this stage, so it's not *that*
> expensive. I don't see a more efficient approach.

Yes, Ali did some quick tests showing no perf drop.

> > > How the port id in mbuf is used exactly?
> 
> Applications that dissociate Rx itself from packet processing, or whenever a
> networking stack is involved. Basically every time some code wonders where a
> packet comes from due to lack of context and looks at m->port for the
> answer (e.g. checking that a packet arrives on the right port given its
> destination address).
> 
> > > What crash are you seeing?
> 
> None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> traffic coming from unknown ports.
> 
> However nothing prevents applications from using m->port as an index of some
> array they allocated to quickly retrieve port context without looking it
> up. They wouldn't expect indices they do not know about in there; assuming
> it will result in a crash is not far fetched.
> 
> > Another way to fix it without performance drop would be to add
> > a new driver op to set the top-level port id.
> > This top-level id would be stored in the private structure of the port,
> > initialized with the port id of the port itself, and used to fill mbufs.
> > 
> > Thoughts?
> 
> Adding a new devop as a fix would be a problem for stable releases, so this
> patch is definitely needed, at least as a first step.
> 
> I'm not against a new API, however would it be worth the trouble? Especially
> considering it would only be used by failsafe-like drivers with something to
> hide from applications which is not the main use case.
> 
> For some PMDs, this operation could only be done at init time before port ID
> is stored in private Rx queue data for fast retrieval. Retrieving it through
> a pointer so it can be updated anytime would make it more expensive than
> necessary for them.

I don't understand this comment.
The port id is currently retrieved via some pointers already.
I suggest to look at private structure, it is not different.

> It's understood that having failsafe in the dataplane has a cost, but even
> with the proposed fix, that cost is dwarfed by the amount of work done by a
> true PMD (and the application) for Rx processing.
> 
> My suggestion is to wait for someone to complain about the performance
> compared to what they had before that fix, only then see what we can do.

OK

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 06:54:22PM +0200, Thomas Monjalon wrote:
<snip>
> > <snip>
> > > > "slave" is a wording from bonding.
> > > > In failsafe, it is sub-device, isn't it?
> > 
> > I don't mind, although grep shows a couple of comments talking about slaves
> > already. Either way I think it fits as those are failsafe's pets, as in
> > failsafe does whatever it wants to them and they don't have a say :)
> > 
> > Does it warrant a v3?
> 
> Yes please, except if Ferruh is already doing the change on apply.

Will do.

<snip>
> > > > I'm afraid the performance drop to be hard.
> > 
> > Mbufs are still hot from the oven at this stage, so it's not *that*
> > expensive. I don't see a more efficient approach.
> 
> Yes, Ali did some quick tests showing no perf drop.

Great.

> > > > How the port id in mbuf is used exactly?
> > 
> > Applications that dissociate Rx itself from packet processing, or whenever a
> > networking stack is involved. Basically every time some code wonders where a
> > packet comes from due to lack of context and looks at m->port for the
> > answer (e.g. checking that a packet arrives on the right port given its
> > destination address).
> > 
> > > > What crash are you seeing?
> > 
> > None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> > traffic coming from unknown ports.
> > 
> > However nothing prevents applications from using m->port as an index of some
> > array they allocated to quickly retrieve port context without looking it
> > up. They wouldn't expect indices they do not know about in there; assuming
> > it will result in a crash is not far fetched.
> > 
> > > Another way to fix it without performance drop would be to add
> > > a new driver op to set the top-level port id.
> > > This top-level id would be stored in the private structure of the port,
> > > initialized with the port id of the port itself, and used to fill mbufs.
> > > 
> > > Thoughts?
> > 
> > Adding a new devop as a fix would be a problem for stable releases, so this
> > patch is definitely needed, at least as a first step.
> > 
> > I'm not against a new API, however would it be worth the trouble? Especially
> > considering it would only be used by failsafe-like drivers with something to
> > hide from applications which is not the main use case.
> > 
> > For some PMDs, this operation could only be done at init time before port ID
> > is stored in private Rx queue data for fast retrieval. Retrieving it through
> > a pointer so it can be updated anytime would make it more expensive than
> > necessary for them.
> 
> I don't understand this comment.
> The port id is currently retrieved via some pointers already.
> I suggest to look at private structure, it is not different.

See "rep->port = rxq->port_id" in mlx4_rxtx.c for instance. Port ID is
cached in private queue data structure (struct rxq) and retrieved there to
avoid looking it up in non-local data structure rxq->priv->dev_data->port.
In fact rxq->priv is not accessed even once during Rx.

> > It's understood that having failsafe in the dataplane has a cost, but even
> > with the proposed fix, that cost is dwarfed by the amount of work done by a
> > true PMD (and the application) for Rx processing.
> > 
> > My suggestion is to wait for someone to complain about the performance
> > compared to what they had before that fix, only then see what we can do.
> 
> OK
> 
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 06:54:22PM +0200, Thomas Monjalon wrote:
<snip>
> > <snip>
> > > > "slave" is a wording from bonding.
> > > > In failsafe, it is sub-device, isn't it?
> > 
> > I don't mind, although grep shows a couple of comments talking about slaves
> > already. Either way I think it fits as those are failsafe's pets, as in
> > failsafe does whatever it wants to them and they don't have a say :)
> > 
> > Does it warrant a v3?
> 
> Yes please, except if Ferruh is already doing the change on apply.

Will do.

<snip>
> > > > I'm afraid the performance drop to be hard.
> > 
> > Mbufs are still hot from the oven at this stage, so it's not *that*
> > expensive. I don't see a more efficient approach.
> 
> Yes, Ali did some quick tests showing no perf drop.

Great.

> > > > How the port id in mbuf is used exactly?
> > 
> > Applications that dissociate Rx itself from packet processing, or whenever a
> > networking stack is involved. Basically every time some code wonders where a
> > packet comes from due to lack of context and looks at m->port for the
> > answer (e.g. checking that a packet arrives on the right port given its
> > destination address).
> > 
> > > > What crash are you seeing?
> > 
> > None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> > traffic coming from unknown ports.
> > 
> > However nothing prevents applications from using m->port as an index of some
> > array they allocated to quickly retrieve port context without looking it
> > up. They wouldn't expect indices they do not know about in there; assuming
> > it will result in a crash is not far fetched.
> > 
> > > Another way to fix it without performance drop would be to add
> > > a new driver op to set the top-level port id.
> > > This top-level id would be stored in the private structure of the port,
> > > initialized with the port id of the port itself, and used to fill mbufs.
> > > 
> > > Thoughts?
> > 
> > Adding a new devop as a fix would be a problem for stable releases, so this
> > patch is definitely needed, at least as a first step.
> > 
> > I'm not against a new API, however would it be worth the trouble? Especially
> > considering it would only be used by failsafe-like drivers with something to
> > hide from applications which is not the main use case.
> > 
> > For some PMDs, this operation could only be done at init time before port ID
> > is stored in private Rx queue data for fast retrieval. Retrieving it through
> > a pointer so it can be updated anytime would make it more expensive than
> > necessary for them.
> 
> I don't understand this comment.
> The port id is currently retrieved via some pointers already.
> I suggest to look at private structure, it is not different.

See "rep->port = rxq->port_id" in mlx4_rxtx.c for instance. Port ID is
cached in private queue data structure (struct rxq) and retrieved there to
avoid looking it up in non-local data structure rxq->priv->dev_data->port.
In fact rxq->priv is not accessed even once during Rx.

> > It's understood that having failsafe in the dataplane has a cost, but even
> > with the proposed fix, that cost is dwarfed by the amount of work done by a
> > true PMD (and the application) for Rx processing.
> > 
> > My suggestion is to wait for someone to complain about the performance
> > compared to what they had before that fix, only then see what we can do.
> 
> OK
> 
> 

-- 
Adrien Mazarguil
6WIND

[dpdk-dev] [PATCH v3] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
--
v3 changes:

Removed unnecessary reference to slavery ("slave" device) for political
correctness.

Also kept *-by lines as in v2.

v2 changes:

Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
"rxq->priv->data->port_id" (since v19.05) and checked compilation against
master this time.

Given the limited scope of that change, reviewed-by/acked-by lines were
kept.
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..fee08fa23 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * sub-devices. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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;
 }
 
-- 
2.11.0

[dpdk-dev] [PATCH v3] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

When passed to the application, Rx packets retain the port ID value
originally set by slave devices. Unfortunately these IDs have no meaning to
applications, which are typically unaware of their existence.

This confuses those caring about the source port field in mbufs (m->port)
which experience issues ranging from traffic drop to crashes.

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>
--
v3 changes:

Removed unnecessary reference to slavery ("slave" device) for political
correctness.

Also kept *-by lines as in v2.

v2 changes:

Modified "rxq->priv->dev->data->port_id" (v18.11-style) to
"rxq->priv->data->port_id" (since v19.05) and checked compilation against
master this time.

Given the limited scope of that change, reviewed-by/acked-by lines were
kept.
---
 drivers/net/failsafe/failsafe_rxtx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/failsafe/failsafe_rxtx.c b/drivers/net/failsafe/failsafe_rxtx.c
index 231c83291..fee08fa23 100644
--- a/drivers/net/failsafe/failsafe_rxtx.c
+++ b/drivers/net/failsafe/failsafe_rxtx.c
@@ -61,6 +61,21 @@ failsafe_set_burst_fn(struct rte_eth_dev *dev, int force_safe)
 	rte_wmb();
 }
 
+/*
+ * Override source port in Rx packets.
+ *
+ * Make Rx packets originate from this PMD instance instead of one of its
+ * sub-devices. This is mandatory to avoid breaking applications.
+ */
+static void
+failsafe_rx_set_port(struct rte_mbuf **rx_pkts, uint16_t nb_pkts, uint16_t port)
+{
+	unsigned int i;
+
+	for (i = 0; i != nb_pkts; ++i)
+		rx_pkts[i]->port = port;
+}
+
 uint16_t
 failsafe_rx_burst(void *queue,
 		  struct rte_mbuf **rx_pkts,
@@ -87,6 +102,9 @@ failsafe_rx_burst(void *queue,
 		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;
 }
 
@@ -112,6 +130,9 @@ failsafe_rx_burst_fast(void *queue,
 		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;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 19:09, Adrien Mazarguil:
> On Thu, Apr 18, 2019 at 06:54:22PM +0200, Thomas Monjalon wrote:
> <snip>
> > > <snip>
> > > > > "slave" is a wording from bonding.
> > > > > In failsafe, it is sub-device, isn't it?
> > > 
> > > I don't mind, although grep shows a couple of comments talking about slaves
> > > already. Either way I think it fits as those are failsafe's pets, as in
> > > failsafe does whatever it wants to them and they don't have a say :)
> > > 
> > > Does it warrant a v3?
> > 
> > Yes please, except if Ferruh is already doing the change on apply.
> 
> Will do.
> 
> <snip>
> > > > > I'm afraid the performance drop to be hard.
> > > 
> > > Mbufs are still hot from the oven at this stage, so it's not *that*
> > > expensive. I don't see a more efficient approach.
> > 
> > Yes, Ali did some quick tests showing no perf drop.
> 
> Great.
> 
> > > > > How the port id in mbuf is used exactly?
> > > 
> > > Applications that dissociate Rx itself from packet processing, or whenever a
> > > networking stack is involved. Basically every time some code wonders where a
> > > packet comes from due to lack of context and looks at m->port for the
> > > answer (e.g. checking that a packet arrives on the right port given its
> > > destination address).
> > > 
> > > > > What crash are you seeing?
> > > 
> > > None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> > > traffic coming from unknown ports.
> > > 
> > > However nothing prevents applications from using m->port as an index of some
> > > array they allocated to quickly retrieve port context without looking it
> > > up. They wouldn't expect indices they do not know about in there; assuming
> > > it will result in a crash is not far fetched.
> > > 
> > > > Another way to fix it without performance drop would be to add
> > > > a new driver op to set the top-level port id.
> > > > This top-level id would be stored in the private structure of the port,
> > > > initialized with the port id of the port itself, and used to fill mbufs.
> > > > 
> > > > Thoughts?
> > > 
> > > Adding a new devop as a fix would be a problem for stable releases, so this
> > > patch is definitely needed, at least as a first step.
> > > 
> > > I'm not against a new API, however would it be worth the trouble? Especially
> > > considering it would only be used by failsafe-like drivers with something to
> > > hide from applications which is not the main use case.
> > > 
> > > For some PMDs, this operation could only be done at init time before port ID
> > > is stored in private Rx queue data for fast retrieval. Retrieving it through
> > > a pointer so it can be updated anytime would make it more expensive than
> > > necessary for them.
> > 
> > I don't understand this comment.
> > The port id is currently retrieved via some pointers already.
> > I suggest to look at private structure, it is not different.
> 
> See "rep->port = rxq->port_id" in mlx4_rxtx.c for instance. Port ID is
> cached in private queue data structure (struct rxq) and retrieved there to
> avoid looking it up in non-local data structure rxq->priv->dev_data->port.
> In fact rxq->priv is not accessed even once during Rx.

OK, thanks for the explanation.

> > > It's understood that having failsafe in the dataplane has a cost, but even
> > > with the proposed fix, that cost is dwarfed by the amount of work done by a
> > > true PMD (and the application) for Rx processing.
> > > 
> > > My suggestion is to wait for someone to complain about the performance
> > > compared to what they had before that fix, only then see what we can do.
> > 
> > OK

Re: [dpdk-dev] [PATCH v2] net/failsafe: fix source port ID in Rx packets

[Thomas Monjalon]

18/04/2019 19:09, Adrien Mazarguil:
> On Thu, Apr 18, 2019 at 06:54:22PM +0200, Thomas Monjalon wrote:
> <snip>
> > > <snip>
> > > > > "slave" is a wording from bonding.
> > > > > In failsafe, it is sub-device, isn't it?
> > > 
> > > I don't mind, although grep shows a couple of comments talking about slaves
> > > already. Either way I think it fits as those are failsafe's pets, as in
> > > failsafe does whatever it wants to them and they don't have a say :)
> > > 
> > > Does it warrant a v3?
> > 
> > Yes please, except if Ferruh is already doing the change on apply.
> 
> Will do.
> 
> <snip>
> > > > > I'm afraid the performance drop to be hard.
> > > 
> > > Mbufs are still hot from the oven at this stage, so it's not *that*
> > > expensive. I don't see a more efficient approach.
> > 
> > Yes, Ali did some quick tests showing no perf drop.
> 
> Great.
> 
> > > > > How the port id in mbuf is used exactly?
> > > 
> > > Applications that dissociate Rx itself from packet processing, or whenever a
> > > networking stack is involved. Basically every time some code wonders where a
> > > packet comes from due to lack of context and looks at m->port for the
> > > answer (e.g. checking that a packet arrives on the right port given its
> > > destination address).
> > > 
> > > > > What crash are you seeing?
> > > 
> > > None, thankfully. In my specific use case, 6WINDGate's stack simply drops
> > > traffic coming from unknown ports.
> > > 
> > > However nothing prevents applications from using m->port as an index of some
> > > array they allocated to quickly retrieve port context without looking it
> > > up. They wouldn't expect indices they do not know about in there; assuming
> > > it will result in a crash is not far fetched.
> > > 
> > > > Another way to fix it without performance drop would be to add
> > > > a new driver op to set the top-level port id.
> > > > This top-level id would be stored in the private structure of the port,
> > > > initialized with the port id of the port itself, and used to fill mbufs.
> > > > 
> > > > Thoughts?
> > > 
> > > Adding a new devop as a fix would be a problem for stable releases, so this
> > > patch is definitely needed, at least as a first step.
> > > 
> > > I'm not against a new API, however would it be worth the trouble? Especially
> > > considering it would only be used by failsafe-like drivers with something to
> > > hide from applications which is not the main use case.
> > > 
> > > For some PMDs, this operation could only be done at init time before port ID
> > > is stored in private Rx queue data for fast retrieval. Retrieving it through
> > > a pointer so it can be updated anytime would make it more expensive than
> > > necessary for them.
> > 
> > I don't understand this comment.
> > The port id is currently retrieved via some pointers already.
> > I suggest to look at private structure, it is not different.
> 
> See "rep->port = rxq->port_id" in mlx4_rxtx.c for instance. Port ID is
> cached in private queue data structure (struct rxq) and retrieved there to
> avoid looking it up in non-local data structure rxq->priv->dev_data->port.
> In fact rxq->priv is not accessed even once during Rx.

OK, thanks for the explanation.

> > > It's understood that having failsafe in the dataplane has a cost, but even
> > > with the proposed fix, that cost is dwarfed by the amount of work done by a
> > > true PMD (and the application) for Rx processing.
> > > 
> > > My suggestion is to wait for someone to complain about the performance
> > > compared to what they had before that fix, only then see what we can do.
> > 
> > OK

Re: [dpdk-dev] [PATCH v3] net/failsafe: fix source port ID in Rx packets

[Ferruh Yigit]

On 4/18/2019 6:20 PM, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH v3] net/failsafe: fix source port ID in Rx packets

[Ferruh Yigit]

On 4/18/2019 6:20 PM, Adrien Mazarguil wrote:
> When passed to the application, Rx packets retain the port ID value
> originally set by slave devices. Unfortunately these IDs have no meaning to
> applications, which are typically unaware of their existence.
> 
> This confuses those caring about the source port field in mbufs (m->port)
> which experience issues ranging from traffic drop to crashes.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 04:06:31PM +0200, David Marchand wrote:
> Hey Adrien,

Hey David!

> On Thu, Apr 18, 2019 at 3:12 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
> wrote:
> 
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> >
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> >
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
<snip>
> Not a big fan of those duplicated rx_burst functions...
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> I suppose the bonding pmd has the same issue.

I don't have much experience with it, however chances are that it's not as
bad as with failsafe since bonding behaves more like a helper that
applications knowingly use to aggregate ports they set up and already know
about. Leaving the original port ID in this case may actually be useful, but
could be optional.

Failsafe on the other hand spawns and manages any number of sub-devices
hidden from the application on its own based on opaque user configuration.
These sub-devices may or may not be present depending on hot-plug events
absorbed by failsafe, which means their port IDs are not only unexpected but
also volatile.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] net/failsafe: fix source port ID in Rx packets

[Adrien Mazarguil]

On Thu, Apr 18, 2019 at 04:06:31PM +0200, David Marchand wrote:
> Hey Adrien,

Hey David!

> On Thu, Apr 18, 2019 at 3:12 PM Adrien Mazarguil <adrien.mazarguil@6wind.com>
> wrote:
> 
> > When passed to the application, Rx packets retain the port ID value
> > originally set by slave devices. Unfortunately these IDs have no meaning to
> > applications, which are typically unaware of their existence.
> >
> > This confuses those caring about the source port field in mbufs (m->port)
> > which experience issues ranging from traffic drop to crashes.
> >
> > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
<snip>
> Not a big fan of those duplicated rx_burst functions...
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> I suppose the bonding pmd has the same issue.

I don't have much experience with it, however chances are that it's not as
bad as with failsafe since bonding behaves more like a helper that
applications knowingly use to aggregate ports they set up and already know
about. Leaving the original port ID in this case may actually be useful, but
could be optional.

Failsafe on the other hand spawns and manages any number of sub-devices
hidden from the application on its own based on opaque user configuration.
These sub-devices may or may not be present depending on hot-plug events
absorbed by failsafe, which means their port IDs are not only unexpected but
also volatile.

-- 
Adrien Mazarguil
6WIND