[PATCH][2.6] first/next_cpu returns values > NR_CPUS

[PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Zwane Mwaikambo]

The following caused some fireworks whilst merging i386 cpu hotplug.

any_online_cpu(0x2) returns 32 on i386 if we're forced to continue past
the only set bit due to the additional find_first_bit in the
find_next_bit i386 implementation. Not wanting to change current
behaviour in the bitops primitives and since the NR_CPUS thing is a
cpumask issue, i've opted to fix next_cpu() and first_cpu() instead.

Signed-off-by: Zwane Mwaikambo <zwane@fsmlabs.com>

Index: linux-2.6.8-rc2-mm1-lch/include/linux/cpumask.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.8-rc2-mm1/include/linux/cpumask.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 cpumask.h
--- linux-2.6.8-rc2-mm1-lch/include/linux/cpumask.h	28 Jul 2004 14:35:22 -0000	1.1.1.1
+++ linux-2.6.8-rc2-mm1-lch/include/linux/cpumask.h	31 Jul 2004 17:57:12 -0000
@@ -207,13 +207,19 @@ static inline void __cpus_shift_left(cpu
 #define first_cpu(src) __first_cpu(&(src), NR_CPUS)
 static inline int __first_cpu(const cpumask_t *srcp, int nbits)
 {
-	return find_first_bit(srcp->bits, nbits);
+	int cpu = find_first_bit(srcp->bits, nbits);
+	if (cpu > NR_CPUS)
+		cpu = NR_CPUS;
+	return cpu;
 }

 #define next_cpu(n, src) __next_cpu((n), &(src), NR_CPUS)
 static inline int __next_cpu(int n, const cpumask_t *srcp, int nbits)
 {
-	return find_next_bit(srcp->bits, nbits, n+1);
+	int cpu = find_next_bit(srcp->bits, nbits, n+1);
+	if (cpu > NR_CPUS)
+		cpu = NR_CPUS;
+	return cpu;
 }

 #define cpumask_of_cpu(cpu)						\

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

On Sat, Jul 31, 2004 at 04:52:18PM -0400, Zwane Mwaikambo wrote:
> The following caused some fireworks whilst merging i386 cpu hotplug.
> any_online_cpu(0x2) returns 32 on i386 if we're forced to continue past
> the only set bit due to the additional find_first_bit in the
> find_next_bit i386 implementation. Not wanting to change current
> behaviour in the bitops primitives and since the NR_CPUS thing is a
> cpumask issue, i've opted to fix next_cpu() and first_cpu() instead.
> Signed-off-by: Zwane Mwaikambo <zwane@fsmlabs.com>

This might save a couple of lines of code.


Index: hotplug-2.6.8-rc2/include/linux/cpumask.h
===================================================================
--- hotplug-2.6.8-rc2.orig/include/linux/cpumask.h	2004-07-29 04:44:59.000000000 -0700
+++ hotplug-2.6.8-rc2/include/linux/cpumask.h	2004-07-31 13:49:15.647650104 -0700
@@ -207,13 +207,13 @@
 #define first_cpu(src) __first_cpu(&(src), NR_CPUS)
 static inline int __first_cpu(const cpumask_t *srcp, int nbits)
 {
-	return find_first_bit(srcp->bits, nbits);
+	return min(NR_CPUS, find_first_bit(srcp->bits, nbits));
 }
 
 #define next_cpu(n, src) __next_cpu((n), &(src), NR_CPUS)
 static inline int __next_cpu(int n, const cpumask_t *srcp, int nbits)
 {
-	return find_next_bit(srcp->bits, nbits, n+1);
+	return min(NR_CPUS, find_next_bit(srcp->bits, nbits, n+1));
 }
 
 #define cpumask_of_cpu(cpu)						\

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Zwane Mwaikambo]

On Sat, 31 Jul 2004, William Lee Irwin III wrote:

> On Sat, Jul 31, 2004 at 04:52:18PM -0400, Zwane Mwaikambo wrote:
> > The following caused some fireworks whilst merging i386 cpu hotplug.
> > any_online_cpu(0x2) returns 32 on i386 if we're forced to continue past
> > the only set bit due to the additional find_first_bit in the
> > find_next_bit i386 implementation. Not wanting to change current
> > behaviour in the bitops primitives and since the NR_CPUS thing is a
> > cpumask issue, i've opted to fix next_cpu() and first_cpu() instead.
> > Signed-off-by: Zwane Mwaikambo <zwane@fsmlabs.com>
>
> This might save a couple of lines of code.

