diff mbox series

net/failsafe: fix source port ID in Rx packets

Message ID 20190418130419.25675-1-adrien.mazarguil@6wind.com (mailing list archive)
State Superseded, archived
Headers
Series net/failsafe: fix source port ID in Rx packets |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Performance-Testing fail build patch failure
ci/Intel-compilation fail Compilation issues

Commit Message

Adrien Mazarguil April 18, 2019, 1:11 p.m. UTC 
  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(+)

Comments

David Marchand April 18, 2019, 2:06 p.m. UTC | #1 
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.
Gaëtan Rivet April 18, 2019, 2:08 p.m. UTC | #2 
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
Ferruh Yigit April 18, 2019, 2:42 p.m. UTC | #3 
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;
>  }
>  
>
Adrien Mazarguil April 18, 2019, 3:08 p.m. UTC | #4 
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 April 19, 2019, 8:08 a.m. UTC | #5 
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.
diff mbox series

Patch

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;
 }