[PATCH net-next-2.6] ip_frag:

[PATCH net-next-2.6] ip_frag:

[Eric Dumazet]

When sending fragmentation expiration ICMP V4/V6 messages,
we can avoid touching device refcount, thanks to RCU

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/ip_fragment.c |    6 +++---
 net/ipv6/reassembly.c  |   13 ++++++-------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 575f9bd..93e50d0 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -206,10 +206,10 @@ static void ip_expire(unsigned long arg)
 		struct sk_buff *head = qp->q.fragments;
 
 		/* Send an ICMP "Fragment Reassembly Timeout" message. */
-		if ((head->dev = dev_get_by_index(net, qp->iif)) != NULL) {
+		rcu_read_lock();
+		if ((head->dev = dev_get_by_index_rcu(net, qp->iif)) != NULL)
 			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
-			dev_put(head->dev);
-		}
+		rcu_read_unlock();
 	}
 out:
 	spin_unlock(&qp->q.lock);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index da5bd0e..dce699f 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -208,18 +208,17 @@ static void ip6_frag_expire(unsigned long data)
 	fq_kill(fq);
 
 	net = container_of(fq->q.net, struct net, ipv6.frags);
-	dev = dev_get_by_index(net, fq->iif);
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, fq->iif);
 	if (!dev)
-		goto out;
+		goto out_rcu_unlock;
 
-	rcu_read_lock();
 	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMTIMEOUT);
 	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
-	rcu_read_unlock();
 
 	/* Don't send error if the first segment did not arrive. */
 	if (!(fq->q.last_in & INET_FRAG_FIRST_IN) || !fq->q.fragments)
-		goto out;
+		goto out_rcu_unlock;
 
 	/*
 	   But use as source device on which LAST ARRIVED
@@ -228,9 +227,9 @@ static void ip6_frag_expire(unsigned long data)
 	 */
 	fq->q.fragments->dev = dev;
 	icmpv6_send(fq->q.fragments, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0, dev);
+out_rcu_unlock:
+	rcu_read_unlock();
 out:
-	if (dev)
-		dev_put(dev);
 	spin_unlock(&fq->q.lock);
 	fq_put(fq);
 }

Re: [PATCH net-next-2.6] ip_frag:

[Joe Perches]

On Wed, 2009-11-04 at 22:54 +0100, Eric Dumazet wrote:
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index 575f9bd..93e50d0 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -206,10 +206,10 @@ static void ip_expire(unsigned long arg)
>  		struct sk_buff *head = qp->q.fragments;
>  
>  		/* Send an ICMP "Fragment Reassembly Timeout" message. */
> -		if ((head->dev = dev_get_by_index(net, qp->iif)) != NULL) {
> +		rcu_read_lock();
> +		if ((head->dev = dev_get_by_index_rcu(net, qp->iif)) != NULL)
>  			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
> -			dev_put(head->dev);
> -		}
> +		rcu_read_unlock();
>  	}

Hi Eric.  If you're going to do more of these, could you please
convert them to the more standard kernel style as well?

	var = func(foo);
	if (var) {
		etc...

Re: [PATCH net-next-2.6] ip_frag:

[Eric Dumazet]

Joe Perches a écrit :
 
> Hi Eric.  If you're going to do more of these, could you please
> convert them to the more standard kernel style as well?
> 
> 	var = func(foo);
> 	if (var) {
> 		etc...
> 
> 

Well, rule of thumb is : dont mix cleanups with functional changes :)

But yes, I suspect David wont shout too much if I do one trivial cleanup
on the fly :)

Anyway my patch title was incomplete...

Thanks

[PATCH net-next-2.6] ip_frag: dont touch device refcount

When sending fragmentation expiration ICMP V4/V6 messages,
we can avoid touching device refcount, thanks to RCU

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/ipv4/ip_fragment.c |    7 ++++---
 net/ipv6/reassembly.c  |   13 ++++++-------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
index 575f9bd..b007f8a 100644
--- a/net/ipv4/ip_fragment.c
+++ b/net/ipv4/ip_fragment.c
@@ -206,10 +206,11 @@ static void ip_expire(unsigned long arg)
 		struct sk_buff *head = qp->q.fragments;
 
 		/* Send an ICMP "Fragment Reassembly Timeout" message. */
-		if ((head->dev = dev_get_by_index(net, qp->iif)) != NULL) {
+		rcu_read_lock();
+		head->dev = dev_get_by_index_rcu(net, qp->iif);
+		if (head->dev)
 			icmp_send(head, ICMP_TIME_EXCEEDED, ICMP_EXC_FRAGTIME, 0);
-			dev_put(head->dev);
-		}
+		rcu_read_unlock();
 	}
 out:
 	spin_unlock(&qp->q.lock);
diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
index da5bd0e..dce699f 100644
--- a/net/ipv6/reassembly.c
+++ b/net/ipv6/reassembly.c
@@ -208,18 +208,17 @@ static void ip6_frag_expire(unsigned long data)
 	fq_kill(fq);
 
 	net = container_of(fq->q.net, struct net, ipv6.frags);
-	dev = dev_get_by_index(net, fq->iif);
+	rcu_read_lock();
+	dev = dev_get_by_index_rcu(net, fq->iif);
 	if (!dev)
-		goto out;
+		goto out_rcu_unlock;
 
-	rcu_read_lock();
 	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMTIMEOUT);
 	IP6_INC_STATS_BH(net, __in6_dev_get(dev), IPSTATS_MIB_REASMFAILS);
-	rcu_read_unlock();
 
 	/* Don't send error if the first segment did not arrive. */
 	if (!(fq->q.last_in & INET_FRAG_FIRST_IN) || !fq->q.fragments)
-		goto out;
+		goto out_rcu_unlock;
 
 	/*
 	   But use as source device on which LAST ARRIVED
@@ -228,9 +227,9 @@ static void ip6_frag_expire(unsigned long data)
 	 */
 	fq->q.fragments->dev = dev;
 	icmpv6_send(fq->q.fragments, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0, dev);
+out_rcu_unlock:
+	rcu_read_unlock();
 out:
-	if (dev)
-		dev_put(dev);
 	spin_unlock(&fq->q.lock);
 	fq_put(fq);
 }

Re: [PATCH net-next-2.6] ip_frag:

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 04 Nov 2009 23:11:49 +0100

> [PATCH net-next-2.6] ip_frag: dont touch device refcount
> 
> When sending fragmentation expiration ICMP V4/V6 messages,
> we can avoid touching device refcount, thanks to RCU
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-next-2.6

Although I was worried a little bit initially about the
ipv4 case, as we're assigning the device pointer to
"skb->dev" and I thought it might be troublesome for
that reference to escape.

But that seems ok, because icmp_send() doesn't do anything interesting
with it's 'skb_in' arg other than look at the attached route, the
packet contents, and things like that.

Thanks.