lkml.org 
[lkml]   [2008]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [patch 2/4] mempolicy: support optional mode flags
    From
    Date
    On Mon, 2008-02-11 at 07:30 -0800, David Rientjes wrote: 
    > With the evolution of mempolicies, it is necessary to support mempolicy
    > mode flags that specify how the policy shall behave in certain
    > circumstances. The most immediate need for mode flag support is to
    > suppress remapping the nodemask of a policy at the time of rebind.
    >
    > With the small number of possible mempolicy modes (currently four) and
    > the large size of the struct mempolicy member that stores this mode
    > (unsigned short), it is possible to store both the mode and optional mode
    > flags in the same member.
    >
    > To do this, it is necessary to mask off the optional mode flag bits when
    > testing the mempolicy mode. A new constant, MPOL_FLAG_SHIFT, indicates
    > the shift necessary to find the flag bits within struct mempolicy's
    > policy member. With this, MPOL_MODE_MASK can be defined to mask off the
    > optional flags, yielding just the mode itself.
    >
    > A helper function:
    >
    > unsigned char mpol_mode(unsigned short mode)
    >
    > has been implemented to return only a policy's mode. Notice that the
    > return type is unsigned char since MPOL_FLAG_SHIFT is currently defined
    > at eight. mpol_flags(unsigned short mode) does the inverse.
    >
    > While this requires frequent use of mpol_mode() within the internal
    > mempolicy code, it is easy to distinguish between actuals that contain
    > only modes and those that contain the entire policy member (modes and
    > flags). If the formal uses enum mempolicy_mode, only the mode itself
    > may successfully be passed because of type checking.

    What is the benefit of pulling the flags and mode apart at the user
    interface, passing them as separate args to mpol_new(), do_* and
    mpol_shared_policy_init() and then stitching them back together in
    mpol_new()? Modes passed in via [sys_]{set_mempolicy|mbind}() and
    those stored in the the shmem_sb_info can already have the flags there,
    so this seems like extra work.

    I think this patch and the previous one [1/4] would be simplified if the
    formal parameters were left as an int [mabye, unsigned] and the flags
    were masked of when necessary in mpol_new() and elsewhere.

    [more comments below]

    >
    > Note that this does not break userspace code that relies on
    > get_mempolicy(&policy, ...) and either 'switch (policy)' statements or
    > 'if (policy == MPOL_INTERLEAVE)' statements. Such applications would
    > need to use optional mode flags when calling set_mempolicy() or mbind()
    > for these previously implemented statements to stop working. If an
    > application does start using optional mode flags, it will need to use
    > mpol_mode() in switch and conditional statements that only test mode.

    > Cc: Paul Jackson <pj@sgi.com>
    > Cc: Christoph Lameter <clameter@sgi.com>
    > Cc: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
    > Cc: Andi Kleen <ak@suse.de>
    > Signed-off-by: David Rientjes <rientjes@google.com>
    > ---
    > fs/hugetlbfs/inode.c | 2 +-
    > include/linux/mempolicy.h | 39 +++++++++++++++++++++--
    > mm/mempolicy.c | 77 ++++++++++++++++++++++++++------------------
    > mm/shmem.c | 15 ++++++--
    > 4 files changed, 93 insertions(+), 40 deletions(-)
    >
    > diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
    > --- a/fs/hugetlbfs/inode.c
    > +++ b/fs/hugetlbfs/inode.c
    > @@ -504,7 +504,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb, uid_t uid,
    > inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
    > INIT_LIST_HEAD(&inode->i_mapping->private_list);
    > info = HUGETLBFS_I(inode);
    > - mpol_shared_policy_init(&info->policy, MPOL_DEFAULT, NULL);
    > + mpol_shared_policy_init(&info->policy, MPOL_DEFAULT, 0, NULL);
    > switch (mode & S_IFMT) {
    > default:
    > init_special_inode(inode, mode, dev);
    > diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
    > --- a/include/linux/mempolicy.h
    > +++ b/include/linux/mempolicy.h
    > @@ -17,7 +17,18 @@ enum mempolicy_mode {
    > MPOL_MAX, /* always last member of mempolicy_mode */
    > };
    >
    > -/* Flags for get_mem_policy */
    > +/*
    > + * The lower MPOL_FLAG_SHIFT bits of the policy mode represent the MPOL_*
    > + * constants defined in enum mempolicy_mode. The upper bits represent
    > + * optional set_mempolicy() MPOL_F_* mode flags.
    > + */
    > +#define MPOL_FLAG_SHIFT (8)
    > +#define MPOL_MODE_MASK ((1 << MPOL_FLAG_SHIFT) - 1)
    > +
    > +/* Flags for set_mempolicy */
    > +#define MPOL_MODE_FLAGS (0) /* legal set_mempolicy() MPOL_* mode flags */
    > +
    > +/* Flags for get_mempolicy */
    > #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 */
    > #define MPOL_F_MEMS_ALLOWED (1<<2) /* return allowed memories */
    > @@ -115,6 +126,16 @@ static inline int mpol_equal(struct mempolicy *a, struct mempolicy *b)
    >
    > #define mpol_set_vma_default(vma) ((vma)->vm_policy = NULL)
    >
    > +static inline unsigned char mpol_mode(unsigned short mode)
    > +{
    > + return mode & MPOL_MODE_MASK;
    > +}
    > +
    > +static inline unsigned short mpol_flags(unsigned short mode)
    > +{
    > + return mode & ~MPOL_MODE_MASK;
    > +}
    > +
    > /*
    > * Tree of shared policies for a shared memory region.
    > * Maintain the policies in a pseudo mm that contains vmas. The vmas
    > @@ -135,7 +156,8 @@ struct shared_policy {
    > };
    >
    > void mpol_shared_policy_init(struct shared_policy *info,
    > - enum mempolicy_mode policy, nodemask_t *nodes);
    > + enum mempolicy_mode policy, unsigned short flags,
    > + nodemask_t *nodes);
    > int mpol_set_shared_policy(struct shared_policy *info,
    > struct vm_area_struct *vma,
    > struct mempolicy *new);
    > @@ -176,6 +198,16 @@ static inline int mpol_equal(struct mempolicy *a, struct mempolicy *b)
    > }
    > #define vma_mpol_equal(a,b) 1
    >
    > +static inline unsigned char mpol_mode(unsigned short mode)
    > +{
    > + return 0;
    > +}
    > +
    > +static inline unsigned short mpol_flags(unsigned short mode)
    > +{
    > + return 0;
    > +}
    > +
    > #define mpol_set_vma_default(vma) do {} while(0)
    >
    > static inline void mpol_free(struct mempolicy *p)
    > @@ -201,7 +233,8 @@ static inline int mpol_set_shared_policy(struct shared_policy *info,
    > }
    >
    > static inline void mpol_shared_policy_init(struct shared_policy *info,
    > - enum mempolicy_type policy, nodemask_t *nodes)
    > + enum mempolicy_type policy, unsigned short flags,
    > + nodemask_t *nodes)
    > {
    > }
    >
    > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
    > --- a/mm/mempolicy.c
    > +++ b/mm/mempolicy.c
    > @@ -203,18 +203,20 @@ static struct zonelist *bind_zonelist(nodemask_t *nodes)
    > }
    >
    > /* Create a new policy */
    > -static struct mempolicy *mpol_new(enum mempolicy_mode mode, nodemask_t *nodes)
    > +static struct mempolicy *mpol_new(enum mempolicy_mode mode,
    > + unsigned short flags, nodemask_t *nodes)
    > {
    > struct mempolicy *policy;
    >
    > - pr_debug("setting mode %d nodes[0] %lx\n",
    > - mode, nodes ? nodes_addr(*nodes)[0] : -1);
    > + pr_debug("setting mode %d flags %d nodes[0] %lx\n",
    > + mode, flags, nodes ? nodes_addr(*nodes)[0] : -1);
    >
    > if (mode == MPOL_DEFAULT)
    > return NULL;
    > policy = kmem_cache_alloc(policy_cache, GFP_KERNEL);
    > if (!policy)
    > return ERR_PTR(-ENOMEM);
    > + flags &= MPOL_MODE_FLAGS;
    > atomic_set(&policy->refcnt, 1);
    > switch (mode) {
    > case MPOL_INTERLEAVE:
    > @@ -240,7 +242,7 @@ static struct mempolicy *mpol_new(enum mempolicy_mode mode, nodemask_t *nodes)
    > default:
    > BUG();
    > }
    > - policy->policy = mode;
    > + policy->policy = mode | flags;
    > policy->cpuset_mems_allowed = cpuset_mems_allowed(current);
    > return policy;
    > }
    > @@ -483,19 +485,20 @@ static void mpol_set_task_struct_flag(void)
    > }
    >
    > /* Set the process memory policy */
    > -static long do_set_mempolicy(enum mempolicy_mode mode, nodemask_t *nodes)
    > +static long do_set_mempolicy(enum mempolicy_mode mode, unsigned short flags,
    > + nodemask_t *nodes)
    > {
    > struct mempolicy *new;
    >
    > if (mpol_check_policy(mode, nodes))
    > return -EINVAL;
    > - new = mpol_new(mode, nodes);
    > + new = mpol_new(mode, flags, nodes);
    > if (IS_ERR(new))
    > return PTR_ERR(new);
    > mpol_free(current->mempolicy);
    > current->mempolicy = new;
    > mpol_set_task_struct_flag();
    > - if (new && new->policy == MPOL_INTERLEAVE)
    > + if (new && mpol_mode(new->policy) == MPOL_INTERLEAVE)
    > current->il_next = first_node(new->v.nodes);
    > return 0;
    > }
    > @@ -506,7 +509,7 @@ static void get_zonemask(struct mempolicy *p, nodemask_t *nodes)
    > int i;
    >
    > nodes_clear(*nodes);
    > - switch (p->policy) {
    > + switch (mpol_mode(p->policy)) {
    > case MPOL_BIND:
    > for (i = 0; p->v.zonelist->zones[i]; i++)
    > node_set(zone_to_nid(p->v.zonelist->zones[i]),
    > @@ -588,7 +591,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
    > goto out;
    > *policy = err;
    > } else if (pol == current->mempolicy &&
    > - pol->policy == MPOL_INTERLEAVE) {
    > + mpol_mode(pol->policy) == MPOL_INTERLEAVE) {
    > *policy = current->il_next;
    > } else {
    > err = -EINVAL;
    > @@ -785,8 +788,8 @@ static struct page *new_vma_page(struct page *page, unsigned long private, int *
    > #endif
    >
    > static long do_mbind(unsigned long start, unsigned long len,
    > - enum mempolicy_mode mode, nodemask_t *nmask,
    > - unsigned long flags)
    > + enum mempolicy_mode mode, unsigned short mode_flags,
    > + nodemask_t *nmask, unsigned long flags)
    > {
    > struct vm_area_struct *vma;
    > struct mm_struct *mm = current->mm;
    > @@ -818,7 +821,7 @@ static long do_mbind(unsigned long start, unsigned long len,
    > if (mpol_check_policy(mode, nmask))
    > return -EINVAL;
    >
    > - new = mpol_new(mode, nmask);
    > + new = mpol_new(mode, mode_flags, nmask);
    > if (IS_ERR(new))
    > return PTR_ERR(new);
    >
    > @@ -829,8 +832,9 @@ static long do_mbind(unsigned long start, unsigned long len,
    > if (!new)
    > flags |= MPOL_MF_DISCONTIG_OK;
    >
    > - pr_debug("mbind %lx-%lx mode:%d nodes:%lx\n",start,start+len,
    > - mode, nmask ? nodes_addr(*nmask)[0] : -1);
    > + pr_debug("mbind %lx-%lx mode:%d flags:%d nodes:%lx\n",
    > + start, start + len, mode, mode_flags,
    > + nmask ? nodes_addr(*nmask)[0] : -1);
    >
    > down_write(&mm->mmap_sem);
    > vma = check_range(mm, start, end, nmask,
    > @@ -929,13 +933,16 @@ asmlinkage long sys_mbind(unsigned long start, unsigned long len,
    > {
    > nodemask_t nodes;
    > int err;
    > + unsigned short mode_flags;
    >
    > + mode_flags = mpol_flags(mode);
    >
    >
    I suggest restricting the flags to the defined flags here. This gives
    us the ability to defined new flags w/o breaking [changing behavior of]
    applications that accidently pass undefined flags. An error return
    forced the user to fix the application [assuming they check error
    returns].

    > + mode = mpol_mode(mode);
    > if (mode >= MPOL_MAX)
    > return -EINVAL;
    > err = get_nodes(&nodes, nmask, maxnode);
    > if (err)
    > return err;
    > - return do_mbind(start, len, mode, &nodes, flags);
    > + return do_mbind(start, len, mode, mode_flags, &nodes, flags);
    > }
    >
    > /* Set the process memory policy */
    > @@ -944,13 +951,16 @@ asmlinkage long sys_set_mempolicy(int mode, unsigned long __user *nmask,
    > {
    > int err;
    > nodemask_t nodes;
    > + unsigned short flags;
    >
    > + flags = mpol_flags(mode);

    Suggest similar arg checking here.

    > + mode = mpol_mode(mode);
    > if (mode < 0 || mode >= MPOL_MAX)

    Now that we're extracting an unsigned char from the mode via
    mpol_mode(), I think we can drop the 'mode < 0 ||'. This would also be
    the case if we didn't pull the flags and mode apart and just used:

    if (mpol_mode(mode) < MPOL_MAX)
    ...

    > return -EINVAL;
    > err = get_nodes(&nodes, nmask, maxnode);
    > if (err)
    > return err;
    > - return do_set_mempolicy(mode, &nodes);
    > + return do_set_mempolicy(mode, flags, &nodes);
    > }
    >
    > asmlinkage long sys_migrate_pages(pid_t pid, unsigned long maxnode,
    > @@ -1152,7 +1162,7 @@ static struct mempolicy * get_vma_policy(struct task_struct *task,
    > pol = vma->vm_ops->get_policy(vma, addr);
    > shared_pol = 1; /* if pol non-NULL, add ref below */
    > } else if (vma->vm_policy &&
    > - vma->vm_policy->policy != MPOL_DEFAULT)
    > + mpol_mode(vma->vm_policy->policy) != MPOL_DEFAULT)
    > pol = vma->vm_policy;
    > }
    > if (!pol)
    > @@ -1167,7 +1177,7 @@ static struct zonelist *zonelist_policy(gfp_t gfp, struct mempolicy *policy)
    > {
    > int nd;
    >
    > - switch (policy->policy) {
    > + switch (mpol_mode(policy->policy)) {
    > case MPOL_PREFERRED:
    > nd = policy->v.preferred_node;
    > if (nd < 0)
    > @@ -1211,7 +1221,8 @@ static unsigned interleave_nodes(struct mempolicy *policy)
    > */
    > unsigned slab_node(struct mempolicy *policy)
    > {
    > - enum mempolicy_mode pol = policy ? policy->policy : MPOL_DEFAULT;
    > + enum mempolicy_mode pol = policy ? mpol_mode(policy->policy) :
    > + MPOL_DEFAULT;
    >
    > switch (pol) {
    > case MPOL_INTERLEAVE:
    > @@ -1297,7 +1308,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
    > struct zonelist *zl;
    >
    > *mpol = NULL; /* probably no unref needed */
    > - if (pol->policy == MPOL_INTERLEAVE) {
    > + if (mpol_mode(pol->policy) == MPOL_INTERLEAVE) {
    > unsigned nid;
    >
    > nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
    > @@ -1307,7 +1318,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
    >
    > zl = zonelist_policy(GFP_HIGHUSER, pol);
    > if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
    > - if (pol->policy != MPOL_BIND)
    > + if (mpol_mode(pol->policy) != MPOL_BIND)
    > __mpol_free(pol); /* finished with pol */
    > else
    > *mpol = pol; /* unref needed after allocation */
    > @@ -1361,7 +1372,7 @@ alloc_page_vma(gfp_t gfp, struct vm_area_struct *vma, unsigned long addr)
    >
    > cpuset_update_task_memory_state();
    >
    > - if (unlikely(pol->policy == MPOL_INTERLEAVE)) {
    > + if (unlikely(mpol_mode(pol->policy) == MPOL_INTERLEAVE)) {
    > unsigned nid;
    >
    > nid = interleave_nid(pol, vma, addr, PAGE_SHIFT);
    > @@ -1409,7 +1420,7 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order)
    > cpuset_update_task_memory_state();
    > if (!pol || in_interrupt() || (gfp & __GFP_THISNODE))
    > pol = &default_policy;
    > - if (pol->policy == MPOL_INTERLEAVE)
    > + if (mpol_mode(pol->policy) == MPOL_INTERLEAVE)
    > return alloc_page_interleave(gfp, order, interleave_nodes(pol));
    > return __alloc_pages(gfp, order, zonelist_policy(gfp, pol));
    > }
    > @@ -1436,7 +1447,7 @@ struct mempolicy *__mpol_copy(struct mempolicy *old)
    > }
    > *new = *old;
    > atomic_set(&new->refcnt, 1);
    > - if (new->policy == MPOL_BIND) {
    > + if (mpol_mode(new->policy) == MPOL_BIND) {
    > int sz = ksize(old->v.zonelist);
    > new->v.zonelist = kmemdup(old->v.zonelist, sz, GFP_KERNEL);
    > if (!new->v.zonelist) {
    > @@ -1454,7 +1465,7 @@ int __mpol_equal(struct mempolicy *a, struct mempolicy *b)
    > return 0;
    > if (a->policy != b->policy)
    > return 0;
    > - switch (a->policy) {
    > + switch (mpol_mode(a->policy)) {
    > case MPOL_DEFAULT:
    > return 1;
    > case MPOL_INTERLEAVE:
    > @@ -1479,7 +1490,7 @@ void __mpol_free(struct mempolicy *p)
    > {
    > if (!atomic_dec_and_test(&p->refcnt))
    > return;
    > - if (p->policy == MPOL_BIND)
    > + if (mpol_mode(p->policy) == MPOL_BIND)
    > kfree(p->v.zonelist);
    > p->policy = MPOL_DEFAULT;
    > kmem_cache_free(policy_cache, p);
    > @@ -1640,7 +1651,8 @@ restart:
    > }
    >
    > void mpol_shared_policy_init(struct shared_policy *info,
    > - enum mempolicy_mode policy, nodemask_t *policy_nodes)
    > + enum mempolicy_mode policy, unsigned short flags,
    > + nodemask_t *policy_nodes)
    > {
    > info->root = RB_ROOT;
    > spin_lock_init(&info->lock);
    > @@ -1649,7 +1661,7 @@ void mpol_shared_policy_init(struct shared_policy *info,
    > struct mempolicy *newpol;
    >
    > /* Falls back to MPOL_DEFAULT on any error */
    > - newpol = mpol_new(policy, policy_nodes);
    > + newpol = mpol_new(policy, flags, policy_nodes);
    > if (!IS_ERR(newpol)) {
    > /* Create pseudo-vma that contains just the policy */
    > struct vm_area_struct pvma;
    > @@ -1745,14 +1757,14 @@ void __init numa_policy_init(void)
    > if (unlikely(nodes_empty(interleave_nodes)))
    > node_set(prefer, interleave_nodes);
    >
    > - if (do_set_mempolicy(MPOL_INTERLEAVE, &interleave_nodes))
    > + if (do_set_mempolicy(MPOL_INTERLEAVE, 0, &interleave_nodes))
    > printk("numa_policy_init: interleaving failed\n");
    > }
    >
    > /* Reset policy of current process to default */
    > void numa_default_policy(void)
    > {
    > - do_set_mempolicy(MPOL_DEFAULT, NULL);
    > + do_set_mempolicy(MPOL_DEFAULT, 0, NULL);
    > }
    >
    > /* Migrate a policy to a different set of nodes */
    > @@ -1768,7 +1780,7 @@ static void mpol_rebind_policy(struct mempolicy *pol,
    > if (nodes_equal(*mpolmask, *newmask))
    > return;
    >
    > - switch (pol->policy) {
    > + switch (mpol_mode(pol->policy)) {
    > case MPOL_DEFAULT:
    > break;
    > case MPOL_INTERLEAVE:
    > @@ -1858,7 +1870,8 @@ static inline int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
    > char *p = buffer;
    > int l;
    > nodemask_t nodes;
    > - enum mempolicy_mode mode = pol ? pol->policy : MPOL_DEFAULT;
    > + enum mempolicy_mode mode = pol ? mpol_mode(pol->policy) :
    > + MPOL_DEFAULT;
    >
    > switch (mode) {
    > case MPOL_DEFAULT:
    > diff --git a/mm/shmem.c b/mm/shmem.c
    > --- a/mm/shmem.c
    > +++ b/mm/shmem.c
    > @@ -1086,6 +1086,7 @@ static int shmem_parse_mpol(char *value, unsigned short *policy,
    > nodemask_t *policy_nodes)
    > {
    > char *nodelist = strchr(value, ':');
    > + char *flags = strchr(value, '=');
    > int err = 1;
    >
    > if (nodelist) {
    > @@ -1096,6 +1097,8 @@ static int shmem_parse_mpol(char *value, unsigned short *policy,
    > if (!nodes_subset(*policy_nodes, node_states[N_HIGH_MEMORY]))
    > goto out;
    > }
    > + if (flags)
    > + *flags++ = '\0';
    > if (!strcmp(value, "default")) {
    > *policy = MPOL_DEFAULT;
    > /* Don't allow a nodelist */
    > @@ -1125,6 +1128,8 @@ static int shmem_parse_mpol(char *value, unsigned short *policy,
    > *policy_nodes = node_states[N_HIGH_MEMORY];
    > err = 0;
    > }
    > + if (flags) {
    > + }
    > out:
    > /* Restore string for error message */
    > if (nodelist)
    > @@ -1154,7 +1159,7 @@ static void shmem_show_mpol(struct seq_file *seq, enum mempolicy_mode policy,
    >
    > seq_printf(seq, ",mpol=%s", policy_string);
    >
    > - if (policy != MPOL_INTERLEAVE ||
    > + if (mpol_mode(policy) != MPOL_INTERLEAVE ||
    > !nodes_equal(policy_nodes, node_states[N_HIGH_MEMORY])) {
    > char buffer[64];
    > int len;
    > @@ -1577,8 +1582,10 @@ shmem_get_inode(struct super_block *sb, int mode, dev_t dev)
    > case S_IFREG:
    > inode->i_op = &shmem_inode_operations;
    > inode->i_fop = &shmem_file_operations;
    > - mpol_shared_policy_init(&info->policy, sbinfo->policy,
    > - &sbinfo->policy_nodes);
    > + mpol_shared_policy_init(&info->policy,
    > + mpol_mode(sbinfo->policy),
    > + mpol_flags(sbinfo->policy),
    > + &sbinfo->policy_nodes);
    > break;
    > case S_IFDIR:
    > inc_nlink(inode);
    > @@ -1592,7 +1599,7 @@ shmem_get_inode(struct super_block *sb, int mode, dev_t dev)
    > * Must not load anything in the rbtree,
    > * mpol_free_shared_policy will not be called.
    > */
    > - mpol_shared_policy_init(&info->policy, MPOL_DEFAULT,
    > + mpol_shared_policy_init(&info->policy, MPOL_DEFAULT, 0,
    > NULL);
    > break;
    > }



    \
     
     \ /
      Last update: 2008-02-13 01:17    [W:0.065 / U:62.220 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site