Re: [rfc][patch] Memory Binding Take 2 (1/1)

[Christoph Hellwig]

> +#ifndef _LINUX_MBIND_H
> +#define _LINUX_MBIND_H
> +
> +#ifdef CONFIG_NUMA
> +
> +#include <linux/mmzone.h>
> +
> +/* Structure to keep track of memory segment (VMA) bindings */
> +struct binding {
> +	struct zonelist	zonelist;
> +};
> +
> +#endif /* CONFIG_NUMA */
> +#endif /* _LINUX_MBIND_H */

Using CONFIG_ without explicitly including config.h is a bad idea.
But the ifdef is unessecary anyway, no one is hurt by having this
struct defintion in the non-numa case.  Also I wonder how you can have
a copyright on a single trivial struct defintion :)  What about just
moving it to mmzone.h, btw?

> +#include <linux/errno.h>
> +#include <linux/mm.h>
> +#include <linux/mbind.h>
> +#include <asm/string.h>

Use <linux/string.h> instead.

> +	binding = (struct binding *)kmalloc(sizeof(struct binding), GFP_KERNEL);

The cast is superflous.

> +	if (!(vma && vma->vm_file && vma->vm_ops && 
> +		vma->vm_ops->nopage == shmem_nopage)) {
> +		/* This isn't a shm segment.  For now, we bail. */
> +		error = -EINVAL;
> +		goto out;
> +	}

This check is just horrible.  Please describe what kind of mappings
this doesn't work for and why and add a VM_ flag for those that
support memory binding.

>  	.private_list	= LIST_HEAD_INIT(swapper_space.private_list),
> +#ifdef CONFIG_NUMA
> +	.binding	= NULL,
> +#endif

You don't need to initialize this at all.