Re: [PATCH 2.6.12-rc5 4/10] mm: manual page migration-rc3 -- add-sys_migrate_pages-rc3.patch

[Dave Hansen <haveblue@us.ibm.com>]

On Wed, 2005-06-22 at 09:39 -0700, Ray Bryant wrote:
> +asmlinkage long
> +sys_migrate_pages(pid_t pid, __u32 count, __u32 *old_nodes, __u32 *new_nodes)
> +{

Should the buffers be marked __user?

> +       if ((count < 1) || (count > MAX_NUMNODES))
> +               return -EINVAL;

Since you have an out_einval:, it's probably best to use it
consistently.  There is another place or two like this.

> +       for (i = 0; i < count; i++) {
> +               int n;
> +
> +               n = tmp_old_nodes[i];
> +               if ((n < 0) || (n >= MAX_NUMNODES))
> +                       goto out_einval;
> +               node_set(n, old_node_mask);
> +
> +               n = tmp_new_nodes[i];
> +               if ((n < 0) || (n >= MAX_NUMNODES) || !node_online(n))
> +                       goto out_einval;
> +               node_set(n, new_node_mask);
> +
> +       }

I know it's a simple operation, but I think I'd probably break out the
array validation into its own function.

Then, replace the above loop with this:

if (!migrate_masks_valid(tmp_old_nodes, count) ||
     migrate_masks_valid(tmp_old_nodes, count))
	goto out_einval;

for (i = 0; i < count; i++) {
	node_set(tmp_old_nodes[i], old_node_mask);
	node_set(tmp_new_nodes[i], new_node_mask);
}

> +static int
> +migrate_vma(struct task_struct *task, struct mm_struct *mm,
> +       struct vm_area_struct *vma, int *node_map)
...
> +       spin_lock(&mm->page_table_lock);
> +       for (vaddr = vma->vm_start; vaddr < vma->vm_end; vaddr += PAGE_SIZE) {
> +               page = follow_page(mm, vaddr, 0);
> +               /*
> +                * follow_page has been known to return pages with zero mapcount
> +                * and NULL mapping.  Skip those pages as well
> +                */
> +               if (page && page_mapcount(page)) {
> +                       if (node_map[page_to_nid(page)] >= 0) {
> +                               if (steal_page_from_lru(page_zone(page), page,
> +                                       &page_list))
> +                                               count++;
> +                               else
> +                                       BUG();
> +                       }
> +               }
> +       }
> +       spin_unlock(&mm->page_table_lock);

Personally, I dislike having so many embedded ifs, especially in a for
loop like that.  I think it's a lot more logical to code it up as a
series of continues, mostly because it's easy to read a continue as,
"skip this page."  You can't always see that as easily with an if().  It
also makes it so that you don't have to wrap the steal_page_from_lru()
call across two lines, which is super-ugly. :)

for (vaddr = vma->vm_start; vaddr < vma->vm_end; vaddr += PAGE_SIZE) {
	page = follow_page(mm, vaddr, 0);
	if (!page || !page_mapcount(page))
		continue;

	if (node_map[page_to_nid(page)] < 0)
		continue;

	if (steal_page_from_lru(page_zone(page), page, &page_list));
		count++;
	else
		BUG();
}

The same kind of thing goes for this if: 

> +       /* call the page migration code to move the pages */
> +       if (count) {
> +               nr_busy = try_to_migrate_pages(&page_list, node_map);
> +
> +               if (nr_busy < 0)
> +                       return nr_busy;
> +
> +               if (nr_busy == 0)
> +                       return count;
> +
> +               /* return the unmigrated pages to the LRU lists */
> +               list_for_each_entry_safe(page, page2, &page_list, lru)
> {
> +                       list_del(&page->lru);
> +                       putback_page_to_lru(page_zone(page), page);
> +               }
> +               return -EAGAIN;
> +       }
> +
> +       return 0;

It looks a lot cleaner if you just do 

	if (!count)
		return count;

	... contents of the if(){} block go here

-- Dave

--
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/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>