Skip to contentSkip to conversation

ip_gre: fix percpu stats accounting

PATCH Applied Pavel Emelyanov 3 participants 11 messages Oct 28, 2010 – Oct 30, 2010

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

PATCH 1/1 ip_gre: fix percpu stats accounting

commit e985aad7 (ip_gre: percpu stats accounting) forgot the fallback tunnel case (gre0).

This is the 4th part of the "foo: fix percpu stats accounting" series ;)

Signed-off-by Pavel Emelyanov <[email protected]>

---

net/ipv4/ip_gre.c +11 -3
index 01087e0..be05d5b 100644
@@ -1321,7 +1321,7 @@ static int ipgre_tunnel_init(struct net_device *dev)
 	return 0;
 }
 
-static void ipgre_fb_tunnel_init(struct net_device *dev)
+static int ipgre_fb_tunnel_init(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	struct iphdr *iph = &tunnel->parms.iph;

@@ -1335,8 +1335,13 @@ static void ipgre_fb_tunnel_init(struct net_device *dev)
 	iph->ihl		= 5;
 	tunnel->hlen		= sizeof(struct iphdr) + 4;
 
+	dev->tstats = alloc_percpu(struct pcpu_tstats);
+	if (!dev->tstats)
+		return -ENOMEM;
+
 	dev_hold(dev);
 	rcu_assign_pointer(ign->tunnels_wc[0], tunnel);
+	return 0;
 }
 
 

@@ -1377,7 +1382,10 @@ static int __net_init ipgre_init_net(struct net *net)
 	}
 	dev_net_set(ign->fb_tunnel_dev, net);
 
-	ipgre_fb_tunnel_init(ign->fb_tunnel_dev);
+	err = ipgre_fb_tunnel_init(ign->fb_tunnel_dev);
+	if (err)
+		goto err_reg_dev;
+
 	ign->fb_tunnel_dev->rtnl_link_ops = &ipgre_link_ops;
 
 	if ((err = register_netdev(ign->fb_tunnel_dev)))

@@ -1386,7 +1394,7 @@ static int __net_init ipgre_init_net(struct net *net)
 	return 0;
 
 err_reg_dev:
-	free_netdev(ign->fb_tunnel_dev);
+	ipgre_dev_free(ign->fb_tunnel_dev);
 err_alloc_dev:
 	return err;
 }
#3 lore raw
Quoted 9 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Thu, 28 Oct 2010 18:33:54 +0200

Quoted 6 lines · Pavel Emelyanov
Quoted 5 lines

commit e985aad7 (ip_gre: percpu stats accounting) forgot the fallback tunnel case (gre0).

This is the 4th part of the "foo: fix percpu stats accounting" series ;)

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Signed-off-by Pavel Emelyanov <[email protected]>

Indeed, right you are ;)

Acked-by Eric Dumazet <[email protected]>

Applied.

Hmm, actually patch is buggy, sorry...

maintainer
#4 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 28 Oct 2010 20:38:58 +0200

Quoted 13 lines · David Miller
Quoted 12 lines

From: Eric Dumazet <[email protected]>
Date: Thu, 28 Oct 2010 18:33:54 +0200

Quoted 9 lines · Pavel Emelyanov
Quoted 7 lines

commit e985aad7 (ip_gre: percpu stats accounting) forgot the fallback tunnel case (gre0).

This is the 4th part of the "foo: fix percpu stats accounting" series ;)

Signed-off-by Pavel Emelyanov <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Indeed, right you are ;)

Acked-by Eric Dumazet <[email protected]>

Applied.

Hmm, actually patch is buggy, sorry...

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

#5 lore raw
Quoted 3 lines · David Miller

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

Cool, I'll provide a patch in a couple of minutes, when tested.

Thanks

maintainer
#6 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 28 Oct 2010 20:47:59 +0200

Quoted 6 lines · David Miller
Quoted 3 lines

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

Cool, I'll provide a patch in a couple of minutes, when tested.

Thanks.

#7 lore raw

PATCH 1/1 ip_gre: fix fallback tunnel setup

In reply to Eric Dumazet
Quoted 7 lines · Eric Dumazet
Quoted 3 lines · David Miller

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

