Philosophical question: Is a UDP multicast datagram for which there is no socket match a drop or an ignore?

DISCUSSION Rick Jones 2 participants 5 messages Sep 30, 2014 – Oct 1, 2014

Subsystems (3)

Networking Network drivers UDP

lore

Share & tools

lore

Rick Jones

author

2014-09-30 23:09 UTC #0 lore raw

I've been looking at some additional perf <mutter> -e skb_kfree_skb results, this time with a laptop connected to a corporate network with a large number of Windows systems sending out what they are wont to send... The laptop is just sitting there no active netperfs or anything :)

I see profile hits for __udp4_lib_mcast_deliver() which has a kfree_skb() call which will happen if either there were no sockets found, or if an integral multiple of ARRAY_SIZE(stack) sockets are found. I'm assuming the latter is exceedingly rare.

Anywho, the philosophical question - is such a situation a drop (indicating the existing kfree_skb()), or is it an ignore (indicating a consume_skb())? Should there be a statistic incremented for either of those?

happy benchmarking,

rick jones

Eric Dumazet

2014-09-30 23:23 UTC #1 lore raw

Quoted 10 lines · Rick Jones

I've been looking at some additional perf <mutter> -e skb_kfree_skb results, this time with a laptop connected to a corporate network with a large number of Windows systems sending out what they are wont to send... The laptop is just sitting there no active netperfs or anything :)
I see profile hits for __udp4_lib_mcast_deliver() which has a kfree_skb() call which will happen if either there were no sockets found, or if an integral multiple of ARRAY_SIZE(stack) sockets are found. I'm assuming the latter is exceedingly rare.

Quoted 4 lines

Anywho, the philosophical question - is such a situation a drop (indicating the existing kfree_skb()), or is it an ignore (indicating a consume_skb())? Should there be a statistic incremented for either of those?

I guess we lack a UDP_MIB_NOPORTS increase here.

Rick Jones

author

2014-10-01 00:22 UTC #2 lore raw

Quoted 11 lines · Eric Dumazet

Quoted 10 lines · Rick Jones
I've been looking at some additional perf <mutter> -e skb_kfree_skb results, this time with a laptop connected to a corporate network with a large number of Windows systems sending out what they are wont to send... The laptop is just sitting there no active netperfs or anything :)
I see profile hits for __udp4_lib_mcast_deliver() which has a kfree_skb() call which will happen if either there were no sockets found, or if an integral multiple of ARRAY_SIZE(stack) sockets are found. I'm assuming the latter is exceedingly rare.

Quoted 6 lines

Quoted 4 lines
Anywho, the philosophical question - is such a situation a drop (indicating the existing kfree_skb()), or is it an ignore (indicating a consume_skb())? Should there be a statistic incremented for either of those?
I guess we lack a UDP_MIB_NOPORTS increase here.

I was going back and forth on that - since it is a multicast it may not have really been directed at us in which case it would be an ignore (and perhaps a new "ignored" stat?). But on the assumption that it should indeed remain a drop, and so a kfree_skb(), something along the lines of:

net/ipv4/udp.c +0 -0

index cd0db54..376e3d3 100644
@@ -1656,6 +1656,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struc
        int dif = skb->dev->ifindex;
        unsigned int count = 0, offset = offsetof(typeof(*sk),

sk_nulls_node);
unsigned int hash2 = 0, hash2_any = 0, use_hash2 =
(hslot->count > 10);
+ unsigned int inner_flushed = 0;

if (use_hash2) {
hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY),
hnum) &
@@ -1694,8 +1695,12 @@ start_lookup:

         */
        if (count) {
                flush_stack(stack, count, skb, count - 1);
-       } else {
+       } else if (!inner_flushed) {
+               UDP_INC_STATS_BH(net, UDP_MIB_NOPORTS, 0);
                kfree_skb(skb);
+       } else {
+               /* there were matches flushed in the for_each */
+               consume_skb(skb);
        }
        return 0;

}

? The idea being that in the unlikely event there were indeed enough matches to trigger the flush_stack in the for_each and only enough for that it will be a consume_skb() and no statistic rather than a kfree_skb() and a statistic increment.

(likely munged by my mailer)

rick

Eric Dumazet

2014-10-01 00:29 UTC #3 lore raw

Quoted 26 lines · Rick Jones

Quoted 11 lines · Eric Dumazet
Quoted 10 lines · Rick Jones
I've been looking at some additional perf <mutter> -e skb_kfree_skb results, this time with a laptop connected to a corporate network with a large number of Windows systems sending out what they are wont to send... The laptop is just sitting there no active netperfs or anything :)
I see profile hits for __udp4_lib_mcast_deliver() which has a kfree_skb() call which will happen if either there were no sockets found, or if an integral multiple of ARRAY_SIZE(stack) sockets are found. I'm assuming the latter is exceedingly rare.
Quoted 6 lines
Quoted 4 lines
Anywho, the philosophical question - is such a situation a drop (indicating the existing kfree_skb()), or is it an ignore (indicating a consume_skb())? Should there be a statistic incremented for either of those?
I guess we lack a UDP_MIB_NOPORTS increase here.
I was going back and forth on that - since it is a multicast it may not have really been directed at us in which case it would be an ignore (and perhaps a new "ignored" stat?). But on the assumption that it should indeed remain a drop, and so a kfree_skb(), something along the lines of:

Quoted 4 lines

? The idea being that in the unlikely event there were indeed enough matches to trigger the flush_stack in the for_each and only enough for that it will be a consume_skb() and no statistic rather than a kfree_skb() and a statistic increment.

Yes, please submit your patch formally, maybe using a more complete form ? ;)

net/ipv4/udp.c +6 -1

index cd0db5471bb5..be7db86046af 100644
@@ -1656,6 +1656,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	int dif = skb->dev->ifindex;
 	unsigned int count = 0, offset = offsetof(typeof(*sk), sk_nulls_node);
 	unsigned int hash2 = 0, hash2_any = 0, use_hash2 = (hslot->count > 10);
+	bool delivered = false;
 
 	if (use_hash2) {
 		hash2_any = udp4_portaddr_hash(net, htonl(INADDR_ANY), hnum) &

@@ -1674,6 +1675,7 @@ start_lookup:
 					dif, hnum)) {
 			if (unlikely(count == ARRAY_SIZE(stack))) {
 				flush_stack(stack, count, skb, ~0);
+				delivered = true;
 				count = 0;
 			}
 			stack[count++] = sk;

@@ -1694,8 +1696,11 @@ start_lookup:
 	 */
 	if (count) {
 		flush_stack(stack, count, skb, count - 1);
-	} else {
+	} else if (!delivered) {
+		UDP_INC_STATS_BH(net, UDP_MIB_NOPORTS, proto == IPPROTO_UDPLITE);
 		kfree_skb(skb);
+	} else {
+		consume_skb(skb);
 	}
 	return 0;
 }

Rick Jones

author

2014-10-01 00:31 UTC #4 lore raw

Quoted 2 lines · Eric Dumazet

Yes, please submit your patch formally, maybe using a more complete form ? ;)

Will do.

rick