Re: [PATCH 7/7] net/mlx5: add parameter for port representors

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Tue, Jun 12, 2018 at 08:02:17AM +0000, Xueming(Steven) Li wrote:
> Hi Adrien,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Adrien Mazarguil
> > Sent: Saturday, May 26, 2018 12:35 AM
> > To: Shahaf Shuler <shahafs@mellanox.com>
> > Cc: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH 7/7] net/mlx5: add parameter for port representors
> > 
> > Prior to this patch, all port representors detected on a given device were probed and Ethernet devices
> > instantiated for each of them.
> > 
> > This patch adds support for the standard "representor" parameter, which implies that port representors
> > are not probed by default anymore, except for the list provided through device arguments.
> > 
> > (Patch based on prior work from Yuanhan Liu)
> > 
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
<snip>
> > @@ -1142,13 +1149,30 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
> >  	struct rte_eth_dev **eth_list = NULL;
> >  	struct ibv_context *ctx;
> >  	struct ibv_device_attr_ex attr;
> > +	struct rte_eth_devargs eth_da;
> 
> Not related to this patch, from this data structure, maximum representor count is 32, 
> customer might use VF on container environment, 32 is far from requirement. We need
> additional work here. A workaround is that users call this api multiple times with different
> representor IDs.

32 ought to be enough for anybody!

Not sure I understand your concern actually. One can't instantiate more
representors than there are DPDK ports because the limit for both is
RTE_MAX_ETHPORTS (i.e. 1 representor = 1 DPDK port). Users who want to spawn
more than 32 DPDK ports overall must increase RTE_MAX_ETHPORTS regardless.

> >  	void *tmp;
> >  	unsigned int i;
> >  	unsigned int j = 0;
> >  	unsigned int n = 0;
> >  	int ret;
> > 
> > +	if (dpdk_dev->devargs) {
> > +		ret = rte_eth_devargs_parse(dpdk_dev->devargs->args, &eth_da);
> > +		if (ret)
> > +			goto error;
> > +	} else {
> > +		memset(&eth_da, 0, sizeof(eth_da));
> > +	}
> >  next:
> > +	if (j) {
> > +		unsigned int k;
> > +
> > +		for (k = 0; k < eth_da.nb_representor_ports; ++k)
> > +			if (eth_da.representor_ports[k] == j - 1)
> > +				break;
> > +		if (k == eth_da.nb_representor_ports)
> > +			goto skip;
> > +	}
> >  	errno = 0;
> >  	ctx = mlx5_glue->open_device(ibv_dev[j]);
> 
> Need a range check for j here.

I think it's properly checked. j == 0 stands for "master device", always
found at index 0 and probed. Representors devices, if any, start at index 1
which triggers the previous block. This block makes sure that a given
representor is indeed enabled before either spawning the related device
(pass through with a valid "j") or skipping it altogether (goto skip).

I intend to leave this patch as is for v2.

-- 
Adrien Mazarguil
6WIND