cpuset aware writeback

---

Patch against 2.6.22-rc6-mm1

fs/buffer.c +2 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/buffer.c 1/fs/buffer.c
@@ -41,6 +41,7 @@
 #include <linux/bitops.h>
 #include <linux/mpage.h>
 #include <linux/bit_spinlock.h>
+#include <linux/cpuset.h>
 
 static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);

@@ -710,6 +711,7 @@ static int __set_page_dirty(struct page 
 		radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 	}
+	cpuset_update_dirty_nodes(mapping, page);
 	write_unlock_irq(&mapping->tree_lock);
 	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

fs/fs-writeback.c +7 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/fs-writeback.c 1/fs/fs-writeback.c
@@ -22,6 +22,7 @@
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
 #include <linux/buffer_head.h>
+#include <linux/cpuset.h>
 #include "internal.h"
 
 int sysctl_inode_debug __read_mostly;

@@ -492,6 +493,12 @@ int generic_sync_sb_inodes(struct super_
 			continue;		/* blockdev has wrong queue */
 		}
 
+		if (!cpuset_intersects_dirty_nodes(mapping, wbc->nodes)) {
+			/* No pages on the nodes under writeback */
+			list_move(&inode->i_list, &sb->s_dirty);
+			continue;
+		}
+
 		/* Was this inode dirtied after sync_sb_inodes was called? */
 		if (time_after(inode->dirtied_when, start))
 			break;

fs/inode.c +3 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/inode.c 1/fs/inode.c
@@ -22,6 +22,7 @@
 #include <linux/bootmem.h>
 #include <linux/inotify.h>
 #include <linux/mount.h>
+#include <linux/cpuset.h>
 
 /*
  * This is needed for the following functions:

@@ -157,6 +158,7 @@ static struct inode *alloc_inode(struct 
 		mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
 		mapping->assoc_mapping = NULL;
 		mapping->backing_dev_info = &default_backing_dev_info;
+		cpuset_init_dirty_nodes(mapping);
 
 		/*
 		 * If the block_device provides a backing_dev_info for client

@@ -264,6 +266,7 @@ void clear_inode(struct inode *inode)
 		bd_forget(inode);
 	if (S_ISCHR(inode->i_mode) && inode->i_cdev)
 		cd_forget(inode);
+	cpuset_clear_dirty_nodes(inode->i_mapping);
 	inode->i_state = I_CLEAR;
 }

include/linux/cpuset.h +59 -0

71 lines not on this page · raw message

include/linux/fs.h +7 -0

diff -uprN -X 0/Documentation/dontdiff 0/include/linux/fs.h 1/include/linux/fs.h
@@ -527,6 +527,13 @@ struct address_space {
 	spinlock_t		private_lock;	/* for use by the address_space */
 	struct list_head	private_list;	/* ditto */
 	struct address_space	*assoc_mapping;	/* ditto */
+#ifdef CONFIG_CPUSETS
+#if MAX_NUMNODES <= BITS_PER_LONG
+	nodemask_t		dirty_nodes;	/* nodes with dirty pages */
+#else
+	nodemask_t		*dirty_nodes;	/* pointer to map if dirty */
+#endif
+#endif
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but

include/linux/writeback.h +1 -0

diff -uprN -X 0/Documentation/dontdiff 0/include/linux/writeback.h 1/include/linux/writeback.h
@@ -63,6 +63,7 @@ struct writeback_control {
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 
 	void *fs_private;		/* For use by ->writepages() */
+	nodemask_t *nodes;		/* Set of nodes of interest */
 };
 
 /*

kernel/cpuset.c +59 -1

78 lines not on this page · raw message

mm/page-writeback.c +2 -0

diff -uprN -X 0/Documentation/dontdiff 0/mm/page-writeback.c 1/mm/page-writeback.c
@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/buffer_head.h>
 #include <linux/pagevec.h>
+#include <linux/cpuset.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -832,6 +833,7 @@ int __set_page_dirty_nobuffers(struct pa
 			radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 		}
+		cpuset_update_dirty_nodes(mapping, page);
 		write_unlock_irq(&mapping->tree_lock);
 		if (mapping->host) {
 			/* !PageAnon && !swapper_space */

Ethan Solomita

author

2007-07-17 21:33 UTC #2 lore raw

PATCH 2/6 cpuset write pdflush nodemask

If we want to support nodeset specific writeout then we need a way to communicate the set of nodes that an operation should affect.

So add a nodemask_t parameter to the pdflush functions and also store the nodemask in the pdflush control structure.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.22-rc6-mm1

fs/buffer.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 1/fs/buffer.c 2/fs/buffer.c
@@ -359,7 +359,7 @@ static void free_more_memory(void)
 	struct zone **zones;
 	pg_data_t *pgdat;
 
