ipv4: release dev refcnt early when destroying inetdev

RFC PATCH In review Nicolas Dichtel 3 participants 8 messages Sep 10, 2010 – Sep 15, 2010

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Nicolas Dichtel

author

2010-09-10 13:35 UTC #0 lore raw

RFC PATCH 1/1 ipv4: release dev refcnt early when destroying inetdev

Hi all,

We got a scalability problem when we try to remove a lot of virtual interfaces. After analysis, we found that a refcnt on a device was released too late. Here is a proposal patch. If we are not missing something, the refcnt can be release before call_rcu(). In IPv6, this is already the case.

Comments are welcome.

Regards,
Nicolas

Quoted 1 line

From 6fe291ff56b1f94599dfaa57dfb0ed4c168b603f Mon Sep 17 00:00:00 2001

From: Nicolas Dichtel <[email protected]>
Date: Fri, 10 Sep 2010 14:52:15 +0200
Subject: [PATCH] ipv4: release dev refcnt early when destroying inetdev

When a virtual device is removed, refcnt on dev is released after rcu barrier, hence we fall always in the msleep(250) of netdev_wait_allrefs(). This causes a long delay when a lot of interfaces are removed.
Refcnt can be released before this rcu barrier, this allows to accelerate the removing of virtual interfaces.

Test of removing 50 ipip tunnel interfaces:
Before the patch:

  real    0m12.804s
  user    0m0.020s
  sys     0m0.000s

After the patch:

  real    0m0.988s
  user    0m0.004s
  sys     0m0.016s

Signed-off-by Wang Xuefu <[email protected]>

Signed-off-by Nicolas Dichtel <[email protected]>

---
 net/ipv4/devinet.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

net/ipv4/devinet.c +3 -1

index da14c49..dd59e79 100644
@@ -131,7 +131,9 @@ static inline void inet_free_ifa(struct in_ifaddr *ifa)
 
 void in_dev_finish_destroy(struct in_device *idev)
 {
+#ifdef NET_REFCNT_DEBUG
 	struct net_device *dev = idev->dev;
+#endif
 
 	WARN_ON(idev->ifa_list);
 	WARN_ON(idev->mc_list);

@@ -139,7 +141,6 @@ void in_dev_finish_destroy(struct in_device *idev)
 	printk(KERN_DEBUG "in_dev_finish_destroy: %p=%s\n",
 	       idev, dev ? dev->name : "NIL");
 #endif
-	dev_put(dev);
 	if (!idev->dead)
 		pr_err("Freeing alive in_device %p\n", idev);
 	else

@@ -215,6 +216,7 @@ static void inetdev_destroy(struct in_device *in_dev)
 	neigh_parms_release(&arp_tbl, in_dev->arp_parms);
 	arp_ifdown(dev);
 
+	dev_put(dev);
 	call_rcu(&in_dev->rcu_head, in_dev_rcu_put);
 }

Signature · 1 line

1.5.4.5

Eric Dumazet

2010-09-10 14:24 UTC #1 lore raw

Quoted 32 lines · Nicolas Dichtel

Hi all,
We got a scalability problem when we try to remove a lot of virtual interfaces. After analysis, we found that a refcnt on a device was released too late. Here is a proposal patch. If we are not missing something, the refcnt can be release before call_rcu(). In IPv6, this is already the case.
Comments are welcome.
Regards,
Nicolas
pièce jointe différences entre fichiers (0001-ipv4-release-dev-refcnt-early-when-destroying-inetd.patch) From 6fe291ff56b1f94599dfaa57dfb0ed4c168b603f Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <[email protected]>
Date: Fri, 10 Sep 2010 14:52:15 +0200
Subject: [PATCH] ipv4: release dev refcnt early when destroying inetdev
When a virtual device is removed, refcnt on dev is released after rcu barrier, hence we fall always in the msleep(250) of netdev_wait_allrefs(). This causes a long delay when a lot of interfaces are removed.
Refcnt can be released before this rcu barrier, this allows to accelerate the removing of virtual interfaces.
Test of removing 50 ipip tunnel interfaces:
Before the patch:
  real    0m12.804s
  user    0m0.020s
  sys     0m0.000s

Quoted 8 lines

After the patch:
  real    0m0.988s
  user    0m0.004s
  sys     0m0.016s
Signed-off-by Wang Xuefu <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---

This is a well known problem, (many patches were sent some months ago) but your patch is not the right solution.

As long as the idev is not yet freed, it can be used and we need to access idev->dev

Nicolas Dichtel

author

2010-09-10 14:57 UTC #2 lore raw

Quoted 40 lines · Eric Dumazet

