Re: [bug ?] do_get_mempolicy()

[Lee Schermerhorn <lee.schermerhorn@hp.com>]

On Thu, 2008-07-03 at 14:44 -0700, David Rientjes wrote:
> On Thu, 3 Jul 2008, John Blackwood wrote:
> 
> > Hi Lee,
> > 
> > I'm having unexpected results with get_mempolicy(2) in 2.6.26, and
> > I am hoping that you can either agree with me, or maybe comment on my
> > misconceptions.
> > 
> > When I have a task with no special task mempolicy (the default mempolicy),
> > when I call get_mempolicy(2), it returns a policy value of 2 (MPOL_BIND)
> > with a NULL nodemask.
> > 
> > I believe that this is because of the code in do_get_mempolicy() that does:
> > 
> >  *policy |= pol->flags;
> > 
> > in the else case when flags do not contain MPOL_F_NODE.
> > 
> > I guess I don't understand why we are ORing in the pol->flags into the
> > *policy value.  For example, when this is for the default_policy, the
> > MPOL_F_LOCAL flag (which has a value of 2) gets stuffed into the *policy
> > location, and a get_mempolicy(2) caller sees this as the MPOL_BIND
> > mempolicy.
> > 
> > Maybe the "*policy |= pol->flags;" line should be removed ?
> > 
> 
> You're right, the flags member of struct mempolicy has subsequently 
> changed to carry "internal" flags that are not supposed to be exposed to 
> userspace via the get_mempolicy() API.
> 
> The following patch probably fixes it.
> 
> Lee?

David:  Just getting back from long weekend.  This looks good.  I was
thinking that, in addition to masking off the internal flags, we need to
shift the flags into the upper half word.  However, external flags are
already shifted to the correct position, so just the mask is needed.

> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Acked-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

> ---
>  mm/mempolicy.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -729,7 +729,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>  	} else {
>  		*policy = pol == &default_policy ? MPOL_DEFAULT :
>  						pol->mode;
> -		*policy |= pol->flags;
> +		*policy |= (pol->flags & MPOL_MODE_FLAGS);
>  	}
>  
>  	if (vma) {