VM deadlock prevention -v5

net/core/skbuff.c +48 -4

+
+	if (size + sizeof(struct skb_shared_info) > PAGE_SIZE)
+		goto out;
+
+	if (!emerg_rx_pages_try_inc())
+		goto out;
+
+	skb = (void *)__get_free_page(gfp_mask | __GFP_EMERG);
+	if (!skb) {
+		WARN_ON(1); /* we were promised memory but didn't get it? */
+		goto dec_out;
+	}
+
+	if (!emerg_rx_pages_try_inc())
+		goto skb_free_out;
+
+	data = (void *)__get_free_page(gfp_mask | __GFP_EMERG);
+	if (!data) {
+		WARN_ON(1); /* we were promised memory but didn't get it? */
+		emerg_rx_pages_dec();
+skb_free_out:
+		free_page((unsigned long)skb);
+		skb = NULL;
+dec_out:
+		emerg_rx_pages_dec();
+		goto out;
+	}
+
+	cache = NULL;
+	goto allocated;
 }
 
 /**

@@ -267,7 +304,7 @@ struct sk_buff *__netdev_alloc_skb(struc
 {
 	struct sk_buff *skb;
 
-	skb = alloc_skb(length + NET_SKB_PAD, gfp_mask);
+	skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask, SKB_ALLOC_RX);
 	if (likely(skb)) {
 		skb_reserve(skb, NET_SKB_PAD);
 		skb->dev = dev;

@@ -315,10 +352,20 @@ static void skb_release_data(struct sk_b
 		if (skb_shinfo(skb)->frag_list)
 			skb_drop_fraglist(skb);
 
-		kfree(skb->head);
+		if (skb->emerg) {
+			free_page((unsigned long)skb->head);
+			emerg_rx_pages_dec();
+		} else
+			kfree(skb->head);
 	}
 }
 
+static void emerg_free_skb(struct kmem_cache *cache, void *objp)
+{
+	free_page((unsigned long)objp);
+	emerg_rx_pages_dec();
+}
+
 /*
  *	Free an skbuff by memory without cleaning the state.
  */

