[dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[Moti Haimovsky]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[Shahaf Shuler]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[Mordechay Haimovsky]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[Shahaf Shuler]

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>

Re: [dpdk-dev] [PATCH] net/mlx4: fix no broadcasts reception in promisc

[Adrien Mazarguil]

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