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

[Shahaf Shuler <shahafs@mellanox.com>]

Monday, April 23, 2018 12:34 PM, Nélio Laranjeiro:
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add
> list devop
> 
> 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.

OK,
Then I wait for the next version for merge. 

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