more numa maxnode confusions

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 <[email protected]> 1.650.933.1373

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.

Correct.

Yes.

Yes.

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

You're welcome.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Revert what?

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

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

It does it, and nothing else is needed.

-Andi

Andrew asked:

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 <[email protected]> 1.650.933.1373

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
===================================================================
@@ -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 <[email protected]> 1.650.933.1373