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 07:57:36AM +0000, Shahaf Shuler wrote:
> 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? 

No.

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

Such information is not reported at all.  The application has to guess.

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

Right it needs a 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.
> 
> I don't understand. 
> In the first patch of the series you add extra verification to check
> the mac address validity.

It only verify the MAC address is not zero, it does not verify the MAC
address is valid in the function context (e.g. unicast in
mlx5_mac_addr_add()).

> But for the MC you claim it should be done on ethdev layer. 

Dito.

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

It is consistent, the PMD only verify the MAC address is not zero and
this in both API.

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