Quoted 30 lines · Nicolas Dichtel
Hi all,
We got a scalability problem when we try to remove a lot of virtual interfaces. After analysis, we found that a refcnt on a device was released too late. Here is a proposal patch. If we are not missing something, the refcnt can be release before call_rcu(). In IPv6, this is already the case.
Comments are welcome.
Regards,
Nicolas
pièce jointe différences entre fichiers (0001-ipv4-release-dev-refcnt-early-when-destroying-inetd.patch) From 6fe291ff56b1f94599dfaa57dfb0ed4c168b603f Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <[email protected]>
Date: Fri, 10 Sep 2010 14:52:15 +0200
Subject: [PATCH] ipv4: release dev refcnt early when destroying inetdev
When a virtual device is removed, refcnt on dev is released after rcu barrier, hence we fall always in the msleep(250) of netdev_wait_allrefs(). This causes a long delay when a lot of interfaces are removed.
Refcnt can be released before this rcu barrier, this allows to accelerate the removing of virtual interfaces.
Test of removing 50 ipip tunnel interfaces:
Before the patch:
  real    0m12.804s
  user    0m0.020s
  sys     0m0.000s
Quoted 8 lines
After the patch:
  real    0m0.988s
  user    0m0.004s
  sys     0m0.016s
Signed-off-by Wang Xuefu <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---

Quoted 5 lines

This is a well known problem, (many patches were sent some months ago) but your patch is not the right solution.
As long as the idev is not yet freed, it can be used and we need to access idev->dev

Is this not true in IPv6? What is the difference?

Regards,
Nicolas

Eric Dumazet

2010-09-10 15:16 UTC #3 lore raw

Quoted 1 line · Nicolas Dichtel

Is this not true in IPv6? What is the difference?

It might be a bug on ipv6, who knows ?

Releasing a reference count, but not setting idev->dev to NULL is a sign something is wrong...

net/ipv6/addrconf.c +1 -0

index 5bc893e..26e39bc 100644
@@ -330,6 +330,7 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 	printk(KERN_DEBUG "in6_dev_finish_destroy: %s\n", dev ? dev->name : "NIL");
 #endif
 	dev_put(dev);
