RE: [PATCH V2,net-next, 3/3] hv_netvsc: Name NICs based on vmbus offer sequence and use async probe

[Haiyang Zhang]

> -----Original Message-----
> From: Roman Kagan <rkagan@xxxxxxxxxxxxx>
> Sent: Tuesday, December 31, 2019 6:35 AM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Cc: sashal@xxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> KY Srinivasan <kys@xxxxxxxxxxxxx>; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>; olaf@xxxxxxxxx; vkuznets
> <vkuznets@xxxxxxxxxx>; davem@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2,net-next, 3/3] hv_netvsc: Name NICs based on vmbus
> offer sequence and use async probe
> 
> On Mon, Dec 30, 2019 at 12:13:34PM -0800, Haiyang Zhang wrote:
> > The dev_num field in vmbus channel structure is assigned to the first
> > available number when the channel is offered. So netvsc driver uses it
> > for NIC naming based on channel offer sequence. Now re-enable the
> > async probing mode for faster probing.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > ---
> >  drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c index f3f9eb8..39c412f 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -2267,10 +2267,14 @@ static int netvsc_probe(struct hv_device *dev,
> >  	struct net_device_context *net_device_ctx;
> >  	struct netvsc_device_info *device_info = NULL;
> >  	struct netvsc_device *nvdev;
> > +	char name[IFNAMSIZ];
> >  	int ret = -ENOMEM;
> >
> > -	net = alloc_etherdev_mq(sizeof(struct net_device_context),
> > -				VRSS_CHANNEL_MAX);
> > +	snprintf(name, IFNAMSIZ, "eth%d", dev->channel->dev_num);
> 
> How is this supposed to work when there are other ethernet device types on the
> system, which may claim the same device names?
> 
> > +	net = alloc_netdev_mqs(sizeof(struct net_device_context), name,
> > +			       NET_NAME_ENUM, ether_setup,
> > +			       VRSS_CHANNEL_MAX, VRSS_CHANNEL_MAX);
> > +
> >  	if (!net)
> >  		goto no_net;
> >
> > @@ -2355,6 +2359,14 @@ static int netvsc_probe(struct hv_device *dev,
> >  		net->max_mtu = ETH_DATA_LEN;
> >
> >  	ret = register_netdevice(net);
> > +
> > +	if (ret == -EEXIST) {
> > +		pr_info("NIC name %s exists, request another name.\n",
> > +			net->name);
> > +		strlcpy(net->name, "eth%d", IFNAMSIZ);
> > +		ret = register_netdevice(net);
> > +	}
> 
> IOW you want the device naming to be predictable, but don't guarantee this?
> 
> I think the problem this patchset is trying to solve is much better solved with a
> udev rule, similar to how it's done for PCI net devices.
> And IMO the primary channel number, being a device's "hardware"
> property, is more suited to be used in the device name, than this completely
> ephemeral device number.

The vmbus number can be affected by other types of devices and/or subchannel
offerings. They are not stable either. That's why this patch set keeps track of the 
offering sequence within the same device type in a new variable "dev_num".

As in my earlier email, to avoid impact by other types of NICs, we should put them
into different naming formats, like "vf*", "enP*", etc. And yes, these can be done in
udev.

But for netvsc (synthetic) NICs, we still want the default naming format "eth*". And
the variable "dev_num" gives them the basis for stable naming with Async probing.

Thanks,
- Haiyang