Nice, i'd prefer that.

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Paul Jackson]

> Zwane, William proposed:
> +	return min(NR_CPUS, find_first_bit(srcp->bits, nbits));

Could you check the kernel text size, for some NUMA config, before and
after adding these min() calls in first_cpu() and next_cpu().  These two
macros are critical to the for_*_cpu() macros.

When I tried it just now on an ia64 sn2_defconfig, NR_CPUS == 512, it
increased each for_*_cpu() loop about 28 bytes of text, for a kernel
text size increase of 1352 bytes (this is on a private kernel I have,
your results will vary).

> The following caused some fireworks whilst merging i386 cpu hotplug.
> any_online_cpu(0x2) returns 32 on i386 if we're forced to continue past
> the only set bit due to the additional find_first_bit in the
> find_next_bit i386 implementation.

Could you explain this a bit more?  What value of NR_CPUS were you
using -- if NR_CPUS == 32, then I'd _expect_ any_online_cpu() to return
32 if none of the bits provided it were online.  The way you phrase
this, it sure seems that you are hinting at a bug in the i386 implementation
of find_next_bit().  But I can't quite make out the code, nor what you're
saying, so I'm still confused.

A specific example might help -- NR_CPUS is this, what's online is that,
called "any_online_cpu()" with so-and-so, expected thus as a return, got
something else instead.

I'd hate to see a bug in i386 find_next_bit() left to stand, at the
expense of increasing sometimes fairly interesting code loops by 28
bytes of text each.  If that's what's happening here ... 

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

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Zwane Mwaikambo]

On Sat, 31 Jul 2004, Paul Jackson wrote:

> When I tried it just now on an ia64 sn2_defconfig, NR_CPUS == 512, it
> increased each for_*_cpu() loop about 28 bytes of text, for a kernel
> text size increase of 1352 bytes (this is on a private kernel I have,
> your results will vary).

I haven't checked the text size increase, but will do.

> Could you explain this a bit more?  What value of NR_CPUS were you
> using -- if NR_CPUS == 32, then I'd _expect_ any_online_cpu() to return
> 32 if none of the bits provided it were online.  The way you phrase
> this, it sure seems that you are hinting at a bug in the i386 implementation
> of find_next_bit().  But I can't quite make out the code, nor what you're
> saying, so I'm still confused.
>
> A specific example might help -- NR_CPUS is this, what's online is that,
> called "any_online_cpu()" with so-and-so, expected thus as a return, got
> something else instead.
>
> I'd hate to see a bug in i386 find_next_bit() left to stand, at the
> expense of increasing sometimes fairly interesting code loops by 28
> bytes of text each.  If that's what's happening here ...

NR_CPUS was 3, the test case may as well be passing first_cpu or next_cpu
a value of 0 for the map. The "bug" in the i386 find_next_bit really
looks like a feature if you look at the code.

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Paul Jackson]

Zwane wrote:
> NR_CPUS was 3, the test case may as well be passing first_cpu or next_cpu
> a value of 0 for the map.

So, if NR_CPUS is 3, and you pass an empty map to any_online_cpu()
on an i386, you get back not 3, as expected, but 32 ??

And this is because find_next_bit(0, 3, 0), for example, returns 32,
correct ??

Well ... no ... I must not be guessing your example right yet.  Because
in the above example, first_cpu(0) will (should ?) return with NR_CPUS,
and the for_each_cpu_mask() inside any_online_cpu() will end there.

Could you give me the rest of the numbers in a specific example?

Please ...

Hmmm ... perhaps you're saying you're passing a non-zero map to
any_online_cpu(), but that the bits set in what you pass aren't
online, which would end up calling find_next_bit().  Yeah - that
must be it.

And indeed the i386 find_next_bit() code can't possibly be honoring a
size < 32, because it doesn't even consider the size value until it has
finished the first word without finding a set bit in the last 32-offset
bits.


