Re: [dpdk-dev] [PATCH v3 1/3] net/mlx5: use Netlink to add/remove MAC addresses

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

On Thu, Mar 22, 2018 at 07:34:50AM +0000, Shahaf Shuler wrote:
> Hi Nelio,
> 
> Wednesday, March 21, 2018 3:40 PM, Nelio Laranjeiro:
> > VF devices are not able to receive traffic unless it fully requests it though
> > Netlink.  This will cause the request to be processed by the PF which will
> > add/remove the MAC address to the VF table if the VF is trusted.
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
> >  drivers/net/mlx5/Makefile      |   1 +
> >  drivers/net/mlx5/mlx5.c        |  22 ++
> >  drivers/net/mlx5/mlx5.h        |  13 +
> >  drivers/net/mlx5/mlx5_ethdev.c |  27 +++
> >  drivers/net/mlx5/mlx5_mac.c    |  25 +-
> >  drivers/net/mlx5/mlx5_nl.c     | 530
> > +++++++++++++++++++++++++++++++++++++++++
> > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > faacfd9d6..6a7e9f310 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -78,6 +78,7 @@ struct mlx5_dev_config {
> >  	unsigned int hw_vlan_strip:1; /* VLAN stripping is supported. */
> >  	unsigned int hw_fcs_strip:1; /* FCS stripping is supported. */
> >  	unsigned int hw_padding:1; /* End alignment padding is supported.
> > */
> > +	unsigned int vf:1; /* This is a VF. */
> >  	unsigned int mps:2; /* Multi-packet send supported mode. */
> >  	unsigned int tunnel_en:1;
> >  	/* Whether tunnel stateless offloads are supported. */ @@ -154,6
> > +155,8 @@ struct priv {
> >  	struct mlx5_dev_config config; /* Device configuration. */
> >  	struct mlx5_verbs_alloc_ctx verbs_alloc_ctx;
> >  	/* Context for Verbs allocator. */
> > +	int nl_socket; /* Netlink socket. */
> > +	uint32_t nl_sn; /* Netlink message sequence number. */
> >  };
> > 
> >  /* mlx5.c */
> > @@ -163,6 +166,7 @@ int mlx5_getenv_int(const char *);
> >  /* mlx5_ethdev.c */
> > 
> >  int mlx5_get_ifname(const struct rte_eth_dev *dev, char
> > (*ifname)[IF_NAMESIZE]);
> > +int mlx5_ifindex(const struct rte_eth_dev *dev);
> >  int mlx5_ifreq(const struct rte_eth_dev *dev, int req, struct ifreq *ifr);  int
> > mlx5_get_mtu(struct rte_eth_dev *dev, uint16_t *mtu);  int
> > mlx5_set_flags(struct rte_eth_dev *dev, unsigned int keep, @@ -297,4
> > +301,13 @@ struct mlx5_mr *mlx5_mr_get(struct rte_eth_dev *dev, struct
> > rte_mempool *mp);  int mlx5_mr_release(struct mlx5_mr *mr);  int
> > mlx5_mr_verify(struct rte_eth_dev *dev);
> > 
> > +/* mlx5_nl.c */
> > +
> > +int mlx5_nl_init(uint32_t nlgroups);
> > +int mlx5_nl_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr
> > +*mac); int mlx5_nl_mac_addr_remove(struct rte_eth_dev *dev, struct
> > +ether_addr *mac); void mlx5_nl_mac_addr_flush(struct rte_eth_dev
> > *dev);
> 
> I think the two below should be introduced only on the next patch of the series. 

Right,

> 
> > +int mlx5_nl_promisc(struct rte_eth_dev *dev, int enable); int
> > +mlx5_nl_allmulti(struct rte_eth_dev *dev, int enable);
> > +
> >  #endif /* RTE_PMD_MLX5_H_ */
> 
> [...]
> 
> > +/**
> > + * Flush all added MAC addresses.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device.
> > + */
> > +void
> > +mlx5_nl_mac_addr_flush(struct rte_eth_dev *dev) {
> > +	int i;
> > +	const struct ether_addr mac_null = { .addr_bytes = { 0 }, };
> > +
> > +	for (i = MLX5_MAX_MAC_ADDRESSES - 1; i >= 0; --i) {
> > +		struct ether_addr *m = &dev->data->mac_addrs[i];
> > +
> > +		if (memcmp(&mac_null, m, ETHER_ADDR_LEN))
> > +			mlx5_nl_mac_addr_remove(dev, m);
> > +	}
> > +}
> 
> What if the DPDK process is terminated ungracefully? I think the MAC
> table will remain with all the MACs which were added. 
> The next run of the process may have un-expected results.
> 
> Should we flush the neighbor mac table also on probe to make sure only
> the VF mac exists? 

In such situation the sysadmin should make the clean up, the DPDK
application cannot consider it is the only one using the device as it is
not the case, Linux still owns the device.
We have no guarantee the admin did not use another MAC address for a
service outside of the DPDK application (even if in such case he should
disable this feature to fully control what happens on the neighbor mac
table).

Thanks,

-- 
Nélio Laranjeiro
6WIND