[bug ?] do_get_mempolicy()

[bug ?] do_get_mempolicy()

[John Blackwood]

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 ?

That is, maybe it was valid at some point, but subsequent changes
make this line of code no longer valid ?

Sorry if I'm out-to-lunch here...

Thanks very much for you time and considerations on this issue.

Re: [bug ?] do_get_mempolicy()

[David Rientjes]

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?

Signed-off-by: David Rientjes <rientjes@google.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) {

Re: [bug ?] do_get_mempolicy()

[David Rientjes]

On Thu, 3 Jul 2008, David Rientjes wrote:

> 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.
> 

John, please give 2.6.26-rc9 a try and let me know if there's any 
outstanding issues.

Thanks.

Re: [bug ?] do_get_mempolicy()

[Lee Schermerhorn]

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) {

Re: [bug ?] do_get_mempolicy()

[Lee Schermerhorn]

On Thu, 2008-07-03 at 16:44 -0400, John Blackwood wrote:
> Hi Lee,

John:  I'm just getting back from a long weekend and I'm wading through
a big e-mail back log.  I'll take a look at this when I get to the end
of my inbox.  I/m probably responsible for that line.  I recall thinking
that get_mempolicy() should return the policy that one would pass back
in to achieve the same effect.  But, I blew it.

> 
> 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 think that need to mask off the internal flags, and shift the
remaining ones up to the correct location.  I'll send a patch, if no one
beats me to it.

> 
> 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 ?
> 
> That is, maybe it was valid at some point, but subsequent changes
> make this line of code no longer valid ?
> 
> Sorry if I'm out-to-lunch here...

No, doesn't appear that way..

> 
> Thanks very much for you time and considerations on this issue.
> 

thanks for reporting it. 

Lee

Re: [bug ?] do_get_mempolicy()

[John Blackwood]

 > Subject: Re: [bug ?] do_get_mempolicy()
 > From: David Rientjes <rientjes@google.com>
 > Date: Mon, 7 Jul 2008 03:05:28 -0400
 > To: "Blackwood, John" <john.blackwood@ccur.com>
 > CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
Lee Schermerhorn <lee.schermerhorn@hp.com>, "Korty, Joe" 
<joe.korty@ccur.com>
 >
 > On Thu, 3 Jul 2008, David Rientjes wrote:
 >
 > > > 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.
 > > >
 >
 > John, please give 2.6.26-rc9 a try and let me know if there's any
 > outstanding issues.

Hi David,

get_mempolicy()/do_get_mempolicy() is working fine for me now.

Thanks for your help and quick response.