Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop

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

On Mon, Apr 23, 2018 at 05:52:08AM +0000, Shahaf Shuler wrote:
> Hi Nelio,
> 
> Small comments before I merge.
> 
> Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:
> > Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list
> > devop
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5.c      |  2 ++
> >  drivers/net/mlx5/mlx5.h      |  2 ++
> >  drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 68783c3ac..887924d07 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@
> > const struct eth_dev_ops mlx5_dev_ops_isolate = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, diff --git
> > a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 6ad41390a..64f025d22 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
> > *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
> > struct ether_addr *mac,
> >  		      uint32_t index, uint32_t vmdq);  int
> > mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr
> > *mac_addr);
> > +int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +			  struct ether_addr *mc_addr_set, uint32_t
> > nb_mc_addr);
> > 
> >  /* mlx5_rss.c */
> > 
> > diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> > index e3aa03bef..5973742a7 100644
> > --- a/drivers/net/mlx5/mlx5_defs.h
> > +++ b/drivers/net/mlx5/mlx5_defs.h
> > @@ -15,8 +15,11 @@
> > 
> >  /* Maximum number of simultaneous unicast MAC addresses. */  #define
> > MLX5_MAX_UC_MAC_ADDRESSES 128
> > +/* Maximum number of simultaneous Multicast MAC addresses. */ #define
> > +MLX5_MAX_MC_MAC_ADDRESSES 128
> >  /* Maximum number of simultaneous MAC addresses. */ -#define
> > MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
> > +#define MLX5_MAX_MAC_ADDRESSES \
> > +	(MLX5_MAX_UC_MAC_ADDRESSES +
> > MLX5_MAX_MC_MAC_ADDRESSES)
> > 
> >  /* Maximum number of simultaneous VLAN filters. */  #define
> > MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c
> > b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644
> > --- a/drivers/net/mlx5/mlx5_mac.c
> > +++ b/drivers/net/mlx5/mlx5_mac.c
> > @@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
> > ether_addr *mac_addr)
> >  		dev->data->port_id);
> >  	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
> > +
> > +/**
> > + * DPDK callback to set multicast addresses list.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device structure.
> > + * @param mac_addr_set
> > + *   Multicast MAC address pointer array.
> > + * @param nb_mac_addr
> > + *   Number of entries in the array.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +int
> > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > +	uint32_t i;
> > +	int ret;
> > +
> 
> We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start
> operate. 

This verification is done in the sub function.

Considering an application calling such API wants to remove/replace the
old list with new entries.
That this new one can be just an addition or totally different list or
even empty.
This new list can be larger than the amount of MAC addresses the PMD can
support.

There are two possibilities:

1. The list is too large:  the application will enable the all multicast
mode to receive the extra mac addresses it needs.
2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to
clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated 
verification?

Note: if an error happens the new list is not committed yet i.e. the
traffic remains untouched.

> > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > MLX5_MAX_MAC_ADDRESSES; ++i)
> > +		mlx5_internal_mac_addr_remove(dev, i);
> > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > +	while (nb_mc_addr--) {
> 
> Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> + warn if it is not. 

Such verification should be done in the public API i.e. ethdev.

> > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > i++);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	if (!dev->data->promiscuous)
> > +		return mlx5_traffic_restart(dev);
> > +	return 0;
> > +}
> > --
> > 2.17.0

Regards,

-- 
Nélio Laranjeiro
6WIND