> The "bug" in the i386 find_next_bit really
> looks like a feature if you look at the code.

What code, what feature, what bug ...  Please be specific.

Are you referring to the apparent (if I am reading the code for
find_next_bit in arch/i386/lib/bitops.c correctly) behaviour
of this find_next_bit() that it's really only coded for size
some multiple of 32?

If so, then wouldn't whether this is a bug or a feature depend on what
the other arch's do, and what (if there is anyway to know) was intended,
and on what other code is expecting, and on what in the long term
will be the least surprising behaviour, resulting in fewest bugs?

That is, are bitmaps only really supposed to work for integral
multiples of unsigned longs, or are they supposed to honor fractional
long sizes?

A quick look at some other arch's find_next_bit() leads me to suspect
that they _do_ handle fractional long sizes, unlike i386.  And it
was certainly my expectation that they should do so (returning,
for example, 3, not 32, on an empty mask if called with size == 3).
These routines _do_ take a size that is a bit count, and I don't
recall seeing any big hairy warnings that size better be a multiple
of BITS_PER_LONG.

If all this is so, then i386 find_next_bit() is wrong.  Possibly other
some arch's too -- it's not code that I can read easily.

If not, then in addition to fixing cpumask.h, we'd better also consider
whether we need to fix:

    drivers/atm/lanai.c:
		vci = find_next_bit(lp, NUM_VCI, vci + 1);
    include/linux/nodemask.h:
		return find_next_bit(srcp->bits, nbits, n+1);
    kernel/sched.c:
		idx = find_next_bit(array->bitmap, MAX_PRIO, idx);
    lib/idr.c:
		m = find_next_bit(&bm, IDR_SIZE, n);
    mm/mempolicy.c:
		next = find_next_bit(policy->v.nodes, MAX_NUMNODES, 1+nid);
    mm/mempolicy.c:
		nid = find_next_bit(pol->v.nodes, MAX_NUMNODES, nid+1);

Adding Matthew Dobson to this thread - since his new nodemask.h
gets hit with this alot harder than cpumask.h, because it is more
common to have a nodemask that isn't a multiple of a long in size.

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

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Zwane Mwaikambo]

On Sun, 1 Aug 2004, Paul Jackson wrote:

> Zwane wrote:
> > NR_CPUS was 3, the test case may as well be passing first_cpu or next_cpu
> > a value of 0 for the map.
>
> So, if NR_CPUS is 3, and you pass an empty map to any_online_cpu()
> on an i386, you get back not 3, as expected, but 32 ??
>
> And this is because find_next_bit(0, 3, 0), for example, returns 32,
> correct ??
>
> Well ... no ... I must not be guessing your example right yet.  Because
> in the above example, first_cpu(0) will (should ?) return with NR_CPUS,
> and the for_each_cpu_mask() inside any_online_cpu() will end there.
>
> Could you give me the rest of the numbers in a specific example?
>
> Please ...

Well you could have just tried it, all you needed to do was paste the code
for find_first_bit into a file and make a simple test case. Then plugging
in a value of 0x0 for the map. so in pseudocode for NR_CPUS = 3;

first_cpu(0) = next_cpu(0) = 32.

> Hmmm ... perhaps you're saying you're passing a non-zero map to
> any_online_cpu(), but that the bits set in what you pass aren't
> online, which would end up calling find_next_bit().  Yeah - that
> must be it.

Yes i did specify that in the original email.

> And indeed the i386 find_next_bit() code can't possibly be honoring a
> size < 32, because it doesn't even consider the size value until it has
> finished the first word without finding a set bit in the last 32-offset
> bits.
>
>
> > The "bug" in the i386 find_next_bit really
> > looks like a feature if you look at the code.
>
> What code, what feature, what bug ...  Please be specific.

The find_next_bit code, the additional find_first_bit on the failure to
find a bit exit path and i'm referring to this bug, the one i'm reporting
now.

> If all this is so, then i386 find_next_bit() is wrong.  Possibly other
> some arch's too -- it's not code that I can read easily.
>
> If not, then in addition to fixing cpumask.h, we'd better also consider
> whether we need to fix:

I was short on time to do a complete audit, the change to cpumask.h is a
necessity anyway.

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

