dev: Reclaim network device indexes

Submitted by Serhey Popovych on June 16, 2017, 4:39 p.m.

Details

Message ID 4d4b3b9dd459fc0487c1d25ef23514b9f01e4bb2.1497631012.git.serhe.popovych@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Serhey Popovych June 16, 2017, 4:39 p.m.
While making dev_new_index() return zero on overrun prevents
from infinite loop, there is no way to recovery mechanisms
since namespace ifindex only increases and never reused
from released network devices.

To address this we introduce dev_free_index() helper which
is used to reclaim released network device index when it is
smaller than last allocated index in namespace.

This also has positive side effect for equal distribution
of network devices per buckets in index hash table. That
positively affects performance of dev_get_by_index() family.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 net/core/dev.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

Comments

David Miller June 20, 2017, 4:42 p.m.
From: Serhey Popovych <serhe.popovych@gmail.com>
Date: Fri, 16 Jun 2017 19:39:34 +0300

> While making dev_new_index() return zero on overrun prevents
> from infinite loop, there is no way to recovery mechanisms
> since namespace ifindex only increases and never reused
> from released network devices.
> 
> To address this we introduce dev_free_index() helper which
> is used to reclaim released network device index when it is
> smaller than last allocated index in namespace.
> 
> This also has positive side effect for equal distribution
> of network devices per buckets in index hash table. That
> positively affects performance of dev_get_by_index() family.
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>

You haven't explained why the current behavior is undesirable,
and why we want reuse.

I don't think we want at all for anything to rely on ifindexes
being allocated one way or another.

Yes, I understand that hash table argument, but that can be solved
other ways.  And what you're talking about is more of an error case
not a normal case.

Patch hide | download patch | download mbox

diff --git a/net/core/dev.c b/net/core/dev.c
index 6f573f7..e1714e7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7031,6 +7031,24 @@  static int dev_new_index(struct net *net)
 	}
 }
 
+/**
+ *	dev_free_index	-	free an ifindex
+ *	@dev: the network device whose index to free
+ *
+ *	Sets network namespace last allocated index to the value
+ *	proceed to the ifindex we want to free.  The caller
+ *	must hold the rtnl semaphore or the dev_base_lock to be
+ *	sure it remains unique.
+ */
+static void dev_free_index(struct net_device *dev)
+{
+	struct net *net = dev_net(dev);
+	int ifindex = dev->ifindex;
+
+	if (--ifindex < net->ifindex)
+		net->ifindex = ifindex;
+}
+
 /* Delayed registration/unregisteration */
 static LIST_HEAD(net_todo_list);
 DECLARE_WAIT_QUEUE_HEAD(netdev_unregistering_wq);
@@ -7454,8 +7472,9 @@  void netif_tx_stop_all_queues(struct net_device *dev)
 
 int register_netdevice(struct net_device *dev)
 {
-	int ret;
 	struct net *net = dev_net(dev);
+	int ifindex = 0;
+	int ret;
 
 	BUG_ON(dev_boot_phase);
 	ASSERT_RTNL();
@@ -7494,7 +7513,7 @@  int register_netdevice(struct net_device *dev)
 
 	ret = -EBUSY;
 	if (dev->ifindex <= 0) {
-		int ifindex = dev_new_index(net);
+		ifindex = dev_new_index(net);
 		if (!ifindex)
 			goto err_uninit;
 		dev->ifindex = ifindex;
@@ -7573,10 +7592,9 @@  int register_netdevice(struct net_device *dev)
 	/* Notify protocols, that a new device appeared. */
 	ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
 	ret = notifier_to_errno(ret);
-	if (ret) {
-		rollback_registered(dev);
-		dev->reg_state = NETREG_UNREGISTERED;
-	}
+	if (ret)
+		goto err_rollback;
+
 	/*
 	 *	Prevent userspace races by waiting until the network
 	 *	device is fully setup before sending notifications.
@@ -7593,6 +7611,14 @@  int register_netdevice(struct net_device *dev)
 		dev->netdev_ops->ndo_uninit(dev);
 	if (dev->priv_destructor)
 		dev->priv_destructor(dev);
+	goto err_free_index;
+
+err_rollback:
+	rollback_registered(dev);
+	dev->reg_state = NETREG_UNREGISTERED;
+err_free_index:
+	if (ifindex)
+		dev_free_index(dev);
 	goto out;
 }
 EXPORT_SYMBOL(register_netdevice);
@@ -7805,9 +7831,11 @@  void netdev_run_todo(void)
 		if (dev->needs_free_netdev)
 			free_netdev(dev);
 
-		/* Report a network device has been unregistered */
 		rtnl_lock();
+		/* Report a network device has been unregistered */
 		dev_net(dev)->dev_unreg_count--;
+		/* Reclaim a network device index */
+		dev_free_index(dev);
 		__rtnl_unlock();
 		wake_up(&netdev_unregistering_wq);
 
@@ -8256,6 +8284,9 @@  int dev_change_net_namespace(struct net_device *dev, struct net *net, const char
 	kobject_uevent(&dev->dev.kobj, KOBJ_REMOVE);
 	netdev_adjacent_del_links(dev);
 
+	/* Free network device index */
+	dev_free_index(dev);
+
 	/* Actually switch the network namespace */
 	dev_net_set(dev, net);