VM deadlock prevention core -v3

From the 'user' side of things it provides a function to mark a 'struct sock'

From the net_device side of things, the extra 'struct net_device *' argument

Hi,

So I try again, please tell me if I'm still on crack and should go detox. However if you do so, I kindly request some words on the how and why of it.

net/core/skbuff.c +42 -0

@@ -43,6 +43,7 @@
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/pagemap.h>
 #include <linux/interrupt.h>
 #include <linux/in.h>
 #include <linux/inet.h>

@@ -125,6 +126,8 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *
  */
 
+#define ceiling_log2(x)	fls((x) - 1)
+
 /**
  *	__alloc_skb	-	allocate a network buffer
  *	@size: size to allocate

@@ -147,6 +150,59 @@ struct sk_buff *__alloc_skb(unsigned int
 	struct sk_buff *skb;
 	u8 *data;
 
+	size = SKB_DATA_ALIGN(size);
+
+	if (gfp_mask & __GFP_MEMALLOC) {
+		/*
+		 * Fallback allocation for memalloc reserves.
+		 *
+		 * the page is populated like so:
+		 *
+		 *   struct sk_buff
+		 *   [ struct sk_buff ]
+		 *   [ atomic_t ]
+		 *   unsigned int
+		 *   struct skb_shared_info
+		 *   char []
+		 *
+		 * We have to do higher order allocations for icky jumbo
+		 * frame drivers :-(. They really should be migrated to
+		 * scather/gather DMA and use skb fragments.
+		 */
+		unsigned int data_offset =
+			sizeof(struct sk_buff) + sizeof(unsigned int);
+		unsigned long length = size + data_offset +
+			sizeof(struct skb_shared_info);
+		unsigned int pages;
+		unsigned int order;
+		struct page *page;
+		void *kaddr;
+
+		/*
+		 * Force fclone alloc in order to fudge a lacking in skb_clone().
+		 */
+		fclone = 1;
+		if (fclone) {
+			data_offset += sizeof(struct sk_buff) + sizeof(atomic_t);
+			length += sizeof(struct sk_buff) + sizeof(atomic_t);
+		}
+		pages = (length + PAGE_SIZE - 1) >> PAGE_SHIFT;
+		order = ceiling_log2(pages);
+		skb = NULL;

net/core/skbuff.c +15 -0

 	data = ____kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
 	if (!data)
 		goto nodata;
 
+struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
+		unsigned length, gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+
+	WARN_ON(gfp_mask & (__GFP_NOMEMALLOC | __GFP_MEMALLOC));
+	gfp_mask &= ~(__GFP_NOMEMALLOC | __GFP_MEMALLOC);
+
+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_NOMEMALLOC);
+	if (skb)
+		goto done;
+
+	if (atomic_read(&dev->rx_reserve_used) >=
+			dev->rx_reserve * dev->memalloc_socks)
+		goto out;

net/core/skbuff.c +39 -4

+done:
+	skb->dev = dev;
+out:
+	return skb;
+}
+
 static void skb_drop_list(struct sk_buff **listp)
 {
 	struct sk_buff *list = *listp;

@@ -313,10 +417,35 @@ static void skb_release_data(struct sk_b
 		if (skb_shinfo(skb)->frag_list)
 			skb_drop_fraglist(skb);
 
-		kfree(skb->head);
+		if (!skb->memalloc)
+			kfree(skb->head);
+		skb->head = NULL;
 	}
 }
 
+/**
+ *	free_skb_pages - frees a memalloced skbuff
+ *	@cache: fake %kmem_cache argument
+ *	@objp: %sk_buff pointer
+ *
+ *	Function is made to look like %kmem_cache_free so we can easily
+ *	substitue the free function in %kfree_skbmem.
+ */
+static void free_skb_pages(struct kmem_cache *cache, void *objp)
+{
+	struct sk_buff *skb = (struct sk_buff *)objp;
+	/*
+	 * The input_dev is the initial input device;
+	 * we have it pinned by virtue of rx_reserve_used not being zero.
+	 */
+	struct net_device *dev = skb->input_dev ?: skb->dev;
+	unsigned int order =
+		*(unsigned int *)(skb->head - sizeof(unsigned int));
+	if (!skb->head)
+		atomic_dec(&dev->rx_reserve_used);
+	free_pages((unsigned long)skb, order);
+}
+
 /*
  *	Free an skbuff by memory without cleaning the state.
  */

