From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Alexander van Heukelum" Subject: Re: [PATCH] Make for_each_cpu_mask a bit smaller Date: Mon, 12 May 2008 13:04:54 +0200 Message-ID: <1210590294.13136.1252653345@webmail.messagingengine.com> References: <20080511135039.GA3286@mailshack.com> <20080511152440.GX19219@parisc-linux.org> <1210522779.16917.1252546109@webmail.messagingengine.com> <20080511220111.GC19219@parisc-linux.org> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20080511220111.GC19219@parisc-linux.org> Sender: linux-kernel-owner@vger.kernel.org To: Matthew Wilcox Cc: Alexander van Heukelum , Andrew Morton , Paul Jackson , Ingo Molnar , Thomas Gleixner , linux-arch , LKML List-Id: linux-arch.vger.kernel.org On Sun, 11 May 2008 16:01:12 -0600, "Matthew Wilcox" said: > On Sun, May 11, 2008 at 06:19:39PM +0200, Alexander van Heukelum wrote: > > > > On Sun, 11 May 2008 09:24:40 -0600, "Matthew Wilcox" > > said: > > > On Sun, May 11, 2008 at 03:50:39PM +0200, Alexander van Heukelum wrote: > > > > #if NR_CPUS > 1 > > > > -#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 > > > > 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. > > > > > > +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. > > 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. But it does not work. It introduces a stray cpu=-1 iteration if cpu happens to be (replaced by) a signed variable. It skips the entire loop if cpu happens to be unsigned. I don't think that using 'for' in a less conventional way is bad if it is hidden in a macro, as long as the name of the macro makes the intention sufficiently clear. 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 :-). Greetings, Alexander > -- > 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." -- Alexander van Heukelum heukelum@fastmail.fm -- http://www.fastmail.fm - mmm... Fastmail... From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1.smtp.messagingengine.com ([66.111.4.25]:57553 "EHLO out1.smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751166AbYELLEz (ORCPT ); Mon, 12 May 2008 07:04:55 -0400 Message-ID: <1210590294.13136.1252653345@webmail.messagingengine.com> From: "Alexander van Heukelum" Content-Disposition: inline Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="ISO-8859-1" MIME-Version: 1.0 References: <20080511135039.GA3286@mailshack.com> <20080511152440.GX19219@parisc-linux.org> <1210522779.16917.1252546109@webmail.messagingengine.com> <20080511220111.GC19219@parisc-linux.org> Subject: Re: [PATCH] Make for_each_cpu_mask a bit smaller In-Reply-To: <20080511220111.GC19219@parisc-linux.org> Date: Mon, 12 May 2008 13:04:54 +0200 Sender: linux-arch-owner@vger.kernel.org List-ID: To: Matthew Wilcox Cc: Alexander van Heukelum , Andrew Morton , Paul Jackson , Ingo Molnar , Thomas Gleixner , linux-arch , LKML Message-ID: <20080512110454.xBJ8XkG7bnUwEwHzxyrFBun5mu_Z-51AxTviMow3T-Q@z> On Sun, 11 May 2008 16:01:12 -0600, "Matthew Wilcox" said: > On Sun, May 11, 2008 at 06:19:39PM +0200, Alexander van Heukelum wrote: > > > > On Sun, 11 May 2008 09:24:40 -0600, "Matthew Wilcox" > > said: > > > On Sun, May 11, 2008 at 03:50:39PM +0200, Alexander van Heukelum wrote: > > > > #if NR_CPUS > 1 > > > > -#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 > > > > 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. > > > > > > +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. > > 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. But it does not work. It introduces a stray cpu=-1 iteration if cpu happens to be (replaced by) a signed variable. It skips the entire loop if cpu happens to be unsigned. I don't think that using 'for' in a less conventional way is bad if it is hidden in a macro, as long as the name of the macro makes the intention sufficiently clear. 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 :-). Greetings, Alexander > -- > 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." -- Alexander van Heukelum heukelum@fastmail.fm -- http://www.fastmail.fm - mmm... Fastmail...