[dpdk-dev,v2,6/7] net/mlx5: fix reception when VLAN is added
Checks
Commit Message
When VLAN is enabled in the Rx side, only packets matching this VLAN are
expected, this also includes the broadcast and all multicast packets.
Fixes: 272733b5ebfd ("net/mlx5: use flow to enable unicast traffic")
Fixes: 6a6b6828fe6a ("net/mlx5: use flow to enable all multi mode")
Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
drivers/net/mlx5/mlx5_trigger.c | 125 +++++++++++++++++++++-------------------
1 file changed, 66 insertions(+), 59 deletions(-)
Comments
On Mon, Oct 23, 2017 at 04:49:56PM +0200, Nelio Laranjeiro wrote:
> @@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
> };
>
> claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
> - } else if (dev->data->all_multicast) {
> + 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 = "\x01\x00\x00\x00\x00\x00",
> + .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
> .type = 0,
> };
>
> claim_zero(mlx5_ctrl_flow(dev, &multicast, &multicast));
Just curious. No need to consider VLAN for multicast here?
> - } else {
> - struct rte_flow_item_eth bcast = {
> - .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> - };
> - struct rte_flow_item_eth ipv6_multi_spec = {
> - .dst.addr_bytes = "\x33\x33\x00\x00\x00\x00",
> - };
> - struct rte_flow_item_eth ipv6_multi_mask = {
> - .dst.addr_bytes = "\xff\xff\x00\x00\x00\x00",
> - };
> - struct rte_flow_item_eth unicast = {
> - .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
> - };
> - struct rte_flow_item_eth unicast_mask = {
> - .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
> - };
> - const unsigned int vlan_filter_n = priv->vlan_filter_n;
> - const struct ether_addr cmp = {
> - .addr_bytes = "\x00\x00\x00\x00\x00\x00",
> - };
> - unsigned int i;
> - unsigned int j;
> - unsigned int unicast_flow = 0;
> - int ret;
> -
> - for (i = 0; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
> - struct ether_addr *mac = &dev->data->mac_addrs[i];
> + }
> + for (i = 0; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
> + struct ether_addr *mac = &dev->data->mac_addrs[i];
>
> - if (!memcmp(mac, &cmp, sizeof(*mac)))
> - continue;
> - memcpy(&unicast.dst.addr_bytes,
> - mac->addr_bytes,
> - ETHER_ADDR_LEN);
> - for (j = 0; j != vlan_filter_n; ++j) {
> - uint16_t vlan = priv->vlan_filter[j];
> + if (!memcmp(mac, &cmp, sizeof(*mac)))
> + continue;
> + memcpy(&unicast.dst.addr_bytes,
> + mac->addr_bytes,
> + ETHER_ADDR_LEN);
> + for (j = 0; j != vlan_filter_n; ++j) {
> + uint16_t vlan = priv->vlan_filter[j];
>
> - struct rte_flow_item_vlan vlan_spec = {
> - .tci = rte_cpu_to_be_16(vlan),
> - };
> - struct rte_flow_item_vlan vlan_mask = {
> - .tci = 0xffff,
> - };
> + struct rte_flow_item_vlan vlan_spec = {
> + .tci = rte_cpu_to_be_16(vlan),
> + };
> + struct rte_flow_item_vlan vlan_mask = {
> + .tci = 0xffff,
> + };
>
> - ret = mlx5_ctrl_flow_vlan(dev, &unicast,
> - &unicast_mask,
> - &vlan_spec,
> - &vlan_mask);
> - if (ret)
> - goto error;
> - unicast_flow = 1;
> - }
> - if (!vlan_filter_n) {
> - ret = mlx5_ctrl_flow(dev, &unicast,
> - &unicast_mask);
> - if (ret)
> - goto error;
> - unicast_flow = 1;
> - }
> + ret = mlx5_ctrl_flow_vlan(dev, &unicast,
> + &unicast_mask,
> + &vlan_spec,
> + &vlan_mask);
> + if (ret)
> + goto error;
> + ret = mlx5_ctrl_flow_vlan(dev, &bcast, &bcast,
> + &vlan_spec, &vlan_mask);
> + if (ret)
> + goto error;
> + ret = mlx5_ctrl_flow_vlan(dev, &ipv6_multi_spec,
> + &ipv6_multi_mask,
> + &vlan_spec, &vlan_mask);
These (bcast and ipv6_multi_mask) can be duplicated multiple times if there are
multiple MAC addrs, is that intended?
Thanks,
Yongseok
On Mon, Oct 23, 2017 at 12:25:45PM -0700, Yongseok Koh wrote:
> On Mon, Oct 23, 2017 at 04:49:56PM +0200, Nelio Laranjeiro wrote:
> > @@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
> > };
> >
> > claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
> > - } else if (dev->data->all_multicast) {
> > + 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 = "\x01\x00\x00\x00\x00\x00",
> > + .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
> > .type = 0,
> > };
> >
> > claim_zero(mlx5_ctrl_flow(dev, &multicast, &multicast));
>
> Just curious. No need to consider VLAN for multicast here?
According to the lib documentation no [1]
"Enable the receipt of any multicast frame by an Ethernet device"
> [...]
> These (bcast and ipv6_multi_mask) can be duplicated multiple times if there are
> multiple MAC addrs, is that intended?
There is in fact an issue in this series, it does not match my final code.
I'll send a v3.
[1] http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h#n2304
On Tue, Oct 24, 2017 at 09:11:42AM +0200, Nélio Laranjeiro wrote:
> On Mon, Oct 23, 2017 at 12:25:45PM -0700, Yongseok Koh wrote:
> > On Mon, Oct 23, 2017 at 04:49:56PM +0200, Nelio Laranjeiro wrote:
> > > @@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
> > > };
> > >
> > > claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
> > > - } else if (dev->data->all_multicast) {
> > > + 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 = "\x01\x00\x00\x00\x00\x00",
> > > + .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
> > > .type = 0,
> > > };
> > >
> > > claim_zero(mlx5_ctrl_flow(dev, &multicast, &multicast));
> >
> > Just curious. No need to consider VLAN for multicast here?
>
> According to the lib documentation no [1]
>
> "Enable the receipt of any multicast frame by an Ethernet device"
>
> > [...]
> > These (bcast and ipv6_multi_mask) can be duplicated multiple times if there are
> > multiple MAC addrs, is that intended?
>
> There is in fact an issue in this series, it does not match my final code.
>
> I'll send a v3.
>
> [1] http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.h#n2304
I've wrongly read your last comment, the patch is correct, it won't add
multiple time the broadcast multicast, it will add one per expected
VLAN.
Example:
testpmd> set promisc all off
testpmd> set allmulti all off
testpmd> rx_vlan add 0 1330
testpmd> rx_vlan add 0 1331
Will cause this code to add a broadcast flow with VLAN TCI 1330 and
another broadcast flow with VLAN TCI 1331, others won't be received.
The user will only receive broadcast packets with VLAN TCI 1330 and
1331. It is what he expects.
In case not VLAN is configured, the broadcast and muilticast flow
insertion are under the condition:
"if (!dev->data->all_multicast && !vlan_filter_n)"
Thanks,
On Tue, Oct 24, 2017 at 09:34:34AM +0200, Nélio Laranjeiro wrote:
> On Tue, Oct 24, 2017 at 09:11:42AM +0200, Nélio Laranjeiro wrote:
> > On Mon, Oct 23, 2017 at 12:25:45PM -0700, Yongseok Koh wrote:
> > > On Mon, Oct 23, 2017 at 04:49:56PM +0200, Nelio Laranjeiro wrote:
> > > > @@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
[...]
> I've wrongly read your last comment, the patch is correct, it won't add
> multiple time the broadcast multicast, it will add one per expected
> VLAN.
>
> Example:
>
> testpmd> set promisc all off
> testpmd> set allmulti all off
> testpmd> rx_vlan add 0 1330
> testpmd> rx_vlan add 0 1331
>
> Will cause this code to add a broadcast flow with VLAN TCI 1330 and
> another broadcast flow with VLAN TCI 1331, others won't be received.
>
> The user will only receive broadcast packets with VLAN TCI 1330 and
> 1331. It is what he expects.
What I meant was, if there are multiple MAC addresses on a port, the bcast/mcast
flows will be repeated. For example, if there are 3 valid addrs in
dev->data->mac_addrs
testpmd> mac_addr add 0 <addr1>
testpmd> mac_addr add 0 <addr2>
testpmd> mac_addr add 0 <addr3>
and 2 vlan filters are configured like your example above, then 6 ucast flows
(2 per an addr) will be added along with 6 bcast flows and 6 mcast flows. But it
only needs 2 bcast flows and 2 mcast flows - one per vlan.
Thanks,
Yongseok
On Tue, Oct 24, 2017 at 06:41:07AM -0700, Yongseok Koh wrote:
> On Tue, Oct 24, 2017 at 09:34:34AM +0200, Nélio Laranjeiro wrote:
> > On Tue, Oct 24, 2017 at 09:11:42AM +0200, Nélio Laranjeiro wrote:
> > > On Mon, Oct 23, 2017 at 12:25:45PM -0700, Yongseok Koh wrote:
> > > > On Mon, Oct 23, 2017 at 04:49:56PM +0200, Nelio Laranjeiro wrote:
> > > > > @@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
> [...]
> > I've wrongly read your last comment, the patch is correct, it won't add
> > multiple time the broadcast multicast, it will add one per expected
> > VLAN.
> >
> > Example:
> >
> > testpmd> set promisc all off
> > testpmd> set allmulti all off
> > testpmd> rx_vlan add 0 1330
> > testpmd> rx_vlan add 0 1331
> >
> > Will cause this code to add a broadcast flow with VLAN TCI 1330 and
> > another broadcast flow with VLAN TCI 1331, others won't be received.
> >
> > The user will only receive broadcast packets with VLAN TCI 1330 and
> > 1331. It is what he expects.
>
> What I meant was, if there are multiple MAC addresses on a port, the bcast/mcast
> flows will be repeated. For example, if there are 3 valid addrs in
> dev->data->mac_addrs
>
> testpmd> mac_addr add 0 <addr1>
> testpmd> mac_addr add 0 <addr2>
> testpmd> mac_addr add 0 <addr3>
>
> and 2 vlan filters are configured like your example above, then 6 ucast flows
> (2 per an addr) will be added along with 6 bcast flows and 6 mcast flows. But it
> only needs 2 bcast flows and 2 mcast flows - one per vlan.
Right, I missed this case.
I'll fix it in a v3.
Thanks,
@@ -251,6 +251,29 @@ mlx5_dev_stop(struct rte_eth_dev *dev)
int
priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
{
+ struct rte_flow_item_eth bcast = {
+ .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
+ };
+ struct rte_flow_item_eth ipv6_multi_spec = {
+ .dst.addr_bytes = "\x33\x33\x00\x00\x00\x00",
+ };
+ struct rte_flow_item_eth ipv6_multi_mask = {
+ .dst.addr_bytes = "\xff\xff\x00\x00\x00\x00",
+ };
+ struct rte_flow_item_eth unicast = {
+ .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
+ };
+ struct rte_flow_item_eth unicast_mask = {
+ .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
+ };
+ const unsigned int vlan_filter_n = priv->vlan_filter_n;
+ const struct ether_addr cmp = {
+ .addr_bytes = "\x00\x00\x00\x00\x00\x00",
+ };
+ unsigned int i;
+ unsigned int j;
+ int ret;
+
if (priv->isolated)
return 0;
if (dev->data->promiscuous) {
@@ -261,75 +284,59 @@ priv_dev_traffic_enable(struct priv *priv, struct rte_eth_dev *dev)
};
claim_zero(mlx5_ctrl_flow(dev, &promisc, &promisc));
- } else if (dev->data->all_multicast) {
+ 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 = "\x01\x00\x00\x00\x00\x00",
+ .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
.type = 0,
};
claim_zero(mlx5_ctrl_flow(dev, &multicast, &multicast));
- } else {
- struct rte_flow_item_eth bcast = {
- .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
- };
- struct rte_flow_item_eth ipv6_multi_spec = {
- .dst.addr_bytes = "\x33\x33\x00\x00\x00\x00",
- };
- struct rte_flow_item_eth ipv6_multi_mask = {
- .dst.addr_bytes = "\xff\xff\x00\x00\x00\x00",
- };
- struct rte_flow_item_eth unicast = {
- .src.addr_bytes = "\x00\x00\x00\x00\x00\x00",
- };
- struct rte_flow_item_eth unicast_mask = {
- .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff",
- };
- const unsigned int vlan_filter_n = priv->vlan_filter_n;
- const struct ether_addr cmp = {
- .addr_bytes = "\x00\x00\x00\x00\x00\x00",
- };
- unsigned int i;
- unsigned int j;
- unsigned int unicast_flow = 0;
- int ret;
-
- for (i = 0; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
- struct ether_addr *mac = &dev->data->mac_addrs[i];
+ }
+ for (i = 0; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
+ struct ether_addr *mac = &dev->data->mac_addrs[i];
- if (!memcmp(mac, &cmp, sizeof(*mac)))
- continue;
- memcpy(&unicast.dst.addr_bytes,
- mac->addr_bytes,
- ETHER_ADDR_LEN);
- for (j = 0; j != vlan_filter_n; ++j) {
- uint16_t vlan = priv->vlan_filter[j];
+ if (!memcmp(mac, &cmp, sizeof(*mac)))
+ continue;
+ memcpy(&unicast.dst.addr_bytes,
+ mac->addr_bytes,
+ ETHER_ADDR_LEN);
+ for (j = 0; j != vlan_filter_n; ++j) {
+ uint16_t vlan = priv->vlan_filter[j];
- struct rte_flow_item_vlan vlan_spec = {
- .tci = rte_cpu_to_be_16(vlan),
- };
- struct rte_flow_item_vlan vlan_mask = {
- .tci = 0xffff,
- };
+ struct rte_flow_item_vlan vlan_spec = {
+ .tci = rte_cpu_to_be_16(vlan),
+ };
+ struct rte_flow_item_vlan vlan_mask = {
+ .tci = 0xffff,
+ };
- ret = mlx5_ctrl_flow_vlan(dev, &unicast,
- &unicast_mask,
- &vlan_spec,
- &vlan_mask);
- if (ret)
- goto error;
- unicast_flow = 1;
- }
- if (!vlan_filter_n) {
- ret = mlx5_ctrl_flow(dev, &unicast,
- &unicast_mask);
- if (ret)
- goto error;
- unicast_flow = 1;
- }
+ ret = mlx5_ctrl_flow_vlan(dev, &unicast,
+ &unicast_mask,
+ &vlan_spec,
+ &vlan_mask);
+ if (ret)
+ goto error;
+ ret = mlx5_ctrl_flow_vlan(dev, &bcast, &bcast,
+ &vlan_spec, &vlan_mask);
+ if (ret)
+ goto error;
+ ret = mlx5_ctrl_flow_vlan(dev, &ipv6_multi_spec,
+ &ipv6_multi_mask,
+ &vlan_spec, &vlan_mask);
+ if (ret)
+ goto error;
}
- if (!unicast_flow)
- return 0;
+ if (!vlan_filter_n) {
+ ret = mlx5_ctrl_flow(dev, &unicast,
+ &unicast_mask);
+ if (ret)
+ goto error;
+ }
+ }
+ if (!dev->data->all_multicast && !vlan_filter_n) {
ret = mlx5_ctrl_flow(dev, &bcast, &bcast);
if (ret)
goto error;