On Sun, Aug 01, 2004 at 03:22:56AM -0400, Zwane Mwaikambo wrote:
> NR_CPUS was 3, the test case may as well be passing first_cpu or next_cpu
> a value of 0 for the map. The "bug" in the i386 find_next_bit really
> looks like a feature if you look at the code.

Hmm. I'm actually somewhat puzzled by this also. Shouldn't things only
check for inequalities between the results of these and NR_CPUS? i.e.
things like:
	if (any_online_cpu(cpus) >= NR_CPUS)
and
	for (cpu = first_cpu(cpus); cpu < NR_CPUS; cpu = next_cpu(cpus))
etc.?

Maybe the few callers that are sensitive to the precise return value
should use min_t(int, NR_CPUS, ...) instead of all callers taking the
branch on behalf of those few.


-- wli

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Paul Jackson]

William wrote:
> Maybe the few callers that are sensitive to the precise return value
> should use min_t(int, NR_CPUS, ...) instead of all callers taking the
> branch on behalf of those few.

Either way - we need consistency.  Either find_next_bit(.., size, ...)
returns exactly size if no more bits, or all its callers tolerate any
return >= size.

I probably prefer the former, because I expect slightly tighter kernel
code now (see my previous post on text size), and fewer bugs in the
future (more clients of find_next_bit will be coded than new
implementations of it), if we go this way.  William's comments suggest
to me he prefers the later.

Either (or both) seems better than what we have.

William - can you read the find_next_bit() implementations in some other
arch's well enough to understand if they are anal about returning
exactly 'size', or content to return something >= size, when they run
out of bits?  That code was a bit denser than I could deal with easily.

If a strong majority of the arch's find_next_bit() are anal, or on the
other hand, are not, then I'd suggest we follow their lead.

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

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

On Sun, Aug 01, 2004 at 06:05:29AM -0700, Paul Jackson wrote:
> Either way - we need consistency.  Either find_next_bit(.., size, ...)
> returns exactly size if no more bits, or all its callers tolerate any
> return >= size.
> I probably prefer the former, because I expect slightly tighter kernel
> code now (see my previous post on text size), and fewer bugs in the
> future (more clients of find_next_bit will be coded than new
> implementations of it), if we go this way.  William's comments suggest
> to me he prefers the later.
> Either (or both) seems better than what we have.
> William - can you read the find_next_bit() implementations in some other
> arch's well enough to understand if they are anal about returning
> exactly 'size', or content to return something >= size, when they run
> out of bits?  That code was a bit denser than I could deal with easily.
> If a strong majority of the arch's find_next_bit() are anal, or on the
> other hand, are not, then I'd suggest we follow their lead.

A strong majority return BITS_PER_LONG-aligned results in this case.


-- wli

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Paul Jackson]

> A strong majority return BITS_PER_LONG-aligned results in this case.

You mean, in Zwane's example, we'd see a return from any_online_cpu() of
32 or 64, not 3 (his NR_CPUS), and not just on i386, but on a majority
of arch's.  That's what you're saying, right?

Ok ... that favors your preference, teaching the users of find_next_bit
to be more tolerant.

Darn.  Your min(nbits, ...) patch looks good, but more is needed.

And could you make it:

+	return min(nbits, find_next_bit(srcp->bits, nbits, n+1));

rather than:

+	return min(NR_CPUS, find_next_bit(srcp->bits, nbits, n+1));

