Make for_each_cpu_mask a bit smaller

Each use of the for_each_cpu_mask
constuct then becomes a few bytes smaller.

This patch
changes for_each_cpu_mask to use only one function: a newly introduced find_next_cpu.

Quoted 3 lines · Alexander

This patch
changes for_each_cpu_mask to use only one function: a newly introduced find_next_cpu.

I believe that it's for_each_cpu_mask which is newly introduced, not find_next_cpu ... just a typo, granted.

Any chance that you could make the same change to nodemask.h? Where practical, I like to keep cpumask.h and nodemask.h the same.

Sure. This patch introduces lib/nodemask.c, but I'm not quite sure if building it should depend on CONFIG_SMP or something else (NUMA?). When is MAX_NUMNODES 1?

I'ld be happy to take a stab at aligning the cpumask and nodemask code even more by uninlining some more functions and using stubs for the MAX_NUMNODES=1 case.

Quoted 3 lines · Alexander

Sure. This patch introduces lib/nodemask.c, but I'm not quite sure if building it should depend on CONFIG_SMP or something else (NUMA?). When is MAX_NUMNODES 1?

Well ... I'm pretty sure it made sense to depend on SMP, back when it was first added. However that might have changed. I recall vaguely that there has been discussion of this CONFIG_SMP dependency every year or so, but I don't have the time right now to dig through the archives and code to figure it out.

So ... offhand ... good questions, but I don't have answers.

Quoted 3 lines

I'ld be happy to take a stab at aligning the cpumask and nodemask code even more by uninlining some more functions and using stubs for the MAX_NUMNODES=1 case.

That could be good ... though could you co-ordinate with Mike Travis first, to minimize the risks of merge conflicts with what he's doing?

You kernel text space saving in the first patch seemed worth going ahead with even if it did conflict a little, and I liked the matching nodemask patch, just to keep things in sync. Other nodemask cleanup is a little lower priority in my book, so should make a modest effort to co-ordinate with more critical patches, to minimize conflict.

Signature · 3 lines

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

Quoted 12 lines · Paul Jackson

Quoted 3 lines · Alexander

Sure. This patch introduces lib/nodemask.c, but I'm not quite sure if building it should depend on CONFIG_SMP or something else (NUMA?). When is MAX_NUMNODES 1?

Well ... I'm pretty sure it made sense to depend on SMP, back when it was first added. However that might have changed. I recall vaguely that there has been discussion of this CONFIG_SMP dependency every year or so, but I don't have the time right now to dig through the archives and code to figure it out.

So ... offhand ... good questions, but I don't have answers.

Quoted 5 lines

Quoted 3 lines

I'ld be happy to take a stab at aligning the cpumask and nodemask code even more by uninlining some more functions and using stubs for the MAX_NUMNODES=1 case.

That could be good ... though could you co-ordinate with Mike Travis first, to minimize the risks of merge conflicts with what he's doing?

I believe the x86#testing tree includes Mike's work? The two patches in this thread apply fine to current x86#testing.

Quoted 6 lines

You kernel text space saving in the first patch seemed worth going ahead with even if it did conflict a little, and I liked the matching nodemask patch, just to keep things in sync. Other nodemask cleanup is a little lower priority in my book, so should make a modest effort to co-ordinate with more critical patches, to minimize conflict.

Thanks for your guidance.

Greetings,
Alexander

Quoted 4 lines