@@ -324,17 +453,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->memalloc ? free_skb_pages : 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:

@@ -347,7 +480,7 @@ void kfree_skbmem(struct sk_buff *skb)
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 
 		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+			free_skb(skbuff_fclone_cache, other);
 		break;
 	};
 }

@@ -434,6 +567,12 @@ struct sk_buff *skb_clone(struct sk_buff
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);

net/core/skbuff.c +74 -0

+/**
+ *	dev_adjust_memalloc - adjust the global memalloc reserve for this device
+ *	@dev: device that has memalloc demands
+ *	@nr_socks: number of new %SOCK_MEMALLOC sockets
+ *
+ *	This function adjusts the memalloc reserve based on device
+ *	demand. For each %SOCK_MEMALLOC socket this device will reserve
+ *	2 * %MAX_PHYS_SEGMENTS pages for outbound traffic (assumption:
+ *	each %SOCK_MEMALLOC socket will have a %request_queue associated)
+ *	and @dev->rx_reserve mtu pages.
+ */
+int dev_adjust_memalloc(struct net_device *dev, int nr_socks)
+{
+	unsigned long flags;
+	unsigned long reserve;
+	int err;
+
+	spin_lock_irqsave(&dev->memalloc_lock, flags);
+
+	dev->memalloc_socks += nr_socks;
+	BUG_ON(dev->memalloc_socks < 0);
+
+	reserve = dev->memalloc_socks *
+		(2 * MAX_PHYS_SEGMENTS +		 /* outbound */
+		 dev->rx_reserve * mtu_pages(dev->mtu)); /* inbound */
+
+	err = adjust_memalloc_reserve(reserve - dev->memalloc_reserve);
+	if (err) {
+		printk(KERN_WARNING
+			"%s: Unable to change RX reserve to: %lu, error: %d\n",
+			dev->name, reserve, err);
+		goto unlock;
+	}
+	dev->memalloc_reserve = reserve;
+
+unlock:
+	spin_unlock_irqrestore(&dev->memalloc_lock, flags);
+	return err;
+}
+EXPORT_SYMBOL_GPL(dev_adjust_memalloc);
 
 /*
  *	Device change register/unregister. These are not inline or static

@@ -2464,6 +2527,9 @@ int dev_set_mtu(struct net_device *dev, 
 		err = dev->change_mtu(dev, new_mtu);
 	else
 		dev->mtu = new_mtu;
+
+	dev_adjust_memalloc(dev, 0);
+
 	if (!err && dev->flags & IFF_UP)
 		raw_notifier_call_chain(&netdev_chain,
 				NETDEV_CHANGEMTU, dev);

@@ -2900,6 +2966,7 @@ int register_netdevice(struct net_device
 #ifdef CONFIG_NET_CLS_ACT
 	spin_lock_init(&dev->ingress_lock);
 #endif
+	spin_lock_init(&dev->memalloc_lock);
 
 	ret = alloc_divert_blk(dev);
 	if (ret)

@@ -3106,6 +3173,28 @@ static void netdev_wait_allrefs(struct n
 	}
 }
 
+/* netdev_wait_memalloc - wait for all outstanding memalloc skbs
+ *
+ * This is called when unregistering network devices.
+ *
+ * Better make sure the skb -> dev mapping is correct, if we leak
+ * some this will stall forever.
+ */
+static void netdev_wait_memalloc(struct net_device *dev)
+{
+	unsigned long warning_time = jiffies;
+	while (atomic_read(&dev->rx_reserve_used) != 0) {
+		msleep(250);
+		if (time_after(jiffies, warning_time + 10 * HZ)) {
+			printk(KERN_EMERG "netdev_wait_memalloc: "
+			       "waiting for %s to become free. SKB "
+			       "count = %d\n",
+			       dev->name, atomic_read(&dev->rx_reserve_used));
+			warning_time = jiffies;
+		}
+	}
+}
+
 /* The sequence is:
  *
  *	rtnl_lock();

@@ -3165,6 +3254,14 @@ void netdev_run_todo(void)
 
 		netdev_wait_allrefs(dev);
 
+		netdev_wait_memalloc(dev);
+
+		/* Get rid of any SOCK_MEMALLOC reserves. */
+		if (dev->memalloc_reserve) {
+			BUG_ON(!dev->memalloc_socks);
+			dev_adjust_memalloc(dev, -dev->memalloc_socks);
+		}
+
 		/* paranoia */
 		BUG_ON(atomic_read(&dev->refcnt));
 		BUG_TRAP(!dev->ip_ptr);

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

