net: reduce cycles spend on ICMP replies that gets rate limited

PATCH In review Jesper Dangaard Brouer 3 participants 6 messages Jan 6, 2017 – Jan 7, 2017

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Jesper Dangaard Brouer

author

2017-01-06 17:39 UTC #0 lore raw

PATCH 1/1 net: reduce cycles spend on ICMP replies that gets rate limited

This patch split the global and per (inet)peer ICMP-reply limiter code, and moves the global limit check to earlier in the packet processing path. Thus, avoid spending cycles on ICMP replies that gets limited/suppressed anyhow.

The global ICMP rate limiter icmp_global_allow() is a good solution, it just happens too late in the process. The kernel goes through allocating memory and route lookup (return path) for the ICMP message, before taking the rate limit decision of not sending the ICMP reply.

Details: The kernels global rate limiter for ICMP messages got added in commit 4cdf507d5452 ("icmp: add a global rate limitation"). It is a token bucket limiter with a global lock. It brilliantly avoids locking congestion by only updating when 20ms (HZ/50) were elapsed. It can then avoids taking lock when credit is exhausted (when under pressure) and time constraint for refill is not yet meet.

Use-case: The specific case I experienced this being a bottleneck is, sending UDP packets to a port with no listener, which obviously result in kernel replying with ICMP Destination Unreachable (type:3), Port Unreachable (code:3), which cause the bottleneck.
After Eric and Paolo optimized the UDP socket code, the kernels PPS
processing capabilities is lower for no-listen ports, than normal UDP sockets. This is bad for capacity planning when restarting a service.

UDP no-listen benchmark 8xCPUs using pktgen_sample04_many_flows.sh:
Baseline: 6.6 Mpps
Patch: 14.5 Mpps

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 net/ipv4/icmp.c |   87 +++++++++++++++++++++++++++++++++++--------------------
 net/ipv6/icmp.c |   49 +++++++++++++++++++++----------
 2 files changed, 90 insertions(+), 46 deletions(-)

net/ipv4/icmp.c +56 ~~-31~~

index 0777ea949223..3d7b447c8b72 100644
@@ -282,6 +282,33 @@ bool icmp_global_allow(void)
 }
 EXPORT_SYMBOL(icmp_global_allow);
 
+static bool icmpv4_mask_allow(struct net *net, int type, int code)
+{
+	if (type > NR_ICMP_TYPES)
+		return true;
+
+	/* Don't limit PMTU discovery. */
+	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED)
+		return true;
+
+	/* Limit if icmp type is enabled in ratemask. */
+	if (!((1 << type) & net->ipv4.sysctl_icmp_ratemask))
+		return true;
+
+	return false;
+}
+
+static bool icmpv4_global_allow(struct net *net, int type, int code)
+{
+	if (icmpv4_mask_allow(net, type, code))
+		return true;
+
+	if (icmp_global_allow())
+		return true;
+
+	return false;
+}
+
 /*
  *	Send an ICMP frame.
  */

@@ -290,34 +317,22 @@ static bool icmpv4_xrlim_allow(struct net *net, struct rtable *rt,
 			       struct flowi4 *fl4, int type, int code)
 {
 	struct dst_entry *dst = &rt->dst;
+	struct inet_peer *peer;
 	bool rc = true;
+	int vif;
 
-	if (type > NR_ICMP_TYPES)
-		goto out;
-
-	/* Don't limit PMTU discovery. */
-	if (type == ICMP_DEST_UNREACH && code == ICMP_FRAG_NEEDED)
+	if (icmpv4_mask_allow(net, type, code))
 		goto out;
 
 	/* No rate limit on loopback */
 	if (dst->dev && (dst->dev->flags&IFF_LOOPBACK))
 		goto out;
 
-	/* Limit if icmp type is enabled in ratemask. */
-	if (!((1 << type) & net->ipv4.sysctl_icmp_ratemask))
-		goto out;
-
-	rc = false;
-	if (icmp_global_allow()) {
-		int vif = l3mdev_master_ifindex(dst->dev);
-		struct inet_peer *peer;
-
-		peer = inet_getpeer_v4(net->ipv4.peers, fl4->daddr, vif, 1);
-		rc = inet_peer_xrlim_allow(peer,
-					   net->ipv4.sysctl_icmp_ratelimit);
-		if (peer)
-			inet_putpeer(peer);
-	}
+	vif = l3mdev_master_ifindex(dst->dev);
+	peer = inet_getpeer_v4(net->ipv4.peers, fl4->daddr, vif, 1);
+	rc = inet_peer_xrlim_allow(peer, net->ipv4.sysctl_icmp_ratelimit);
+	if (peer)
+		inet_putpeer(peer);
 out:
 	return rc;
 }