@@ -326,17 +373,21 @@ void kfree_skbmem(struct sk_buff *skb)
 {
 	struct sk_buff *other;
 	atomic_t *fclone_ref;
+	void (*free_skb)(struct kmem_cache *, void *);
 
 	skb_release_data(skb);
+
+	free_skb = skb->emerg ? emerg_free_skb : kmem_cache_free;
+
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
-		kmem_cache_free(skbuff_head_cache, skb);
+		free_skb(skbuff_head_cache, skb);
 		break;
 
 	case SKB_FCLONE_ORIG:
 		fclone_ref = (atomic_t *) (skb + 2);
 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+			free_skb(skbuff_fclone_cache, skb);
 		break;
 
 	case SKB_FCLONE_CLONE:

net/core/skbuff.c +15 -1

@@ -435,6 +486,17 @@ struct sk_buff *skb_clone(struct sk_buff
 		atomic_t *fclone_ref = (atomic_t *) (n + 1);
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);
+	} else if (skb->emerg) {
+		if (!emerg_rx_pages_try_inc())
+			return NULL;
+
+		n = (void *)__get_free_page(gfp_mask | __GFP_EMERG);
+		if (!n) {
+			WARN_ON(1);
+			emerg_rx_pages_dec();
+			return NULL;
+		}
+		n->fclone = SKB_FCLONE_UNAVAILABLE;
 	} else {
 		n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 		if (!n)

@@ -470,6 +532,7 @@ struct sk_buff *skb_clone(struct sk_buff
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	C(ipvs_property);
 #endif
+	C(emerg);
 	C(protocol);
 	n->destructor = NULL;
 #ifdef CONFIG_NETFILTER

@@ -690,7 +753,21 @@ int pskb_expand_head(struct sk_buff *skb
 
 	size = SKB_DATA_ALIGN(size);
 
-	data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
+	if (skb->emerg) {
+		if (size + sizeof(struct skb_shared_info) > PAGE_SIZE)
+			goto nodata;

net/core/skbuff.c +2 -0

+	} else
+		data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
 	if (!data)
 		goto nodata;
 

Index: linux-2.6/net/ipv4/icmp.c
===================================================================

net/ipv4/icmp.c +3 -0

@@ -938,6 +938,9 @@ int icmp_rcv(struct sk_buff *skb)
 			goto error;
 	}
 
+	if (unlikely(skb->emerg))
+		goto drop;
+
 	if (!pskb_pull(skb, sizeof(struct icmphdr)))
 		goto error;
 

Index: linux-2.6/net/ipv4/tcp_ipv4.c
===================================================================

net/ipv4/tcp_ipv4.c +3 -0

@@ -1093,6 +1093,9 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	if (!sk)
 		goto no_tcp_socket;
 
+	if (unlikely(skb->emerg && !sk_is_vmio(sk)))
+		goto discard_and_relse;
+
 process:
 	if (sk->sk_state == TCP_TIME_WAIT)
 		goto do_time_wait;

Index: linux-2.6/net/ipv4/udp.c
===================================================================

net/ipv4/udp.c +7 -1

@@ -1136,7 +1136,12 @@ int udp_rcv(struct sk_buff *skb)
 	sk = udp_v4_lookup(saddr, uh->source, daddr, uh->dest, skb->dev->ifindex);
 
 	if (sk != NULL) {
-		int ret = udp_queue_rcv_skb(sk, skb);
+		int ret;
+
+		if (unlikely(skb->emerg && !sk_is_vmio(sk)))
+			goto drop_noncritical;
+
+		ret = udp_queue_rcv_skb(sk, skb);
 		sock_put(sk);
 
 		/* a return value > 0 means to resubmit the input, but

@@ -1147,6 +1152,7 @@ int udp_rcv(struct sk_buff *skb)
 		return 0;
 	}
 
+drop_noncritical:
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto drop;
 	nf_reset(skb);

Index: linux-2.6/net/ipv4/af_inet.c
===================================================================

net/ipv4/af_inet.c +3 -0

@@ -132,6 +132,9 @@ void inet_sock_destruct(struct sock *sk)
 {
 	struct inet_sock *inet = inet_sk(sk);
 
+	if (sk_is_vmio(sk))
+		sk_clear_vmio(sk);
+
 	__skb_queue_purge(&sk->sk_receive_queue);
 	__skb_queue_purge(&sk->sk_error_queue);
 

Index: linux-2.6/net/core/sock.c
===================================================================

net/core/sock.c +3 -0

@@ -111,6 +111,7 @@
 #include <linux/poll.h>
 #include <linux/tcp.h>
 #include <linux/init.h>
+#include <linux/blkdev.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -195,6 +196,102 @@ __u32 sysctl_rmem_default = SK_RMEM_MAX;
 /* Maximal space eaten by iovec or ancilliary data plus some space */
 int sysctl_optmem_max = sizeof(unsigned long)*(2*UIO_MAXIOV + 512);
 
+static DEFINE_SPINLOCK(memalloc_lock);
+static int memalloc_reserve;

net/core/sock.c +11 -0

+ *
+ *	NOTE:
+ *	   %TX_RESERVE_PAGES is an upper-bound of memory used for TX hence
+ *	   we need not account the pages like we do for %RX_RESERVE_PAGES.
+ *
+ *	On top of this comes a one time charge of:
+ *	  %RX_RESERVE_PAGES pages -
+ * 		number of pages alloted for emergency skb service to critical
+ * 		sockets.
+ */
+int sk_adjust_memalloc(int socks, int request_queues)

net/core/sock.c +5 -0

+
+	err = adjust_memalloc_reserve(reserve - memalloc_reserve);
+	if (err) {
+		printk(KERN_WARNING
+			"Unable to change reserve to: %d pages, error: %d\n",

net/core/sock.c +25 -0

+
+/**
+ *	sk_set_vmio - sets %SOCK_VMIO
+ *	@sk: socket to set it on
+ *
+ *	Set %SOCK_VMIO on a socket and increase the memalloc reserve
+ *	accordingly.
+ */
+int sk_set_vmio(struct sock *sk)
+{
+	int err = 0;
+
+	if (!sock_flag(sk, SOCK_VMIO) &&
+			!(err = sk_adjust_memalloc(1, 0))) {
+		sock_set_flag(sk, SOCK_VMIO);
+		sk->sk_allocation |= __GFP_EMERG;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(sk_set_vmio);
+
+int sk_clear_vmio(struct sock *sk)
+{
+	int err = 0;

include/linux/skbuff.h struct sk_buff { · line 287

Quoted 8 lines · Peter Zijlstra

include/linux/skbuff.h +2 -1

@@ -282,7 +282,8 @@ struct sk_buff {
 				nfctinfo:3;
 	__u8			pkt_type:3,
 				fclone:2,
-				ipvs_property:1;
+				ipvs_property:1,
+				emerg:1;
 	__be16			protocol;

Why not 'emergency'? Looks like 'emerge' with a typo now. ;-)

include/net/sock.h enum sock_flags { · line 394

Quoted 5 lines

include/net/sock.h +1 -0

@@ -391,6 +391,7 @@ enum sock_flags {
 	SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */
 	SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */
 	SOCK_QUEUE_SHRUNK, /* write queue has been shrunk recently */
+	SOCK_VMIO, /* promise to never block on receive */

It might be used for IO related to the VM, but that doesn't tell _what_ it does. It also does much more than just not blocking on receive, so overal, aren't both the vmio name and the comment slightly misleading?

include/net/sock.h static inline int sock_flag(struct sock · line 442

Quoted 5 lines

include/net/sock.h +5 -0

+static inline int emerg_rx_pages_try_inc(void)
+{
+	return atomic_read(&vmio_socks) &&
+		atomic_add_unless(&emerg_rx_pages_used, 1, RX_RESERVE_PAGES);
+}

It looks cleaner to move that first check to the caller, as it is often redundant and in the other cases makes it more clear what the caller is really doing.

mm/page_alloc.c EXPORT_SYMBOL(zone_table); · line 85

Quoted 5 lines

mm/page_alloc.c +1 -0

@@ -82,6 +82,7 @@ EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "DMA32", "Normal", "HighMem" };
 int min_free_kbytes = 1024;
+int var_free_kbytes;

Using var_free_pages makes the code slightly simpler, as all that needless convertion isn't needed anymore. Perhaps the same is true for min_free_kbytes...

net/core/skbuff.c struct sk_buff *__alloc_skb(unsigned int · line 201

Quoted 5 lines

net/core/skbuff.c +4 -0

+noskb:
+	/* Attempt emergency allocation when RX skb. */
+	if (!(flags & SKB_ALLOC_RX))
+		goto out;

So only incoming skb allocation is guaranteed? What about outgoing skbs? What am I missing? Or can we sleep then, and increasing var_free_kbytes is sufficient to guarantee it?

Quoted 36 lines

net/core/skbuff.c +12 -2

+static void emerg_free_skb(struct kmem_cache *cache, void *objp)
+{
+	free_page((unsigned long)objp);
+	emerg_rx_pages_dec();
+}
+
 /*
  *	Free an skbuff by memory without cleaning the state.
  */

@@ -326,17 +373,21 @@ void kfree_skbmem(struct sk_buff *skb)
 {
 	struct sk_buff *other;
 	atomic_t *fclone_ref;
+	void (*free_skb)(struct kmem_cache *, void *);
 
 	skb_release_data(skb);
+
+	free_skb = skb->emerg ? emerg_free_skb : kmem_cache_free;
+
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
-		kmem_cache_free(skbuff_head_cache, skb);
+		free_skb(skbuff_head_cache, skb);
 		break;
 
 	case SKB_FCLONE_ORIG:
 		fclone_ref = (atomic_t *) (skb + 2);
 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+			free_skb(skbuff_fclone_cache, skb);
 		break;
 
 	case SKB_FCLONE_CLONE:

@@ -349,7 +400,7 @@ void kfree_skbmem(struct sk_buff *skb)
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 

net/core/skbuff.c void kfree_skbmem(struct sk_buff *skb) · line 406

Quoted 6 lines

net/core/skbuff.c +1 -1

 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+			free_skb(skbuff_fclone_cache, other);
 		break;
 	};
 }

I don't have the original code in front of me, but isn't it possible to add a "goto free" which has all the freeing in one place? That would get rid of the function pointer stuff and emerg_free_skb.

Quoted 30 lines

net/core/skbuff.c +12 -0

@@ -435,6 +486,17 @@ struct sk_buff *skb_clone(struct sk_buff
 		atomic_t *fclone_ref = (atomic_t *) (n + 1);
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);
+	} else if (skb->emerg) {
+		if (!emerg_rx_pages_try_inc())
+			return NULL;
+
+		n = (void *)__get_free_page(gfp_mask | __GFP_EMERG);
+		if (!n) {
+			WARN_ON(1);
+			emerg_rx_pages_dec();
+			return NULL;
+		}
+		n->fclone = SKB_FCLONE_UNAVAILABLE;
 	} else {
 		n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 		if (!n)

@@ -470,6 +532,7 @@ struct sk_buff *skb_clone(struct sk_buff
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	C(ipvs_property);
 #endif
+	C(emerg);
 	C(protocol);
 	n->destructor = NULL;
 #ifdef CONFIG_NETFILTER

@@ -690,7 +753,21 @@ int pskb_expand_head(struct sk_buff *skb
 
 	size = SKB_DATA_ALIGN(size);
 

Quoted 4 lines

net/core/skbuff.c +3 -1

-	data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
+	if (skb->emerg) {
+		if (size + sizeof(struct skb_shared_info) > PAGE_SIZE)
+			goto nodata;

net/core/skbuff.c int pskb_expand_head(struct sk_buff *skb · line 768

Quoted 10 lines

net/core/skbuff.c +10 -0

+
+		if (!emerg_rx_pages_try_inc())
+			goto nodata;
+
+		data = (void *)__get_free_page(gfp_mask | __GFP_EMERG);
+		if (!data) {
+			WARN_ON(1);
+			emerg_rx_pages_dec();
+			goto nodata;
+		}

There seems to be some pattern occuring here, what about a new function?

Are these functions only called for incoming skbs? If not, calling emerg_rx_pages_try_inc() is the wrong thing to do. A quick search says they aren't. Add a RX check? Or is that fixed by SKB_FCLONE_UNAVAILABLE? If so, why are skb_clone() and pskb_expand_head() modified at all? (Probably ignorance on my end.)

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 210

Quoted 3 lines

net/core/sock.c +3 -0

+/**
+ *	sk_adjust_memalloc - adjust the global memalloc reserve for critical RX
+ *	@nr_socks: number of new %SOCK_VMIO sockets

I don't see a parameter named nr_socks? request_queues isn't docuemnted?

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 217

Quoted 7 lines

net/core/sock.c +7 -0

+ *
+ *	This function adjusts the memalloc reserve based on system demand.
+ *	For each %SOCK_VMIO socket this device will reserve enough
+ *	to send a few large packets (64k) at a time: %TX_RESERVE_PAGES.
+ *
+ *	Assumption:
+ *	 - each %SOCK_VMIO socket will have a %request_queue associated.

If this is assumed, then why is the request_queue parameter needed?

Quoted 10 lines

+ *
+ * NOTE:
+ * %TX_RESERVE_PAGES is an upper-bound of memory used for TX hence
+ * we need not account the pages like we do for %RX_RESERVE_PAGES.
+ *
+ * On top of this comes a one time charge of:
+ * %RX_RESERVE_PAGES pages -
+ * number of pages alloted for emergency skb service to critical
+ * sockets.
+ */

Quoted 5 lines

net/core/sock.c +5 -0

+int sk_adjust_memalloc(int socks, int request_queues)
+{
+	unsigned long flags;
+	int reserve;
+	int nr_socks;

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 238

Quoted 6 lines

net/core/sock.c +6 -0

+	int err;
+
+	spin_lock_irqsave(&memalloc_lock, flags);
+
+	atomic_add(socks, &vmio_socks);
+	nr_socks = atomic_read(&vmio_socks);

nr_socks = atomic_add_return(socks, &vmio_socks);

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 239

Quoted 1 line

net/core/sock.c +1 -0

+	BUG_ON(socks < 0);

Shouldn't this be nr_socks < 0?

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 243

Quoted 4 lines

net/core/sock.c +4 -0

+	vmio_request_queues += request_queues;
+
+	reserve = vmio_request_queues * TX_RESERVE_PAGES + /* outbound */
+		  (!!socks) * RX_RESERVE_PAGES;            /* inbound */

If the assumption that each VMIO socket will have a request_queue associated is true, then we only need to check if vmio_request_queues !=0, right?

Quoted 11 lines

net/core/sock.c +11 -0

+
+	err = adjust_memalloc_reserve(reserve - memalloc_reserve);
+	if (err) {
+		printk(KERN_WARNING
+			"Unable to change reserve to: %d pages, error: %d\n",
+			reserve, err);
+		goto unlock;
+	}
+	memalloc_reserve = reserve;
+
+unlock:

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 258

Quoted 4 lines

net/core/sock.c +4 -0

+	spin_unlock_irqrestore(&memalloc_lock, flags);
+	return err;
+}
+EXPORT_SYMBOL_GPL(sk_adjust_memalloc);

You can get rid of the memalloc_reserve and vmio_request_queues variables if you want, they aren't really needed for anything. If using them reduces the total code size I'd keep them though.

Quoted 7 lines

net/core/sock.c +7 -0

+int sk_clear_vmio(struct sock *sk)
+{
+	int err = 0;
+
+	if (sock_flag(sk, SOCK_VMIO) &&
+			!(err = sk_adjust_memalloc(-1, 0))) {
+		sock_reset_flag(sk, SOCK_VMIO);

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 293

Quoted 6 lines

net/core/sock.c +6 -0

+		sk->sk_allocation &= ~__GFP_EMERG;
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(sk_clear_vmio);

It seems wiser to always reset the flags, even if sk_adjust_memalloc fails.

This patch looks much better than the previous one, not much cruft left.

Quoted 1 line · Indan Zupancic

Why not 'emergency'? Looks like 'emerge' with a typo now. ;-)

hehe, me lazy, you gentoo ;-)
sed -i -e 's/emerg/emregency/g' -e 's/EMERG/EMERGENCY/g' *.patch

Quoted 9 lines

include/net/sock.h enum sock_flags { · line 394

Quoted 5 lines

include/net/sock.h +1 -0

@@ -391,6 +391,7 @@ enum sock_flags {
 	SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */
 	SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */
 	SOCK_QUEUE_SHRUNK, /* write queue has been shrunk recently */
+	SOCK_VMIO, /* promise to never block on receive */

It might be used for IO related to the VM, but that doesn't tell _what_ it does. It also does much more than just not blocking on receive, so overal, aren't both the vmio name and the comment slightly misleading?

I'm so having trouble with this name; I had SOCK_NONBLOCKING for a while, but that is a very bad name because nonblocking has this well defined meaning when talking about sockets, and this is not that.

Hence I came up with the VMIO, because that is the only selecting criteria for being special. - I'll fix up the comment.

Quoted 8 lines

mm/page_alloc.c EXPORT_SYMBOL(zone_table); · line 85

Quoted 5 lines

mm/page_alloc.c +1 -0

@@ -82,6 +82,7 @@ EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "DMA32", "Normal", "HighMem" };
 int min_free_kbytes = 1024;
+int var_free_kbytes;

Using var_free_pages makes the code slightly simpler, as all that needless convertion isn't needed anymore. Perhaps the same is true for min_free_kbytes...

't seems I'm a bit puzzled as to what you mean here.

Quoted 8 lines

net/core/skbuff.c struct sk_buff *__alloc_skb(unsigned int · line 201

Quoted 4 lines

net/core/skbuff.c +4 -0

+noskb:
+	/* Attempt emergency allocation when RX skb. */
+	if (!(flags & SKB_ALLOC_RX))
+		goto out;

So only incoming skb allocation is guaranteed? What about outgoing skbs? What am I missing? Or can we sleep then, and increasing var_free_kbytes is sufficient to guarantee it?

->sk_allocation |= __GFP_EMERGENCY - will take care of the outgoing packets. Also, since one only sends a limited number of packets out and then will wait for answers, we do not need to worry about fragmentation issues that much in this case.

Quoted 36 lines

Quoted 36 lines

net/core/skbuff.c +12 -2

+static void emerg_free_skb(struct kmem_cache *cache, void *objp)
+{
+	free_page((unsigned long)objp);
+	emerg_rx_pages_dec();
+}
+
 /*
  *	Free an skbuff by memory without cleaning the state.
  */

@@ -326,17 +373,21 @@ void kfree_skbmem(struct sk_buff *skb)
 {
 	struct sk_buff *other;
 	atomic_t *fclone_ref;
+	void (*free_skb)(struct kmem_cache *, void *);
 
 	skb_release_data(skb);
+
+	free_skb = skb->emerg ? emerg_free_skb : kmem_cache_free;
+
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
-		kmem_cache_free(skbuff_head_cache, skb);
+		free_skb(skbuff_head_cache, skb);
 		break;
 
 	case SKB_FCLONE_ORIG:
 		fclone_ref = (atomic_t *) (skb + 2);
 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+			free_skb(skbuff_fclone_cache, skb);
 		break;
 
 	case SKB_FCLONE_CLONE:

@@ -349,7 +400,7 @@ void kfree_skbmem(struct sk_buff *skb)
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 

Quoted 10 lines

net/core/skbuff.c void kfree_skbmem(struct sk_buff *skb) · line 406

Quoted 6 lines

net/core/skbuff.c +1 -1

 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+			free_skb(skbuff_fclone_cache, other);
 		break;
 	};
 }

I don't have the original code in front of me, but isn't it possible to add a "goto free" which has all the freeing in one place? That would get rid of the function pointer stuff and emerg_free_skb.

perhaps, yes, however I prefer this one, it allows access to the size.

Quoted 5 lines

Are these functions only called for incoming skbs? If not, calling emerg_rx_pages_try_inc() is the wrong thing to do. A quick search says they aren't. Add a RX check? Or is that fixed by SKB_FCLONE_UNAVAILABLE? If so, why are skb_clone() and pskb_expand_head() modified at all? (Probably ignorance on my end.)

Well, no, however only incoming skbs can have skb->emergency set to begin with.

Quoted 3 lines

You can get rid of the memalloc_reserve and vmio_request_queues variables if you want, they aren't really needed for anything. If using them reduces the total code size I'd keep them though.

I find my version easier to read, but that might just be the way my brain works.

Quoted 14 lines · Peter Zijlstra

Quoted 9 lines

include/net/sock.h enum sock_flags { · line 394

Quoted 5 lines

include/net/sock.h +1 -0

@@ -391,6 +391,7 @@ enum sock_flags {
 	SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */
 	SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */
 	SOCK_QUEUE_SHRUNK, /* write queue has been shrunk recently */
+	SOCK_VMIO, /* promise to never block on receive */

It might be used for IO related to the VM, but that doesn't tell _what_ it does. It also does much more than just not blocking on receive, so overal, aren't both the vmio name and the comment slightly misleading?

I'm so having trouble with this name; I had SOCK_NONBLOCKING for a while, but that is a very bad name because nonblocking has this well defined meaning when talking about sockets, and this is not that.

Quoted 2 lines

Hence I came up with the VMIO, because that is the only selecting criteria for being special. - I'll fix up the comment.

It's nice and short, but it might be weird if someone after a while finds another way of using this stuff. And it's relation to 'emergency' looks unclear. So maybe calling both the same makes most sense, no matter how you name it.

Quoted 9 lines

Quoted 8 lines

mm/page_alloc.c EXPORT_SYMBOL(zone_table); · line 85

Quoted 5 lines

mm/page_alloc.c +1 -0

@@ -82,6 +82,7 @@ EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "DMA32", "Normal", "HighMem" };
 int min_free_kbytes = 1024;
+int var_free_kbytes;

Using var_free_pages makes the code slightly simpler, as all that needless convertion isn't needed anymore. Perhaps the same is true for min_free_kbytes...

Quoted 1 line

't seems I'm a bit puzzled as to what you mean here.

I mean to store the variable reserve in pages instead of kilobytes. Currently you're converting from the one to the other both when setting and when using the value. That doesn't make much sense and can be avoided by storing the value in pages from the start.

void kfree_skbmem(struct sk_buff *skb)

{
	struct sk_buff *other;
	atomic_t *fclone_ref;
	struct kmem_cache *cache = skbuff_head_cache;
	struct sk_buff *free = skb;

	skb_release_data(skb);
	switch (skb->fclone) {
	case SKB_FCLONE_UNAVAILABLE:
		goto free;

	case SKB_FCLONE_ORIG:
		fclone_ref = (atomic_t *) (skb + 2);
		if (atomic_dec_and_test(fclone_ref)){
			cache = skbuff_fclone_cache;
			goto free;
		}
		break;

	case SKB_FCLONE_CLONE:
		fclone_ref = (atomic_t *) (skb + 1);
		other = skb - 1;

		/* The clone portion is available for
		 * fast-cloning again.
		 */
		skb->fclone = SKB_FCLONE_UNAVAILABLE;

		if (atomic_dec_and_test(fclone_ref)){
			cache = skbuff_fclone_cache;
			free = other;
			goto free;
		}
		break;

	};
	return;
free:
	if (!skb->emergency)
		kmem_cache_free(cache, free);
	else
		emergency_rx_free(free, kmem_cache_size(cache));
}

Quoted 6 lines

Quoted 3 lines

You can get rid of the memalloc_reserve and vmio_request_queues variables if you want, they aren't really needed for anything. If using them reduces the total code size I'd keep them though.

I find my version easier to read, but that might just be the way my brain works.

Maybe true, but I believe my version is more natural in the sense that it makes more clear what the code is doing. Less bookkeeping, more real work, so to speak.

But after another look things seem a bit shaky, in the locking corner anyway.

sk_adjust_memalloc() calls adjust_memalloc_reserve(), which changes var_free_kbytes and then calls setup_per_zone_pages_min(), which does the real work. But it reads min_free_kbytes without holding any locks. In mainline that's fine as the function is only called by the proc handler and in obscure memory hotplug stuff. But with your code it can also be called at any moment when a VMIO socket is made, which now races with the proc callback. More a theoretical than a real problem, but still slightly messy.

adjust_memalloc_reserve() has no locking at all, while it might be called concurrently from different sources. Luckily sk_adjust_memalloc() is the only user, and which uses its own spinlock for synchronization, so things go well by accident now. It seems cleaner to move that spinlock so that it protects var|min_free_kbytes instead.

mm/page_alloc.c +10 -0

+int adjust_memalloc_reserve(int pages)
+{
+	int kbytes;
+	int err = 0;
+
+	kbytes = var_free_kbytes + (pages << (PAGE_SHIFT - 10));
+	if (kbytes < 0) {
+		err = -EINVAL;
+		goto out;
+	}

Shouldn't that be a BUG_ON instead?

Also, I'm really past caring what the thing
is called ;-) But if ppl object I guess its easy enough to run yet another sed command over the patches.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

You can get rid of the memalloc_reserve and vmio_request_queues variables if you want, they aren't really needed for anything. If using them reduces the total code size I'd keep them though.

I find my version easier to read, but that might just be the way my brain works.

Maybe true, but I believe my version is more natural in the sense that it makes more clear what the code is doing. Less bookkeeping, more real work, so to speak.

Ok, I'll have another look at it, perhaps my gray matter has shifted ;-)

Ah, no accident there, I'm fully aware that there would need to be a spinlock in adjust_memalloc_reserve() if there were another caller. (I even had it there for some time) - added comment.

(quoted diff) +7 -0

+static inline void * emergency_rx_alloc(size_t size, gfp_t gfp_mask)
+{
+	void * page = NULL;
+
+	if (size > PAGE_SIZE)
+		return page;
+

Quoted 3 lines · Peter Zijlstra

Ah, no accident there, I'm fully aware that there would need to be a spinlock in adjust_memalloc_reserve() if there were another caller. (I even had it there for some time) - added comment.

Good that you're aware of it. Thing is, how much sense does the split-up into adjust_memalloc_reserve() and sk_adjust_memalloc() make at this point? Why not merge the code of adjust_memalloc_reserve() with sk_adjust_memalloc() and only add adjust_memalloc_reserve() when it's really needed? It saves an export.

Better to put the lock next to min_free_kbytes, both for readability and cache behaviour. And it satisfies the "lock data, not code" mantra.

If you prefer to avoid cmpxchg (which is often used in atomic_add_unless and can be expensive) then you can use something like:

net/core/sock.c __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 205

Quoted 11 lines

net/core/sock.c +7 -0

@@ -195,6 +196,86 @@ __u32 sysctl_rmem_default = SK_RMEM_MAX;
 /* Maximal space eaten by iovec or ancilliary data plus some space */
 int sysctl_optmem_max = sizeof(unsigned long)*(2*UIO_MAXIOV + 512);
 
+static DEFINE_SPINLOCK(memalloc_lock);
+static int memalloc_reserve;
+static unsigned int vmio_request_queues;
+
+atomic_t vmio_socks;
+atomic_t emergency_rx_pages_used;
+EXPORT_SYMBOL_GPL(vmio_socks);

Is this export needed? It's only used in net/core/skbuff.c and net/core/sock.c, which are compiled into one module.

Review __u32 sysctl_rmem_default = SK_RMEM_MAX; · line 206

Quoted 1 line

(quoted diff) +1 -0

+EXPORT_SYMBOL_GPL(emergency_rx_pages_used);

Same here. It's only used by code in sock.c and skbuff.c, and no external code calls emergency_rx_alloc(), nor emergency_rx_free().

I think I depleted my usefulness, there isn't much left to say for me. It's up to the big guys to decide about the merrit of this patch.

IMHO:

- This patch isn't really a framework, more a minimal fix for one specific, though important problem. But it's small and doesn't have much impact

- If Evgeniy's network allocator is as good as it looks, then why can't it replace the existing one? Just adding private subsystem specific memory allocators seems wrong. I might be missing the big picture, but it looks like memory allocator things should be at least synchronized and discussed with Christoph Lameter and his "modular slab allocator" patch.

All in all it seems it will take a while until Evgeniy's code will be merged, so I think applying Peter's patch soonish and removing it again the moment it becomes unnecessary is reasonable.

Quoted 4 lines · Indan Zupancic

Good that you're aware of it. Thing is, how much sense does the split-up into adjust_memalloc_reserve() and sk_adjust_memalloc() make at this point? Why not merge the code of adjust_memalloc_reserve() with sk_adjust_memalloc() and only add adjust_memalloc_reserve() when it's really needed? It saves an export.

mm/ vs net/core/

Well, perhaps, its merit is that is allows for full service for a few sockets even under severe memory pressure. And it provides the primitives to solve this problem for all instances, (NBD, iSCSI, NFS, AoE, ...) hence framework.

Evgeniy's allocator does not cater for this, so even if it were to replace all the allocation stuff, we would still need the SOCK_VMIO and all protocol hooks this patch introduces.

Quoted 5 lines

- If Evgeniy's network allocator is as good as it looks, then why can't it replace the existing one? Just adding private subsystem specific memory allocators seems wrong. I might be missing the big picture, but it looks like memory allocator things should be at least synchronized and discussed with Christoph Lameter and his "modular slab allocator" patch.

SLAB is very very good in that is will not suffer from external fragmentation (one could suffer from external fragmentation when viewing the slab allocator from the page allocation layer - but most of that is avoidable by allocation strategies in the slab layer), it does however suffer from internal fragmentation - by design.

For variable size allocators it has been proven that for each allocator there is an allocation pattern that will defeat it. And figuring the pattern out and proving it will not happen in a long-running system is hard hard work.

(free block coalescence is not a guarantee against fragmentation; there is even evidence that delayed coalescence will reduce fragmentation - it introduces history and this extra information can help predict the future.)

This is exactly why long running systems (like our kernel) love slabs.

For those interested in memory allocators, this paper is a good (albeit a bit dated) introduction:
http://citeseer.ist.psu.edu/wilson95dynamic.html

That said, it might be that Evgeniy's allocator works out for our network load - only time will tell, the math is not tractable afaik.

Provide a proper a_ops->swapfile() implementation for NFS. This will set the NFS socket to SOCK_VMIO and put the socket reconnection under PF_MEMALLOC (I hope this is enough, otherwise more work needs to be done).

Signed-off-by Peter Zijlstra <[email protected]>

---
 fs/nfs/file.c               |   21 ++++++++++++++++++++-
 include/linux/sunrpc/xprt.h |    4 +++-
 net/sunrpc/xprtsock.c       |   16 ++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/nfs/file.c
===================================================================

fs/nfs/file.c +11 -1

@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/pagemap.h>
 #include <linux/smp_lock.h>
+#include <net/sock.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -317,7 +318,25 @@ static int nfs_release_page(struct page 
 
 static int nfs_swapfile(struct address_space *mapping, int enable)
 {
-	return 0;
+	int err = -EINVAL;
+	struct rpc_clnt *client = NFS_CLIENT(mapping->host);
+	struct sock *sk = client->cl_xprt->inet;
+
+	if (enable) {
+		client->cl_xprt->swapper = 1;
+		/*
+		 * keep one extra sock reference so the reserve won't dip
+		 * when the socket gets reconnected.
+		 */

fs/nfs/file.c +0 -0

 const struct address_space_operations nfs_file_aops = {

Index: linux-2.6/net/sunrpc/xprtsock.c
===================================================================

net/sunrpc/xprtsock.c +12 -0

@@ -1014,6 +1014,7 @@ static void xs_udp_connect_worker(void *
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *) args;
 	struct socket *sock = xprt->sock;
+	unsigned long pflags = current->flags;
 	int err, status = -EIO;
 
 	if (xprt->shutdown || xprt->addr.sin_port == 0)

@@ -1021,6 +1022,9 @@ static void xs_udp_connect_worker(void *
 
 	dprintk("RPC:      xs_udp_connect_worker for xprt %p\n", xprt);
 
+	if (xprt->swapper)
+		current->flags |= PF_MEMALLOC;
+
 	/* Start by resetting any existing state */
 	xs_close(xprt);
 

@@ -1054,6 +1058,9 @@ static void xs_udp_connect_worker(void *
 		xprt->sock = sock;
 		xprt->inet = sk;
 
+		if (xprt->swapper)
+			sk_set_vmio(sk);
+
 		write_unlock_bh(&sk->sk_callback_lock);
 	}
 	xs_udp_do_set_buffer_size(xprt);

@@ -1061,6 +1068,7 @@ static void xs_udp_connect_worker(void *
 out:
 	xprt_wake_pending_tasks(xprt, status);
 	xprt_clear_connecting(xprt);
+	current->flags = pflags;
 }
 
 /*

@@ -1097,11 +1105,15 @@ static void xs_tcp_connect_worker(void *
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *)args;
 	struct socket *sock = xprt->sock;
+	unsigned long pflags = current->flags;
 	int err, status = -EIO;
 
 	if (xprt->shutdown || xprt->addr.sin_port == 0)
 		goto out;
 
+	if (xprt->swapper)
+		current->flags |= PF_MEMALLOC;
+
 	dprintk("RPC:      xs_tcp_connect_worker for xprt %p\n", xprt);
 
 	if (!xprt->sock) {

@@ -1170,10 +1182,14 @@ static void xs_tcp_connect_worker(void *
 				break;
 		}

Grumble... If your patches are targetting NFS, could you please at the very least Cc [email protected] and/or myself.

Quoted 43 lines

Provide a proper a_ops->swapfile() implementation for NFS. This will set the NFS socket to SOCK_VMIO and put the socket reconnection under PF_MEMALLOC (I hope this is enough, otherwise more work needs to be done).

Signed-off-by Peter Zijlstra <[email protected]>

---
 fs/nfs/file.c               |   21 ++++++++++++++++++++-
 include/linux/sunrpc/xprt.h |    4 +++-
 net/sunrpc/xprtsock.c       |   16 ++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/nfs/file.c
===================================================================

fs/nfs/file.c +16 -1

@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/pagemap.h>
 #include <linux/smp_lock.h>
+#include <net/sock.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -317,7 +318,25 @@ static int nfs_release_page(struct page 
 
 static int nfs_swapfile(struct address_space *mapping, int enable)
 {
-	return 0;
+	int err = -EINVAL;
+	struct rpc_clnt *client = NFS_CLIENT(mapping->host);
+	struct sock *sk = client->cl_xprt->inet;
+
+	if (enable) {
+		client->cl_xprt->swapper = 1;
+		/*
+		 * keep one extra sock reference so the reserve won't dip
+		 * when the socket gets reconnected.
+		 */
+		sk_adjust_memalloc(1, 1);
+		err = sk_set_vmio(sk);
+	} else if (client->cl_xprt->swapper) {
+		client->cl_xprt->swapper = 0;
+		sk_adjust_memalloc(-1, -1);

fs/nfs/file.c static int nfs_release_page(struct page · line 340

Quoted 5 lines

fs/nfs/file.c +4 -0

+		err = sk_clear_vmio(sk);
+	}
+
+	return err;
 }

This all belongs in net/sunrpc/xprtsock.c. The NFS code has no business screwing around with the internals of the sunrpc transport.

Quoted 56 lines

fs/nfs/file.c +0 -0

 const struct address_space_operations nfs_file_aops = {

Index: linux-2.6/net/sunrpc/xprtsock.c
===================================================================

net/sunrpc/xprtsock.c +12 -0

@@ -1014,6 +1014,7 @@ static void xs_udp_connect_worker(void *
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *) args;
 	struct socket *sock = xprt->sock;
+	unsigned long pflags = current->flags;
 	int err, status = -EIO;
 
 	if (xprt->shutdown || xprt->addr.sin_port == 0)

@@ -1021,6 +1022,9 @@ static void xs_udp_connect_worker(void *
 
 	dprintk("RPC:      xs_udp_connect_worker for xprt %p\n", xprt);
 
+	if (xprt->swapper)
+		current->flags |= PF_MEMALLOC;
+
 	/* Start by resetting any existing state */
 	xs_close(xprt);
 

@@ -1054,6 +1058,9 @@ static void xs_udp_connect_worker(void *
 		xprt->sock = sock;
 		xprt->inet = sk;
 
+		if (xprt->swapper)
+			sk_set_vmio(sk);
+
 		write_unlock_bh(&sk->sk_callback_lock);
 	}
 	xs_udp_do_set_buffer_size(xprt);

@@ -1061,6 +1068,7 @@ static void xs_udp_connect_worker(void *
 out:
 	xprt_wake_pending_tasks(xprt, status);
 	xprt_clear_connecting(xprt);
+	current->flags = pflags;
 }
 
 /*

@@ -1097,11 +1105,15 @@ static void xs_tcp_connect_worker(void *
 {
 	struct rpc_xprt *xprt = (struct rpc_xprt *)args;
 	struct socket *sock = xprt->sock;
+	unsigned long pflags = current->flags;
 	int err, status = -EIO;
 
 	if (xprt->shutdown || xprt->addr.sin_port == 0)
 		goto out;
 
+	if (xprt->swapper)
+		current->flags |= PF_MEMALLOC;
+
 	dprintk("RPC:      xs_tcp_connect_worker for xprt %p\n", xprt);
 

Quoted 4 lines

net/sunrpc/xprtsock.c +0 -0

 	if (!xprt->sock) {

@@ -1170,10 +1182,14 @@ static void xs_tcp_connect_worker(void *
 				break;
 		}

net/sunrpc/xprtsock.c static void xs_tcp_connect_worker(void * · line 1193

Quoted 10 lines

net/sunrpc/xprtsock.c +4 -0

 	}
+
+	if (xprt->swapper)
+		sk_set_vmio(xprt->inet);
 out:
 	xprt_wake_pending_tasks(xprt, status);
 out_clear:
 	xprt_clear_connecting(xprt);
+	current->flags = pflags;
 }

How does this guarantee that the socket reconnection won't fail?

Also, what about the case of rpc_malloc()? Can't that cause rpciod to deadlock when you add NFS swap into the equation?

	/* Check if this were the first socks: */
	if (nr_socks - socks == 0)
		reserve += RX_RESERVE_PAGES;

The basic premises is that network sockets serving the VM need undisturbed functionality in the face of severe memory shortage.

This patch-set provides the framework to provide this.

Quoted 4 lines · Peter Zijlstra

The basic premises is that network sockets serving the VM need undisturbed functionality in the face of severe memory shortage.

This patch-set provides the framework to provide this.

Hmmm.. Is it not possible to avoid the memory pools by guaranteeing that a certain number of page is easily reclaimable?

Quoted 4 lines · Peter Zijlstra

The basic premises is that network sockets serving the VM need undisturbed functionality in the face of severe memory shortage.

This patch-set provides the framework to provide this.

Hmmm.. Is it not possible to avoid the memory pools by guaranteeing that a certain number of page is easily reclaimable?