DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc
@ 2018-01-07 11:04 Moti Haimovsky
  2018-01-25 15:37 ` [dpdk-dev] [dpdk-stable] " Shahaf Shuler
  2018-01-26 17:03 ` [dpdk-dev] " Adrien Mazarguil
  0 siblings, 2 replies; 5+ messages in thread
From: Moti Haimovsky @ 2018-01-07 11:04 UTC (permalink / raw)
  To: adrienm; +Cc: dev, Moti Haimovsky, stable

This patch fixes the issue of mlx4 not receiving broadcast packets
when configured to work promiscuous mode.

Fixes: eacaac7bae36 ("net/mlx4: restore promisc and allmulti support")
Cc: stable@dpdk.org

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
 drivers/net/mlx4/mlx4_flow.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
index 69025da..ec13f5a 100644
--- a/drivers/net/mlx4/mlx4_flow.c
+++ b/drivers/net/mlx4/mlx4_flow.c
@@ -1223,9 +1223,12 @@ struct mlx4_drop {
  *
  * Various flow rules are created depending on the mode the device is in:
  *
- * 1. Promiscuous: port MAC + catch-all (VLAN filtering is ignored).
- * 2. All multicast: port MAC/VLAN + catch-all multicast.
- * 3. Otherwise: port MAC/VLAN + broadcast MAC/VLAN.
+ * 1. Promiscuous:
+ *       port MAC + broadcast + catch-all (VLAN filtering is ignored).
+ * 2. All multicast:
+ *       port MAC/VLAN + broadcast + catch-all multicast.
+ * 3. Otherwise:
+ *       port MAC/VLAN + broadcast MAC/VLAN.
  *
  * About MAC flow rules:
  *
@@ -1304,9 +1307,6 @@ struct mlx4_drop {
 		!priv->dev->data->promiscuous ?
 		&vlan_spec.tci :
 		NULL;
-	int broadcast =
-		!priv->dev->data->promiscuous &&
-		!priv->dev->data->all_multicast;
 	uint16_t vlan = 0;
 	struct rte_flow *flow;
 	unsigned int i;
@@ -1340,7 +1340,7 @@ struct mlx4_drop {
 			rule_vlan = NULL;
 		}
 	}
-	for (i = 0; i != RTE_DIM(priv->mac) + broadcast; ++i) {
+	for (i = 0; i != RTE_DIM(priv->mac) + 1; ++i) {
 		const struct ether_addr *mac;
 
 		/* Broadcasts are handled by an extra iteration. */
@@ -1404,7 +1404,7 @@ struct mlx4_drop {
 			goto next_vlan;
 	}
 	/* Take care of promiscuous and all multicast flow rules. */
-	if (!broadcast) {
+	if (priv->dev->data->promiscuous || priv->dev->data->all_multicast) {
 		for (flow = LIST_FIRST(&priv->flows);
 		     flow && flow->internal;
 		     flow = LIST_NEXT(flow, next)) {
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc
  2018-01-07 11:04 [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc Moti Haimovsky
@ 2018-01-25 15:37 ` Shahaf Shuler
  2018-01-25 17:12   ` Mordechay Haimovsky
  2018-01-26 17:03 ` [dpdk-dev] " Adrien Mazarguil
  1 sibling, 1 reply; 5+ messages in thread
From: Shahaf Shuler @ 2018-01-25 15:37 UTC (permalink / raw)
  To: Mordechay Haimovsky, Adrien Mazarguil; +Cc: dev, Mordechay Haimovsky, stable

Sunday, January 7, 2018 1:05 PM, Moti Haimovsky
> Subject: [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in
> promisc
> 
> This patch fixes the issue of mlx4 not receiving broadcast packets when
> configured to work promiscuous mode.
> 
> Fixes: eacaac7bae36 ("net/mlx4: restore promisc and allmulti support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
>  drivers/net/mlx4/mlx4_flow.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index 69025da..ec13f5a 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -1223,9 +1223,12 @@ struct mlx4_drop {
>   *
>   * Various flow rules are created depending on the mode the device is in:
>   *
> - * 1. Promiscuous: port MAC + catch-all (VLAN filtering is ignored).
> - * 2. All multicast: port MAC/VLAN + catch-all multicast.
> - * 3. Otherwise: port MAC/VLAN + broadcast MAC/VLAN.
> + * 1. Promiscuous:
> + *       port MAC + broadcast + catch-all (VLAN filtering is ignored).
> + * 2. All multicast:
> + *       port MAC/VLAN + broadcast + catch-all multicast.
> + * 3. Otherwise:
> + *       port MAC/VLAN + broadcast MAC/VLAN.
>   *
>   * About MAC flow rules:
>   *
> @@ -1304,9 +1307,6 @@ struct mlx4_drop {
>  		!priv->dev->data->promiscuous ?
>  		&vlan_spec.tci :
>  		NULL;
> -	int broadcast =
> -		!priv->dev->data->promiscuous &&
> -		!priv->dev->data->all_multicast;
>  	uint16_t vlan = 0;
>  	struct rte_flow *flow;
>  	unsigned int i;
> @@ -1340,7 +1340,7 @@ struct mlx4_drop {
>  			rule_vlan = NULL;
>  		}
>  	}
> -	for (i = 0; i != RTE_DIM(priv->mac) + broadcast; ++i) {
> +	for (i = 0; i != RTE_DIM(priv->mac) + 1; ++i) {
>  		const struct ether_addr *mac;
> 
>  		/* Broadcasts are handled by an extra iteration. */ @@ -
> 1404,7 +1404,7 @@ struct mlx4_drop {
>  			goto next_vlan;
>  	}
>  	/* Take care of promiscuous and all multicast flow rules. */
> -	if (!broadcast) {
> +	if (priv->dev->data->promiscuous || priv->dev->data->all_multicast)

Please help me to understand the issue.
The promisc and allmulti spec and mask contains the broadcast addresses. 
They are not received because of the ibv flow rule type (MC_DEFAULT / ALL_DEFAULT) ?

> {
>  		for (flow = LIST_FIRST(&priv->flows);
>  		     flow && flow->internal;
>  		     flow = LIST_NEXT(flow, next)) {
> --
> 1.8.3.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc
  2018-01-25 15:37 ` [dpdk-dev] [dpdk-stable] " Shahaf Shuler
@ 2018-01-25 17:12   ` Mordechay Haimovsky
  2018-01-25 17:45     ` Shahaf Shuler
  0 siblings, 1 reply; 5+ messages in thread
From: Mordechay Haimovsky @ 2018-01-25 17:12 UTC (permalink / raw)
  To: Shahaf Shuler, Adrien Mazarguil; +Cc: dev, stable

The mlx4 does not support "don't care" bits in its flow spec mask, therefore we do cannot use 
The following rules used by mlx5 for promiscuous and allmulti:
        if (dev->data->promiscuous) {
                struct rte_flow_item_eth promisc = {
                        .dst.addr_bytes = "\x00\x00\x00\x00\x00\x00",
                        .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
                        .type = 0,
                };

                claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
                return 0;
        }
        if (dev->data->all_multicast) {
                struct rte_flow_item_eth multicast = {
                        .dst.addr_bytes = "\x01\x00\x00\x00\x00\x00",
                        .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
                        .type = 0,
                };

What we can use instead  In order to place the device in promiscuous mode is:
    struct all_flow_attr {
        struct ibv_flow_attr     attr;
    } __attribute__((packed)) all_attr = {
        .attr = {
            .comp_mask  = 0,
            .type       = IBV_FLOW_ATTR_ALL_DEFAULT,
            .size       = sizeof(all_attr),
            .priority   = 0,
            .num_of_specs = 0,
            .port       = PORT_NUM,
            .flags      = 0,
        },
    };
    struct ibv_flow *all_flow;
    all_flow = ibv_create_flow(qp, &all_attr.attr);

Now *_DEFAULT rules are low priority rules which catch the appropriate traffic which was not caught by another rule.
There is probably another rule for BC which is added in the driver  that catches the BC, that is why we do not see it. 
When we add an additional BC rule, we will also get it.

Moti

> -----Original Message-----
> From: Shahaf Shuler
> Sent: Thursday, January 25, 2018 5:38 PM
> To: Mordechay Haimovsky <motih@mellanox.com>; Adrien Mazarguil
> <adrienm@mellanox.com>
> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>;
> stable@dpdk.org
> Subject: RE: [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in
> promisc
> 
> Sunday, January 7, 2018 1:05 PM, Moti Haimovsky
> > Subject: [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception
> > in promisc
> >
> > This patch fixes the issue of mlx4 not receiving broadcast packets
> > when configured to work promiscuous mode.
> >
> > Fixes: eacaac7bae36 ("net/mlx4: restore promisc and allmulti support")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> > ---
> >  drivers/net/mlx4/mlx4_flow.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/mlx4/mlx4_flow.c
> > b/drivers/net/mlx4/mlx4_flow.c index 69025da..ec13f5a 100644
> > --- a/drivers/net/mlx4/mlx4_flow.c
> > +++ b/drivers/net/mlx4/mlx4_flow.c
> > @@ -1223,9 +1223,12 @@ struct mlx4_drop {
> >   *
> >   * Various flow rules are created depending on the mode the device is in:
> >   *
> > - * 1. Promiscuous: port MAC + catch-all (VLAN filtering is ignored).
> > - * 2. All multicast: port MAC/VLAN + catch-all multicast.
> > - * 3. Otherwise: port MAC/VLAN + broadcast MAC/VLAN.
> > + * 1. Promiscuous:
> > + *       port MAC + broadcast + catch-all (VLAN filtering is ignored).
> > + * 2. All multicast:
> > + *       port MAC/VLAN + broadcast + catch-all multicast.
> > + * 3. Otherwise:
> > + *       port MAC/VLAN + broadcast MAC/VLAN.
> >   *
> >   * About MAC flow rules:
> >   *
> > @@ -1304,9 +1307,6 @@ struct mlx4_drop {
> >  		!priv->dev->data->promiscuous ?
> >  		&vlan_spec.tci :
> >  		NULL;
> > -	int broadcast =
> > -		!priv->dev->data->promiscuous &&
> > -		!priv->dev->data->all_multicast;
> >  	uint16_t vlan = 0;
> >  	struct rte_flow *flow;
> >  	unsigned int i;
> > @@ -1340,7 +1340,7 @@ struct mlx4_drop {
> >  			rule_vlan = NULL;
> >  		}
> >  	}
> > -	for (i = 0; i != RTE_DIM(priv->mac) + broadcast; ++i) {
> > +	for (i = 0; i != RTE_DIM(priv->mac) + 1; ++i) {
> >  		const struct ether_addr *mac;
> >
> >  		/* Broadcasts are handled by an extra iteration. */ @@ -
> > 1404,7 +1404,7 @@ struct mlx4_drop {
> >  			goto next_vlan;
> >  	}
> >  	/* Take care of promiscuous and all multicast flow rules. */
> > -	if (!broadcast) {
> > +	if (priv->dev->data->promiscuous || priv->dev->data->all_multicast)
> 
> Please help me to understand the issue.
> The promisc and allmulti spec and mask contains the broadcast addresses.
> They are not received because of the ibv flow rule type (MC_DEFAULT /
> ALL_DEFAULT) ?
> 
> > {
> >  		for (flow = LIST_FIRST(&priv->flows);
> >  		     flow && flow->internal;
> >  		     flow = LIST_NEXT(flow, next)) {
> > --
> > 1.8.3.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc
  2018-01-25 17:12   ` Mordechay Haimovsky
@ 2018-01-25 17:45     ` Shahaf Shuler
  0 siblings, 0 replies; 5+ messages in thread
From: Shahaf Shuler @ 2018-01-25 17:45 UTC (permalink / raw)
  To: Mordechay Haimovsky, Adrien Mazarguil; +Cc: dev, stable

Thursday, January 25, 2018 7:12 PM, Mordechay Haimovsky:
not support "don't care" bits in its flow spec mask, therefore
> we do cannot use The following rules used by mlx5 for promiscuous and
> allmulti:
>         if (dev->data->promiscuous) {
>                 struct rte_flow_item_eth promisc = {
>                         .dst.addr_bytes = "\x00\x00\x00\x00\x00\x00",
>                         .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
>                         .type = 0,
>                 };
> 
>                 claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
>                 return 0;
>         }
>         if (dev->data->all_multicast) {
>                 struct rte_flow_item_eth multicast = {
>                         .dst.addr_bytes = "\x01\x00\x00\x00\x00\x00",
>                         .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
>                         .type = 0,
>                 };
> 
> What we can use instead  In order to place the device in promiscuous mode
> is:
>     struct all_flow_attr {
>         struct ibv_flow_attr     attr;
>     } __attribute__((packed)) all_attr = {
>         .attr = {
>             .comp_mask  = 0,
>             .type       = IBV_FLOW_ATTR_ALL_DEFAULT,
>             .size       = sizeof(all_attr),
>             .priority   = 0,
>             .num_of_specs = 0,
>             .port       = PORT_NUM,
>             .flags      = 0,
>         },
>     };
>     struct ibv_flow *all_flow;
>     all_flow = ibv_create_flow(qp, &all_attr.attr);
> 
> Now *_DEFAULT rules are low priority rules which catch the appropriate
> traffic which was not caught by another rule.
> There is probably another rule for BC which is added in the driver  that
> catches the BC, that is why we do not see it.
> When we add an additional BC rule, we will also get it.

Thanks, understood. 

7, 2018 1:05 PM, Moti Haimovsky
> > > Subject: [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception
> > > in promisc
> > >
> > > This patch fixes the issue of mlx4 not receiving broadcast packets
> > > when configured to work promiscuous mode.
> > >
> > > Fixes: eacaac7bae36 ("net/mlx4: restore promisc and allmulti
> > > support")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> > > ---
> > >  drivers/net/mlx4/mlx4_flow.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx4/mlx4_flow.c
> > > b/drivers/net/mlx4/mlx4_flow.c index 69025da..ec13f5a 100644
> > > --- a/drivers/net/mlx4/mlx4_flow.c
> > > +++ b/drivers/net/mlx4/mlx4_flow.c
> > > @@ -1223,9 +1223,12 @@ struct mlx4_drop {
> > >   *
> > >   * Various flow rules are created depending on the mode the device is in:
> > >   *
> > > - * 1. Promiscuous: port MAC + catch-all (VLAN filtering is ignored).
> > > - * 2. All multicast: port MAC/VLAN + catch-all multicast.
> > > - * 3. Otherwise: port MAC/VLAN + broadcast MAC/VLAN.
> > > + * 1. Promiscuous:
> > > + *       port MAC + broadcast + catch-all (VLAN filtering is ignored).

No need to add here anything. Promiscuous includes broadcasts by definition.  

> > > + * 2. All multicast:
> > > + *       port MAC/VLAN + broadcast + catch-all multicast.

No need to add here anything. All multi includes broadcasts by definition.  

If no further comments from Adrien, and you agree, I can fix those two during the merge (no need for another version).

Adrien - please have a look on it tomorrow 

Reviewed-by: Shahaf Shuler <shahafs@mellanox.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc
  2018-01-07 11:04 [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc Moti Haimovsky
  2018-01-25 15:37 ` [dpdk-dev] [dpdk-stable] " Shahaf Shuler
@ 2018-01-26 17:03 ` Adrien Mazarguil
  1 sibling, 0 replies; 5+ messages in thread
From: Adrien Mazarguil @ 2018-01-26 17:03 UTC (permalink / raw)
  To: Moti Haimovsky; +Cc: dev, stable

Hi Moti,

On Sun, Jan 07, 2018 at 01:04:46PM +0200, Moti Haimovsky wrote:
> This patch fixes the issue of mlx4 not receiving broadcast packets
> when configured to work promiscuous mode.

Also in allmulticast mode, right? Both IBV_FLOW_ATTR_MC_DEFAULT and
IBV_FLOW_ATTR_ALL_DEFAULT priorities are too low and may end up in this
situation. You could add it usually occurs on VFs.

> Fixes: eacaac7bae36 ("net/mlx4: restore promisc and allmulti support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>

If you confirm this problem covers allmulticast mode, please update the
commit log. Otherwise, patch looks good, thanks.

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-01-26 17:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-07 11:04 [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc Moti Haimovsky
2018-01-25 15:37 ` [dpdk-dev] [dpdk-stable] " Shahaf Shuler
2018-01-25 17:12   ` Mordechay Haimovsky
2018-01-25 17:45     ` Shahaf Shuler
2018-01-26 17:03 ` [dpdk-dev] " Adrien Mazarguil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).