for consistency of presentation?  All the cpu and node mask macros of
this form (#define wrapped static inline) use the inline's parameter
names in the body of the inline, not what the define passed as those
params, including another 'nbits' in this very line of code.

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

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

On Sun, Aug 01, 2004 at 06:36:32AM -0700, Paul Jackson wrote:
> You mean, in Zwane's example, we'd see a return from any_online_cpu() of
> 32 or 64, not 3 (his NR_CPUS), and not just on i386, but on a majority
> of arch's.  That's what you're saying, right?
> Ok ... that favors your preference, teaching the users of find_next_bit
> to be more tolerant.
> Darn.  Your min(nbits, ...) patch looks good, but more is needed.
> And could you make it:
> +	return min(nbits, find_next_bit(srcp->bits, nbits, n+1));
> rather than:
> +	return min(NR_CPUS, find_next_bit(srcp->bits, nbits, n+1));
> for consistency of presentation?  All the cpu and node mask macros of
> this form (#define wrapped static inline) use the inline's parameter
> names in the body of the inline, not what the define passed as those
> params, including another 'nbits' in this very line of code.

No problem.


Index: hotplug-2.6.8-rc2/include/linux/cpumask.h
===================================================================
--- hotplug-2.6.8-rc2.orig/include/linux/cpumask.h	2004-07-29 04:44:59.000000000 -0700
+++ hotplug-2.6.8-rc2/include/linux/cpumask.h	2004-08-01 06:32:31.615472016 -0700
@@ -207,13 +207,13 @@
 #define first_cpu(src) __first_cpu(&(src), NR_CPUS)
 static inline int __first_cpu(const cpumask_t *srcp, int nbits)
 {
-	return find_first_bit(srcp->bits, nbits);
+	return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
 }
 
 #define next_cpu(n, src) __next_cpu((n), &(src), NR_CPUS)
 static inline int __next_cpu(int n, const cpumask_t *srcp, int nbits)
 {
-	return find_next_bit(srcp->bits, nbits, n+1);
+	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
 }
 
 #define cpumask_of_cpu(cpu)						\

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Paul Jackson]

> No problem.

Thanks.

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

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Matthew Dobson]

On Sun, 2004-08-01 at 06:41, William Lee Irwin III wrote:
> No problem.
> 
> 
> Index: hotplug-2.6.8-rc2/include/linux/cpumask.h
> ===================================================================
> --- hotplug-2.6.8-rc2.orig/include/linux/cpumask.h	2004-07-29 04:44:59.000000000 -0700
> +++ hotplug-2.6.8-rc2/include/linux/cpumask.h	2004-08-01 06:32:31.615472016 -0700
> @@ -207,13 +207,13 @@
>  #define first_cpu(src) __first_cpu(&(src), NR_CPUS)
>  static inline int __first_cpu(const cpumask_t *srcp, int nbits)
>  {
> -	return find_first_bit(srcp->bits, nbits);
> +	return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
>  }
>  
>  #define next_cpu(n, src) __next_cpu((n), &(src), NR_CPUS)
>  static inline int __next_cpu(int n, const cpumask_t *srcp, int nbits)
>  {
> -	return find_next_bit(srcp->bits, nbits, n+1);
> +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
>  }
>  
>  #define cpumask_of_cpu(cpu)						\

I'll add the nodemask.h fix here, too.

-Matt

--- linux-2.6.8-rc2-mm1/include/linux/nodemask.h	2004-07-28 10:50:51.000000000 -0700
+++ linux-2.6.8-rc2-mm1/include/linux/nodemask.h.fixed	2004-08-02 14:56:12.000000000 -0700
@@ -216,13 +216,13 @@ static inline void __nodes_shift_left(no
 #define first_node(src) __first_node(&(src), MAX_NUMNODES)
 static inline int __first_node(const nodemask_t *srcp, int nbits)
 {
-	return find_first_bit(srcp->bits, nbits);
+	return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
 }
 
 #define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
 static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
 {
-	return find_next_bit(srcp->bits, nbits, n+1);
+	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
 }
 
 #define nodemask_of_node(node)						\

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[OGAWA Hirofumi]

Matthew Dobson <colpatch@us.ibm.com> writes:

> >  #define first_cpu(src) __first_cpu(&(src), NR_CPUS)
> >  static inline int __first_cpu(const cpumask_t *srcp, int nbits)
> >  {
> > -	return find_first_bit(srcp->bits, nbits);
> > +	return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
> >  }
> >  
> >  #define next_cpu(n, src) __next_cpu((n), &(src), NR_CPUS)
> >  static inline int __next_cpu(int n, const cpumask_t *srcp, int nbits)
> >  {
> > -	return find_next_bit(srcp->bits, nbits, n+1);
> > +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
> >  }

>  #define first_node(src) __first_node(&(src), MAX_NUMNODES)
>  static inline int __first_node(const nodemask_t *srcp, int nbits)
>  {
> -	return find_first_bit(srcp->bits, nbits);
> +	return min_t(int, nbits, find_first_bit(srcp->bits, nbits));
>  }
>  
>  #define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
>  static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
>  {
> -	return find_next_bit(srcp->bits, nbits, n+1);
> +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
>  }

Shouldn't these use simply min()?  I worry min_t() may hide the real bug...
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

Matthew Dobson <colpatch@us.ibm.com> writes:
>>  #define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
>>  static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
>>  {
>> -	return find_next_bit(srcp->bits, nbits, n+1);
>> +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
>>  }

On Fri, Aug 06, 2004 at 01:50:23AM +0900, OGAWA Hirofumi wrote:
> Shouldn't these use simply min()?  I worry min_t() may hide the real bug...

min_t() is harmless here. Some 64-bit architectures use long as the
return type of find_next_bit(), but its precision isn't needed, so
min_t() merely prevents up a useless warning.


-- wli

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Andrew Morton]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
>  >  #define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
>  >  static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
>  >  {
>  > -	return find_next_bit(srcp->bits, nbits, n+1);
>  > +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
>  >  }
> 
>  Shouldn't these use simply min()?  I worry min_t() may hide the real bug...

