Net vm deadlock fix, version 5

RFC In review Daniel Phillips 2 participants 2 messages Aug 8, 2005 – Aug 9, 2005

Subsystems (11)

Networking Network drivers Memory management MM core +7 more subsystems

lore

Share & tools

lore

Daniel Phillips

author

2005-08-08 10:12 UTC #0 lore raw

RFC 1/1 Net vm deadlock fix, version 5

Hi,

This version introduces the idea of having a network driver adjust the global memalloc reserve when it brings an interface up or down. The interface is:

int adjust_memalloc_reserve(int bytes)

which is just a thin shell over the min_free_kbytes interface that already exists.

The driver estimates the maximum number of pages it could ever need from the reserve and saves it in the net_device struct, so that exactly the same number of pages will be added to the reserve on bring-up and removed from it on bring-down.

The tricky part is figuring out how many pages could ever be needed. I wrote some helper functions to estimate how many pages a given number of skbs would use. These functions are deficient in a number of ways, but today the point isn't to be exactly correct, but to give a sense of what the complete solution will look like.

I am still continuously comparing the memalloc way of doing things to the mempool way. Today I had a look at the number of different places where mempools would be needed on the pre-protocol packet delivery path. It is scary.

With the memalloc way, the interfaces are fairly lightweight, but the accounting isn't as precise. It doesn't need to be precise either, we really just have to be sure that the reservation is more than enough to handle a single block IO transaction. But actually, the code tries to arrange for about 50 in flight at once, which is a very big safety factor.

Adjusting the global memalloc reserve accurately is more of a concern. If the adjustment wildly low, then a lowly network driver could end up eating the whole global reserve and wedge the entire system. So my ongoing effort will be directed at accounting the reserve limits in pages, rather than skbs as currently. This gets tricky, because skbs are not allocated for pages, they are allocated from slab caches. Maybe skbs really should be allocated from pages, it would not be much different from the way block IO works. Or maybe I just need to roll up my sleeves and create an interface where I can find out exactly how many additional pages my skb allocation actually consumed.

Anyway, I am now at least making an attempt to account for global memalloc pool usage sanely, whereas traditionally we have just made the memalloc reserve "big" and hoped for the best. (At this point, somebody stands up and says "use mempool" and I say, you fix the patch to use mempool. It's messier than you think.)

The e1000 driver currently does not implement the deadlock-avoidance path if packet splitting is enabled, because other bits of memory start showing up in the packet path and the accounting gets more complex than I was prepared to tackle today. Maybe tomorrow.

Regards,

Daniel

drivers/net/e1000/e1000_main.c +10 -3

diff -up --recursive 2.6.12.3.clean/drivers/net/e1000/e1000_main.c 2.6.12.3/drivers/net/e1000/e1000_main.c
@@ -309,6 +309,14 @@ e1000_up(struct e1000_adapter *adapter)
 			e1000_phy_reset(&adapter->hw);
 	}
 
+	netdev->memalloc_pages = estimate_skb_pages(netdev->rx_reserve,
+		adapter->rx_buffer_len + NET_IP_ALIGN);
+	if ((err = adjust_memalloc_reserve(netdev->memalloc_pages))) {
+		DPRINTK(PROBE, ERR,
+		    "Unable to allocate rx reserve Error: %d\n", err);
+		return err;
+	}
+
 	e1000_set_multi(netdev);
 
 	e1000_restore_vlan(adapter);

@@ -386,6 +394,7 @@ e1000_down(struct e1000_adapter *adapter
 		mii_reg |= MII_CR_POWER_DOWN;
 		e1000_write_phy_reg(&adapter->hw, PHY_CTRL, mii_reg);
 		mdelay(1);
+	adjust_memalloc_reserve(-netdev->memalloc_pages);
 	}
 }

@@ -3242,7 +3251,7 @@ e1000_alloc_rx_buffers_ps(struct e1000_a
 				cpu_to_le64(ps_page_dma->ps_page_dma[j]);
 		}
 