net/ipv4/af_inet.c +0 -0

@@ -132,6 +132,14 @@ void inet_sock_destruct(struct sock *sk)
 {

Quoted 1 line · Peter Zijlstra

Hi,

Hello, Peter.

Quoted 2 lines

So I try again, please tell me if I'm still on crack and should go detox. However if you do so, I kindly request some words on the how and why of it.

I think you should talk with doctor in that case, but not with kernel hackers :)

I have some comments about implementation, not overall design, since we have slightly diametral points of view there.

....

Quoted 23 lines

net/core/skbuff.c +3 -0

@@ -43,6 +43,7 @@
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/pagemap.h>
 #include <linux/interrupt.h>
 #include <linux/in.h>
 #include <linux/inet.h>

@@ -125,6 +126,8 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *
  */
 
+#define ceiling_log2(x)	fls((x) - 1)
+
 /**
  *	__alloc_skb	-	allocate a network buffer
  *	@size: size to allocate

@@ -147,6 +150,59 @@ struct sk_buff *__alloc_skb(unsigned int
 	struct sk_buff *skb;
 	u8 *data;
 

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

Quoted 1 line

net/core/skbuff.c +1 -0

+	size = SKB_DATA_ALIGN(size);

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

Quoted 5 lines

net/core/skbuff.c +5 -0

+
+	if (gfp_mask & __GFP_MEMALLOC) {
+		/*
+		 * Fallback allocation for memalloc reserves.
+

Quoted 38 lines

net/core/skbuff.c +38 -0

+		 * the page is populated like so:
+		 *
+		 *   struct sk_buff
+		 *   [ struct sk_buff ]
+		 *   [ atomic_t ]
+		 *   unsigned int
+		 *   struct skb_shared_info
+		 *   char []
+		 *
+		 * We have to do higher order allocations for icky jumbo
+		 * frame drivers :-(. They really should be migrated to
+		 * scather/gather DMA and use skb fragments.
+		 */
+		unsigned int data_offset =
+			sizeof(struct sk_buff) + sizeof(unsigned int);
+		unsigned long length = size + data_offset +
+			sizeof(struct skb_shared_info);
+		unsigned int pages;
+		unsigned int order;
+		struct page *page;
+		void *kaddr;
+
+		/*
+		 * Force fclone alloc in order to fudge a lacking in skb_clone().
+		 */
+		fclone = 1;
+		if (fclone) {
+			data_offset += sizeof(struct sk_buff) + sizeof(atomic_t);
+			length += sizeof(struct sk_buff) + sizeof(atomic_t);
+		}
+		pages = (length + PAGE_SIZE - 1) >> PAGE_SHIFT;
+		order = ceiling_log2(pages);
+		skb = NULL;
+		if (!(page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order)))
+			goto out;
+
+		kaddr = pfn_to_kaddr(page_to_pfn(page));
+		skb = (struct sk_buff *)kaddr;

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

