diff mbox

[1/2] udp: cleanup __udp4_lib_mcast_deliver

Message ID 200911052033.21964.lgrijincu@ixiacom.com
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Lucian Adrian Grijincu Nov. 5, 2009, 6:33 p.m. UTC 
__udp4_lib_mcast_deliver always returned 0.
It's caller can return 0 explicitly to make things clearer.

Also, we don't need the spin_lock() on the hslot to free the skb.

Signed-off-by: Lucian Adrian Grijincu <lgrijincu@ixiacom.com>
---
 net/ipv4/udp.c |   57 +++++++++++++++++++++++++++++--------------------------
 1 files changed, 30 insertions(+), 27 deletions(-)

Comments

David Miller Nov. 6, 2009, 8:42 a.m. UTC | #1 
From: Lucian Adrian Grijincu <lgrijincu@ixiacom.com>
Date: Thu, 5 Nov 2009 20:33:21 +0200

> 
> __udp4_lib_mcast_deliver always returned 0.
> It's caller can return 0 explicitly to make things clearer.
> 
> Also, we don't need the spin_lock() on the hslot to free the skb.
> 
> Signed-off-by: Lucian Adrian Grijincu <lgrijincu@ixiacom.com>

I think the current code is much easier to understand than
your version.

Getting rid of the useless return value is fine, but the
new do{}while() loop et al. is less readable to me.

I'm not applying these two patches, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucian Adrian Grijincu Nov. 6, 2009, 2:04 p.m. UTC | #2 
În data de Vin 06 Noi 2009 10:42:15 David Miller a scris:
> Getting rid of the useless return value is fine, but the
> new do{}while() loop et al. is less readable to me.

It's not new.
I just moved it:
previously:
	if(cond) {
		do {}
		 while()
	} else {return}

now:
	if (!cond)
		return;
	do {}
	while {}

Also this moves a consume_skb() from a path protected by spin locks.

As far as I understand it, the spin locks protect the hslot, and freeing the 
skb does not walk/change or interact with the hslot in any way.
diff mbox

Patch

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 4274c1c..425b2d4 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1198,12 +1198,12 @@  drop:
  *	Note: called only from the BH handler context,
  *	so we don't need to lock the hashes.
  */
-static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
+static void __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 				    struct udphdr  *uh,
 				    __be32 saddr, __be32 daddr,
 				    struct udp_table *udptable)
 {
-	struct sock *sk;
+	struct sock *sk, *sknext;
 	struct udp_hslot *hslot = udp_hashslot(udptable, net, ntohs(uh->dest));
 	int dif;
 
@@ -1211,31 +1211,32 @@  static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	sk = sk_nulls_head(&hslot->head);
 	dif = skb->dev->ifindex;
 	sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
-	if (sk) {
-		struct sock *sknext = NULL;
-
-		do {
-			struct sk_buff *skb1 = skb;
-
-			sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
-						   daddr, uh->source, saddr,
-						   dif);
-			if (sknext)
-				skb1 = skb_clone(skb, GFP_ATOMIC);
-
-			if (skb1) {
-				int ret = udp_queue_rcv_skb(sk, skb1);
-				if (ret > 0)
-					/* we should probably re-process instead
-					 * of dropping packets here. */
-					kfree_skb(skb1);
-			}
-			sk = sknext;
-		} while (sknext);
-	} else
+	if (!sk) {
+		spin_unlock(&hslot->lock);
 		consume_skb(skb);
+		return;
+	}
+
+	do {
+		struct sk_buff *skb1 = skb;
+
+		sknext = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
+					   daddr, uh->source, saddr,
+					   dif);
+		if (sknext)
+			skb1 = skb_clone(skb, GFP_ATOMIC);
+
+		if (skb1) {
+			int ret = udp_queue_rcv_skb(sk, skb1);
+			if (ret > 0)
+				/* we should probably re-process instead
+				 * of dropping packets here. */
+				kfree_skb(skb1);
+		}
+		sk = sknext;
+	} while (sknext);
 	spin_unlock(&hslot->lock);
-	return 0;
+	return;
 }
 
 /* Initialize UDP checksum. If exited with zero value (success),
@@ -1314,9 +1315,11 @@  int __udp4_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 	saddr = ip_hdr(skb)->saddr;
 	daddr = ip_hdr(skb)->daddr;
 
-	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST))
-		return __udp4_lib_mcast_deliver(net, skb, uh,
+	if (rt->rt_flags & (RTCF_BROADCAST|RTCF_MULTICAST)) {
+		__udp4_lib_mcast_deliver(net, skb, uh,
 				saddr, daddr, udptable);
+		return 0;
+	}
 
 	sk = __udp4_lib_lookup_skb(skb, uh->source, uh->dest, udptable);