-		skb = dev_alloc_skb(adapter->rx_ps_bsize0 + NET_IP_ALIGN);
+		skb = dev_memalloc_skb(netdev, adapter->rx_ps_bsize0 + NET_IP_ALIGN);
 
 		if(unlikely(!skb))
 			break;

@@ -3253,8 +3262,6 @@ e1000_alloc_rx_buffers_ps(struct e1000_a
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_ps_bsize0;
 		buffer_info->dma = pci_map_single(pdev, skb->data,

include/linux/gfp.h +1 -0

diff -up --recursive 2.6.12.3.clean/include/linux/gfp.h 2.6.12.3/include/linux/gfp.h
@@ -39,6 +39,7 @@ struct vm_area_struct;
 #define __GFP_COMP	0x4000u	/* Add compound page metadata */
 #define __GFP_ZERO	0x8000u	/* Return zeroed page on success */
 #define __GFP_NOMEMALLOC 0x10000u /* Don't use emergency reserves */
+#define __GFP_MEMALLOC  0x20000u /* Use emergency reserves */
 
 #define __GFP_BITS_SHIFT 20	/* Room for 20 __GFP_FOO bits */
 #define __GFP_BITS_MASK ((1 << __GFP_BITS_SHIFT) - 1)

include/linux/mmzone.h +1 -0

diff -up --recursive 2.6.12.3.clean/include/linux/mmzone.h 2.6.12.3/include/linux/mmzone.h
@@ -378,6 +378,7 @@ int min_free_kbytes_sysctl_handler(struc
 extern int sysctl_lowmem_reserve_ratio[MAX_NR_ZONES-1];
 int lowmem_reserve_ratio_sysctl_handler(struct ctl_table *, int, struct file *,
 					void __user *, size_t *, loff_t *);
+int adjust_memalloc_reserve(int bytes);
 
 #include <linux/topology.h>
 /* Returns the number of the current Node. */

include/linux/netdevice.h +46 -0

58 lines not on this page · raw message

include/linux/skbuff.h +2 -0

diff -up --recursive 2.6.12.3.clean/include/linux/skbuff.h 2.6.12.3/include/linux/skbuff.h
@@ -994,6 +994,8 @@ static inline struct sk_buff *__dev_allo
 extern struct sk_buff *__dev_alloc_skb(unsigned int length, int gfp_mask);
 #endif
 
+unsigned estimate_skb_pages(unsigned howmany, unsigned size);
+
 /**
  *	dev_alloc_skb - allocate an skbuff for sending
  *	@length: length to allocate

include/linux/slab.h +1 -0

diff -up --recursive 2.6.12.3.clean/include/linux/slab.h 2.6.12.3/include/linux/slab.h
@@ -65,6 +65,7 @@ extern void *kmem_cache_alloc(kmem_cache
 extern void kmem_cache_free(kmem_cache_t *, void *);
 extern unsigned int kmem_cache_size(kmem_cache_t *);
 extern kmem_cache_t *kmem_find_general_cachep(size_t size, int gfpflags);
+unsigned kmem_estimate_pages(kmem_cache_t *cache, unsigned num);
 
 /* Size description struct for general caches. */
 struct cache_sizes {

include/net/sock.h +6 -0

diff -up --recursive 2.6.12.3.clean/include/net/sock.h 2.6.12.3/include/net/sock.h
@@ -382,6 +382,7 @@ enum sock_flags {
 	SOCK_NO_LARGESEND, /* whether to sent large segments or not */
 	SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */
 	SOCK_QUEUE_SHRUNK, /* write queue has been shrunk recently */
+	SOCK_MEMALLOC, /* protocol can use memalloc reserve */
 };
 
 static inline void sock_set_flag(struct sock *sk, enum sock_flags flag)

@@ -399,6 +400,11 @@ static inline int sock_flag(struct sock 
 	return test_bit(flag, &sk->sk_flags);
 }
 
+static inline int is_memalloc_sock(struct sock *sk)
+{
+	return sock_flag(sk, SOCK_MEMALLOC);
+}
+
 static inline void sk_acceptq_removed(struct sock *sk)
 {
 	sk->sk_ack_backlog--;

mm/page_alloc.c +14 -2

diff -up --recursive 2.6.12.3.clean/mm/page_alloc.c 2.6.12.3/mm/page_alloc.c
@@ -802,8 +802,8 @@ __alloc_pages(unsigned int __nocast gfp_
 
 	/* This allocation should allow future memory freeing. */
 
-	if (((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE)))
-			&& !in_interrupt()) {
+	if ((((p->flags & PF_MEMALLOC) || unlikely(test_thread_flag(TIF_MEMDIE)))
+			&& !in_interrupt()) || (gfp_mask & __GFP_MEMALLOC)) {
 		if (!(gfp_mask & __GFP_NOMEMALLOC)) {
 			/* go through the zonelist yet again, ignoring mins */
 			for (i = 0; (z = zones[i]) != NULL; i++) {

@@ -2075,6 +2075,18 @@ static void setup_per_zone_pages_min(voi
 	}
 }
 
+int adjust_memalloc_reserve(int pages)
+{
+	int kbytes = pages << (PAGE_SHIFT - 10);
+	if (min_free_kbytes + kbytes < 0)
+		return -EINVAL;
+	min_free_kbytes += kbytes;
+	setup_per_zone_pages_min();
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(adjust_memalloc_reserve);
+
 /*
  * Initialise min_free_kbytes.
  *

mm/slab.c +5 -0

diff -up --recursive 2.6.12.3.clean/mm/slab.c 2.6.12.3/mm/slab.c
@@ -2353,6 +2353,11 @@ out:
 	return 0;
 }
 
+unsigned kmem_estimate_pages(kmem_cache_t *cache, unsigned num)
+{
+	return ((num + cache->num - 1) / cache->num) << cache->gfporder;
+}
+
 #ifdef CONFIG_NUMA
 /**
  * kmem_cache_alloc_node - Allocate an object on the specified node

net/core/skbuff.c +9 -0

diff -up --recursive 2.6.12.3.clean/net/core/skbuff.c 2.6.12.3/net/core/skbuff.c
@@ -44,6 +44,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>

@@ -167,6 +168,14 @@ nodata:
 	goto out;
 }
 
+unsigned estimate_skb_pages(unsigned num, unsigned size)
+{
+	int slab_pages = kmem_estimate_pages(skbuff_head_cache, num);
+	int data_space = num * (1 << (ffs(size + 16 - 1) + 1));
+	int data_pages = (data_space + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
+	return slab_pages + data_pages;
+}
+
 /**
  *	alloc_skb_from_cache	-	allocate a network buffer
  *	@cp: kmem_cache from which to allocate the data area

net/ethernet/eth.c +1 -0

diff -up --recursive 2.6.12.3.clean/net/ethernet/eth.c 2.6.12.3/net/ethernet/eth.c
@@ -281,6 +281,7 @@ void ether_setup(struct net_device *dev)
 	dev->mtu		= 1500; /* eth_mtu */
 	dev->addr_len		= ETH_ALEN;
 	dev->tx_queue_len	= 1000;	/* Ethernet wants good queues */	
+	dev->rx_reserve		= 50;
 	dev->flags		= IFF_BROADCAST|IFF_MULTICAST;
 	
 	memset(dev->broadcast,0xFF, ETH_ALEN);

net/ipv4/icmp.c +5 -0

diff -up --recursive 2.6.12.3.clean/net/ipv4/icmp.c 2.6.12.3/net/ipv4/icmp.c
@@ -944,6 +944,11 @@ int icmp_rcv(struct sk_buff *skb)
 	default:;
 	}
 
+	if (dev_reserve_used(skb->dev)) {
+		dev_unreserve_one(skb->dev);
+		goto drop;
+	}
+
 	if (!pskb_pull(skb, sizeof(struct icmphdr)))
 		goto error;

net/ipv4/tcp_ipv4.c +6 -0

diff -up --recursive 2.6.12.3.clean/net/ipv4/tcp_ipv4.c 2.6.12.3/net/ipv4/tcp_ipv4.c
@@ -1766,6 +1766,12 @@ int tcp_v4_rcv(struct sk_buff *skb)
 	if (!sk)
 		goto no_tcp_socket;
 
+	if (unlikely(dev_reserve_used(skb->dev))) {
+		dev_unreserve_one(skb->dev);
+		if (!is_memalloc_sock(sk))
+			goto discard_and_relse;
+	}
+
 process:
 	if (sk->sk_state == TCP_TIME_WAIT)
 		goto do_time_wait;

net/ipv4/udp.c +7 -0

diff -up --recursive 2.6.12.3.clean/net/ipv4/udp.c 2.6.12.3/net/ipv4/udp.c
@@ -1152,6 +1152,12 @@ int udp_rcv(struct sk_buff *skb)
 	sk = udp_v4_lookup(saddr, uh->source, daddr, uh->dest, skb->dev->ifindex);
 
 	if (sk != NULL) {
+		if (unlikely(dev_reserve_used(skb->dev))) {
+			dev_unreserve_one(skb->dev);
+			if (!is_memalloc_sock(sk))
+				goto drop_noncritical;
+		}
+
 		int ret = udp_queue_rcv_skb(sk, skb);
 		sock_put(sk);

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Daniel Phillips

2005-08-09 03:37 UTC #1 lore raw

Hi,

A couple of goofs. First, the sysctl interface to min_free_kbytes could stomp on any in-kernel adjustments. Now there are two variables, summed in
setup_per_zone_pages_min: min_free_kbytes and var_free_kbytes. The adjust_memalloc_reserve operates only the latter, so the user can freely twiddle the base reserve without interfering with in-kernel adjustments.

Second, the skb resource estimation code was grossly wrong where it attempted to calculate ceiling(log2(size)), now fixed and tested this time.

To save bandwidth, just the relevant parts of the patch included below.

Regards,

Daniel

mm/page_alloc.c +15 -1

diff -up --recursive 2.6.12.3.clean/mm/page_alloc.c 2.6.12.3/mm/page_alloc.c
@@ -73,6 +73,7 @@ EXPORT_SYMBOL(zone_table);
 
 static char *zone_names[MAX_NR_ZONES] = { "DMA", "Normal", "HighMem" };
 int min_free_kbytes = 1024;
+int var_free_kbytes;
 
 unsigned long __initdata nr_kernel_pages;
 unsigned long __initdata nr_all_pages;

@@ -2029,7 +2030,8 @@ static void setup_per_zone_lowmem_reserv
  */
 static void setup_per_zone_pages_min(void)
 {
-	unsigned long pages_min = min_free_kbytes >> (PAGE_SHIFT - 10);
+	unsigned pages_min = (min_free_kbytes + var_free_kbytes)
+		>> (PAGE_SHIFT - 10);
 	unsigned long lowmem_pages = 0;
 	struct zone *zone;
 	unsigned long flags;

@@ -2075,6 +2077,18 @@ static void setup_per_zone_pages_min(voi
 	}
 }
 
+int adjust_memalloc_reserve(int pages)
+{
+	int kbytes = var_free_kbytes + (pages << (PAGE_SHIFT - 10));
+	if (kbytes < 0)
+		return -EINVAL;
+	var_free_kbytes = kbytes;
+	setup_per_zone_pages_min();
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(adjust_memalloc_reserve);
+
 /*
  * Initialise min_free_kbytes.
  *

net/core/skbuff.c +9 -0

diff -up --recursive 2.6.12.3.clean/net/core/skbuff.c 2.6.12.3/net/core/skbuff.c
@@ -167,6 +168,15 @@ nodata:
 	goto out;
 }
 
+#define ceiling_log2(x) fls(x - 1)
+unsigned estimate_skb_pages(unsigned num, unsigned size)
+{
+	int slab_pages = kmem_estimate_pages(skbuff_head_cache, num);
+	int data_space = num * (1 << ceiling_log2(size + 16));
+	int data_pages = (data_space + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
+	return slab_pages + data_pages;
+}
+
 /**
  *	alloc_skb_from_cache	-	allocate a network buffer
  *	@cp: kmem_cache from which to allocate the data area