+	idev->dev = NULL;
 	if (!idev->dead) {
 		pr_warning("Freeing alive inet6 device %p\n", idev);
 		return;

Eric Dumazet

2010-09-13 22:24 UTC #6 lore raw

PATCH 1/1 net: use rcu_barrier() in rollback_registered_many

In reply to Eric Dumazet

Quoted 42 lines · Eric Dumazet

Quoted 32 lines · Nicolas Dichtel
Hi all,
We got a scalability problem when we try to remove a lot of virtual interfaces. After analysis, we found that a refcnt on a device was released too late. Here is a proposal patch. If we are not missing something, the refcnt can be release before call_rcu(). In IPv6, this is already the case.
Comments are welcome.
Regards,
Nicolas
pièce jointe différences entre fichiers (0001-ipv4-release-dev-refcnt-early-when-destroying-inetd.patch) From 6fe291ff56b1f94599dfaa57dfb0ed4c168b603f Mon Sep 17 00:00:00 2001
From: Nicolas Dichtel <[email protected]>
Date: Fri, 10 Sep 2010 14:52:15 +0200
Subject: [PATCH] ipv4: release dev refcnt early when destroying inetdev
When a virtual device is removed, refcnt on dev is released after rcu barrier, hence we fall always in the msleep(250) of netdev_wait_allrefs(). This causes a long delay when a lot of interfaces are removed.
Refcnt can be released before this rcu barrier, this allows to accelerate the removing of virtual interfaces.
Test of removing 50 ipip tunnel interfaces:
Before the patch:
  real    0m12.804s
  user    0m0.020s
  sys     0m0.000s
Quoted 8 lines
After the patch:
  real    0m0.988s
  user    0m0.004s
  sys     0m0.016s
Signed-off-by Wang Xuefu <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---

Quoted 7 lines

This is a well known problem, (many patches were sent some months ago) but your patch is not the right solution.
As long as the idev is not yet freed, it can be used and we need to access idev->dev

I believe I understood one problem.

In rollback_registered_many(), we call the inetdev_event() (and inetdev_destroy() at line 4844 :

call_netdevice_notifiers(NETDEV_UNREGISTER, dev);

Then, we call synchronize_net() at line 4870

So by the time netdev_wait_allrefs() is called, we should have called in_dev_finish_destroy()

But using synchronize_net() is a bit wrong here :

"It waits until all pre-existing rcu readers have completed."

We have no guarantee all call_rcu() that we posted to dismantle the device completed :

- If number of online cpus is 1, synchronize_net() is a no op
- If our thread migrates to another cpu, synchronize_net() can returns while old callbacks are not yet processed.

We should probably use rcu_barrier() instead, to wait for all outstanding RCU callbacks to complete.

I also believe the order of netdevice notifiers is wrong (we dont set priority), and that we should call fib_netdev_event() _before_ dst_dev_event(). This needs another patch.

Thanks

[PATCH] net: use rcu_barrier() in rollback_registered_many

netdev_wait_allrefs() waits that all references to a device vanishes.

It currently uses a _very_ pessimistic 250 ms delay between each probe. Some users reported that no more than 4 devices can be dismantled per second, this is a pretty serious problem for some setups.

Most of the time, a refcount is about to be released by an RCU callback, that is still in flight because rollback_registered_many() uses a synchronize_rcu() call instead of rcu_barrier(). Problem is visible if number of online cpus is one, because synchronize_rcu() is then a no op.

time to remove 50 ipip tunnels on a UP machine :

before patch : real 11.910s
after patch : real 1.250s

Reported-by Nicolas Dichtel <[email protected]>

Reported-by Octavian Purdila <[email protected]>

Reported-by Benjamin LaHaise <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/dev.c +1 -1

index fc2dc93..6de5a82 100644
@@ -4867,7 +4867,7 @@ static void rollback_registered_many(struct list_head *head)
 	dev = list_first_entry(head, struct net_device, unreg_list);
 	call_netdevice_notifiers(NETDEV_UNREGISTER_BATCH, dev);
 
-	synchronize_net();
+	rcu_barrier();
 
 	list_for_each_entry(dev, head, unreg_list)
 		dev_put(dev);

David Miller

maintainer

2010-09-14 20:45 UTC #4 lore raw

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Fri, 10 Sep 2010 17:16:14 +0200

Quoted 8 lines · Nicolas Dichtel

Quoted 2 lines
Is this not true in IPv6? What is the difference?
It might be a bug on ipv6, who knows ?
Releasing a reference count, but not setting idev->dev to NULL is a sign something is wrong...

If anything this is more of a BUG trap than a true correctness patch, but either way if you want me to apply this please formally submit this with a proper commit message and signoff, thanks!

Quoted 18 lines

net/ipv6/addrconf.c +1 -1
index 5bc893e..26e39bc 100644
@@ -330,6 +330,7 @@ void in6_dev_finish_destroy(struct inet6_dev *idev)
 	printk(KERN_DEBUG "in6_dev_finish_destroy: %s\n", dev ? dev->name : "NIL");
 #endif
 	dev_put(dev);
+	idev->dev = NULL;
 	if (!idev->dead) {
 		pr_warning("Freeing alive inet6 device %p\n", idev);
 		return;
 
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer Applied

2010-09-14 21:27 UTC #7 lore raw

Re: [PATCH] net: use rcu_barrier() in rollback_registered_many

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Tue, 14 Sep 2010 00:24:54 +0200

Quoted 15 lines

[PATCH] net: use rcu_barrier() in rollback_registered_many
netdev_wait_allrefs() waits that all references to a device vanishes.
It currently uses a _very_ pessimistic 250 ms delay between each probe. Some users reported that no more than 4 devices can be dismantled per second, this is a pretty serious problem for some setups.
Most of the time, a refcount is about to be released by an RCU callback, that is still in flight because rollback_registered_many() uses a synchronize_rcu() call instead of rcu_barrier(). Problem is visible if number of online cpus is one, because synchronize_rcu() is then a no op.
time to remove 50 ipip tunnels on a UP machine :

Quoted 7 lines

before patch : real 11.910s
after patch : real 1.250s
Reported-by Nicolas Dichtel <[email protected]>
Reported-by Octavian Purdila <[email protected]>
Reported-by Benjamin LaHaise <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

Applied, thanks Eric.

Eric Dumazet

2010-09-15 06:01 UTC #5 lore raw

In reply to David Miller

Quoted 7 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Fri, 10 Sep 2010 17:16:14 +0200
Quoted 4 lines · Nicolas Dichtel
Quoted 2 lines
Is this not true in IPv6? What is the difference?

Quoted 8 lines

Quoted 4 lines
It might be a bug on ipv6, who knows ?
Releasing a reference count, but not setting idev->dev to NULL is a sign something is wrong...
If anything this is more of a BUG trap than a true correctness patch, but either way if you want me to apply this please formally submit this with a proper commit message and signoff, thanks!

This was not a patch for inclusion, this was to show my point. I suspect this will trigger NULL dereference at some points later...

I do think a correct patch would be to change ipv6 to mimic ipv4 way of course. I'll take a look at this later.

Thanks