From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Samudrala, Sridhar" Subject: Re: [RFC] hv_netvsc: automatically name slave VF network device Date: Tue, 19 Dec 2017 14:44:37 -0800 Message-ID: References: <20171219193537.22587-1-sthemmin@microsoft.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Stephen Hemminger To: Stephen Hemminger , netdev@vger.kernel.org Return-path: Received: from mga02.intel.com ([134.134.136.20]:33203 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753195AbdLSWoi (ORCPT ); Tue, 19 Dec 2017 17:44:38 -0500 In-Reply-To: <20171219193537.22587-1-sthemmin@microsoft.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/19/2017 11:35 AM, Stephen Hemminger wrote: > Rename the VF device to ethX_vf based on the ethX as the > synthetic device. This eliminates the need for delay on setup, > and the PCI (udev based) naming is not reproducible on Hyper-V > anyway. The name of the VF does not matter since all control > operations take place the primary device. It does make the > user experience better to associate the names. > > Based on feedback from all.systems.go talk. > The downside is that it requires exporting a symbol from netdev > core which makes it harder to backport. > > Signed-off-by: Stephen Hemminger > --- > drivers/net/hyperv/hyperv_net.h | 1 - > drivers/net/hyperv/netvsc_drv.c | 53 +++++++++++------------------------------ > net/core/dev.c | 1 + > 3 files changed, 15 insertions(+), 40 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index 0db3bd1ea06f..5013189123e6 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -762,7 +762,6 @@ struct net_device_context { > /* State to manage the associated VF interface. */ > struct net_device __rcu *vf_netdev; > struct netvsc_vf_pcpu_stats __percpu *vf_stats; > - struct delayed_work vf_takeover; > > /* 1: allocated, serial number is valid. 0: not allocated */ > u32 vf_alloc; > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index c5584c2d440e..8a0afe9a5064 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -49,7 +49,6 @@ > #define RING_SIZE_MIN 64 > > #define LINKCHANGE_INT (2 * HZ) > -#define VF_TAKEOVER_INT (HZ / 10) > > static unsigned int ring_size __ro_after_init = 128; > module_param(ring_size, uint, S_IRUGO); > @@ -1760,7 +1759,7 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct sk_buff **pskb) > static int netvsc_vf_join(struct net_device *vf_netdev, > struct net_device *ndev) > { > - struct net_device_context *ndev_ctx = netdev_priv(ndev); > + char vf_name[IFNAMSIZ]; > int ret; > > ret = netdev_rx_handler_register(vf_netdev, > @@ -1783,23 +1782,14 @@ static int netvsc_vf_join(struct net_device *vf_netdev, > /* set slave flag before open to prevent IPv6 addrconf */ > vf_netdev->flags |= IFF_SLAVE; > > - schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT); > - > call_netdevice_notifiers(NETDEV_JOIN, vf_netdev); > > - netdev_info(vf_netdev, "joined to %s\n", ndev->name); > - return 0; > - > -upper_link_failed: > - netdev_rx_handler_unregister(vf_netdev); > -rx_handler_failed: > - return ret; > -} > - > -static void __netvsc_vf_setup(struct net_device *ndev, > - struct net_device *vf_netdev) > -{ > - int ret; > + /* set the name of VF device based on upper device name */ > + snprintf(vf_name, IFNAMSIZ, "%s_vf", ndev->name); > + ret = dev_change_name(vf_netdev, vf_name); > + if (ret != 0) > + netdev_warn(vf_netdev, > + "can not rename device: (%d)\n", ret); It is possible that upper device name can change after this call.  I noticed this when i tried this approach with virtio_net. Also, what should happen if the upper device is unloaded? Should we rename the VF name? > > /* Align MTU of VF with master */ > ret = dev_set_mtu(vf_netdev, ndev->mtu); > @@ -1807,34 +1797,21 @@ static void __netvsc_vf_setup(struct net_device *ndev, > netdev_warn(vf_netdev, > "unable to change mtu to %u\n", ndev->mtu); > > + /* If upper device is already up, bring up slave */ > if (netif_running(ndev)) { > ret = dev_open(vf_netdev); > if (ret) > netdev_warn(vf_netdev, > "unable to open: %d\n", ret); > } > -} > - > -/* Setup VF as slave of the synthetic device. > - * Runs in workqueue to avoid recursion in netlink callbacks. > - */ > -static void netvsc_vf_setup(struct work_struct *w) > -{ > - struct net_device_context *ndev_ctx > - = container_of(w, struct net_device_context, vf_takeover.work); > - struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx); > - struct net_device *vf_netdev; > - > - if (!rtnl_trylock()) { > - schedule_delayed_work(&ndev_ctx->vf_takeover, 0); > - return; > - } > > - vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev); > - if (vf_netdev) > - __netvsc_vf_setup(ndev, vf_netdev); > + netdev_info(vf_netdev, "joined to %s\n", ndev->name); > + return 0; > > - rtnl_unlock(); > + upper_link_failed: > + netdev_rx_handler_unregister(vf_netdev); > + rx_handler_failed: > + return ret; > } > > static int netvsc_register_vf(struct net_device *vf_netdev) > @@ -1904,7 +1881,6 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev) > return NOTIFY_DONE; > > net_device_ctx = netdev_priv(ndev); > - cancel_delayed_work_sync(&net_device_ctx->vf_takeover); > > netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name); > > @@ -1947,7 +1923,6 @@ static int netvsc_probe(struct hv_device *dev, > > spin_lock_init(&net_device_ctx->lock); > INIT_LIST_HEAD(&net_device_ctx->reconfig_events); > - INIT_DELAYED_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup); > > net_device_ctx->vf_stats > = netdev_alloc_pcpu_stats(struct netvsc_vf_pcpu_stats); > diff --git a/net/core/dev.c b/net/core/dev.c > index c7db39926769..f51358a90efa 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -1238,6 +1238,7 @@ int dev_change_name(struct net_device *dev, const char *newname) > > return err; > } > +EXPORT_SYMBOL(dev_change_name); > > /** > * dev_set_alias - change ifalias of a device