Thread (6 messages) 6 messages, 3 authors, 2006-08-11

Re: [RFC][PATCH] VM deadlock prevention core -v3

From: Evgeniy Polyakov <hidden>
Date: 2006-08-10 14:03:30
Also in: linux-mm, lkml

On Thu, Aug 10, 2006 at 03:32:49PM +0200, Peter Zijlstra (a.p.zijlstra@chello.nl) wrote:
Hi,
Hello, Peter.
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 hunk ↗ jump to hunk
--- linux-2.6.orig/net/core/skbuff.c
+++ linux-2.6/net/core/skbuff.c
@@ -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;
+		if (!(page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order)))
+			goto out;
+
+		kaddr = pfn_to_kaddr(page_to_pfn(page));
+		skb = (struct sk_buff *)kaddr;
+
+		*((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 hunk ↗ jump to hunk
+		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;
 
 	/* Get the DATA. Size must match skb_add_mtu(). */
-	size = SKB_DATA_ALIGN(size);
Bad sign.
 	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);
+	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 hunk ↗ jump to hunk
+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);
 	} else {
+		/*
+		 * should we special-case skb->memalloc cloning?
+		 * for now fudge it by forcing fast-clone alloc.
+		 */
+		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 hunk ↗ jump to hunk
@@ -686,6 +825,8 @@ int pskb_expand_head(struct sk_buff *skb
 	if (skb_shared(skb))
 		BUG();
 
+	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 hunk ↗ jump to hunk
+/**
+ *	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
===================================================================
--- linux-2.6.orig/net/ipv4/af_inet.c
+++ linux-2.6/net/ipv4/af_inet.c
@@ -132,6 +132,14 @@ void inet_sock_destruct(struct sock *sk)
 {
 	struct inet_sock *inet = inet_sk(sk);
 
+	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.

-- 
	Evgeniy Polyakov
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help