more numa maxnode confusions

more numa maxnode confusions

[Paul Jackson]

I believe that a merge error on my part is contributing further to the
confusions over the meaning of the 'maxnode' parameter to the numa
mbind/set_mempolicy/get_mempolicy calls.  The question is whether
maxnode should be passed in as the number of bits, or one more than the
number of bits (64 or 65 if MAX_NUMNODES is 64, for example).

I will call these two choices the N64 and N65 choices.

As best as I can reconstruct the events, the following has happened over
the last month:

 1) In the beginning, Andi designed this numa interface to be N65.

 2) About Aug 9, Brent Casavant sent in a patch changing the set (not get)
    side calls, sys_mbind and sys_set_mempolicy, to N64.  This patch
    removed the following line from the implementation of get_nodes() in
    mm/mempolicy.c:

	--maxnode;

 3) Due presumably to a merge confusion on my part, my subsequent
    cpuset patch (the original, largest one now dated Sept 2)
    put this line back in, making the interface N65 again.

So currently, a quick reading of the code (could easily be wrong) tells
me that the compat interfaces and the 'get' side (get_mempolicy) are
always N65, but that the native 'set' side is N64 without my cpuset
patch (but with Brent's patch), but N65 with my cpuset patch, which adds
back in the "--maxnode" line to get_nodes().

Currently, by my reading, Linus' bk tree has the mixed N64/N65
interfaces, since it has Brent's patch, but not my cpuset patch.
Andrew's *-mm tree has the pure N65 interface, due to my cpuset patch
reversing Brent's patch.

My guess is that Andi wants this all N65, and that he didn't agree to
Brent's patch.  If Andi understands different, that's fine -- I'm not
trying to reopen that battle.

Andi:

 0) Are my above statements anywhere close to correct?

 1) Should the "--maxnode" be re-inserted in get_nodes()?

 2) Should it be re-inserted by a separate patch from you,
    rather than as a hidden side affect of my cpuset patch?
    I will gladly remove that line from my cpuset patch, in
    favor of a one-liner from you that re-inserts that line.

Brent:

 1) Would you agree to having your partial N64 patch reversed,
    if my efforts to channel Andi's thoughts have succeeded,
    and he wants the "--maxnode" in the code?

Andrew:

 1) Once it's settled on where we (Andi, mostly) wants to go,
    how do you want the patch(es)?  It seems silly for me to
    send in a patch that undoes the re-insertion, just so Andi
    can send in another patch that redoes it.  I will gladly do
    pretty much anything you and Andi agree to here, but you may
    have to spell it out to me in small, simple words ;).

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: more numa maxnode confusions

[Andi Kleen]

On Sun, Sep 12, 2004 at 08:02:53PM -0700, Paul Jackson wrote:
>  2) About Aug 9, Brent Casavant sent in a patch changing the set (not get)
>     side calls, sys_mbind and sys_set_mempolicy, to N64.  This patch
>     removed the following line from the implementation of get_nodes() in
>     mm/mempolicy.c:
> 
> 	--maxnode;

Ah, I wasn't aware that this patch got merged into mainline. 
That was a bad thing, because it broke the ABI used by libnuma
subtly.

Please whoever merged it revert it.

> Currently, by my reading, Linus' bk tree has the mixed N64/N65
> interfaces, since it has Brent's patch, but not my cpuset patch.
> Andrew's *-mm tree has the pure N65 interface, due to my cpuset patch
> reversing Brent's patch.
> 
> My guess is that Andi wants this all N65, and that he didn't agree to
> Brent's patch.  If Andi understands different, that's fine -- I'm not
> trying to reopen that battle.

Correct.


> 
> Andi:
> 
>  0) Are my above statements anywhere close to correct?

Yes.

> 
>  1) Should the "--maxnode" be re-inserted in get_nodes()?

Yes.

> 
>  2) Should it be re-inserted by a separate patch from you,
>     rather than as a hidden side affect of my cpuset patch?
>     I will gladly remove that line from my cpuset patch, in
>     favor of a one-liner from you that re-inserts that line.

Yes. I appended a patch. Linus or Andrew, please apply it.

Thanks for catching this.

-Andi


----------------------------------------------------------------

Fix ABI in set_mempolicy() that got broken by an earlier change.

Add a check for very big input values and prevent excessive
looping in the kernel.


diff -u linux-2.6.9rc1-bk19/mm/mempolicy.c-o linux-2.6.9rc1-bk19/mm/mempolicy.c
--- linux-2.6.9rc1-bk19/mm/mempolicy.c-o	2004-09-13 08:51:46.000000000 +0200
+++ linux-2.6.9rc1-bk19/mm/mempolicy.c	2004-09-13 08:53:58.000000000 +0200
@@ -132,6 +132,7 @@
 	unsigned long nlongs;
 	unsigned long endmask;
 
+	--maxnode;
 	bitmap_zero(nodes, MAX_NUMNODES);
 	if (maxnode == 0 || !nmask)
 		return 0;
