Re: [PATCH net-next] ip: avoid OOM kills with large UDP sends over loopback

On 6/22/21 6:54 PM, Jakub Kicinski wrote:

On Tue, 22 Jun 2021 16:12:11 +0200 Eric Dumazet wrote:

quoted

On 6/22/21 1:13 AM, Jakub Kicinski wrote:

quoted

Dave observed number of machines hitting OOM on the UDP send
path. The workload seems to be sending large UDP packets over
loopback. Since loopback has MTU of 64k kernel will try to
allocate an skb with up to 64k of head space. This has a good
chance of failing under memory pressure. What's worse if
the message length is <32k the allocation may trigger an
OOM killer.

This is entirely avoidable, we can use an skb with frags.

The scenario is unlikely and always using frags requires
an extra allocation so opt for using fallback, rather
then always using frag'ed/paged skb when payload is large.

Note that the size heuristic (header_len > PAGE_SIZE)
is not entirely accurate, __alloc_skb() will add ~400B
to size. Occasional order-1 allocation should be fine,
though, we are primarily concerned with order-3.

Reported-by: Dave Jones <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

quoted

quoted

+static inline void sk_allocation_push(struct sock *sk, gfp_t flag, gfp_t *old)
+{
+	*old = sk->sk_allocation;
+	sk->sk_allocation |= flag;
+}
+  

This is not thread safe.

Remember UDP sendmsg() does not lock the socket for non-corking sends.

Ugh, you're right :(

quoted

quoted

+static inline void sk_allocation_pop(struct sock *sk, gfp_t old)
+{
+	sk->sk_allocation = old;
+}
+
 static inline void sk_acceptq_removed(struct sock *sk)
 {
 	WRITE_ONCE(sk->sk_ack_backlog, sk->sk_ack_backlog - 1);

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c3efc7d658f6..a300c2c65d57 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -1095,9 +1095,24 @@ static int __ip_append_data(struct sock *sk,
 				alloclen += rt->dst.trailer_len;
 
 			if (transhdrlen) {
-				skb = sock_alloc_send_skb(sk,
-						alloclen + hh_len + 15,
+				size_t header_len = alloclen + hh_len + 15;
+				gfp_t sk_allocation;
+
+				if (header_len > PAGE_SIZE)
+					sk_allocation_push(sk, __GFP_NORETRY,
+							   &sk_allocation);
+				skb = sock_alloc_send_skb(sk, header_len,
 						(flags & MSG_DONTWAIT), &err);
+				if (header_len > PAGE_SIZE) {
+					BUILD_BUG_ON(MAX_HEADER >= PAGE_SIZE);
+
+					sk_allocation_pop(sk, sk_allocation);
+					if (unlikely(!skb) && !paged &&
+					    rt->dst.dev->features & NETIF_F_SG) {
+						paged = true;
+						goto alloc_new_skb;
+					}
+				}  

What about using 	sock_alloc_send_pskb(... PAGE_ALLOC_COSTLY_ORDER)
(as we did in unix_dgram_sendmsg() for large packets), for SG enabled interfaces ?

PAGE_ALLOC_COSTLY_ORDER in itself is more of a problem than a solution.
AFAIU the app sends messages primarily above the ~60kB mark, which is
above COSTLY, and those do not trigger OOM kills. All OOM kills we see
have order=3. Checking with Rik and Johannes W that's expected, OOM
killer is only invoked for allocations <= COSTLY, larger ones will just
return NULL and let us deal with it (e.g. by falling back).

I  really thought alloc_skb_with_frags() was already handling low-memory-conditions.

(alloc_skb_with_frags() is called from sock_alloc_send_pskb())

If it is not, lets fix it, because af_unix sockets will have the same issue ?

So adding GFP_NORETRY is key for 0 < order <= COSTLY,
skb_page_frag_refill()-style.

quoted

We do not _have_ to put all the payload in skb linear part,
we could instead use page frags (order-0 if high order pages are not available)