@@ -396,6 +411,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	struct inet_sock *inet;
 	__be32 daddr, saddr;
 	u32 mark = IP4_REPLY_MARK(net, skb->mark);
+	int type = icmp_param->data.icmph.type;
+	int code = icmp_param->data.icmph.code;
 
 	if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb))
 		return;

@@ -405,6 +422,10 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 		return;
 	inet = inet_sk(sk);
 
+	/* global icmp_msgs_per_sec */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
 	icmp_param->data.icmph.checksum = 0;
 
 	inet->tos = ip_hdr(skb)->tos;

@@ -433,8 +454,7 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
 	rt = ip_route_output_key(net, &fl4);
 	if (IS_ERR(rt))
 		goto out_unlock;
-	if (icmpv4_xrlim_allow(net, rt, &fl4, icmp_param->data.icmph.type,
-			       icmp_param->data.icmph.code))
+	if (icmpv4_xrlim_allow(net, rt, &fl4, type, code))
 		icmp_push_reply(icmp_param, &fl4, &ipc, &rt);
 	ip_rt_put(rt);
 out_unlock:

@@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 
-	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
-	if (!icmp_param)
-		return;
-
 	sk = icmp_xmit_lock(net);
 	if (!sk)
-		goto out_free;
+		goto out;
+
+	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		goto out_unlock;
 
 	/*
 	 *	Construct source address and options.

@@ -682,7 +706,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 	mark = IP4_REPLY_MARK(net, skb_in->mark);
 
 	if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
-		goto out_unlock;
+		goto out_free;
 
 
 	/*

@@ -706,8 +730,9 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 	rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos, mark,
 			       type, code, icmp_param);
 	if (IS_ERR(rt))
-		goto out_unlock;
+		goto out_free;
 
+	/* peer icmp_ratelimit */
 	if (!icmpv4_xrlim_allow(net, rt, &fl4, type, code))
 		goto ende;

@@ -727,10 +752,10 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 	icmp_push_reply(icmp_param, &fl4, &ipc, &rt);
 ende:
 	ip_rt_put(rt);
-out_unlock:
-	icmp_xmit_unlock(sk);
 out_free:
 	kfree(icmp_param);
+out_unlock:
+	icmp_xmit_unlock(sk);
 out:;
 }
 EXPORT_SYMBOL(icmp_send);

net/ipv6/icmp.c +34 ~~-15~~

78 lines not on this page · raw message

Eric Dumazet

2017-01-06 19:40 UTC #1 lore raw

Quoted 10 lines · Jesper Dangaard Brouer

net/ipv4/icmp.c +0 -4

@@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 
-	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
-	if (!icmp_param)
-		return;
-
 	sk = icmp_xmit_lock(net);
 	if (!sk)

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 681

Quoted 10 lines

net/ipv4/icmp.c +9 -1

-		goto out_free;
+		goto out;
+
+	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		goto out_unlock;

Truth be told, I have no idea why we allocate dynamic memory for "struct icmp_bxm " : It is 112 bytes.

Eric Dumazet

2017-01-06 22:08 UTC #2 lore raw

Quoted 7 lines · Eric Dumazet

Quoted 4 lines · Jesper Dangaard Brouer
(quoted diff) +0 -0
@@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 

Quoted 17 lines

Quoted 6 lines

net/ipv4/icmp.c +0 -4

-	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
-	if (!icmp_param)
-		return;
-
 	sk = icmp_xmit_lock(net);
 	if (!sk)

net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 681

Quoted 10 lines