Quoted 5 lines

net/core/skbuff.c +5 -0

+
+		*((unsigned int *)(kaddr + data_offset -
+					sizeof(unsigned int))) = order;
+		data = (u8 *)(kaddr + data_offset);
+

Tricky, but since you are using own allocator here, you could change it to be not so aggressive - i.e. do not round size to number of pages.

Quoted 6 lines

net/core/skbuff.c +3 -0

+		goto allocated;
+	}
+
 	cache = fclone ? skbuff_fclone_cache : skbuff_head_cache;
 

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

Quoted 6 lines

net/core/skbuff.c +0 -1

 	/* Get the HEAD */

@@ -155,12 +211,13 @@ struct sk_buff *__alloc_skb(unsigned int
 		goto out;
 
 	/* Get the DATA. Size must match skb_add_mtu(). */
-	size = SKB_DATA_ALIGN(size);

Bad sign.

Quoted 24 lines

net/core/skbuff.c +20 -0

 	data = ____kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
 	if (!data)
 		goto nodata;
 
+struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
+		unsigned length, gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+
+	WARN_ON(gfp_mask & (__GFP_NOMEMALLOC | __GFP_MEMALLOC));
+	gfp_mask &= ~(__GFP_NOMEMALLOC | __GFP_MEMALLOC);
+
+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_NOMEMALLOC);
+	if (skb)
+		goto done;
+
+	if (atomic_read(&dev->rx_reserve_used) >=
+			dev->rx_reserve * dev->memalloc_socks)
+		goto out;
+
+	/*
+	 * pre-inc guards against a race with netdev_wait_memalloc()
+	 */
+	atomic_inc(&dev->rx_reserve_used);

net/core/skbuff.c struct sk_buff *__netdev_alloc_skb(struc · line 372

Quoted 5 lines

net/core/skbuff.c +5 -0

+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_MEMALLOC);
+	if (unlikely(!skb)) {
+		atomic_dec(&dev->rx_reserve_used);
+		goto out;
+	}

Since you have added atomic operation in that path, you can use device's reference counter instead and do not care that it can dissapear.

Quoted 8 lines

net/core/skbuff.c +4 -0

