Re: [PATCH 1/2] mm: page cache mempolicy for page cache allocation

[Andi Kleen]

> +/* policy selection bits are passed from user shifted left by this amount */
> +#define REQUEST_POLICY_SHIFT	16
> +#define REQUEST_POLICY_PAGE     POLICY_PAGE << REQUEST_POLICY_SHIFT
> +#define REQUEST_POLICY_PAGECACHE POLICY_PAGECACHE << REQUEST_POLICY_SHIFT
> +#define REQUEST_POLICY_MASK     (0x3FFF) << REQUEST_POLICY_SHIFT

Please put brackets around the macros. Putting them around numbers
is not needed though @)


> +#define REQUEST_POLICY_DEFAULT  (0x8000) << REQUEST_POLICY_SHIFT
> +
>  /* Flags for get_mem_policy */
>  #define MPOL_F_NODE	(1<<0)	/* return next IL mode instead of node mask */
>  #define MPOL_F_ADDR	(1<<1)	/* look up vma using address */
> @@ -31,6 +54,8 @@
>  #include <linux/slab.h>
>  #include <linux/rbtree.h>
>  #include <asm/semaphore.h>
> +#include <linux/sched.h>
> +#include <asm/current.h>

Why is that needed? I don't see any users for this.  Please avoid this 
if possible, we already have too much include dependency spagetti.


> --- linux-2.6.9-rc2-mm1.orig/include/linux/sched.h	2004-09-16 12:54:41.000000000 -0700
> +++ linux-2.6.9-rc2-mm1/include/linux/sched.h	2004-09-22 08:48:45.000000000 -0700
> @@ -31,6 +31,8 @@
>  #include <linux/pid.h>
>  #include <linux/percpu.h>
>  
> +#include <linux/mempolicy.h>

I also don't see why this should be needed. Please remove.

> +	for(i=0;i<NR_MEM_POLICIES;i++)

There should be more spaces here (similar in other loops) 


>  	int err, pval;
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma = NULL;
> -	struct mempolicy *pol = current->mempolicy;
> +	struct mempolicy *pol = NULL;
> +	int policy_type, request_policy_default;
>  
>  	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
>  		return -EINVAL;
>  	if (nmask != NULL && maxnode < numnodes)
>  		return -EINVAL;
> +
> +	policy_type = (flags & REQUEST_POLICY_MASK) > REQUEST_POLICY_SHIFT;
> +	request_policy_default = (flags & REQUEST_POLICY_DEFAULT);

Why is that not an MPOL_F_* ? 

>  /* Slow path of a mempolicy copy */
>  struct mempolicy *__mpol_copy(struct mempolicy *old)
> @@ -1093,8 +1146,8 @@ void __init numa_policy_init(void)
>  	/* Set interleaving policy for system init. This way not all
>  	   the data structures allocated at system boot end up in node zero. */
>  
> -	if (sys_set_mempolicy(MPOL_INTERLEAVE, nodes_addr(node_online_map),
> -							MAX_NUMNODES) < 0)
> +	if (sys_set_mempolicy(REQUEST_POLICY_PAGE | MPOL_INTERLEAVE, 
> +		nodes_addr(node_online_map), MAX_NUMNODES) < 0)

That's definitely wrong, the boot time interleaving is not for the page
cache but for all allocations. There are not even page cache allocations
that early.

Overall when I look at all the complications you add for the per process
page policy which doesn't even have a demonstrated need I'm not sure
it is really worth it.

>  		printk("numa_policy_init: interleaving failed\n");
>  }
>  
> @@ -1102,5 +1155,5 @@ void __init numa_policy_init(void)
>   * Assumes fs == KERNEL_DS */
>  void numa_default_policy(void)
>  {
> -	sys_set_mempolicy(MPOL_DEFAULT, NULL, 0);
> +	sys_set_mempolicy(REQUEST_POLICY_PAGE | MPOL_DEFAULT, NULL, 0);

Same.

-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/