Signature · 3 lines

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+#define for_each_cpu_mask(cpu, mask)			\
+	for ((cpu) = ~((typeof(cpu))0);			\

There is no need for such a complicated expression, -1 will work for every (arithmetic) type.

The for_each_cpu_mask loop is used quite often in the kernel. It makes use of two functions: first_cpu and next_cpu. This patch changes for_each_cpu_mask to use only the latter. Because next_cpu finds the next eligible cpu _after_ the given one, the iteration variable has to be initialized to -1 and next_cpu has to be called with this value before the first iteration. An x86_64 defconfig kernel (from sched/latest) is about 2500 bytes smaller with this patch applied:

text data bss dec hex filename
6222517 917952 749932 7890401 7865e1 vmlinux.orig
6219922 917952 749932 7887806 785bbe vmlinux

The same size reduction is seen for defconfig+MAXSMP

text data bss dec hex filename
6241772 2563968 1492716 10298456 9d2458 vmlinux.orig
6239211 2563968 1492716 10295895 9d1a57 vmlinux

0", the answer is "no, find_next_bit returns the next set bit that's = the number passed in, which is why we need both the cpu++ and

include/linux/cpumask.h static inline void __cpus_fold(cpumask_t *dstp, const cpumask_t *origp, · line 359

Quoted 9 lines · Alexander van Heukelum

#if NR_CPUS > 1

include/linux/cpumask.h +4 -4

-#define for_each_cpu_mask(cpu, mask)		\
-	for ((cpu) = first_cpu(mask);		\
-		(cpu) < NR_CPUS;		\
-		(cpu) = next_cpu((cpu), (mask)))
+#define for_each_cpu_mask(cpu, mask)				\
+	for ((cpu) = 0;						\
+		(cpu) = find_next_cpu_mask((cpu), &(mask)),	\
+		(cpu) < NR_CPUS; (cpu)++)

For anyone else having similar cognitive dissonance while reading this thinking "But won't the first call to find_next_cpu_mask return a number

Quoted 2 lines

0", the answer is "no, find_next_bit returns the next set bit that's = the number passed in, which is why we need both the cpu++ and

find_next_cpu_mask".

lib/cpumask.c int __next_cpu(int n, const cpumask_t *srcp) · line 22

Quoted 5 lines

lib/cpumask.c +5 -0

+int find_next_cpu_mask(int n, const cpumask_t *srcp)
+{
+	return find_next_bit(srcp->bits, NR_CPUS, n);
+}
+EXPORT_SYMBOL(find_next_cpu_mask);

Maybe a better name for this function would help. I can't think of a good one right now though.

Quoted 13 lines · Matthew Wilcox

include/linux/cpumask.h static inline void __cpus_fold(cpumask_t *dstp, const cpumask_t *origp, · line 359

Quoted 9 lines · Alexander van Heukelum

#if NR_CPUS > 1

include/linux/cpumask.h +4 -4

-#define for_each_cpu_mask(cpu, mask)		\
-	for ((cpu) = first_cpu(mask);		\
-		(cpu) < NR_CPUS;		\
-		(cpu) = next_cpu((cpu), (mask)))
+#define for_each_cpu_mask(cpu, mask)				\
+	for ((cpu) = 0;						\
+		(cpu) = find_next_cpu_mask((cpu), &(mask)),	\
+		(cpu) < NR_CPUS; (cpu)++)

For anyone else having similar cognitive dissonance while reading this thinking "But won't the first call to find_next_cpu_mask return a number

Quoted 3 lines

Quoted 2 lines

0", the answer is "no, find_next_bit returns the next set bit that's = the number passed in, which is why we need both the cpu++ and

find_next_cpu_mask".

That's how it works, indeed.

Quoted 8 lines

lib/cpumask.c int __next_cpu(int n, const cpumask_t *srcp) · line 22

Quoted 5 lines

lib/cpumask.c +5 -0

+int find_next_cpu_mask(int n, const cpumask_t *srcp)
+{
+	return find_next_bit(srcp->bits, NR_CPUS, n);
+}
+EXPORT_SYMBOL(find_next_cpu_mask);

Maybe a better name for this function would help. I can't think of a good one right now though.

I can't think of a better name, and there is find_next_bit of which find_next_cpu_mask is just a wrapper. I think the name is good enough.

Quoted 31 lines · Alexander van Heukelum

Quoted 13 lines · Matthew Wilcox

include/linux/cpumask.h static inline void __cpus_fold(cpumask_t *dstp, const cpumask_t *origp, · line 359

Quoted 9 lines · Alexander van Heukelum

#if NR_CPUS > 1

include/linux/cpumask.h +4 -4

-#define for_each_cpu_mask(cpu, mask)		\
-	for ((cpu) = first_cpu(mask);		\
-		(cpu) < NR_CPUS;		\
-		(cpu) = next_cpu((cpu), (mask)))
+#define for_each_cpu_mask(cpu, mask)				\
+	for ((cpu) = 0;						\
+		(cpu) = find_next_cpu_mask((cpu), &(mask)),	\
+		(cpu) < NR_CPUS; (cpu)++)

For anyone else having similar cognitive dissonance while reading this thinking "But won't the first call to find_next_cpu_mask return a number

Quoted 3 lines

Quoted 2 lines

0", the answer is "no, find_next_bit returns the next set bit that's = the number passed in, which is why we need both the cpu++ and

find_next_cpu_mask".

That's how it works, indeed.

Quoted 8 lines

lib/cpumask.c int __next_cpu(int n, const cpumask_t *srcp) · line 22

Quoted 5 lines

lib/cpumask.c +5 -0

+int find_next_cpu_mask(int n, const cpumask_t *srcp)
+{
+	return find_next_bit(srcp->bits, NR_CPUS, n);
+}
+EXPORT_SYMBOL(find_next_cpu_mask);

Maybe a better name for this function would help. I can't think of a good one right now though.

Quoted 2 lines

I can't think of a better name, and there is find_next_bit of which find_next_cpu_mask is just a wrapper. I think the name is good enough.

How about doing it this way?

#define for_each_cpu_mask(cpu, mask)				\
	for ((cpu) = -1;					\
	     (cpu) < NR_CPUS;					\
	     (cpu) = find_next_cpu_mask((cpu), &(mask)))

int find_next_cpu_mask(int n, const cpumask_t *srcp)

{
	return find_next_bit(srcp->bits, NR_CPUS, ++n);
}

That actually behaves the way I'd expect a function called 'find_next_cpu_mask' to work. It also abuses the 'for' condtion less and might take a little less text space.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Quoted 10 lines

#define for_each_cpu_mask(cpu, mask)				\
	for ((cpu) = -1;					\
	     (cpu) < NR_CPUS;					\
	     (cpu) = find_next_cpu_mask((cpu), &(mask)))

int find_next_cpu_mask(int n, const cpumask_t *srcp)

{
	return find_next_bit(srcp->bits, NR_CPUS, ++n);
}

Quoted 3 lines

That actually behaves the way I'd expect a function called 'find_next_cpu_mask' to work. It also abuses the 'for' condtion less and might take a little less text space.

But it does not work.

I think of find_next_cpu_mask(cpu, mask) as: "find next cpu-index in mask, starting at index cpu". And similar with find_next_bit.

As for the text-space argument, I think you might be right. Just not on i386/x86_64 where initialising a register to -1 can be done in three bytes, initialising to 0 in two bytes and an increment in one byte :-).