@@ -434,6 +567,12 @@ struct sk_buff *skb_clone(struct sk_buff
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);
 	} else {
+		/*
+		 * should we special-case skb->memalloc cloning?
+		 * for now fudge it by forcing fast-clone alloc.
+		 */

net/core/skbuff.c struct sk_buff *skb_clone(struct sk_buff · line 578

Quoted 5 lines

net/core/skbuff.c +2 -0

+		BUG_ON(skb->memalloc);
+
 		n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 		if (!n)
 			return NULL;

Ugh... cloning is a one of the shoulders of giant where Linux network stack is staying...

Quoted 4 lines

net/core/skbuff.c +0 -0

@@ -686,6 +825,8 @@ int pskb_expand_head(struct sk_buff *skb
 	if (skb_shared(skb))
 		BUG();
 

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

Quoted 5 lines

net/core/skbuff.c +2 -0

+	BUG_ON(skb->memalloc);
+
 	size = SKB_DATA_ALIGN(size);
 
 	data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);

And that is a bug.
That operation can happen even with usual receiving processing.

Quoted 8 lines

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

net/ipv4/af_inet.c +0 -0

@@ -132,6 +132,14 @@ void inet_sock_destruct(struct sock *sk)
 {
 	struct inet_sock *inet = inet_sk(sk);
 

net/ipv4/af_inet.c void inet_sock_destruct(struct sock *sk) · line 142

Quoted 8 lines

net/ipv4/af_inet.c +8 -0

+	if (sk_is_memalloc(sk)) {
+		struct net_device *dev = ip_dev_find(inet->rcv_saddr);
+		if (dev) {
+			dev_adjust_memalloc(dev, -1);
+			dev_put(dev);
+		}
+	}
+

This looks very strange - you decrement reference counter both in socket destruction code and in netdevice destruction code.

Quoted 39 lines · Evgeniy Polyakov

Quoted 1 line · Peter Zijlstra

Hi,

Hello, Peter.

Quoted 2 lines

So I try again, please tell me if I'm still on crack and should go detox. However if you do so, I kindly request some words on the how and why of it.

I think you should talk with doctor in that case, but not with kernel hackers :)

I have some comments about implementation, not overall design, since we have slightly diametral points of view there.

....

Quoted 23 lines

net/core/skbuff.c +3 -0

@@ -43,6 +43,7 @@
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/pagemap.h>
 #include <linux/interrupt.h>
 #include <linux/in.h>
 #include <linux/inet.h>

@@ -125,6 +126,8 @@ EXPORT_SYMBOL(skb_truesize_bug);
  *
  */
 
+#define ceiling_log2(x)	fls((x) - 1)
+
 /**
  *	__alloc_skb	-	allocate a network buffer
  *	@size: size to allocate

@@ -147,6 +150,59 @@ struct sk_buff *__alloc_skb(unsigned int
 	struct sk_buff *skb;
 	u8 *data;
 

Quoted 1 line

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

Quoted 1 line

net/core/skbuff.c +1 -0

+	size = SKB_DATA_ALIGN(size);

I moved it here.

Quoted 5 lines

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

Quoted 5 lines

net/core/skbuff.c +5 -0

+
+	if (gfp_mask & __GFP_MEMALLOC) {
+		/*
+		 * Fallback allocation for memalloc reserves.
+

* This allocator is build on alloc_pages() so that freed
* skbuffs return to the memalloc reserve imediately. SLAB
* memory might not ever be returned.

This was missing,...

Quoted 38 lines

Quoted 38 lines

net/core/skbuff.c +38 -0

+		 * the page is populated like so:
+		 *
+		 *   struct sk_buff
+		 *   [ struct sk_buff ]
+		 *   [ atomic_t ]
+		 *   unsigned int
+		 *   struct skb_shared_info
+		 *   char []
+		 *
+		 * We have to do higher order allocations for icky jumbo
+		 * frame drivers :-(. They really should be migrated to
+		 * scather/gather DMA and use skb fragments.
+		 */
+		unsigned int data_offset =
+			sizeof(struct sk_buff) + sizeof(unsigned int);
+		unsigned long length = size + data_offset +
+			sizeof(struct skb_shared_info);
+		unsigned int pages;
+		unsigned int order;
+		struct page *page;
+		void *kaddr;
+
+		/*
+		 * Force fclone alloc in order to fudge a lacking in skb_clone().
+		 */
+		fclone = 1;
+		if (fclone) {
+			data_offset += sizeof(struct sk_buff) + sizeof(atomic_t);
+			length += sizeof(struct sk_buff) + sizeof(atomic_t);
+		}
+		pages = (length + PAGE_SIZE - 1) >> PAGE_SHIFT;
+		order = ceiling_log2(pages);
+		skb = NULL;
+		if (!(page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order)))
+			goto out;
+
+		kaddr = pfn_to_kaddr(page_to_pfn(page));
+		skb = (struct sk_buff *)kaddr;

Quoted 8 lines

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

Quoted 5 lines

net/core/skbuff.c +5 -0

+
+		*((unsigned int *)(kaddr + data_offset -
+					sizeof(unsigned int))) = order;
+		data = (u8 *)(kaddr + data_offset);
+

Tricky, but since you are using own allocator here, you could change it to be not so aggressive - i.e. do not round size to number of pages.

I'm not sure I follow you, I'm explicitly using alloc_pages()/free_page(), if
I were to go smart here, I'd loose the whole reason for doing so.

Quoted 10 lines

Quoted 10 lines

net/core/skbuff.c +3 -0

+		goto allocated;
+	}
+
 	cache = fclone ? skbuff_fclone_cache : skbuff_head_cache;
 
 	/* Get the HEAD */

