Re: [dpdk-dev] [dpdk-dev, 5/7] net/mlx5: add port representor awareness

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

On Tue, Jun 12, 2018 at 01:57:45PM +0000, Xueming(Steven) Li wrote:
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Tuesday, June 12, 2018 9:20 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev,5/7] net/mlx5: add port representor awareness
> > 
> > On Mon, Jun 11, 2018 at 01:05:55PM +0000, Xueming(Steven) Li wrote:
> > > Hi Adrien,
> > >
> > > Couldn't find your original email from inbox anyway, have to start a new thread here.
> > <snip>
> > > > +static int
> > > > +mlx5_cmp_ibv_name(const void *a, const void *b) {
> > > > +	const char *name_a = (*(const struct ibv_device *const *)a)->name;
> > > > +	const char *name_b = (*(const struct ibv_device *const *)b)->name;
> > > > +	size_t i = 0;
> > > > +
> > > > +	while (name_a[i] && name_a[i] == name_b[i])
> > > > +		++i;
> > > > +	return atoi(name_a + i) - atoi(name_b + i);
> > >
> > > Comparing "1" and "10" here will return 0, does this matter?
> > 
> > Sure it does! The whole point of this function is precisely to avoid this kind of issues. I'll fix it
> > for v2, thanks.
> > 
> > <snip>
> > > > +	if (n > 1) {
> > > > +		/*
> > > > +		 * The existence of several matching entries means port
> > > > +		 * representors have been instantiated. No existing Verbs
> > > > +		 * call nor /sys entries can tell them apart at this point.
> > > > +		 *
> > > > +		 * While definitely hackish, assume their names are numbered
> > > > +		 * based on order of creation with master device first,
> > > > +		 * followed by first port representor, followed by the
> > > > +		 * second one and so on.
> > > > +		 */
> > > > +		DRV_LOG(WARNING,
> > > > +			"probing device with port representors involves"
> > > > +			" heuristics with uncertain outcome");
> > > > +		qsort(ibv_match, n, sizeof(*ibv_match), mlx5_cmp_ibv_name);
> > > > +		DRV_LOG(WARNING, "assuming \"%s\" is the master device",
> > > > +			ibv_match[0]->name);
> > > > +		for (ret = 1; ret < n; ++ret)
> > > > +			DRV_LOG(WARNING,
> > > > +				"assuming \"%s\" is port representor #%d",
> > > > +				ibv_match[ret]->name, ret - 1);
> > >
> > > Such dump will appear when attaching each rep port, how about just do
> > > it for PF in DEBUG level?
> > 
> > It occurs only once when probing the master device and detecting the presence of representors, not for
> > each of them.
> > 
> > I prefer to leave it as a warning because this detection approach, while an undeniable improvement
> > over not checking anything and ending up configuring the wrong netdevice, is unfortunately not 100%
> > accurate. This will be improved, however users must be warned of possible issues in the meantime.
> 
> Yes, the list is different when VF number changed outside, a full dump should be helpful, how about set it to DEBUG or INFO level?
> Users don't need to know this, just for debug purpose.

Because by "assuming" things, there's a slight possibility for the PMD to be
wrong. It's not a mere debug message.

Using the wrong IB device may silently wreak havoc on some systems,
therefore since the PMD can't be sure, users are warned about this fact and
what IB devices will be used. This calls for extra attention and manual
checks (where possible) *before* an issue is encountered, until we replace
this piece of code with a safer approach.

I think a WARNING level is warranted.

-- 
Adrien Mazarguil
6WIND