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

[Shahaf Shuler]

Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:

[...]

> > > +/**
> > > + * 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.

I see only assert. Did I missed anything? 

> 
> 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.

How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

> 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?

At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault. 

> 
> 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.

I don't understand. 
In the first patch of the series you add extra verification to check the mac address validity. But for the MC you claim it should be done on ethdev layer. 

It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch. 


> 
> > > +		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