-	wakeup_pdflush(1024);
+	wakeup_pdflush(1024, NULL);
 	yield();
 
 	for_each_online_pgdat(pgdat) {

fs/super.c +2 -2

diff -uprN -X 0/Documentation/dontdiff 1/fs/super.c 2/fs/super.c
@@ -615,7 +615,7 @@ int do_remount_sb(struct super_block *sb
 	return 0;
 }
 
-static void do_emergency_remount(unsigned long foo)
+static void do_emergency_remount(unsigned long foo, nodemask_t *bar)
 {
 	struct super_block *sb;

@@ -643,7 +643,7 @@ static void do_emergency_remount(unsigne
 
 void emergency_remount(void)
 {
-	pdflush_operation(do_emergency_remount, 0);
+	pdflush_operation(do_emergency_remount, 0, NULL);
 }
 
 /*

fs/sync.c +4 -4

diff -uprN -X 0/Documentation/dontdiff 1/fs/sync.c 2/fs/sync.c
@@ -21,9 +21,9 @@
  * sync everything.  Start out by waking pdflush, because that writes back
  * all queues in parallel.
  */
-static void do_sync(unsigned long wait)
+static void do_sync(unsigned long wait, nodemask_t *unused)
 {
-	wakeup_pdflush(0);
+	wakeup_pdflush(0, NULL);
 	sync_inodes(0);		/* All mappings, inodes and their blockdevs */
 	DQUOT_SYNC(NULL);
 	sync_supers();		/* Write the superblocks */

@@ -38,13 +38,13 @@ static void do_sync(unsigned long wait)
 
 asmlinkage long sys_sync(void)
 {
-	do_sync(1);
+	do_sync(1, NULL);
 	return 0;
 }
 
 void emergency_sync(void)
 {
-	pdflush_operation(do_sync, 0);
+	pdflush_operation(do_sync, 0, NULL);
 }
 
 /*

include/linux/writeback.h +3 -2

diff -uprN -X 0/Documentation/dontdiff 1/include/linux/writeback.h 2/include/linux/writeback.h
@@ -92,7 +92,7 @@ static inline void inode_sync_wait(struc
 /*
  * mm/page-writeback.c
  */
-int wakeup_pdflush(long nr_pages);
+int wakeup_pdflush(long nr_pages, nodemask_t *nodes);
 void laptop_io_completion(void);
 void laptop_sync_completion(void);
 void throttle_vm_writeout(gfp_t gfp_mask);

@@ -123,7 +123,8 @@ balance_dirty_pages_ratelimited(struct a
 typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc,
 				void *data);
 
-int pdflush_operation(void (*fn)(unsigned long), unsigned long arg0);
+int pdflush_operation(void (*fn)(unsigned long, nodemask_t *nodes),
+			unsigned long arg0, nodemask_t *nodes);
 int generic_writepages(struct address_space *mapping,
 		       struct writeback_control *wbc);
 int write_cache_pages(struct address_space *mapping,

mm/page-writeback.c +9 -9

diff -uprN -X 0/Documentation/dontdiff 1/mm/page-writeback.c 2/mm/page-writeback.c
@@ -101,7 +101,7 @@ EXPORT_SYMBOL(laptop_mode);
 /* End of sysctl-exported parameters */
 
 
-static void background_writeout(unsigned long _min_pages);
+static void background_writeout(unsigned long _min_pages, nodemask_t *nodes);
 
 /*
  * Work out the current dirty-memory clamping and background writeout

@@ -272,7 +272,7 @@ static void balance_dirty_pages(struct a
 	 */
 	if ((laptop_mode && pages_written) ||
 	     (!laptop_mode && (nr_reclaimable > background_thresh)))
-		pdflush_operation(background_writeout, 0);
+		pdflush_operation(background_writeout, 0, NULL);
 }
 
 void set_page_dirty_balance(struct page *page)

@@ -362,7 +362,7 @@ void throttle_vm_writeout(gfp_t gfp_mask
  * writeback at least _min_pages, and keep writing until the amount of dirty
  * memory is less than the background threshold, or until we're all clean.
  */
-static void background_writeout(unsigned long _min_pages)
+static void background_writeout(unsigned long _min_pages, nodemask_t *unused)
 {
 	long min_pages = _min_pages;
 	struct writeback_control wbc = {

@@ -402,12 +402,12 @@ static void background_writeout(unsigned
  * the whole world.  Returns 0 if a pdflush thread was dispatched.  Returns
  * -1 if all pdflush threads were busy.
  */
-int wakeup_pdflush(long nr_pages)
+int wakeup_pdflush(long nr_pages, nodemask_t *nodes)
 {
 	if (nr_pages == 0)
 		nr_pages = global_page_state(NR_FILE_DIRTY) +
 				global_page_state(NR_UNSTABLE_NFS);
-	return pdflush_operation(background_writeout, nr_pages);
+	return pdflush_operation(background_writeout, nr_pages, nodes);
 }
 
 static void wb_timer_fn(unsigned long unused);

@@ -431,7 +431,7 @@ static DEFINE_TIMER(laptop_mode_wb_timer
  * older_than_this takes precedence over nr_to_write.  So we'll only write back
  * all dirty pages if they are all attached to "old" mappings.
  */
-static void wb_kupdate(unsigned long arg)
+static void wb_kupdate(unsigned long arg, nodemask_t *unused)
 {
 	unsigned long oldest_jif;
 	unsigned long start_jif;

@@ -489,18 +489,18 @@ int dirty_writeback_centisecs_handler(ct
 
 static void wb_timer_fn(unsigned long unused)
 {
-	if (pdflush_operation(wb_kupdate, 0) < 0)
+	if (pdflush_operation(wb_kupdate, 0, NULL) < 0)
 		mod_timer(&wb_timer, jiffies + HZ); /* delay 1 second */
 }
 
-static void laptop_flush(unsigned long unused)
+static void laptop_flush(unsigned long unused, nodemask_t *unused2)
 {
 	sys_sync();
 }
 
 static void laptop_timer_fn(unsigned long unused)
 {
-	pdflush_operation(laptop_flush, 0);
+	pdflush_operation(laptop_flush, 0, NULL);
 }
 
 /*

mm/pdflush.c +12 -3

diff -uprN -X 0/Documentation/dontdiff 1/mm/pdflush.c 2/mm/pdflush.c
@@ -83,10 +83,12 @@ static unsigned long last_empty_jifs;
  */
 struct pdflush_work {
 	struct task_struct *who;	/* The thread */
-	void (*fn)(unsigned long);	/* A callback function */
+	void (*fn)(unsigned long, nodemask_t *); /* A callback function */
 	unsigned long arg0;		/* An argument to the callback */
 	struct list_head list;		/* On pdflush_list, when idle */
 	unsigned long when_i_went_to_sleep;
+	int have_nodes;			/* Nodes were specified */
+	nodemask_t nodes;		/* Nodes of interest */
 };
 
 static int __pdflush(struct pdflush_work *my_work)

@@ -124,7 +126,8 @@ static int __pdflush(struct pdflush_work
 		}
 		spin_unlock_irq(&pdflush_lock);
 
-		(*my_work->fn)(my_work->arg0);
+		(*my_work->fn)(my_work->arg0,
+			my_work->have_nodes ? &my_work->nodes : NULL);
 
 		/*
 		 * Thread creation: For how long have there been zero

@@ -198,7 +201,8 @@ static int pdflush(void *dummy)
  * Returns zero if it indeed managed to find a worker thread, and passed your
  * payload to it.
  */
-int pdflush_operation(void (*fn)(unsigned long), unsigned long arg0)
+int pdflush_operation(void (*fn)(unsigned long, nodemask_t *),
+			unsigned long arg0, nodemask_t *nodes)
 {
 	unsigned long flags;
 	int ret = 0;

@@ -218,6 +222,11 @@ int pdflush_operation(void (*fn)(unsigne
 			last_empty_jifs = jiffies;
 		pdf->fn = fn;
 		pdf->arg0 = arg0;
+		if (nodes) {
+			pdf->nodes = *nodes;
+			pdf->have_nodes = 1;
+		} else
+			pdf->have_nodes = 0;
 		wake_up_process(pdf->who);
 		spin_unlock_irqrestore(&pdflush_lock, flags);
 	}

mm/vmscan.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 1/mm/vmscan.c 2/mm/vmscan.c
@@ -1183,7 +1183,7 @@ unsigned long try_to_free_pages(struct z
 		 */
 		if (total_scanned > sc.swap_cluster_max +
 					sc.swap_cluster_max / 2) {
-			wakeup_pdflush(laptop_mode ? 0 : total_scanned);
+			wakeup_pdflush(laptop_mode ? 0 : total_scanned, NULL);
 			sc.may_writepage = 1;
 		}

Ethan Solomita

author

2007-07-17 21:34 UTC #3 lore raw

PATCH 3/6 cpuset write throttle

Make page writeback obey cpuset constraints

Currently dirty throttling does not work properly in a cpuset.

If f.e a cpuset contains only 1/10th of available memory then all of the memory of a cpuset can be dirtied without any writes being triggered. If all of the cpusets memory is dirty then only 10% of total memory is dirty. The background writeback threshold is usually set at 10% and the synchrononous threshold at 40%. So we are still below the global limits while the dirty ratio in the cpuset is 100%! Writeback throttling and background writeout do not work at all in such scenarios.

This patch makes dirty writeout cpuset aware. When determining the dirty limits in get_dirty_limits() we calculate values based on the nodes that are reachable from the current process (that has been dirtying the page). Then we can trigger writeout based on the dirty ratio of the memory in the cpuset.

We trigger writeout in a a cpuset specific way. We go through the dirty inodes and search for inodes that have dirty pages on the nodes of the active cpuset. If an inode fulfills that requirement then we begin writeout of the dirty pages of that inode.

Adding up all the counters for each node in a cpuset may seem to be quite an expensive operation (in particular for large cpusets with hundreds of nodes) compared to just accessing the global counters if we do not have a cpuset. However, please remember that the global counters were only introduced recently. Before 2.6.18 we did add up per processor counters for each processor on each invocation of get_dirty_limits(). We now add per node information which I think is equal or less effort since there are less nodes than processors.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.22-rc6-mm1

mm/page-writeback.c +105 ~~-51~~

diff -uprN -X 0/Documentation/dontdiff 2/mm/page-writeback.c 3/mm/page-writeback.c
@@ -103,6 +103,14 @@ EXPORT_SYMBOL(laptop_mode);
 
 static void background_writeout(unsigned long _min_pages, nodemask_t *nodes);
 
+struct dirty_limits {
+	long thresh_background;
+	long thresh_dirty;
+	unsigned long nr_dirty;
+	unsigned long nr_unstable;
+	unsigned long nr_writeback;
+};
+
 /*
  * Work out the current dirty-memory clamping and background writeout
  * thresholds.

@@ -121,13 +129,15 @@ static void background_writeout(unsigned
  * clamping level.
  */
 
-static unsigned long highmem_dirtyable_memory(unsigned long total)
+static unsigned long highmem_dirtyable_memory(nodemask_t *nodes, unsigned long total)
 {
 #ifdef CONFIG_HIGHMEM
 	int node;
 	unsigned long x = 0;
 
-	for_each_online_node(node) {
+	if (nodes == NULL)
+		nodes = &node_online_mask;
+	for_each_node_mask(node, *nodes) {
 		struct zone *z =
 			&NODE_DATA(node)->node_zones[ZONE_HIGHMEM];

@@ -154,26 +164,74 @@ static unsigned long determine_dirtyable
 	x = global_page_state(NR_FREE_PAGES)
 		+ global_page_state(NR_INACTIVE)
 		+ global_page_state(NR_ACTIVE);
-	x -= highmem_dirtyable_memory(x);
+	x -= highmem_dirtyable_memory(NULL, x);
 	return x + 1;	/* Ensure that we never return 0 */
 }
 
-static void
-get_dirty_limits(long *pbackground, long *pdirty,
-					struct address_space *mapping)
+static int
+get_dirty_limits(struct dirty_limits *dl, struct address_space *mapping,
+					nodemask_t *nodes)
 {
 	int background_ratio;		/* Percentages */
 	int dirty_ratio;
 	int unmapped_ratio;
 	long background;
 	long dirty;
-	unsigned long available_memory = determine_dirtyable_memory();
+	unsigned long available_memory;
+	unsigned long nr_mapped;
 	struct task_struct *tsk;
+	int is_subset = 0;
 
-	unmapped_ratio = 100 - ((global_page_state(NR_FILE_MAPPED) +
-				global_page_state(NR_ANON_PAGES)) * 100) /
-					available_memory;
+#ifdef CONFIG_CPUSETS
+	if (unlikely(nodes &&
+			!nodes_subset(node_online_map, *nodes))) {
+		int node;
 
+		/*
+		 * Calculate the limits relative to the current cpuset.
+		 *
+		 * We do not disregard highmem because all nodes (except
+		 * maybe node 0) have either all memory in HIGHMEM (32 bit) or
+		 * all memory in non HIGHMEM (64 bit). If we would disregard
+		 * highmem then cpuset throttling would not work on 32 bit.
+		 */
+		is_subset = 1;
+		memset(dl, 0, sizeof(struct dirty_limits));
+		available_memory = 0;
+		nr_mapped = 0;
+		for_each_node_mask(node, *nodes) {
+			if (!node_online(node))
+				continue;
+			dl->nr_dirty += node_page_state(node, NR_FILE_DIRTY);
+			dl->nr_unstable +=
+				node_page_state(node, NR_UNSTABLE_NFS);
+			dl->nr_writeback +=
+				node_page_state(node, NR_WRITEBACK);
+			available_memory +=
+				node_page_state(node, NR_ACTIVE) +
+				node_page_state(node, NR_INACTIVE) +
+				node_page_state(node, NR_FREE_PAGES);
+			nr_mapped += node_page_state(node, NR_FILE_MAPPED) +
+				node_page_state(node, NR_ANON_PAGES);
+		}
+		available_memory -= highmem_dirtyable_memory(nodes,
+							     available_memory);
+		/* Ensure that we return >= 0 */
+		if (available_memory <= 0)
+			available_memory = 1;
+	} else
+#endif
+	{
+		/* Global limits */
+		dl->nr_dirty = global_page_state(NR_FILE_DIRTY);
+		dl->nr_unstable = global_page_state(NR_UNSTABLE_NFS);
+		dl->nr_writeback = global_page_state(NR_WRITEBACK);
+		available_memory = determine_dirtyable_memory();
+		nr_mapped = global_page_state(NR_FILE_MAPPED) +
+			global_page_state(NR_ANON_PAGES);
+	}
+
+	unmapped_ratio = 100 - (nr_mapped * 100 / available_memory);
 	dirty_ratio = vm_dirty_ratio;
 	if (dirty_ratio > unmapped_ratio / 2)
 		dirty_ratio = unmapped_ratio / 2;

@@ -192,8 +250,9 @@ get_dirty_limits(long *pbackground, long
 		background += background / 4;
 		dirty += dirty / 4;
 	}
-	*pbackground = background;
-	*pdirty = dirty;
+	dl->thresh_background = background;
+	dl->thresh_dirty = dirty;
+	return is_subset;
 }
 
 /*

@@ -206,8 +265,7 @@ get_dirty_limits(long *pbackground, long
 static void balance_dirty_pages(struct address_space *mapping)
 {
 	long nr_reclaimable;
-	long background_thresh;
-	long dirty_thresh;
+	struct dirty_limits dl;
 	unsigned long pages_written = 0;
 	unsigned long write_chunk = sync_writeback_pages();

@@ -222,11 +280,12 @@ static void balance_dirty_pages(struct a
 			.range_cyclic	= 1,
 		};
 
-		get_dirty_limits(&background_thresh, &dirty_thresh, mapping);
-		nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-		if (nr_reclaimable + global_page_state(NR_WRITEBACK) <=
-			dirty_thresh)
+		if (get_dirty_limits(&dl, mapping,
+				&cpuset_current_mems_allowed))
+			wbc.nodes = &cpuset_current_mems_allowed;
+		nr_reclaimable = dl.nr_dirty + dl.nr_unstable;
+		if (nr_reclaimable + dl.nr_writeback <=
+			dl.thresh_dirty)
 				break;
 
 		if (!dirty_exceeded)

@@ -240,13 +299,10 @@ static void balance_dirty_pages(struct a
 		 */
 		if (nr_reclaimable) {
 			writeback_inodes(&wbc);
-			get_dirty_limits(&background_thresh,
-					 	&dirty_thresh, mapping);
-			nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-			if (nr_reclaimable +
-				global_page_state(NR_WRITEBACK)
-					<= dirty_thresh)
+			get_dirty_limits(&dl, mapping,
+				&cpuset_current_mems_allowed);
+			nr_reclaimable = dl.nr_dirty + dl.nr_unstable;
+			if (nr_reclaimable + dl.nr_writeback <= dl.thresh_dirty)
 						break;
 			pages_written += write_chunk - wbc.nr_to_write;
 			if (pages_written >= write_chunk)

@@ -255,8 +311,8 @@ static void balance_dirty_pages(struct a
 		congestion_wait(WRITE, HZ/10);
 	}
 
-	if (nr_reclaimable + global_page_state(NR_WRITEBACK)
-		<= dirty_thresh && dirty_exceeded)
+	if (nr_reclaimable + dl.nr_writeback
+		<= dl.thresh_dirty && dirty_exceeded)
 			dirty_exceeded = 0;
 
 	if (writeback_in_progress(bdi))

@@ -271,8 +327,9 @@ static void balance_dirty_pages(struct a
 	 * background_thresh, to keep the amount of dirty memory low.
 	 */
 	if ((laptop_mode && pages_written) ||
-	     (!laptop_mode && (nr_reclaimable > background_thresh)))
-		pdflush_operation(background_writeout, 0, NULL);
+	     (!laptop_mode && (nr_reclaimable > dl.thresh_background)))
+		pdflush_operation(background_writeout, 0,
+			&cpuset_current_mems_allowed);
 }
 
 void set_page_dirty_balance(struct page *page)

@@ -329,8 +386,7 @@ EXPORT_SYMBOL(balance_dirty_pages_rateli
 
 void throttle_vm_writeout(gfp_t gfp_mask)
 {
-	long background_thresh;
-	long dirty_thresh;
+	struct dirty_limits dl;
 
 	if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO)) {
 		/*

@@ -342,27 +398,26 @@ void throttle_vm_writeout(gfp_t gfp_mask
 		return;
 	}
 
-        for ( ; ; ) {
-		get_dirty_limits(&background_thresh, &dirty_thresh, NULL);
+	for ( ; ; ) {
+		get_dirty_limits(&dl, NULL, &node_online_map);
+
+		/*
+		 * Boost the allowable dirty threshold a bit for page
+		 * allocators so they don't get DoS'ed by heavy writers
+		 */
+		dl.thresh_dirty += dl.thresh_dirty / 10; /* wheeee... */
 
-                /*
-                 * Boost the allowable dirty threshold a bit for page
-                 * allocators so they don't get DoS'ed by heavy writers
-                 */
-                dirty_thresh += dirty_thresh / 10;      /* wheeee... */
-
-                if (global_page_state(NR_UNSTABLE_NFS) +
-			global_page_state(NR_WRITEBACK) <= dirty_thresh)
-                        	break;
-                congestion_wait(WRITE, HZ/10);
-        }
+		if (dl.nr_unstable + dl.nr_writeback <= dl.thresh_dirty)
+			break;
+		congestion_wait(WRITE, HZ/10);
+	}
 }
 
 /*
  * writeback at least _min_pages, and keep writing until the amount of dirty
  * memory is less than the background threshold, or until we're all clean.
  */
-static void background_writeout(unsigned long _min_pages, nodemask_t *unused)
+static void background_writeout(unsigned long _min_pages, nodemask_t *nodes)
 {
 	long min_pages = _min_pages;
 	struct writeback_control wbc = {

@@ -375,12 +430,11 @@ static void background_writeout(unsigned
 	};
 
 	for ( ; ; ) {
-		long background_thresh;
-		long dirty_thresh;
+		struct dirty_limits dl;
 
-		get_dirty_limits(&background_thresh, &dirty_thresh, NULL);
-		if (global_page_state(NR_FILE_DIRTY) +
-			global_page_state(NR_UNSTABLE_NFS) < background_thresh
+		if (get_dirty_limits(&dl, NULL, nodes))
+			wbc.nodes = nodes;
+		if (dl.nr_dirty + dl.nr_unstable < dl.thresh_background
 				&& min_pages <= 0)
 			break;
 		wbc.encountered_congestion = 0;

Ethan Solomita

author

2007-07-17 21:35 UTC #4 lore raw

PATCH 4/6 cpuset write vmscan

Direct reclaim: cpuset aware writeout

During direct reclaim we traverse down a zonelist and are carefully checking each zone if its a member of the active cpuset. But then we call pdflush without enforcing the same restrictions. In a larger system this may have the effect of a massive amount of pages being dirtied and then either

A. No writeout occurs because global dirty limits have not been reached

B. Writeout starts randomly for some dirty inode in the system. Pdflush
may just write out data for nodes in another cpuset and miss doing proper dirty handling for the current cpuset.

In both cases dirty pages in the zones of interest may not be affected and writeout may not occur as necessary.

Fix that by restricting pdflush to the active cpuset. Writeout will occur from direct reclaim the same way as without a cpuset.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.22-rc6-mm1

mm/vmscan.c +2 -1

diff -uprN -X 0/Documentation/dontdiff 3/mm/vmscan.c 4/mm/vmscan.c
@@ -1183,7 +1183,8 @@ unsigned long try_to_free_pages(struct z
 		 */
 		if (total_scanned > sc.swap_cluster_max +
 					sc.swap_cluster_max / 2) {
-			wakeup_pdflush(laptop_mode ? 0 : total_scanned, NULL);
+			wakeup_pdflush(laptop_mode ? 0 : total_scanned,
+				&cpuset_current_mems_allowed);
 			sc.may_writepage = 1;
 		}

Ethan Solomita

author

2007-07-17 21:36 UTC #5 lore raw

PATCH 5/6 cpuset write vm writeout

Throttle VM writeout in a cpuset aware way

This bases the vm throttling from the reclaim path on the dirty ratio of the cpuset. Note that a cpuset is only effective if shrink_zone is called from direct reclaim.

kswapd has a cpuset context that includes the whole machine. VM throttling will only work during synchrononous reclaim and not from kswapd.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.22-rc6-mm1

include/linux/writeback.h +1 -1

diff -uprN -X 0/Documentation/dontdiff 4/include/linux/writeback.h 5/include/linux/writeback.h
@@ -95,7 +95,7 @@ static inline void inode_sync_wait(struc
 int wakeup_pdflush(long nr_pages, nodemask_t *nodes);
 void laptop_io_completion(void);
 void laptop_sync_completion(void);
-void throttle_vm_writeout(gfp_t gfp_mask);
+void throttle_vm_writeout(nodemask_t *nodes,gfp_t gfp_mask);
 
 /* These are exported to sysctl. */
 extern int dirty_background_ratio;

mm/page-writeback.c +2 -2

diff -uprN -X 0/Documentation/dontdiff 4/mm/page-writeback.c 5/mm/page-writeback.c
@@ -384,7 +384,7 @@ void balance_dirty_pages_ratelimited_nr(
 }
 EXPORT_SYMBOL(balance_dirty_pages_ratelimited_nr);
 
-void throttle_vm_writeout(gfp_t gfp_mask)
+void throttle_vm_writeout(nodemask_t *nodes, gfp_t gfp_mask)
 {
 	struct dirty_limits dl;

@@ -399,7 +399,7 @@ void throttle_vm_writeout(gfp_t gfp_mask
 	}
 
 	for ( ; ; ) {
-		get_dirty_limits(&dl, NULL, &node_online_map);
+		get_dirty_limits(&dl, NULL, nodes);
 
 		/*
 		 * Boost the allowable dirty threshold a bit for page

mm/vmscan.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 4/mm/vmscan.c 5/mm/vmscan.c
@@ -1064,7 +1064,7 @@ static unsigned long shrink_zone(int pri
 		}
 	}
 
-	throttle_vm_writeout(sc->gfp_mask);
+	throttle_vm_writeout(&cpuset_current_mems_allowed, sc->gfp_mask);
 
 	atomic_dec(&zone->reclaim_in_progress);
 	return nr_reclaimed;

Ethan Solomita

author

2007-07-17 21:37 UTC #6 lore raw

PATCH 6/6 cpuset dirty limits

Per cpuset dirty ratios

This implements dirty ratios per cpuset. Two new files are added to the cpuset directories:

background_dirty_ratio Percentage at which background writeback starts

throttle_dirty_ratio Percentage at which the application is throttled
and we start synchrononous writeout.

Both variables are set to -1 by default which means that the global limits (/proc/sys/vm/vm_dirty_ratio and /proc/sys/vm/dirty_background_ratio) are used for a cpuset.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.22-rc6-mm1

include/linux/cpuset.h +1 -0

diff -uprN -X 0/Documentation/dontdiff 6/include/linux/cpuset.h 7/include/linux/cpuset.h
@@ -76,6 +76,7 @@ extern void cpuset_track_online_nodes(vo
 
 extern int current_cpuset_is_being_rebound(void);
 
+extern void cpuset_get_current_ratios(int *background, int *ratio);
 /*
  * We need macros since struct address_space is not defined yet
  */

kernel/cpuset.c +78 -1

146 lines not on this page · raw message

mm/page-writeback.c +3 -2

diff -uprN -X 0/Documentation/dontdiff 6/mm/page-writeback.c 7/mm/page-writeback.c
@@ -219,6 +219,7 @@ get_dirty_limits(struct dirty_limits *dl
 		/* Ensure that we return >= 0 */
 		if (available_memory <= 0)
 			available_memory = 1;
+		cpuset_get_current_ratios(&background_ratio, &dirty_ratio);
 	} else
 #endif
 	{

@@ -229,17 +230,17 @@ get_dirty_limits(struct dirty_limits *dl
 		available_memory = determine_dirtyable_memory();
 		nr_mapped = global_page_state(NR_FILE_MAPPED) +
 			global_page_state(NR_ANON_PAGES);
+		dirty_ratio = vm_dirty_ratio;
+		background_ratio = dirty_background_ratio;
 	}
 
 	unmapped_ratio = 100 - (nr_mapped * 100 / available_memory);
-	dirty_ratio = vm_dirty_ratio;
 	if (dirty_ratio > unmapped_ratio / 2)
 		dirty_ratio = unmapped_ratio / 2;
 
 	if (dirty_ratio < 5)
 		dirty_ratio = 5;
 
-	background_ratio = dirty_background_ratio;
 	if (background_ratio >= dirty_ratio)
 		background_ratio = dirty_ratio / 2;

Christoph Lameter

2007-07-23 20:18 UTC #7 lore raw

Quoted 2 lines · On Tue, 17 Jul 2007 14:23:14 -0700 Ethan Solomita <[email protected]> wrote:

These patches are mostly unchanged from Chris Lameter's original changelist posted previously to linux-mm.

Thanks for keeping these patches up to date. Add you signoff if you did modifications to a patch. Also include the description of the tests in the introduction to the patchset.

Ethan Solomita

author

2007-07-23 21:30 UTC #8 lore raw

Quoted 9 lines · Christoph Lameter

Quoted 2 lines · On Tue, 17 Jul 2007 14:23:14 -0700 Ethan Solomita <[email protected]> wrote:
These patches are mostly unchanged from Chris Lameter's original changelist posted previously to linux-mm.
Thanks for keeping these patches up to date. Add you signoff if you did modifications to a patch. Also include the description of the tests in the introduction to the patchset.

So switch from an Ack to a signed-off? OK, and I'll add descriptions of
testing. Everyone other than you has been silent on these patches. Does silence equal consent?
-- Ethan

Christoph Lameter

2007-07-23 21:53 UTC #9 lore raw

Quoted 4 lines · Ethan Solomita

Quoted 3 lines · Christoph Lameter
On Tue, 17 Jul 2007 14:23:14 -0700
Ethan Solomita <[email protected]> wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
These patches are mostly unchanged from Chris Lameter's original changelist posted previously to linux-mm.
Thanks for keeping these patches up to date. Add you signoff if you did modifications to a patch. Also include the description of the tests in the introduction to the patchset.
So switch from an Ack to a signed-off? OK, and I'll add descriptions of

No. Do a signed-off if you have modified the patch.

Quoted 2 lines

testing. Everyone other than you has been silent on these patches. Does silence equal consent?

Sometimes. Howerver, the audience for NUMA and cpusets is rather limited.

Ethan Solomita

author

2007-09-12 01:32 UTC #10 lore raw

Perform writeback and dirty throttling with awareness of cpuset mem_allowed.

The theory of operation has two primary elements:

1. Add a nodemask per mapping which indicates the nodes which have set PageDirty on any page of the mappings.

2. Add a nodemask argument to wakeup_pdflush() which is propagated down to sync_sb_inodes.

Other callers do not limit inode writeback, passing in a NULL nodemask pointer.

For testing I did a variety of basic tests, verifying individual features of the test. To verify that it fixes the core problem, I created a stress test which involved using cpusets and mems_allowed to split memory so that all daemons had memory set aside for them, and my memory stress test had a separate set of memory. The stress test was mmaping 7GB of a very large file on disk. It then scans the entire 7GB of memory reading and modifying each byte. 7GB is more than the amount of physical memory made available to the stress test.

Using iostat I can see the initial period of reading from disk, followed by a period of simultaneous reads and writes as dirty bytes are pushed to make room for new reads.

In a separate log-in, in the other cpuset, I am running:

while true; do date | tee -a date.txt; sleep 5; done

date.txt resides on the same disk as the large file mentioned above. The above while-loop serves the dual purpose of providing me visual clues of progress along with the opportunity for the "tee" command to become throttled writing to the disk.

The effect of this patchset is straightforward. Without it there are long hangs between appearances of the date. With it the dates are all 5 (or sometimes 6) seconds apart.

I also added printks to the kernel to verify that, without these patches, the tee was being throttled (along with lots of other things), and with the patch only pdflush is being throttled.

These patches are mostly unchanged from Chris Lameter's original changelist posted previously to linux-mm.

Ethan Solomita

author

2007-09-12 01:36 UTC #11 lore raw

PATCH 1/6 cpuset write dirty map

Add a dirty map to struct address_space

This patch implements the management of dirty node maps for an address space through the following functions:

cpuset_clear_dirty_nodes(mapping) Clear the map of dirty nodes

cpuset_update_nodes(mapping, page) Record a node in the dirty nodes map

cpuset_init_dirty_nodes(mapping) First time init of the map

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.23-rc4-mm1

fs/buffer.c +2 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/buffer.c 1/fs/buffer.c
@@ -41,6 +41,7 @@
 #include <linux/bitops.h>
 #include <linux/mpage.h>
 #include <linux/bit_spinlock.h>
+#include <linux/cpuset.h>
 
 static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);

@@ -723,6 +724,7 @@ static int __set_page_dirty(struct page 
 		radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 	}
+	cpuset_update_dirty_nodes(mapping, page);
 	write_unlock_irq(&mapping->tree_lock);
 	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

fs/fs-writeback.c +7 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/fs-writeback.c 1/fs/fs-writeback.c
@@ -22,6 +22,7 @@
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
 #include <linux/buffer_head.h>
+#include <linux/cpuset.h>
 #include "internal.h"
 
 int sysctl_inode_debug __read_mostly;

@@ -476,6 +477,12 @@ int generic_sync_sb_inodes(struct super_
 			continue;		/* blockdev has wrong queue */
 		}
 
+		if (!cpuset_intersects_dirty_nodes(mapping, wbc->nodes)) {
+			/* No pages on the nodes under writeback */
+			list_move(&inode->i_list, &sb->s_dirty);
+			continue;
+		}
+
 		/* Was this inode dirtied after sync_sb_inodes was called? */
 		if (time_after(inode->dirtied_when, start))
 			break;

fs/inode.c +3 -0

diff -uprN -X 0/Documentation/dontdiff 0/fs/inode.c 1/fs/inode.c
@@ -22,6 +22,7 @@
 #include <linux/bootmem.h>
 #include <linux/inotify.h>
 #include <linux/mount.h>
+#include <linux/cpuset.h>
 
 /*
  * This is needed for the following functions:

@@ -157,6 +158,7 @@ static struct inode *alloc_inode(struct 
 		mapping_set_gfp_mask(mapping, GFP_HIGHUSER_PAGECACHE);
 		mapping->assoc_mapping = NULL;
 		mapping->backing_dev_info = &default_backing_dev_info;
+		cpuset_init_dirty_nodes(mapping);
 
 		/*
 		 * If the block_device provides a backing_dev_info for client

@@ -264,6 +266,7 @@ void clear_inode(struct inode *inode)
 		bd_forget(inode);
 	if (S_ISCHR(inode->i_mode) && inode->i_cdev)
 		cd_forget(inode);
+	cpuset_clear_dirty_nodes(inode->i_mapping);
 	inode->i_state = I_CLEAR;
 }

include/linux/cpuset.h +59 -0

71 lines not on this page · raw message

include/linux/fs.h +7 -0

diff -uprN -X 0/Documentation/dontdiff 0/include/linux/fs.h 1/include/linux/fs.h
@@ -516,6 +516,13 @@ struct address_space {
 	spinlock_t		private_lock;	/* for use by the address_space */
 	struct list_head	private_list;	/* ditto */
 	struct address_space	*assoc_mapping;	/* ditto */
+#ifdef CONFIG_CPUSETS
+#if MAX_NUMNODES <= BITS_PER_LONG
+	nodemask_t		dirty_nodes;	/* nodes with dirty pages */
+#else
+	nodemask_t		*dirty_nodes;	/* pointer to map if dirty */
+#endif
+#endif
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but

include/linux/writeback.h +1 -0

diff -uprN -X 0/Documentation/dontdiff 0/include/linux/writeback.h 1/include/linux/writeback.h
@@ -62,6 +62,7 @@ struct writeback_control {
 	unsigned for_writepages:1;	/* This is a writepages() call */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 	void *fs_private;               /* For use by ->writepages() */
+	nodemask_t *nodes;		/* Set of nodes of interest */
 };
 
 /*

kernel/cpuset.c +59 -1

78 lines not on this page · raw message

mm/page-writeback.c +2 -0

diff -uprN -X 0/Documentation/dontdiff 0/mm/page-writeback.c 1/mm/page-writeback.c
@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/buffer_head.h>
 #include <linux/pagevec.h>
+#include <linux/cpuset.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -832,6 +833,7 @@ int __set_page_dirty_nobuffers(struct pa
 			radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 		}
+		cpuset_update_dirty_nodes(mapping, page);
 		write_unlock_irq(&mapping->tree_lock);
 		if (mapping->host) {
 			/* !PageAnon && !swapper_space */

Ethan Solomita

author

2007-09-12 01:38 UTC #22 lore raw

PATCH 2/6 cpuset write pdflush nodemask

If we want to support nodeset specific writeout then we need a way to communicate the set of nodes that an operation should affect.

So add a nodemask_t parameter to the pdflush functions and also store the nodemask in the pdflush control structure.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.23-rc4-mm1

fs/buffer.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 1/fs/buffer.c 2/fs/buffer.c
@@ -372,7 +372,7 @@ static void free_more_memory(void)
 	struct zone **zones;
 	pg_data_t *pgdat;
 
-	wakeup_pdflush(1024);
+	wakeup_pdflush(1024, NULL);
 	yield();
 
 	for_each_online_pgdat(pgdat) {

fs/super.c +2 -2

diff -uprN -X 0/Documentation/dontdiff 1/fs/super.c 2/fs/super.c
@@ -616,7 +616,7 @@ int do_remount_sb(struct super_block *sb
 	return 0;
 }
 
-static void do_emergency_remount(unsigned long foo)
+static void do_emergency_remount(unsigned long foo, nodemask_t *bar)
 {
 	struct super_block *sb;

@@ -644,7 +644,7 @@ static void do_emergency_remount(unsigne
 
 void emergency_remount(void)
 {
-	pdflush_operation(do_emergency_remount, 0);
+	pdflush_operation(do_emergency_remount, 0, NULL);
 }
 
 /*

fs/sync.c +4 -4

diff -uprN -X 0/Documentation/dontdiff 1/fs/sync.c 2/fs/sync.c
@@ -21,9 +21,9 @@
  * sync everything.  Start out by waking pdflush, because that writes back
  * all queues in parallel.
  */
-static void do_sync(unsigned long wait)
+static void do_sync(unsigned long wait, nodemask_t *unused)
 {
-	wakeup_pdflush(0);
+	wakeup_pdflush(0, NULL);
 	sync_inodes(0);		/* All mappings, inodes and their blockdevs */
 	DQUOT_SYNC(NULL);
 	sync_supers();		/* Write the superblocks */

@@ -38,13 +38,13 @@ static void do_sync(unsigned long wait)
 
 asmlinkage long sys_sync(void)
 {
-	do_sync(1);
+	do_sync(1, NULL);
 	return 0;
 }
 
 void emergency_sync(void)
 {
-	pdflush_operation(do_sync, 0);
+	pdflush_operation(do_sync, 0, NULL);
 }
 
 /*

include/linux/writeback.h +3 -2

diff -uprN -X 0/Documentation/dontdiff 1/include/linux/writeback.h 2/include/linux/writeback.h
@@ -91,7 +91,7 @@ static inline void inode_sync_wait(struc
 /*
  * mm/page-writeback.c
  */
-int wakeup_pdflush(long nr_pages);
+int wakeup_pdflush(long nr_pages, nodemask_t *nodes);
 void laptop_io_completion(void);
 void laptop_sync_completion(void);
 void throttle_vm_writeout(gfp_t gfp_mask);

@@ -122,7 +122,8 @@ balance_dirty_pages_ratelimited(struct a
 typedef int (*writepage_t)(struct page *page, struct writeback_control *wbc,
 				void *data);
 
-int pdflush_operation(void (*fn)(unsigned long), unsigned long arg0);
+int pdflush_operation(void (*fn)(unsigned long, nodemask_t *nodes),
+			unsigned long arg0, nodemask_t *nodes);
 int generic_writepages(struct address_space *mapping,
 		       struct writeback_control *wbc);
 int write_cache_pages(struct address_space *mapping,

mm/page-writeback.c +9 -9

diff -uprN -X 0/Documentation/dontdiff 1/mm/page-writeback.c 2/mm/page-writeback.c
@@ -101,7 +101,7 @@ EXPORT_SYMBOL(laptop_mode);
 /* End of sysctl-exported parameters */
 
 
-static void background_writeout(unsigned long _min_pages);
+static void background_writeout(unsigned long _min_pages, nodemask_t *nodes);
 
 /*
  * Work out the current dirty-memory clamping and background writeout

@@ -272,7 +272,7 @@ static void balance_dirty_pages(struct a
 	 */
 	if ((laptop_mode && pages_written) ||
 	     (!laptop_mode && (nr_reclaimable > background_thresh)))
-		pdflush_operation(background_writeout, 0);
+		pdflush_operation(background_writeout, 0, NULL);
 }
 
 void set_page_dirty_balance(struct page *page)

@@ -362,7 +362,7 @@ void throttle_vm_writeout(gfp_t gfp_mask
  * writeback at least _min_pages, and keep writing until the amount of dirty
  * memory is less than the background threshold, or until we're all clean.
  */
-static void background_writeout(unsigned long _min_pages)
+static void background_writeout(unsigned long _min_pages, nodemask_t *unused)
 {
 	long min_pages = _min_pages;
 	struct writeback_control wbc = {

@@ -402,12 +402,12 @@ static void background_writeout(unsigned
  * the whole world.  Returns 0 if a pdflush thread was dispatched.  Returns
  * -1 if all pdflush threads were busy.
  */
-int wakeup_pdflush(long nr_pages)
+int wakeup_pdflush(long nr_pages, nodemask_t *nodes)
 {
 	if (nr_pages == 0)
 		nr_pages = global_page_state(NR_FILE_DIRTY) +
 				global_page_state(NR_UNSTABLE_NFS);
-	return pdflush_operation(background_writeout, nr_pages);
+	return pdflush_operation(background_writeout, nr_pages, nodes);
 }
 
 static void wb_timer_fn(unsigned long unused);

@@ -431,7 +431,7 @@ static DEFINE_TIMER(laptop_mode_wb_timer
  * older_than_this takes precedence over nr_to_write.  So we'll only write back
  * all dirty pages if they are all attached to "old" mappings.
  */
-static void wb_kupdate(unsigned long arg)
+static void wb_kupdate(unsigned long arg, nodemask_t *unused)
 {
 	unsigned long oldest_jif;
 	unsigned long start_jif;

@@ -489,18 +489,18 @@ int dirty_writeback_centisecs_handler(ct
 
 static void wb_timer_fn(unsigned long unused)
 {
-	if (pdflush_operation(wb_kupdate, 0) < 0)
+	if (pdflush_operation(wb_kupdate, 0, NULL) < 0)
 		mod_timer(&wb_timer, jiffies + HZ); /* delay 1 second */
 }
 
-static void laptop_flush(unsigned long unused)
+static void laptop_flush(unsigned long unused, nodemask_t *unused2)
 {
 	sys_sync();
 }
 
 static void laptop_timer_fn(unsigned long unused)
 {
-	pdflush_operation(laptop_flush, 0);
+	pdflush_operation(laptop_flush, 0, NULL);
 }
 
 /*

mm/pdflush.c +12 -3

diff -uprN -X 0/Documentation/dontdiff 1/mm/pdflush.c 2/mm/pdflush.c
@@ -83,10 +83,12 @@ static unsigned long last_empty_jifs;
  */
 struct pdflush_work {
 	struct task_struct *who;	/* The thread */
-	void (*fn)(unsigned long);	/* A callback function */
+	void (*fn)(unsigned long, nodemask_t *); /* A callback function */
 	unsigned long arg0;		/* An argument to the callback */
 	struct list_head list;		/* On pdflush_list, when idle */
 	unsigned long when_i_went_to_sleep;
+	int have_nodes;			/* Nodes were specified */
+	nodemask_t nodes;		/* Nodes of interest */
 };
 
 static int __pdflush(struct pdflush_work *my_work)

@@ -124,7 +126,8 @@ static int __pdflush(struct pdflush_work
 		}
 		spin_unlock_irq(&pdflush_lock);
 
-		(*my_work->fn)(my_work->arg0);
+		(*my_work->fn)(my_work->arg0,
+			my_work->have_nodes ? &my_work->nodes : NULL);
 
 		/*
 		 * Thread creation: For how long have there been zero

@@ -198,7 +201,8 @@ static int pdflush(void *dummy)
  * Returns zero if it indeed managed to find a worker thread, and passed your
  * payload to it.
  */
-int pdflush_operation(void (*fn)(unsigned long), unsigned long arg0)
+int pdflush_operation(void (*fn)(unsigned long, nodemask_t *),
+			unsigned long arg0, nodemask_t *nodes)
 {
 	unsigned long flags;
 	int ret = 0;

@@ -218,6 +222,11 @@ int pdflush_operation(void (*fn)(unsigne
 			last_empty_jifs = jiffies;
 		pdf->fn = fn;
 		pdf->arg0 = arg0;
+		if (nodes) {
+			pdf->nodes = *nodes;
+			pdf->have_nodes = 1;
+		} else
+			pdf->have_nodes = 0;
 		wake_up_process(pdf->who);
 		spin_unlock_irqrestore(&pdflush_lock, flags);
 	}

mm/vmscan.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 1/mm/vmscan.c 2/mm/vmscan.c
@@ -1301,7 +1301,7 @@ unsigned long do_try_to_free_pages(struc
 		 */
 		if (total_scanned > sc->swap_cluster_max +
 					sc->swap_cluster_max / 2) {
-			wakeup_pdflush(laptop_mode ? 0 : total_scanned);
+			wakeup_pdflush(laptop_mode ? 0 : total_scanned, NULL);
 			sc->may_writepage = 1;
 		}

Ethan Solomita

author

2007-09-12 01:39 UTC #23 lore raw

PATCH 3/6 cpuset write throttle

Make page writeback obey cpuset constraints

Currently dirty throttling does not work properly in a cpuset.

Signed-off-by Christoph Lameter <[email protected]>

Signed-off-by Ethan Solomita <[email protected]>

---

Patch against 2.6.23-rc4-mm1

mm/page-writeback.c +106 ~~-50~~

diff -uprN -X 0/Documentation/dontdiff 2/mm/page-writeback.c 3/mm/page-writeback.c
@@ -103,6 +103,14 @@ EXPORT_SYMBOL(laptop_mode);
 
 static void background_writeout(unsigned long _min_pages, nodemask_t *nodes);
 
+struct dirty_limits {
+	long thresh_background;
+	long thresh_dirty;
+	unsigned long nr_dirty;
+	unsigned long nr_unstable;
+	unsigned long nr_writeback;
+};
+
 /*
  * Work out the current dirty-memory clamping and background writeout
  * thresholds.

@@ -121,16 +129,20 @@ static void background_writeout(unsigned
  * clamping level.
  */
 
-static unsigned long highmem_dirtyable_memory(unsigned long total)
+static unsigned long highmem_dirtyable_memory(nodemask_t *nodes, unsigned long total)
 {
 #ifdef CONFIG_HIGHMEM
 	int node;
 	unsigned long x = 0;
 
+	if (nodes == NULL)
+		nodes = &node_online_mask;
 	for_each_node_state(node, N_HIGH_MEMORY) {
 		struct zone *z =
 			&NODE_DATA(node)->node_zones[ZONE_HIGHMEM];
 
+		if (!node_isset(node, nodes))
+			continue;
 		x += zone_page_state(z, NR_FREE_PAGES)
 			+ zone_page_state(z, NR_INACTIVE)
 			+ zone_page_state(z, NR_ACTIVE);

@@ -154,26 +166,74 @@ static unsigned long determine_dirtyable
 	x = global_page_state(NR_FREE_PAGES)
 		+ global_page_state(NR_INACTIVE)
 		+ global_page_state(NR_ACTIVE);
-	x -= highmem_dirtyable_memory(x);
+	x -= highmem_dirtyable_memory(NULL, x);
 	return x + 1;	/* Ensure that we never return 0 */
 }
 
-static void
-get_dirty_limits(long *pbackground, long *pdirty,
-					struct address_space *mapping)
+static int
+get_dirty_limits(struct dirty_limits *dl, struct address_space *mapping,
+					nodemask_t *nodes)
 {
 	int background_ratio;		/* Percentages */
 	int dirty_ratio;
 	int unmapped_ratio;
 	long background;
 	long dirty;
-	unsigned long available_memory = determine_dirtyable_memory();
+	unsigned long available_memory;
+	unsigned long nr_mapped;
 	struct task_struct *tsk;
+	int is_subset = 0;
 
-	unmapped_ratio = 100 - ((global_page_state(NR_FILE_MAPPED) +
-				global_page_state(NR_ANON_PAGES)) * 100) /
-					available_memory;
+#ifdef CONFIG_CPUSETS
+	if (unlikely(nodes &&
+			!nodes_subset(node_online_map, *nodes))) {
+		int node;
 
+		/*
+		 * Calculate the limits relative to the current cpuset.
+		 *
+		 * We do not disregard highmem because all nodes (except
+		 * maybe node 0) have either all memory in HIGHMEM (32 bit) or
+		 * all memory in non HIGHMEM (64 bit). If we would disregard
+		 * highmem then cpuset throttling would not work on 32 bit.
+		 */
+		is_subset = 1;
+		memset(dl, 0, sizeof(struct dirty_limits));
+		available_memory = 0;
+		nr_mapped = 0;
+		for_each_node_mask(node, *nodes) {
+			if (!node_online(node))
+				continue;
+			dl->nr_dirty += node_page_state(node, NR_FILE_DIRTY);
+			dl->nr_unstable +=
+				node_page_state(node, NR_UNSTABLE_NFS);
+			dl->nr_writeback +=
+				node_page_state(node, NR_WRITEBACK);
+			available_memory +=
+				node_page_state(node, NR_ACTIVE) +
+				node_page_state(node, NR_INACTIVE) +
+				node_page_state(node, NR_FREE_PAGES);
+			nr_mapped += node_page_state(node, NR_FILE_MAPPED) +
+				node_page_state(node, NR_ANON_PAGES);
+		}
+		available_memory -= highmem_dirtyable_memory(nodes,
+							     available_memory);
+		/* Ensure that we return >= 0 */
+		if (available_memory <= 0)
+			available_memory = 1;
+	} else
+#endif
+	{
+		/* Global limits */
+		dl->nr_dirty = global_page_state(NR_FILE_DIRTY);
+		dl->nr_unstable = global_page_state(NR_UNSTABLE_NFS);
+		dl->nr_writeback = global_page_state(NR_WRITEBACK);
+		available_memory = determine_dirtyable_memory();
+		nr_mapped = global_page_state(NR_FILE_MAPPED) +
+			global_page_state(NR_ANON_PAGES);
+	}
+
+	unmapped_ratio = 100 - (nr_mapped * 100 / available_memory);
 	dirty_ratio = vm_dirty_ratio;
 	if (dirty_ratio > unmapped_ratio / 2)
 		dirty_ratio = unmapped_ratio / 2;

@@ -192,8 +252,9 @@ get_dirty_limits(long *pbackground, long
 		background += background / 4;
 		dirty += dirty / 4;
 	}
-	*pbackground = background;
-	*pdirty = dirty;
+	dl->thresh_background = background;
+	dl->thresh_dirty = dirty;
+	return is_subset;
 }
 
 /*

@@ -206,8 +267,7 @@ get_dirty_limits(long *pbackground, long
 static void balance_dirty_pages(struct address_space *mapping)
 {
 	long nr_reclaimable;
-	long background_thresh;
-	long dirty_thresh;
+	struct dirty_limits dl;
 	unsigned long pages_written = 0;
 	unsigned long write_chunk = sync_writeback_pages();

@@ -222,11 +282,12 @@ static void balance_dirty_pages(struct a
 			.range_cyclic	= 1,
 		};
 
-		get_dirty_limits(&background_thresh, &dirty_thresh, mapping);
-		nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-		if (nr_reclaimable + global_page_state(NR_WRITEBACK) <=
-			dirty_thresh)
+		if (get_dirty_limits(&dl, mapping,
+				&cpuset_current_mems_allowed))
+			wbc.nodes = &cpuset_current_mems_allowed;
+		nr_reclaimable = dl.nr_dirty + dl.nr_unstable;
+		if (nr_reclaimable + dl.nr_writeback <=
+			dl.thresh_dirty)
 				break;
 
 		if (!dirty_exceeded)

@@ -240,13 +301,10 @@ static void balance_dirty_pages(struct a
 		 */
 		if (nr_reclaimable) {
 			writeback_inodes(&wbc);
-			get_dirty_limits(&background_thresh,
-					 	&dirty_thresh, mapping);
-			nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-			if (nr_reclaimable +
-				global_page_state(NR_WRITEBACK)
-					<= dirty_thresh)
+			get_dirty_limits(&dl, mapping,
+				&cpuset_current_mems_allowed);
+			nr_reclaimable = dl.nr_dirty + dl.nr_unstable;
+			if (nr_reclaimable + dl.nr_writeback <= dl.thresh_dirty)
 						break;
 			pages_written += write_chunk - wbc.nr_to_write;
 			if (pages_written >= write_chunk)

@@ -255,8 +313,8 @@ static void balance_dirty_pages(struct a
 		congestion_wait(WRITE, HZ/10);
 	}
 
-	if (nr_reclaimable + global_page_state(NR_WRITEBACK)
-		<= dirty_thresh && dirty_exceeded)
+	if (nr_reclaimable + dl.nr_writeback
+		<= dl.thresh_dirty && dirty_exceeded)
 			dirty_exceeded = 0;
 
 	if (writeback_in_progress(bdi))

@@ -271,8 +329,9 @@ static void balance_dirty_pages(struct a
 	 * background_thresh, to keep the amount of dirty memory low.
 	 */
 	if ((laptop_mode && pages_written) ||
-	     (!laptop_mode && (nr_reclaimable > background_thresh)))
-		pdflush_operation(background_writeout, 0, NULL);
+	     (!laptop_mode && (nr_reclaimable > dl.thresh_background)))
+		pdflush_operation(background_writeout, 0,
+			&cpuset_current_mems_allowed);
 }
 
 void set_page_dirty_balance(struct page *page)

@@ -329,8 +388,7 @@ EXPORT_SYMBOL(balance_dirty_pages_rateli
 
 void throttle_vm_writeout(gfp_t gfp_mask)
 {
-	long background_thresh;
-	long dirty_thresh;
+	struct dirty_limits dl;
 
 	if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO)) {
 		/*

@@ -342,27 +400,26 @@ void throttle_vm_writeout(gfp_t gfp_mask
 		return;
 	}
 
-        for ( ; ; ) {
-		get_dirty_limits(&background_thresh, &dirty_thresh, NULL);
+	for ( ; ; ) {
+		get_dirty_limits(&dl, NULL, &node_online_map);
+
+		/*
+		 * Boost the allowable dirty threshold a bit for page
+		 * allocators so they don't get DoS'ed by heavy writers
+		 */
+		dl.thresh_dirty += dl.thresh_dirty / 10; /* wheeee... */
 
-                /*
-                 * Boost the allowable dirty threshold a bit for page
-                 * allocators so they don't get DoS'ed by heavy writers
-                 */
-                dirty_thresh += dirty_thresh / 10;      /* wheeee... */
-
-                if (global_page_state(NR_UNSTABLE_NFS) +
-			global_page_state(NR_WRITEBACK) <= dirty_thresh)
-                        	break;
-                congestion_wait(WRITE, HZ/10);
-        }
+		if (dl.nr_unstable + dl.nr_writeback <= dl.thresh_dirty)
+			break;
+		congestion_wait(WRITE, HZ/10);
+	}
 }
 
 /*
  * writeback at least _min_pages, and keep writing until the amount of dirty
  * memory is less than the background threshold, or until we're all clean.
  */
-static void background_writeout(unsigned long _min_pages, nodemask_t *unused)
+static void background_writeout(unsigned long _min_pages, nodemask_t *nodes)
 {
 	long min_pages = _min_pages;
 	struct writeback_control wbc = {

@@ -375,12 +432,11 @@ static void background_writeout(unsigned
 	};
 
 	for ( ; ; ) {
-		long background_thresh;
-		long dirty_thresh;
+		struct dirty_limits dl;
 
-		get_dirty_limits(&background_thresh, &dirty_thresh, NULL);
-		if (global_page_state(NR_FILE_DIRTY) +
-			global_page_state(NR_UNSTABLE_NFS) < background_thresh
+		if (get_dirty_limits(&dl, NULL, nodes))
+			wbc.nodes = nodes;
+		if (dl.nr_dirty + dl.nr_unstable < dl.thresh_background
 				&& min_pages <= 0)
 			break;
 		wbc.encountered_congestion = 0;

Ethan Solomita

author

2007-09-12 01:40 UTC #36 lore raw

PATCH 4/6 cpuset write vmscan

Direct reclaim: cpuset aware writeout

A. No writeout occurs because global dirty limits have not been reached

B. Writeout starts randomly for some dirty inode in the system. Pdflush
may just write out data for nodes in another cpuset and miss doing proper dirty handling for the current cpuset.

In both cases dirty pages in the zones of interest may not be affected and writeout may not occur as necessary.

Fix that by restricting pdflush to the active cpuset. Writeout will occur from direct reclaim the same way as without a cpuset.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.23-rc4-mm1

mm/vmscan.c +2 -1

diff -uprN -X 0/Documentation/dontdiff 3/mm/vmscan.c 4/mm/vmscan.c
@@ -1301,7 +1301,8 @@ unsigned long do_try_to_free_pages(struc
 		 */
 		if (total_scanned > sc->swap_cluster_max +
 					sc->swap_cluster_max / 2) {
-			wakeup_pdflush(laptop_mode ? 0 : total_scanned, NULL);
+			wakeup_pdflush(laptop_mode ? 0 : total_scanned,
+				&cpuset_current_mems_allowed);
 			sc->may_writepage = 1;
 		}

Ethan Solomita

author

2007-09-12 01:41 UTC #37 lore raw

PATCH 5/6 cpuset write vm writeout

Throttle VM writeout in a cpuset aware way

This bases the vm throttling from the reclaim path on the dirty ratio of the cpuset. Note that a cpuset is only effective if shrink_zone is called from direct reclaim.

kswapd has a cpuset context that includes the whole machine. VM throttling will only work during synchrononous reclaim and not from kswapd.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.23-rc4-mm1

include/linux/writeback.h +1 -1

diff -uprN -X 0/Documentation/dontdiff 4/include/linux/writeback.h 5/include/linux/writeback.h
@@ -94,7 +94,7 @@ static inline void inode_sync_wait(struc
 int wakeup_pdflush(long nr_pages, nodemask_t *nodes);
 void laptop_io_completion(void);
 void laptop_sync_completion(void);
-void throttle_vm_writeout(gfp_t gfp_mask);
+void throttle_vm_writeout(nodemask_t *nodes,gfp_t gfp_mask);
 
 /* These are exported to sysctl. */
 extern int dirty_background_ratio;

mm/page-writeback.c +2 -2

diff -uprN -X 0/Documentation/dontdiff 4/mm/page-writeback.c 5/mm/page-writeback.c
@@ -386,7 +386,7 @@ void balance_dirty_pages_ratelimited_nr(
 }
 EXPORT_SYMBOL(balance_dirty_pages_ratelimited_nr);
 
-void throttle_vm_writeout(gfp_t gfp_mask)
+void throttle_vm_writeout(nodemask_t *nodes, gfp_t gfp_mask)
 {
 	struct dirty_limits dl;

@@ -401,7 +401,7 @@ void throttle_vm_writeout(gfp_t gfp_mask
 	}
 
 	for ( ; ; ) {
-		get_dirty_limits(&dl, NULL, &node_online_map);
+		get_dirty_limits(&dl, NULL, nodes);
 
 		/*
 		 * Boost the allowable dirty threshold a bit for page

mm/vmscan.c +1 -1

diff -uprN -X 0/Documentation/dontdiff 4/mm/vmscan.c 5/mm/vmscan.c
@@ -1185,7 +1185,7 @@ static unsigned long shrink_zone(int pri
 		}
 	}
 
-	throttle_vm_writeout(sc->gfp_mask);
+	throttle_vm_writeout(&cpuset_current_mems_allowed, sc->gfp_mask);
 
 	atomic_dec(&zone->reclaim_in_progress);
 	return nr_reclaimed;

Ethan Solomita

author

2007-09-12 01:42 UTC #38 lore raw

PATCH 6/6 cpuset dirty limits

Per cpuset dirty ratios

This implements dirty ratios per cpuset. Two new files are added to the cpuset directories:

background_dirty_ratio Percentage at which background writeback starts

throttle_dirty_ratio Percentage at which the application is throttled
and we start synchrononous writeout.

Both variables are set to -1 by default which means that the global limits (/proc/sys/vm/vm_dirty_ratio and /proc/sys/vm/dirty_background_ratio) are used for a cpuset.

Signed-off-by Christoph Lameter <[email protected]>

---

Patch against 2.6.23-rc4-mm1

include/linux/cpuset.h +1 -0

diff -uprN -X 0/Documentation/dontdiff 5/include/linux/cpuset.h 7/include/linux/cpuset.h
@@ -77,6 +77,7 @@ extern void cpuset_track_online_nodes(vo
 
 extern int current_cpuset_is_being_rebound(void);
 
+extern void cpuset_get_current_ratios(int *background, int *ratio);
 /*
  * We need macros since struct address_space is not defined yet
  */

kernel/cpuset.c +78 -1

146 lines not on this page · raw message

mm/page-writeback.c +3 -2

diff -uprN -X 0/Documentation/dontdiff 5/mm/page-writeback.c 7/mm/page-writeback.c
@@ -221,6 +221,7 @@ get_dirty_limits(struct dirty_limits *dl
 		/* Ensure that we return >= 0 */
 		if (available_memory <= 0)
 			available_memory = 1;
+		cpuset_get_current_ratios(&background_ratio, &dirty_ratio);
 	} else
 #endif
 	{

@@ -231,17 +232,17 @@ get_dirty_limits(struct dirty_limits *dl
 		available_memory = determine_dirtyable_memory();
 		nr_mapped = global_page_state(NR_FILE_MAPPED) +
 			global_page_state(NR_ANON_PAGES);
+		dirty_ratio = vm_dirty_ratio;
+		background_ratio = dirty_background_ratio;
 	}
 
 	unmapped_ratio = 100 - (nr_mapped * 100 / available_memory);
-	dirty_ratio = vm_dirty_ratio;
 	if (dirty_ratio > unmapped_ratio / 2)
 		dirty_ratio = unmapped_ratio / 2;
 
 	if (dirty_ratio < 5)
 		dirty_ratio = 5;
 
-	background_ratio = dirty_background_ratio;
 	if (background_ratio >= dirty_ratio)
 		background_ratio = dirty_ratio / 2;

Andrew Morton

maintainer

2007-09-14 23:15 UTC #12 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 1 line · On Tue, 11 Sep 2007 18:36:34 -0700 Ethan Solomita <[email protected]> wrote:

Add a dirty map to struct address_space

I get a tremendous number of rejects trying to wedge this stuff on top of Peter's mm-dirty-balancing-for-tasks changes. More rejects than I am prepared to partially-fix so that I can usefully look at these changes in tkdiff, so this is all based on a quick peek at the diff itself..

Quoted 25 lines

In a NUMA system it is helpful to know where the dirty pages of a mapping are located. That way we will be able to implement writeout for applications that are constrained to a portion of the memory of the system as required by cpusets.
This patch implements the management of dirty node maps for an address space through the following functions:
cpuset_clear_dirty_nodes(mapping) Clear the map of dirty nodes
cpuset_update_nodes(mapping, page) Record a node in the dirty nodes map
cpuset_init_dirty_nodes(mapping) First time init of the map
The dirty map may be stored either directly in the mapping (for NUMA systems with less then BITS_PER_LONG nodes) or separately allocated for systems with a large number of nodes (f.e. IA64 with 1024 nodes).
Updating the dirty map may involve allocating it first for large configurations. Therefore we protect the allocation and setting of a node in the map through the tree_lock. The tree_lock is already taken when a page is dirtied so there is no additional locking overhead if we insert the updating of the nodemask there.

Quoted 6 lines

The dirty map is only cleared (or freed) when the inode is cleared. At that point no pages are attached to the inode anymore and therefore it can be done without any locking. The dirty map therefore records all nodes that have been used for dirty pages by that inode until the inode is no longer used.

It'd be nice to see some discussion regarding the memory consumption of this patch and the associated tradeoffs.

Quoted 3 lines

...
+#if MAX_NUMNODES <= BITS_PER_LONG

The patch is sprinkled full of this conditional.

I don't understand why this is being done. afaict it isn't described in a code comment (it should be) nor even in the changelogs?

Given its overall complexity and its likelihood to change in the future, I'd suggest that this conditional be centralised in a single place. Something like

  /*
   * nice comment goes here
   */
  #if MAX_NUMNODES <= BITS_PER_LONG
  #define CPUSET_DIRTY_LIMITS 1
  #else
  #define CPUSET_DIRTY_LIMITS 0
  #endif

Then use #if CPUSET_DIRTY_LIMITS everywhere else.

(This is better than #ifdef CPUSET_DIRTY_LIMITS because we'll et a warning if someone typos '#if CPUSET_DITRY_LIMITS')

Even better would be to calculate CPUSET_DIRTY_LIMITS within Kconfig, but I suspect you'll need to jump through unfeasible hoops to do that sort of calculation within Kconfig.

include/linux/fs.h struct address_space { · line 525

Quoted 13 lines

include/linux/fs.h +7 -0

@@ -516,6 +516,13 @@ struct address_space {
 	spinlock_t		private_lock;	/* for use by the address_space */
 	struct list_head	private_list;	/* ditto */
 	struct address_space	*assoc_mapping;	/* ditto */
+#ifdef CONFIG_CPUSETS
+#if MAX_NUMNODES <= BITS_PER_LONG
+	nodemask_t		dirty_nodes;	/* nodes with dirty pages */
+#else
+	nodemask_t		*dirty_nodes;	/* pointer to map if dirty */
+#endif
+#endif

afacit there is no code comment and no changelog text which explains the above design decision? There should be, please.

There is talk of making cpusets available with CONFIG_SMP=n. Will this new feature be available in that case? (it should be).

include/linux/writeback.h struct writeback_control { · line 66

Quoted 12 lines

include/linux/fs.h +0 -0

 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but

diff -uprN -X 0/Documentation/dontdiff 0/include/linux/writeback.h 1/include/linux/writeback.h

include/linux/writeback.h +1 -0

@@ -62,6 +62,7 @@ struct writeback_control {
 	unsigned for_writepages:1;	/* This is a writepages() call */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 	void *fs_private;               /* For use by ->writepages() */
+	nodemask_t *nodes;		/* Set of nodes of interest */
 };

That comment is a bit terse. It's always good to be lavish when commenting data structures, for understanding those is key to understanding a design.

Quoted 22 lines

include/linux/writeback.h +0 -0

/*

diff -uprN -X 0/Documentation/dontdiff 0/kernel/cpuset.c 1/kernel/cpuset.c

kernel/cpuset.c +2 -1

@@ -4,7 +4,7 @@
  *  Processor and Memory placement constraints for sets of tasks.
  *
  *  Copyright (C) 2003 BULL SA.
- *  Copyright (C) 2004-2006 Silicon Graphics, Inc.
+ *  Copyright (C) 2004-2007 Silicon Graphics, Inc.
  *  Copyright (C) 2006 Google, Inc
  *
  *  Portions derived from Patrick Mochel's sysfs code.

@@ -14,6 +14,7 @@
  *  2003-10-22 Updates by Stephen Hemminger.
  *  2004 May-July Rework by Paul Jackson.
  *  2006 Rework by Paul Menage to use generic containers
+ *  2007 Cpuset writeback by Christoph Lameter.
  *
  *  This file is subject to the terms and conditions of the GNU General Public
  *  License.  See the file COPYING in the main directory of the Linux

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1758

Quoted 4 lines

kernel/cpuset.c +1 -0

@@ -1754,6 +1755,63 @@ int cpuset_mem_spread_node(void)
 }
 EXPORT_SYMBOL_GPL(cpuset_mem_spread_node);
 
+#if MAX_NUMNODES > BITS_PER_LONG

waah. In other places we do "MAX_NUMNODES <= BITS_PER_LONG"

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1766

Quoted 8 lines

kernel/cpuset.c +8 -0

+
+/*
+ * Special functions for NUMA systems with a large number of nodes.
+ * The nodemask is pointed to from the address space structures.
+ * The attachment of the dirty_node mask is protected by the
+ * tree_lock. The nodemask is freed only when the inode is cleared
+ * (and therefore unused, thus no locking necessary).
+ */

hmm, OK, there's a hint as to wghat's going on.

It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1774

Quoted 8 lines

kernel/cpuset.c +8 -0

+void cpuset_update_dirty_nodes(struct address_space *mapping,
+			struct page *page)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+	int node = page_to_nid(page);
+
+	if (!nodes) {
+		nodes = kmalloc(sizeof(nodemask_t), GFP_ATOMIC);

Does it have to be atomic? atomic is weak and can fail.

If some callers can do GFP_KERNEL and some can only do GFP_ATOMIC then we should at least pass the gfp_t into this function so it can do the stronger allocation when possible.

Quoted 10 lines

kernel/cpuset.c +10 -0

+		if (!nodes)
+			return;
+
+		*nodes = NODE_MASK_NONE;
+		mapping->dirty_nodes = nodes;
+	}
+
+	if (!node_isset(node, *nodes))
+		node_set(node, *nodes);
+}

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1794

Quoted 10 lines

kernel/cpuset.c +10 -0

+
+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+
+	if (nodes) {
+		mapping->dirty_nodes = NULL;
+		kfree(nodes);
+	}
+}

Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.

Quoted 37 lines

kernel/cpuset.c +19 -0

+/*
+ * Called without the tree_lock. The nodemask is only freed when the inode
+ * is cleared and therefore this is safe.
+ */
+int cpuset_intersects_dirty_nodes(struct address_space *mapping,
+			nodemask_t *mask)
+{
+	nodemask_t *dirty_nodes = mapping->dirty_nodes;
+
+	if (!mask)
+		return 1;
+
+	if (!dirty_nodes)
+		return 0;
+
+	return nodes_intersects(*dirty_nodes, *mask);
+}
+#endif
+
 /**
  * cpuset_excl_nodes_overlap - Do we overlap @p's mem_exclusive ancestors?
  * @p: pointer to task_struct of some other task.

diff -uprN -X 0/Documentation/dontdiff 0/mm/page-writeback.c 1/mm/page-writeback.c

mm/page-writeback.c +1 -0

@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/buffer_head.h>
 #include <linux/pagevec.h>
+#include <linux/cpuset.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

mm/page-writeback.c int __set_page_dirty_nobuffers(struct pa · line 839

Quoted 7 lines

mm/page-writeback.c +1 -0

@@ -832,6 +833,7 @@ int __set_page_dirty_nobuffers(struct pa
 			radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 		}
+		cpuset_update_dirty_nodes(mapping, page);
 		write_unlock_irq(&mapping->tree_lock);
 		if (mapping->host) {
 			/* !PageAnon && !swapper_space */

Andrew Morton

maintainer

2007-09-14 23:15 UTC #39 lore raw

Re: [PATCH 6/6] cpuset dirty limits

Quoted 68 lines · On Tue, 11 Sep 2007 18:42:16 -0700 Ethan Solomita <[email protected]> wrote:

Per cpuset dirty ratios
This implements dirty ratios per cpuset. Two new files are added to the cpuset directories:
background_dirty_ratio Percentage at which background writeback starts
throttle_dirty_ratio Percentage at which the application is throttled
and we start synchrononous writeout.
Both variables are set to -1 by default which means that the global limits (/proc/sys/vm/vm_dirty_ratio and /proc/sys/vm/dirty_background_ratio) are used for a cpuset.
Signed-off-by Christoph Lameter <[email protected]>
Acked-by Ethan Solomita <[email protected]>
---
Patch against 2.6.23-rc4-mm1
diff -uprN -X 0/Documentation/dontdiff 5/include/linux/cpuset.h 7/include/linux/cpuset.h
include/linux/cpuset.h +1 -0
@@ -77,6 +77,7 @@ extern void cpuset_track_online_nodes(vo
 
 extern int current_cpuset_is_being_rebound(void);
 
+extern void cpuset_get_current_ratios(int *background, int *ratio);
 /*
  * We need macros since struct address_space is not defined yet
  */
diff -uprN -X 0/Documentation/dontdiff 5/kernel/cpuset.c 7/kernel/cpuset.c
kernel/cpuset.c +8 -0
@@ -51,6 +51,7 @@
 #include <linux/time.h>
 #include <linux/backing-dev.h>
 #include <linux/sort.h>
+#include <linux/writeback.h>
 
 #include <asm/uaccess.h>
 #include <asm/atomic.h>
@@ -92,6 +93,9 @@ struct cpuset {
 	int mems_generation;
 
 	struct fmeter fmeter;		/* memory_pressure filter */
+
+	int background_dirty_ratio;
+	int throttle_dirty_ratio;
 };
 
 /* Retrieve the cpuset for a container */
@@ -169,6 +173,8 @@ static struct cpuset top_cpuset = {
 	.flags = ((1 << CS_CPU_EXCLUSIVE) | (1 << CS_MEM_EXCLUSIVE)),
 	.cpus_allowed = CPU_MASK_ALL,
 	.mems_allowed = NODE_MASK_ALL,
+	.background_dirty_ratio = -1,
+	.throttle_dirty_ratio = -1,
 };
 
 /*
@@ -785,6 +791,21 @@ static int update_flag(cpuset_flagbits_t
 	return 0;
 }
 
+static int update_int(int *cs_int, char *buf, int min, int max)
+{

kernel/cpuset.c static int update_flag(cpuset_flagbits_t · line 805

Quoted 10 lines

kernel/cpuset.c +10 -0

+	char *endp;
+	int val;
+
+	val = simple_strtol(buf, &endp, 10);
+	if (val < min || val > max)
+		return -EINVAL;
+
+	mutex_lock(&callback_mutex);
+	*cs_int = val;
+	mutex_unlock(&callback_mutex);

I don't think this locking does anything?

Quoted 7 lines

kernel/cpuset.c +3 -0

+	return 0;
+}
+
 /*
  * Frequency meter - How fast is some event occurring?
  *
...

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1825

Quoted 10 lines

kernel/cpuset.c +10 -0

+void cpuset_get_current_ratios(int *background_ratio, int *throttle_ratio)
+{
+	int background = -1;
+	int throttle = -1;
+	struct task_struct *tsk = current;
+
+	task_lock(tsk);
+	background = task_cs(tsk)->background_dirty_ratio;
+	throttle = task_cs(tsk)->throttle_dirty_ratio;
+	task_unlock(tsk);

ditto?

Satyam Sharma

2007-09-14 23:47 UTC #13 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 32 lines · Andrew Morton

Quoted 7 lines · On Tue, 11 Sep 2007 18:36:34 -0700 Ethan Solomita <[email protected]> wrote:
The dirty map may be stored either directly in the mapping (for NUMA systems with less then BITS_PER_LONG nodes) or separately allocated for systems with a large number of nodes (f.e. IA64 with 1024 nodes).
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 struct address_space { · line 525
Quoted 10 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +7 -0
@@ -516,6 +516,13 @@ struct address_space {
      spinlock_t              private_lock;   /* for use by the address_space */
      struct list_head        private_list;   /* ditto */
      struct address_space    *assoc_mapping; /* ditto */
+#ifdef CONFIG_CPUSETS
+#if MAX_NUMNODES <= BITS_PER_LONG
+     nodemask_t              dirty_nodes;    /* nodes with dirty pages */
+#else
+     nodemask_t              *dirty_nodes;   /* pointer to map if dirty */
+#endif
+#endif
afacit there is no code comment and no changelog text which explains the above design decision? There should be, please.
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 struct address_space {
Quoted 7 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +7 -0
+/*
+ * Special functions for NUMA systems with a large number of nodes.
+ * The nodemask is pointed to from the address space structures.
+ * The attachment of the dirty_node mask is protected by the
+ * tree_lock. The nodemask is freed only when the inode is cleared
+ * (and therefore unused, thus no locking necessary).
+ */

Quoted 5 lines

hmm, OK, there's a hint as to wghat's going on.
It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.

Looks like just an optimization to me ... Ethan wants to economize and not bloat struct address_space too much.

So, if sizeof(nodemask_t) == sizeof(long), i.e. when:
MAX_NUMNODES <= BITS_PER_LONG, then we'll be adding only sizeof(long) extra bytes to the struct (by plonking the object itself into it).

But even when MAX_NUMNODES > BITS_PER_LONG, because we're storing a pointer, and because sizeof(void *) == sizeof(long), so again the maximum bloat addition to struct address_space would only be sizeof(long) bytes.

I didn't see the original mail, but if the #ifdeffery for this conditional is too much
as a result of this optimization, Ethan should probably just do away with all of it
entirely, and simply put a full nodemask_t object (irrespective of MAX_NUMNODES) into the struct. After all, struct task_struct does the same unconditionally ... but admittedly, there are several times more address_space struct's resident in memory at any given time than there are task_struct's, so this optimization does make sense too ...

Quoted 35 lines

Quoted 10 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +10 -0
+             if (!nodes)
+                     return;
+
+             *nodes = NODE_MASK_NONE;
+             mapping->dirty_nodes = nodes;
+     }
+
+     if (!node_isset(node, *nodes))
+             node_set(node, *nodes);
+}
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700
Quoted 10 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +10 -0
+
+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+     nodemask_t *nodes = mapping->dirty_nodes;
+
+     if (nodes) {
+             mapping->dirty_nodes = NULL;
+             kfree(nodes);
+     }
+}
Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.
Quoted 11 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +11 -0
+/*
+ * Called without the tree_lock. The nodemask is only freed when the inode
+ * is cleared and therefore this is safe.
+ */
+int cpuset_intersects_dirty_nodes(struct address_space *mapping,
+                     nodemask_t *mask)
+{
+     nodemask_t *dirty_nodes = mapping->dirty_nodes;
+
+     if (!mask)
+             return 1;

Quoted 8 lines

include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700
Quoted 8 lines
include/linux/fs.h 2007-09-11 14:36:24.000000000 -0700 renamed from include/linux/fs.h 2007-09-11 14:35:58.000000000 -0700 +8 -0
+
+     if (!dirty_nodes)
+             return 0;
+
+     return nodes_intersects(*dirty_nodes, *mask);
+}
+#endif
+

Andrew Morton

maintainer

2007-09-15 00:07 UTC #14 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 7 lines · On Sat, 15 Sep 2007 05:17:48 +0530 "Satyam Sharma" <[email protected]>…

Quoted 3 lines
It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.
Looks like just an optimization to me ... Ethan wants to economize and not bloat struct address_space too much.

Quoted 7 lines

So, if sizeof(nodemask_t) == sizeof(long), i.e. when:
MAX_NUMNODES <= BITS_PER_LONG, then we'll be adding only sizeof(long) extra bytes to the struct (by plonking the object itself into it).
But even when MAX_NUMNODES > BITS_PER_LONG, because we're storing a pointer, and because sizeof(void *) == sizeof(long), so again the maximum bloat addition to struct address_space would only be sizeof(long) bytes.

yup.

Note that "It's unobvious" != "It's unobvious to me". I review code for understandability-by-others, not for understandability-by-me.

Quoted 9 lines

I didn't see the original mail, but if the #ifdeffery for this conditional is too much
as a result of this optimization, Ethan should probably just do away with all of it
entirely, and simply put a full nodemask_t object (irrespective of MAX_NUMNODES) into the struct. After all, struct task_struct does the same unconditionally ... but admittedly, there are several times more address_space struct's resident in memory at any given time than there are task_struct's, so this optimization does make sense too ...

I think the optimisation is (probably) desirable, but it would be best to describe the tradeoff in the changelog and to add some suitable code-commentary for those who read the code in a year's time and to avoid sprinkling the logic all over the tree.

Satyam Sharma

2007-09-15 00:16 UTC #15 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 33 lines · Andrew Morton

Quoted 7 lines · On Sat, 15 Sep 2007 05:17:48 +0530 "Satyam Sharma" <[email protected]>…
Quoted 3 lines
It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.
Looks like just an optimization to me ... Ethan wants to economize and not bloat struct address_space too much.
Quoted 7 lines
So, if sizeof(nodemask_t) == sizeof(long), i.e. when:
MAX_NUMNODES <= BITS_PER_LONG, then we'll be adding only sizeof(long) extra bytes to the struct (by plonking the object itself into it).
But even when MAX_NUMNODES > BITS_PER_LONG, because we're storing a pointer, and because sizeof(void *) == sizeof(long), so again the maximum bloat addition to struct address_space would only be sizeof(long) bytes.
yup.
Note that "It's unobvious" != "It's unobvious to me". I review code for understandability-by-others, not for understandability-by-me.
Quoted 9 lines
I didn't see the original mail, but if the #ifdeffery for this conditional is too much
as a result of this optimization, Ethan should probably just do away with all of it
entirely, and simply put a full nodemask_t object (irrespective of MAX_NUMNODES) into the struct. After all, struct task_struct does the same unconditionally ... but admittedly, there are several times more address_space struct's resident in memory at any given time than there are task_struct's, so this optimization does make sense too ...

Quoted 4 lines

I think the optimisation is (probably) desirable, but it would be best to describe the tradeoff in the changelog and to add some suitable code-commentary for those who read the code in a year's time and to avoid sprinkling the logic all over the tree.

True, the other option could be to put the /pointer/ in there unconditionally, but that would slow down the MAX_NUMNODES <= BITS_PER_LONG case, which (after grepping through defconfigs) appears to be the common case on all archs other than ia64. So I think your idea of making that conditional centralized in the code with an accompanying comment is the way to go here ...

Satyam

Mike Travis

2007-09-17 18:37 UTC #16 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 5 lines · Satyam Sharma

True, the other option could be to put the /pointer/ in there unconditionally, but that would slow down the MAX_NUMNODES <= BITS_PER_LONG case, which (after grepping through defconfigs) appears to be the common case on all archs other than ia64. So I think your idea of making that conditional centralized in the code with an accompanying comment is the way to go here ...

It won't be long before arch's other than ia64 will have MAX_NUMNODES > BITS_PER_LONG. While it won't be the norm, we should account for it now.

Thanks,
Mike

Christoph Lameter

2007-09-17 19:00 UTC #40 lore raw

Re: [PATCH 6/6] cpuset dirty limits

Quoted 5 lines · Andrew Morton

kernel/cpuset.c static int update_flag(cpuset_flagbits_t · line 805
Quoted 3 lines
kernel/cpuset.c +3 -0
+	mutex_lock(&callback_mutex);
+	*cs_int = val;
+	mutex_unlock(&callback_mutex);
I don't think this locking does anything?

Locking is wrong here. The lock needs to be taken before the cs pointer is dereferenced from the caller.

Quoted 11 lines

kernel/cpuset.c +7 -0

+	return 0;
+}
+
 /*
  * Frequency meter - How fast is some event occurring?
  *
...
+void cpuset_get_current_ratios(int *background_ratio, int *throttle_ratio)
+{
+	int background = -1;
+	int throttle = -1;

Quoted 8 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1825
Quoted 6 lines
kernel/cpuset.c +6 -0
+	struct task_struct *tsk = current;
+
+	task_lock(tsk);
+	background = task_cs(tsk)->background_dirty_ratio;
+	throttle = task_cs(tsk)->throttle_dirty_ratio;
+	task_unlock(tsk);
ditto?

It is required to take the task lock while dereferencing the tasks cpuset pointer.

Christoph Lameter

2007-09-17 19:10 UTC #17 lore raw

Re: [PATCH 1/6] cpuset write dirty map

In reply to Christoph Lameter

Quoted 2 lines · Andrew Morton

It'd be nice to see some discussion regarding the memory consumption of this patch and the associated tradeoffs.

On small NUMA system < 64 nodes you basically have an additional word added to the address_space structure. On large NUMA we may have to allocate very large per node arrays of up to 1024 nodes which may result in 128 bytes used. However, the very large systems are rare thus we want to do the allocation dynamically.

Quoted 2 lines

afacit there is no code comment and no changelog text which explains the above design decision? There should be, please.

Hmmmm... There was an extensive discussion on that one early in the year and I had it in the overview of the earlier releases. Ethan: Maybe get that piece into the overview?

Quoted 2 lines

There is talk of making cpusets available with CONFIG_SMP=n. Will this new feature be available in that case? (it should be).

Yes. Cpuset with CONFIG_SMP=n has been run for years on IA64 with Tony Luck's special configurations.

Quoted 5 lines

Quoted 3 lines
EXPORT_SYMBOL_GPL(cpuset_mem_spread_node);
+#if MAX_NUMNODES > BITS_PER_LONG
waah. In other places we do "MAX_NUMNODES <= BITS_PER_LONG"

#if !(MAX_NUMNODES <= BITS_PER_LONG)

Quoted 8 lines

Quoted 7 lines
+/*
+ * Special functions for NUMA systems with a large number of nodes.
+ * The nodemask is pointed to from the address space structures.
+ * The attachment of the dirty_node mask is protected by the
+ * tree_lock. The nodemask is freed only when the inode is cleared
+ * (and therefore unused, thus no locking necessary).
+ */

Quoted 5 lines

hmm, OK, there's a hint as to wghat's going on.
It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.

A pointer has the size of BITS_PER_LONG. If we have less nodes then we just waste memory and processing by having a pointer there.

Quoted 11 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1774
Quoted 8 lines
kernel/cpuset.c +8 -0
+void cpuset_update_dirty_nodes(struct address_space *mapping,
+			struct page *page)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+	int node = page_to_nid(page);
+
+	if (!nodes) {
+		nodes = kmalloc(sizeof(nodemask_t), GFP_ATOMIC);
Does it have to be atomic? atomic is weak and can fail.

Quoted 3 lines

If some callers can do GFP_KERNEL and some can only do GFP_ATOMIC then we should at least pass the gfp_t into this function so it can do the stronger allocation when possible.

This is called from __set_page_dirty_nobuffers. There is no allocation context available at that point. If the allocation fails then we do not allocate a tracking structure which means that no targeted writeout occurs.

Quoted 4 lines

kernel/cpuset.c +4 -0

+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+

Quoted 8 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1794
Quoted 5 lines
kernel/cpuset.c +5 -0
+	if (nodes) {
+		mapping->dirty_nodes = NULL;
+		kfree(nodes);
+	}
+}
Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.

You just quoted the comment above that explains the locking requirements. Here it is again:

/*
+ * Special functions for NUMA systems with a large number of nodes.
+ * The nodemask is pointed to from the address space structures.
+ * The attachment of the dirty_node mask is protected by the
+ * tree_lock. The nodemask is freed only when the inode is cleared
+ * (and therefore unused, thus no locking necessary).
+ */

Ethan Solomita

author

2007-09-19 00:23 UTC #41 lore raw

Re: [PATCH 6/6] cpuset dirty limits

Quoted 9 lines · Christoph Lameter

Quoted 4 lines · Andrew Morton
kernel/cpuset.c static int update_flag(cpuset_flagbits_t · line 805
Quoted 3 lines
kernel/cpuset.c +3 -0
+	mutex_lock(&callback_mutex);
+	*cs_int = val;
+	mutex_unlock(&callback_mutex);
I don't think this locking does anything?
Locking is wrong here. The lock needs to be taken before the cs pointer is dereferenced from the caller.

I think we can just remove the callback_mutex lock. Since the change is
coming from an update to a cpuset filesystem file, the cpuset is not going anywhere since the inode is open. And I don't see that any code really cares whether the dirty ratios change out from under them.

Quoted 20 lines

Quoted 11 lines

kernel/cpuset.c +6 -0

+	return 0;
+}
+
 /*
  * Frequency meter - How fast is some event occurring?
  *
...
+void cpuset_get_current_ratios(int *background_ratio, int *throttle_ratio)
+{
+	int background = -1;

Quoted 8 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1825

Quoted 7 lines

kernel/cpuset.c +7 -0

+	int throttle = -1;
+	struct task_struct *tsk = current;
+
+	task_lock(tsk);
+	background = task_cs(tsk)->background_dirty_ratio;
+	throttle = task_cs(tsk)->throttle_dirty_ratio;
+	task_unlock(tsk);

ditto?

Quoted 2 lines

It is required to take the task lock while dereferencing the tasks cpuset pointer.

Agreed.
-- Ethan

Ethan Solomita

author

2007-09-19 00:51 UTC #18 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 9 lines · Andrew Morton

Quoted 1 line · On Tue, 11 Sep 2007 18:36:34 -0700 Ethan Solomita <[email protected]> wrote:
Add a dirty map to struct address_space
I get a tremendous number of rejects trying to wedge this stuff on top of Peter's mm-dirty-balancing-for-tasks changes. More rejects than I am prepared to partially-fix so that I can usefully look at these changes in tkdiff, so this is all based on a quick peek at the diff itself..

This isn't surprising. We're both changing the calculation of dirty
limits. If his code is already into your workspace, then I'll have to do the merging after you release it.

Quoted 6 lines

Quoted 1 line
+#if MAX_NUMNODES <= BITS_PER_LONG
The patch is sprinkled full of this conditional.
I don't understand why this is being done. afaict it isn't described in a code comment (it should be) nor even in the changelogs?

I can add comments.

Quoted 13 lines

Given its overall complexity and its likelihood to change in the future, I'd suggest that this conditional be centralised in a single place. Something like
  /*
   * nice comment goes here
   */
  #if MAX_NUMNODES <= BITS_PER_LONG
  #define CPUSET_DIRTY_LIMITS 1
  #else
  #define CPUSET_DIRTY_LIMITS 0
  #endif

Quoted 4 lines

Then use #if CPUSET_DIRTY_LIMITS everywhere else.
(This is better than #ifdef CPUSET_DIRTY_LIMITS because we'll et a warning if someone typos '#if CPUSET_DITRY_LIMITS')

I can add something like this. Probably something like:

CPUSET_DIRTY_LIMITS_USEPTR

Quoted 8 lines

include/linux/fs.h +2 -0

@@ -516,6 +516,13 @@ struct address_space {
 	spinlock_t		private_lock;	/* for use by the address_space */
 	struct list_head	private_list;	/* ditto */
 	struct address_space	*assoc_mapping;	/* ditto */
+#ifdef CONFIG_CPUSETS
+#if MAX_NUMNODES <= BITS_PER_LONG

Quoted 8 lines

include/linux/fs.h struct address_space { · line 525
Quoted 5 lines
include/linux/fs.h +5 -0
+	nodemask_t		dirty_nodes;	/* nodes with dirty pages */
+#else
+	nodemask_t		*dirty_nodes;	/* pointer to map if dirty */
+#endif
+#endif
afacit there is no code comment and no changelog text which explains the above design decision? There should be, please.

OK.

Quoted 3 lines

There is talk of making cpusets available with CONFIG_SMP=n. Will this new feature be available in that case? (it should be).

I'm not sure how useful it would be in that scenario, but for
consistency we should still be able to specify varying dirty ratios (from patch 6/6). The above code wouldn't mean anything SMP=n since there's only the one node. We'd just be indicating whether the inode has any dirty pages, which we already know.

Quoted 17 lines

include/linux/writeback.h struct writeback_control { · line 66
Quoted 13 lines
include/linux/fs.h +0 -0
 } __attribute__((aligned(sizeof(long))));
 	/*
 	 * On most architectures that alignment is already the case; but
diff -uprN -X 0/Documentation/dontdiff 0/include/linux/writeback.h 1/include/linux/writeback.h
include/linux/writeback.h +1 -0
@@ -62,6 +62,7 @@ struct writeback_control {
 	unsigned for_writepages:1;	/* This is a writepages() call */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 	void *fs_private;               /* For use by ->writepages() */
+	nodemask_t *nodes;		/* Set of nodes of interest */
 };
That comment is a bit terse. It's always good to be lavish when commenting data structures, for understanding those is key to understanding a design.

Quoted 25 lines

include/linux/writeback.h +0 -0

/*

diff -uprN -X 0/Documentation/dontdiff 0/kernel/cpuset.c 1/kernel/cpuset.c

kernel/cpuset.c +2 -1

@@ -4,7 +4,7 @@
  *  Processor and Memory placement constraints for sets of tasks.
  *
  *  Copyright (C) 2003 BULL SA.
- *  Copyright (C) 2004-2006 Silicon Graphics, Inc.
+ *  Copyright (C) 2004-2007 Silicon Graphics, Inc.
  *  Copyright (C) 2006 Google, Inc
  *
  *  Portions derived from Patrick Mochel's sysfs code.

@@ -14,6 +14,7 @@
  *  2003-10-22 Updates by Stephen Hemminger.
  *  2004 May-July Rework by Paul Jackson.
  *  2006 Rework by Paul Menage to use generic containers
+ *  2007 Cpuset writeback by Christoph Lameter.
  *
  *  This file is subject to the terms and conditions of the GNU General Public
  *  License.  See the file COPYING in the main directory of the Linux

@@ -1754,6 +1755,63 @@ int cpuset_mem_spread_node(void)
 }
 EXPORT_SYMBOL_GPL(cpuset_mem_spread_node);

Quoted 3 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1758
Quoted 1 line
kernel/cpuset.c +1 -0
+#if MAX_NUMNODES > BITS_PER_LONG
waah. In other places we do "MAX_NUMNODES <= BITS_PER_LONG"

Your sanity is important to me. Will fix.

Quoted 12 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1766

Quoted 9 lines

kernel/cpuset.c +8 -0

+
+/*
+ * Special functions for NUMA systems with a large number of nodes.
+ * The nodemask is pointed to from the address space structures.
+ * The attachment of the dirty_node mask is protected by the
+ * tree_lock. The nodemask is freed only when the inode is cleared
+ * (and therefore unused, thus no locking necessary).
+ */

hmm, OK, there's a hint as to wghat's going on.

Quoted 3 lines

It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and we might want to tweak that in the future. Yet another argument for centralising this comparison.

I'll add a comment to make it more obvious. The point is to only add
one long to the data structure, no matter sizeof(nodemask_t), adding either a nodemask_t or a pointer to a nodemask_t if nodemask_t is too large.

Quoted 12 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1774
Quoted 9 lines
kernel/cpuset.c +8 -0
+void cpuset_update_dirty_nodes(struct address_space *mapping,
+			struct page *page)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+	int node = page_to_nid(page);
+
+	if (!nodes) {
+		nodes = kmalloc(sizeof(nodemask_t), GFP_ATOMIC);
Does it have to be atomic? atomic is weak and can fail.

Quoted 3 lines

If some callers can do GFP_KERNEL and some can only do GFP_ATOMIC then we should at least pass the gfp_t into this function so it can do the stronger allocation when possible.

I was going to say that sanity would be improved by just allocing the
nodemask at inode alloc time. A failure here could be a problem because below cpuset_intersects_dirty_nodes() assumes that a NULL nodemask pointer means that there are no dirty nodes, thus preventing dirty pages from getting written to disk. i.e. This must never fail.

Given that we allocate it always at the beginning, I'm leaning towards
just allocating it within mapping no matter its size. It will make the code much much simpler, and save me writing all the comments we've been discussing. 8-)

How disastrous would this be? Is the need to support a 1024 node system
with 1,000,000 open mostly-read-only files thus needing to spend 120MB of extra memory on my nodemasks a real scenario and a showstopper?

Quoted 17 lines

kernel/cpuset.c +15 -0

+		if (!nodes)
+			return;
+
+		*nodes = NODE_MASK_NONE;
+		mapping->dirty_nodes = nodes;
+	}
+
+	if (!node_isset(node, *nodes))
+		node_set(node, *nodes);
+}
+
+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+

Quoted 8 lines

kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1794
Quoted 5 lines
kernel/cpuset.c +5 -0
+	if (nodes) {
+		mapping->dirty_nodes = NULL;
+		kfree(nodes);
+	}
+}
Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.

I'll add a comment. Such a race should not be possible. It is called
only from clear_inode() which is used when the inode is being freed "with extreme prejudice" (from its comments). I can add a check that i_state I_FREEING is set. Would that do?

Quoted 32 lines

kernel/cpuset.c +19 -0

+/*
+ * Called without the tree_lock. The nodemask is only freed when the inode
+ * is cleared and therefore this is safe.
+ */
+int cpuset_intersects_dirty_nodes(struct address_space *mapping,
+			nodemask_t *mask)
+{
+	nodemask_t *dirty_nodes = mapping->dirty_nodes;
+
+	if (!mask)
+		return 1;
+
+	if (!dirty_nodes)
+		return 0;
+
+	return nodes_intersects(*dirty_nodes, *mask);
+}
+#endif
+
 /**
  * cpuset_excl_nodes_overlap - Do we overlap @p's mem_exclusive ancestors?
  * @p: pointer to task_struct of some other task.

diff -uprN -X 0/Documentation/dontdiff 0/mm/page-writeback.c 1/mm/page-writeback.c

mm/page-writeback.c +1 -0

@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/buffer_head.h>
 #include <linux/pagevec.h>
+#include <linux/cpuset.h>

Quoted 13 lines

mm/page-writeback.c int __set_page_dirty_nobuffers(struct pa · line 839

Quoted 13 lines

mm/page-writeback.c +1 -0

 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -832,6 +833,7 @@ int __set_page_dirty_nobuffers(struct pa
 			radix_tree_tag_set(&mapping->page_tree,
 				page_index(page), PAGECACHE_TAG_DIRTY);
 		}
+		cpuset_update_dirty_nodes(mapping, page);
 		write_unlock_irq(&mapping->tree_lock);
 		if (mapping->host) {
 			/* !PageAnon && !swapper_space */

Andrew Morton

maintainer

2007-09-19 02:14 UTC #19 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 22 lines · On Tue, 18 Sep 2007 17:51:49 -0700 Ethan Solomita <[email protected]> wrote:

Quoted 12 lines
kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1774
Quoted 9 lines
kernel/cpuset.c +8 -0
+void cpuset_update_dirty_nodes(struct address_space *mapping,
+			struct page *page)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+	int node = page_to_nid(page);
+
+	if (!nodes) {
+		nodes = kmalloc(sizeof(nodemask_t), GFP_ATOMIC);
Does it have to be atomic? atomic is weak and can fail.
Quoted 3 lines
If some callers can do GFP_KERNEL and some can only do GFP_ATOMIC then we should at least pass the gfp_t into this function so it can do the stronger allocation when possible.
I was going to say that sanity would be improved by just allocing the
nodemask at inode alloc time. A failure here could be a problem because below cpuset_intersects_dirty_nodes() assumes that a NULL nodemask pointer means that there are no dirty nodes, thus preventing dirty pages from getting written to disk. i.e. This must never fail.

Quoted 8 lines

Given that we allocate it always at the beginning, I'm leaning towards
just allocating it within mapping no matter its size. It will make the code much much simpler, and save me writing all the comments we've been discussing. 8-)
How disastrous would this be? Is the need to support a 1024 node system
with 1,000,000 open mostly-read-only files thus needing to spend 120MB of extra memory on my nodemasks a real scenario and a showstopper?

None of this is very nice. Yes, it would be good to save all that memory and yes, I_DIRTY_PAGES inodes are very much the uncommon case.

But if a failed GFP_ATOMIC allocation results in data loss then that's a showstopper.

How hard would it be to handle the allocation failure in a more friendly manner? Say, if the allocation failed then point mapping->dirty_nodes at some global all-ones nodemask, and then special-case that nodemask in the freeing code?

Quoted 26 lines

Quoted 17 lines
Quoted 17 lines
kernel/cpuset.c +15 -0
+		if (!nodes)
+			return;
+
+		*nodes = NODE_MASK_NONE;
+		mapping->dirty_nodes = nodes;
+	}
+
+	if (!node_isset(node, *nodes))
+		node_set(node, *nodes);
+}
+
+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+
Quoted 8 lines
kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1794
Quoted 5 lines
kernel/cpuset.c +5 -0
+	if (nodes) {
+		mapping->dirty_nodes = NULL;
+		kfree(nodes);
+	}
+}
Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.

Quoted 4 lines

I'll add a comment. Such a race should not be possible. It is called
only from clear_inode() which is used when the inode is being freed "with extreme prejudice" (from its comments). I can add a check that i_state I_FREEING is set. Would that do?

Sounds sane.

Christoph Lameter

2007-09-19 17:06 UTC #21 lore raw

Re: [PATCH 1/6] cpuset write dirty map

Quoted 11 lines · Ethan Solomita

Quoted 5 lines
Does it have to be atomic? atomic is weak and can fail.
If some callers can do GFP_KERNEL and some can only do GFP_ATOMIC then we should at least pass the gfp_t into this function so it can do the stronger allocation when possible.
I was going to say that sanity would be improved by just allocing the
nodemask at inode alloc time. A failure here could be a problem because below cpuset_intersects_dirty_nodes() assumes that a NULL nodemask pointer means that there are no dirty nodes, thus preventing dirty pages from getting written to disk. i.e. This must never fail.

Hmmm. It should assume that there is no tracking thus any node can be dirty? Match by default?

Quoted 8 lines

Given that we allocate it always at the beginning, I'm leaning towards
just allocating it within mapping no matter its size. It will make the code much much simpler, and save me writing all the comments we've been discussing. 8-)
How disastrous would this be? Is the need to support a 1024 node system
with 1,000,000 open mostly-read-only files thus needing to spend 120MB of extra memory on my nodemasks a real scenario and a showstopper?

Consider that a 1024 node system has more than 4TB of memory. If that system is running as a fileserver then you get into some issues. But then 120MB are not that big of a deal. Its more the cache footprint issue I would think. Having a NULL there avoids touching a 128 byte nodemask. I think your approach should be fine.

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
kernel/cpuset.c +4 -0
+void cpuset_clear_dirty_nodes(struct address_space *mapping)
+{
+	nodemask_t *nodes = mapping->dirty_nodes;
+
Quoted 8 lines
kernel/cpuset.c int cpuset_mem_spread_node(void) · line 1794
Quoted 5 lines
kernel/cpuset.c +5 -0
+	if (nodes) {
+		mapping->dirty_nodes = NULL;
+		kfree(nodes);
+	}
+}
Can this race with cpuset_update_dirty_nodes()? And with itself? If not, a comment which describes the locking requirements would be good.

Quoted 4 lines

I'll add a comment. Such a race should not be possible. It is called
only from clear_inode() which is used when the inode is being freed "with extreme prejudice" (from its comments). I can add a check that i_state I_FREEING is set. Would that do?

There is already a comment saying that it cannot happen.

Christoph Lameter

2007-09-19 17:08 UTC #20 lore raw

Re: [PATCH 1/6] cpuset write dirty map