net/ipv4/icmp.c +9 -1

-		goto out_free;
+		goto out;
+
+	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		goto out_unlock;

You could call icmp_xmit_lock() _after_ checking global limit perhaps.

That would remove one atomic op.

if (!icmpv4_global_allow(net, type, code))

    goto out;

sk = icmp_xmit_lock(net);
if (!sk)

    goto out;

David Miller

maintainer

2017-01-07 03:10 UTC #3 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 06 Jan 2017 14:08:06 -0800

Quoted 26 lines · Eric Dumazet

Quoted 7 lines · Jesper Dangaard Brouer
Quoted 6 lines
(quoted diff) +0 -0
@@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 
Quoted 17 lines
Quoted 6 lines
(quoted diff) +0 -4
-	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
-	if (!icmp_param)
-		return;
-
 	sk = icmp_xmit_lock(net);
 	if (!sk)
Review void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 681
Quoted 10 lines
(quoted diff) +9 -1
-		goto out_free;
+		goto out;
+
+	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		goto out_unlock;

Quoted 1 line

You could call icmp_xmit_lock() _after_ checking global limit perhaps.

BTW Eric, you asked about kmalloc() allocation, you were CC:'d in the patch which did this :-)

commit 9a99d4a50cb8ce516adf0f2436138d4c8e6e4535

Author: Cong Wang <[email protected]>
Date:   Sun Jun 2 15:00:52 2013 +0000

icmp: avoid allocating large struct on stack

struct icmp_bxm is a large struct, reduce stack usage by allocating it on heap.

Cc Eric Dumazet <[email protected]>

Cc Joe Perches <[email protected]>

Cc David S. Miller <[email protected]>

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

Signed-off-by David S. Miller <[email protected]>

net/ipv4/icmp.c +23 ~~-17~~

index 2864ca3..5f7d11a 100644
@@ -482,7 +482,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 {
 	struct iphdr *iph;
 	int room;
-	struct icmp_bxm icmp_param;
+	struct icmp_bxm *icmp_param;
 	struct rtable *rt = skb_rtable(skb_in);
 	struct ipcm_cookie ipc;
 	struct flowi4 fl4;

@@ -558,9 +558,13 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		return;
+
 	sk = icmp_xmit_lock(net);
 	if (sk == NULL)
-		return;
+		goto out_free;
 
 	/*
 	 *	Construct source address and options.

@@ -586,7 +590,7 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 					   IPTOS_PREC_INTERNETCONTROL) :
 					  iph->tos;
 
-	if (ip_options_echo(&icmp_param.replyopts.opt.opt, skb_in))
+	if (ip_options_echo(&icmp_param->replyopts.opt.opt, skb_in))
 		goto out_unlock;

@@ -594,19 +598,19 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 	 *	Prepare data for ICMP header.
 	 */
 
-	icmp_param.data.icmph.type	 = type;
-	icmp_param.data.icmph.code	 = code;
-	icmp_param.data.icmph.un.gateway = info;
-	icmp_param.data.icmph.checksum	 = 0;
-	icmp_param.skb	  = skb_in;
-	icmp_param.offset = skb_network_offset(skb_in);
+	icmp_param->data.icmph.type	 = type;
+	icmp_param->data.icmph.code	 = code;
+	icmp_param->data.icmph.un.gateway = info;
+	icmp_param->data.icmph.checksum	 = 0;
+	icmp_param->skb	  = skb_in;
+	icmp_param->offset = skb_network_offset(skb_in);
 	inet_sk(sk)->tos = tos;
 	ipc.addr = iph->saddr;
-	ipc.opt = &icmp_param.replyopts.opt;
+	ipc.opt = &icmp_param->replyopts.opt;
 	ipc.tx_flags = 0;
 
 	rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr, tos,
-			       type, code, &icmp_param);
+			       type, code, icmp_param);
 	if (IS_ERR(rt))
 		goto out_unlock;

@@ -618,19 +622,21 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 	room = dst_mtu(&rt->dst);
 	if (room > 576)
 		room = 576;