@@ -155,12 +211,13 @@ struct sk_buff *__alloc_skb(unsigned int
 		goto out;
 

Quoted 4 lines

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

Quoted 2 lines

net/core/skbuff.c +0 -1

 	/* Get the DATA. Size must match skb_add_mtu(). */
-	size = SKB_DATA_ALIGN(size);

Bad sign.

See above.

Quoted 24 lines

Quoted 24 lines

net/core/skbuff.c +20 -0

 	data = ____kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);
 	if (!data)
 		goto nodata;
 
+struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
+		unsigned length, gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+
+	WARN_ON(gfp_mask & (__GFP_NOMEMALLOC | __GFP_MEMALLOC));
+	gfp_mask &= ~(__GFP_NOMEMALLOC | __GFP_MEMALLOC);
+
+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_NOMEMALLOC);
+	if (skb)
+		goto done;
+
+	if (atomic_read(&dev->rx_reserve_used) >=
+			dev->rx_reserve * dev->memalloc_socks)
+		goto out;
+
+	/*
+	 * pre-inc guards against a race with netdev_wait_memalloc()
+	 */
+	atomic_inc(&dev->rx_reserve_used);

Quoted 8 lines

net/core/skbuff.c struct sk_buff *__netdev_alloc_skb(struc · line 372

Quoted 5 lines

net/core/skbuff.c +5 -0

+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_MEMALLOC);
+	if (unlikely(!skb)) {
+		atomic_dec(&dev->rx_reserve_used);
+		goto out;
+	}

Since you have added atomic operation in that path, you can use device's reference counter instead and do not care that it can dissapear.

Is that the sole reason taking a reference on the device is bad?

Quoted 8 lines

Quoted 8 lines

net/core/skbuff.c +4 -0

