On Tue, 18 Jan 2011 15:28:44 -0800
Andrew Morton [off-list ref] wrote:

On Tue, 18 Jan 2011 12:18:11 +0100
Miklos Szeredi [off-list ref] wrote:

quoted

+int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
+{
+	int error;
+	struct mem_cgroup *memcg = NULL;

I'm suspecting that the unneeded initialisation was added to suppress a
warning?

I removed it, and didn't get a warning.  I expected to.

Really, uninitialized_var() is better.  It avoids adding extra code
and, unlike "= 0" it is self-documenting.

quoted

+	VM_BUG_ON(!PageLocked(old));
+	VM_BUG_ON(!PageLocked(new));
+	VM_BUG_ON(new->mapping);
+
+	/*
+	 * This is not page migration, but prepare_migration and
+	 * end_migration does enough work for charge replacement.
+	 *
+	 * In the longer term we probably want a specialized function
+	 * for moving the charge from old to new in a more efficient
+	 * manner.
+	 */
+	error = mem_cgroup_prepare_migration(old, new, &memcg, gfp_mask);
+	if (error)
+		return error;
+
+	error = radix_tree_preload(gfp_mask & ~__GFP_HIGHMEM);
+	if (!error) {
+		struct address_space *mapping = old->mapping;
+		pgoff_t offset = old->index;
+
+		page_cache_get(new);
+		new->mapping = mapping;
+		new->index = offset;
+
+		spin_lock_irq(&mapping->tree_lock);
+		__remove_from_page_cache(old);
+		error = radix_tree_insert(&mapping->page_tree, offset, new);
+		BUG_ON(error);
+		mapping->nrpages++;
+		__inc_zone_page_state(new, NR_FILE_PAGES);
+		if (PageSwapBacked(new))
+			__inc_zone_page_state(new, NR_SHMEM);
+		spin_unlock_irq(&mapping->tree_lock);
+		radix_tree_preload_end();
+		page_cache_release(old);
+		mem_cgroup_end_migration(memcg, old, new, true);

This is all pretty ugly and inefficient.

We call __remove_from_page_cache() which does a radix-tree lookup and
then fiddles a bunch of accounting things.

Then we immediately do the same radix-tree lookup and then undo the
accounting changes which we just did.  And we do it in an open-coded
fashion, thus giving the kernel yet another code site where various
operations need to be kept in sync.

Would it not be better to do a single radix_tree_lookup_slot(),
overwrite the pointer therein and just leave all the ancilliary
accounting unaltered?

Poke?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>