Cool, I'll provide a patch in a couple of minutes, when tested.

I believe the right fix is this one, Pavel what do you think ?

With your patch, we allocate the per_cpu data twice for the fallback tunnel, thus leaking memory.

Thanks

Note: free_percpu(NULL) is legal

[PATCH] ip_gre: fix fallback tunnel setup

Before making the fallback tunnel visible to lookups, we should make sure it is completely setup, once ipgre_tunnel_init() had been called and tstats per_cpu pointer allocated.

move rcu_assign_pointer(ign->tunnels_wc[0], tunnel); from ipgre_fb_tunnel_init() to ipgre_init_net()

Based on a patch from Pavel Emelyanov

Reported-by Pavel Emelyanov <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>
---
 net/ipv4/ip_gre.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
net/ipv4/ip_gre.c +3 -3
index 01087e0..70ff77f 100644
@@ -1325,7 +1325,6 @@ static void ipgre_fb_tunnel_init(struct net_device *dev)
 {
 	struct ip_tunnel *tunnel = netdev_priv(dev);
 	struct iphdr *iph = &tunnel->parms.iph;
-	struct ipgre_net *ign = net_generic(dev_net(dev), ipgre_net_id);
 
 	tunnel->dev = dev;
 	strcpy(tunnel->parms.name, dev->name);

@@ -1336,7 +1335,6 @@ static void ipgre_fb_tunnel_init(struct net_device *dev)
 	tunnel->hlen		= sizeof(struct iphdr) + 4;
 
 	dev_hold(dev);
-	rcu_assign_pointer(ign->tunnels_wc[0], tunnel);
 }
 
 

@@ -1383,10 +1381,12 @@ static int __net_init ipgre_init_net(struct net *net)
 	if ((err = register_netdev(ign->fb_tunnel_dev)))
 		goto err_reg_dev;
 
+	rcu_assign_pointer(ign->tunnels_wc[0],
+			   netdev_priv(ign->fb_tunnel_dev));
 	return 0;
 
 err_reg_dev:
-	free_netdev(ign->fb_tunnel_dev);
+	ipgre_dev_free(ign->fb_tunnel_dev);
 err_alloc_dev:
 	return err;
 }
author Acked-by on v1 1/1
#8 lore raw

Re: [PATCH] ip_gre: fix fallback tunnel setup

Quoted 10 lines · Eric Dumazet
Quoted 7 lines · Eric Dumazet
Quoted 3 lines · David Miller

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

Cool, I'll provide a patch in a couple of minutes, when tested.

I believe the right fix is this one, Pavel what do you think ?

Indeed. I missed the fact, that the gre driver uses ndo_init for all devices including the fb one :(

Acked-by Pavel Emelyanov <[email protected]>
Quoted 2 lines

With your patch, we allocate the per_cpu data twice for the fallback tunnel, thus leaking memory.

Yup...

Quoted 1 line

Thanks

#9 lore raw

Re: [PATCH] ip_gre: fix fallback tunnel setup

Quoted 4 lines · Pavel Emelyanov

Indeed. I missed the fact, that the gre driver uses ndo_init for all devices including the fb one :(

Acked-by Pavel Emelyanov <[email protected]>

Well, I discovered this right now, it was not that obvious, maybe we should use similar setup for all fb tunnels as well...

Thanks !

maintainer Applied
#10 lore raw

Re: [PATCH] ip_gre: fix fallback tunnel setup

In reply to Pavel Emelyanov

From: Pavel Emelyanov <[email protected]>
Date: Thu, 28 Oct 2010 23:29:10 +0400

Quoted 10 lines · Eric Dumazet
Quoted 9 lines · Eric Dumazet
Quoted 7 lines · David Miller
Quoted 3 lines

I am still able to revert this I think without screwing up publicly visible history, so I will double check and do the revert if I can.

Cool, I'll provide a patch in a couple of minutes, when tested.

Quoted 6 lines
Quoted 1 line

I believe the right fix is this one, Pavel what do you think ?

Indeed. I missed the fact, that the gre driver uses ndo_init for all devices including the fb one :(

Acked-by Pavel Emelyanov <[email protected]>

Applied.