@@ -434,6 +567,12 @@ struct sk_buff *skb_clone(struct sk_buff
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);
 	} else {
+		/*
+		 * should we special-case skb->memalloc cloning?
+		 * for now fudge it by forcing fast-clone alloc.
+		 */

Quoted 8 lines

net/core/skbuff.c struct sk_buff *skb_clone(struct sk_buff · line 578

Quoted 5 lines

net/core/skbuff.c +2 -0

+		BUG_ON(skb->memalloc);
+
 		n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 		if (!n)
 			return NULL;

Ugh... cloning is a one of the shoulders of giant where Linux network stack is staying...

Yes, I'm aware of that, I have a plan to fix this, however I haven't had time
to implement it. My immediate concern is the point wrt. the net_device mapping.

My idea was: instead of the order, store the size, and allocate clone skbuffs in the available room at the end of the page(s), allocating extra pages
if needed.

Quoted 4 lines

Quoted 4 lines

net/core/skbuff.c +0 -0

@@ -686,6 +825,8 @@ int pskb_expand_head(struct sk_buff *skb
 	if (skb_shared(skb))
 		BUG();
 

Quoted 8 lines

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

Quoted 5 lines

net/core/skbuff.c +2 -0

+	BUG_ON(skb->memalloc);
+
 	size = SKB_DATA_ALIGN(size);
 
 	data = kmalloc(size + sizeof(struct skb_shared_info), gfp_mask);

And that is a bug.
That operation can happen even with usual receiving processing.

Yes, more allocator work..

Quoted 8 lines

Quoted 8 lines

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

net/ipv4/af_inet.c +0 -0

@@ -132,6 +132,14 @@ void inet_sock_destruct(struct sock *sk)
 {
 	struct inet_sock *inet = inet_sk(sk);
 

Quoted 11 lines

net/ipv4/af_inet.c void inet_sock_destruct(struct sock *sk) · line 142

Quoted 8 lines

net/ipv4/af_inet.c +8 -0

+	if (sk_is_memalloc(sk)) {
+		struct net_device *dev = ip_dev_find(inet->rcv_saddr);
+		if (dev) {
+			dev_adjust_memalloc(dev, -1);
+			dev_put(dev);
+		}
+	}
+

This looks very strange - you decrement reference counter both in socket destruction code and in netdevice destruction code.

This is the right place; however I was not sure net_device destruction implied
destruction of all related sockets; in case that is not so, you will have to
clean up there, because this destructor will not find the device any longer.

Quoted 6 lines

Quoted 2 lines

Tricky, but since you are using own allocator here, you could change it to be not so aggressive - i.e. do not round size to number of pages.

I'm not sure I follow you, I'm explicitly using alloc_pages()/free_page(), if
I were to go smart here, I'd loose the whole reason for doing so.

You can use page to put there several skbs for example or at least add there a fclone (fast clone).

Quoted 29 lines

Quoted 28 lines

Quoted 15 lines

net/core/skbuff.c +15 -0

+struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
+		unsigned length, gfp_t gfp_mask)
+{
+	struct sk_buff *skb;
+
+	WARN_ON(gfp_mask & (__GFP_NOMEMALLOC | __GFP_MEMALLOC));
+	gfp_mask &= ~(__GFP_NOMEMALLOC | __GFP_MEMALLOC);
+
+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_NOMEMALLOC);
+	if (skb)
+		goto done;
+
+	if (atomic_read(&dev->rx_reserve_used) >=
+			dev->rx_reserve * dev->memalloc_socks)
+		goto out;

net/core/skbuff.c struct sk_buff *__netdev_alloc_skb(struc · line 372

Quoted 10 lines

net/core/skbuff.c +10 -0

+
+	/*
+	 * pre-inc guards against a race with netdev_wait_memalloc()
+	 */
+	atomic_inc(&dev->rx_reserve_used);
+	skb = ___netdev_alloc_skb(dev, length, gfp_mask | __GFP_MEMALLOC);
+	if (unlikely(!skb)) {
+		atomic_dec(&dev->rx_reserve_used);
+		goto out;
+	}

Since you have added atomic operation in that path, you can use device's reference counter instead and do not care that it can dissapear.

Quoted 1 line

Is that the sole reason taking a reference on the device is bad?

Taking a reference is bad due to performance reasons, since atomic increment is not that cheap. If you do it for one variable for the purpose of reference counting you can use device's refcnt istead, which will solve some races.

Quoted 22 lines

Quoted 8 lines

Quoted 8 lines

net/core/skbuff.c +4 -0

@@ -434,6 +567,12 @@ struct sk_buff *skb_clone(struct sk_buff
 		n->fclone = SKB_FCLONE_CLONE;
 		atomic_inc(fclone_ref);
 	} else {
+		/*
+		 * should we special-case skb->memalloc cloning?
+		 * for now fudge it by forcing fast-clone alloc.
+		 */

Quoted 8 lines

net/core/skbuff.c struct sk_buff *skb_clone(struct sk_buff · line 578

Quoted 5 lines

net/core/skbuff.c +2 -0

+		BUG_ON(skb->memalloc);
+
 		n = kmem_cache_alloc(skbuff_head_cache, gfp_mask);
 		if (!n)
 			return NULL;

Ugh... cloning is a one of the shoulders of giant where Linux network stack is staying...

Yes, I'm aware of that, I have a plan to fix this, however I haven't had time
to implement it. My immediate concern is the point wrt. the net_device mapping.

Quoted 4 lines

My idea was: instead of the order, store the size, and allocate clone skbuffs in the available room at the end of the page(s), allocating extra pages
if needed.

You can check if requested skb with fclone fits allocated pages, and if so use fclone magic, otherwise postpone clone allocation until it is required.

Sockets can live without network devices at all, I expect it is enough to clean up in socket destructor, since packets can come from different devices into the same socket.

You are right if the reserve wasn't device bound - which I will abandon because you are right that with multi-path routing, bridge device and other advanced goodies this scheme is broken in that there is no unambiguous mapping from sockets to devices.