@@ -145,6 +146,8 @@
 	/* When the user specified more nodes than supported just check
 	   if the non supported part is all zero. */
 	if (nlongs > BITS_TO_LONGS(MAX_NUMNODES)) {
+		if (nlongs > PAGE_SIZE/sizeof(long))
+			return -EINVAL;
 		for (k = BITS_TO_LONGS(MAX_NUMNODES); k < nlongs; k++) {
 			unsigned long t;
 			if (get_user(t,  nmask + k))

Re: more numa maxnode confusions

[Paul Jackson]

Andi writes:
> Thanks for catching this.

You're welcome.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: more numa maxnode confusions

[Andrew Morton]

Andi Kleen <ak@suse.de> wrote:
>
> On Sun, Sep 12, 2004 at 08:02:53PM -0700, Paul Jackson wrote:
> >  2) About Aug 9, Brent Casavant sent in a patch changing the set (not get)
> >     side calls, sys_mbind and sys_set_mempolicy, to N64.  This patch
> >     removed the following line from the implementation of get_nodes() in
> >     mm/mempolicy.c:
> > 
> > 	--maxnode;
> 
> Ah, I wasn't aware that this patch got merged into mainline. 
> That was a bad thing, because it broke the ABI used by libnuma
> subtly.
> 
> Please whoever merged it revert it.

Revert what?

> ...
>
> Yes. I appended a patch. Linus or Andrew, please apply it.
> 

Does this patch perform the above reversion, or is something additional
needed?

Re: more numa maxnode confusions

[Andi Kleen]

On Mon, Sep 13, 2004 at 12:15:48AM -0700, Andrew Morton wrote:
> Andi Kleen <ak@suse.de> wrote:
> >
> > On Sun, Sep 12, 2004 at 08:02:53PM -0700, Paul Jackson wrote:
> > >  2) About Aug 9, Brent Casavant sent in a patch changing the set (not get)
> > >     side calls, sys_mbind and sys_set_mempolicy, to N64.  This patch
> > >     removed the following line from the implementation of get_nodes() in
> > >     mm/mempolicy.c:
> > > 
> > > 	--maxnode;
> > 
> > Ah, I wasn't aware that this patch got merged into mainline. 
> > That was a bad thing, because it broke the ABI used by libnuma
> > subtly.
> > 
> > Please whoever merged it revert it.
> 
> Revert what?


http://linux.bkbits.net:8080/linux-2.6/cset@412b8943seoOgs2sAUXJeG1qynkYHQ?nav=index.html|src/|src/mm|related/mm/mempolicy.c

> 
> > ...
> >
> > Yes. I appended a patch. Linus or Andrew, please apply it.
> > 
> 
> Does this patch perform the above reversion, or is something additional
> needed?

It does it, and nothing else is needed.

-Andi

Re: more numa maxnode confusions

[Paul Jackson]

Andrew asked:
> Revert what?

The immediate change Andi wants reverted only matters at present in
Linus' bk tree.  My main cpuset patch in your *-mm tree already does the
reversion in *-mm (unfortunately - collision details follow ...).

So I presume that Linus' will apply Andi's reversion patch of earlier this
evening to his bk tree.

But then when you pull in Linus's latest bk changes into your linus.patch,
this will collide with my main cpuset patch.

Both patches will be trying to add back in the same line:

	--maxnode;

to get_nodes() in mm/mempolicy.c.

My guess is that now that you and Linus know about this, you two can
handle the collision by hand - both new lines of code agree on what's to
be done: add the above line back in.

But if there is some other permutation of patches that I can send that
would be smoother, let me know.

The one alternative I can think of that would allow everyone to put this
back on autopilot and forget the details, would be to _remove_ the
following segment of my cpusets-big-numa-cpu-and-memory-placement.patch:

@@ -133,6 +134,7 @@ static int get_nodes(unsigned long *node
 	unsigned long nlongs;
 	unsigned long endmask;
 
+	--maxnode;
 	bitmap_zero(nodes, MAX_NUMNODES);
 	if (maxnode == 0 || !nmask)
 		return 0;

so that Andi's latest reversion path applied cleanly when it came back
at you from Linus' bk tree.  But I understand that usually you like to
layer new patches, not replace or edit existing ones.

Go ahead and remove the above segment, if that seems best to you.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

[PATCH] undo more numa maxnode confusions

[Paul Jackson]

Argh ... undo Andi's patch that undid Brent's patch that undid Andi's API ...

It was Linus' bk tree, not Andrew's *-mm tree, that needed the
"--maxnode" put back in.  Andrew's tree already has the "--maxnode"
that it should have in the "get_nodes()" routine.

If you apply Andi's patch to Andrew's *-mm tree, then a second nearby
similar looking routine "get_zonemask()" gets the second "--maxnode",
and she doesn't build anymore, failing with:

  mm/mempolicy.c: In function `get_zonemask':
  mm/mempolicy.c:419: error: `maxnode' undeclared (first use in this function)

This applies on top of 2.6.9-rc1-mm5.

Index: 2.6.9-rc1-mm5/mm/mempolicy.c
===================================================================
--- 2.6.9-rc1-mm5.orig/mm/mempolicy.c	2004-09-13 04:23:57.000000000 -0700
+++ 2.6.9-rc1-mm5/mm/mempolicy.c	2004-09-13 05:00:15.000000000 -0700
@@ -416,7 +416,6 @@ static void get_zonemask(struct mempolic
 {
 	int i;
 
-	--maxnode;
 	bitmap_zero(nodes, MAX_NUMNODES);
 	switch (p->policy) {
 	case MPOL_BIND:


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373