The problem is that on some architectures, find_next_bit() returns an
unsigned long, on others it returns an int and I think some even return a
long.

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Roman Zippel]

Hi,

On Thu, 5 Aug 2004, Andrew Morton wrote:

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
> >
> >  >  #define next_node(n, src) __next_node((n), &(src), MAX_NUMNODES)
> >  >  static inline int __next_node(int n, const nodemask_t *srcp, int nbits)
> >  >  {
> >  > -	return find_next_bit(srcp->bits, nbits, n+1);
> >  > +	return min_t(int, nbits, find_next_bit(srcp->bits, nbits, n+1));
> >  >  }
> > 
> >  Shouldn't these use simply min()?  I worry min_t() may hide the real bug...
> 
> The problem is that on some architectures, find_next_bit() returns an
> unsigned long, on others it returns an int and I think some even return a
> long.

Personally I prefer to cast only one argument:

+	return min(nbits, (int)find_next_bit(srcp->bits, nbits, n+1));

If the nbits type should change, it's not as easy to miss.

bye, Roman

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[Zwane Mwaikambo]

On Sun, 1 Aug 2004, William Lee Irwin III wrote:

> On Sun, Aug 01, 2004 at 03:22:56AM -0400, Zwane Mwaikambo wrote:
> > NR_CPUS was 3, the test case may as well be passing first_cpu or next_cpu
> > a value of 0 for the map. The "bug" in the i386 find_next_bit really
> > looks like a feature if you look at the code.
>
> Hmm. I'm actually somewhat puzzled by this also. Shouldn't things only
> check for inequalities between the results of these and NR_CPUS? i.e.
> things like:
> 	if (any_online_cpu(cpus) >= NR_CPUS)
> and
> 	for (cpu = first_cpu(cpus); cpu < NR_CPUS; cpu = next_cpu(cpus))
> etc.?
>
> Maybe the few callers that are sensitive to the precise return value
> should use min_t(int, NR_CPUS, ...) instead of all callers taking the
> branch on behalf of those few.

The problem is that next_cpu(0) will assign the incorrect value of '32'
to variable cpu so when you exit the loop, you'll have some silly number.
This really should be covered in the API, especially since that's what the
API states is the expected behaviour.

Re: [PATCH][2.6] first/next_cpu returns values > NR_CPUS

[William Lee Irwin III]

On Sun, 1 Aug 2004, William Lee Irwin III wrote:
>> Maybe the few callers that are sensitive to the precise return value
>> should use min_t(int, NR_CPUS, ...) instead of all callers taking the
>> branch on behalf of those few.

On Sun, Aug 01, 2004 at 10:54:03AM -0400, Zwane Mwaikambo wrote:
> The problem is that next_cpu(0) will assign the incorrect value of '32'
> to variable cpu so when you exit the loop, you'll have some silly number.
> This really should be covered in the API, especially since that's what the
> API states is the expected behaviour.

I see. AFAICT this can also be dealt with by the caller and is far less
common than total insensitivity to which value >= NR_CPUS is involved.


-- wli