-	room -= sizeof(struct iphdr) + icmp_param.replyopts.opt.opt.optlen;
+	room -= sizeof(struct iphdr) + icmp_param->replyopts.opt.opt.optlen;
 	room -= sizeof(struct icmphdr);
 
-	icmp_param.data_len = skb_in->len - icmp_param.offset;
-	if (icmp_param.data_len > room)
-		icmp_param.data_len = room;
-	icmp_param.head_len = sizeof(struct icmphdr);
+	icmp_param->data_len = skb_in->len - icmp_param->offset;
+	if (icmp_param->data_len > room)
+		icmp_param->data_len = room;
+	icmp_param->head_len = sizeof(struct icmphdr);
 
-	icmp_push_reply(&icmp_param, &fl4, &ipc, &rt);
+	icmp_push_reply(icmp_param, &fl4, &ipc, &rt);
 ende:
 	ip_rt_put(rt);
 out_unlock:
 	icmp_xmit_unlock(sk);
+out_free:
+	kfree(icmp_param);
 out:;
 }
 EXPORT_SYMBOL(icmp_send);

Jesper Dangaard Brouer

author

2017-01-07 10:31 UTC #4 lore raw

Quoted 9 lines · On Fri, 06 Jan 2017 22:10:42 -0500 (EST) David Miller <[email protected]>…

BTW Eric, you asked about kmalloc() allocation, you were CC:'d in the patch which did this :-)
commit 9a99d4a50cb8ce516adf0f2436138d4c8e6e4535
Author: Cong Wang <[email protected]>
Date:   Sun Jun 2 15:00:52 2013 +0000
icmp: avoid allocating large struct on stack

Quoted 8 lines

struct icmp_bxm is a large struct, reduce stack usage by allocating it on heap.
Cc Eric Dumazet <[email protected]>
Cc Joe Perches <[email protected]>
Cc David S. Miller <[email protected]>
Signed-off-by Cong Wang <[email protected]>
Signed-off-by David S. Miller <[email protected]>

Did a quick revert, and tested again. It is not the major bottleneck, but we do save something. The major bottleneck is still the call to __ip_route_output_key_hash (invoked by icmp_route_lookup).

Single flow improvement from 1719182 pps to 1783368 pps.
- 64186 pps
- (1/1783368-1/1719182)*10^9 = -20.93 nanosec
* 4GHz approx = 20.93*4 = 83.72 cycles

The optimal SLUB fast-path on this machine is 54 cycles(tsc) 13.557 ns, thus the saving is actually higher than expected. But low compared to avoiding the icmp_route_lookup.

Signature · 4 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Jesper Dangaard Brouer

author

2017-01-07 09:28 UTC #5 lore raw

In reply to Eric Dumazet

Quoted 26 lines · On Fri, 06 Jan 2017 14:08:06 -0800 Eric Dumazet <[email protected]> wrote:

Quoted 7 lines · Eric Dumazet
Quoted 4 lines · Jesper Dangaard Brouer
(quoted diff) +0 -0
@@ -648,13 +668,17 @@ void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
 		}
 	}
 
Quoted 17 lines
Quoted 6 lines
net/ipv4/icmp.c +0 -4
-	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
-	if (!icmp_param)
-		return;
-
 	sk = icmp_xmit_lock(net);
 	if (!sk)
net/ipv4/icmp.c void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info) · line 681
Quoted 10 lines
net/ipv4/icmp.c +9 -1
-		goto out_free;
+		goto out;
+
+	/* Check global sysctl_icmp_msgs_per_sec ratelimit */
+	if (!icmpv4_global_allow(net, type, code))
+		goto out_unlock;
+
+	icmp_param = kmalloc(sizeof(*icmp_param), GFP_ATOMIC);
+	if (!icmp_param)
+		goto out_unlock;  

Quoted 3 lines

You could call icmp_xmit_lock() _after_ checking global limit perhaps.
That would remove one atomic op.

The reason I don't do this is, because icmp_xmit_lock() disables BH and icmp_global_allow() have a comment that it need to run with BH-disabled. Thus, I'm depending on the BH-disable from icmp_xmit_lock().

I would have to move out the BH-disable part of icmp_xmit_lock, to do this. Would that be acceptable?

Signature · 4 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer