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

[Adrien Mazarguil]

On Tue, Jun 12, 2018 at 08:02:17AM +0000, Xueming(Steven) Li wrote:
> Hi Adrien,
> 
> > -----Original Message-----
> > From: dev <dev-boun...@dpdk.org> On Behalf Of Adrien Mazarguil
> > Sent: Saturday, May 26, 2018 12:35 AM
> > To: Shahaf Shuler <shah...@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.mazarg...@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