[PATCH] consolidate all within() implementations

[PATCH] consolidate all within() implementations

[Peter Oberparleiter]

From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>

This patch consolidates a number of different implementations of the
within() function which checks whether an address is within a specified
address range. Apart from parameter typing, existing implementations can
be classified in two categories which differ in the way the range is
specified:

  1) by start and end address
  2) by start and size

These categories are covered by the within() macro (case 1) and the
within_len() macro (case 2). Both macros can be used with any pointer
or pointer-equivalent type as parameter.

Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
---
 arch/x86/mm/pageattr.c |    7 +------
 include/linux/kernel.h |   24 ++++++++++++++++++++++++
 kernel/lockdep.c       |   10 +++-------
 kernel/module.c        |   25 ++++++++++---------------
 4 files changed, 38 insertions(+), 28 deletions(-)

Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
--- linux-2.6.26-rc3.orig/include/linux/kernel.h
+++ linux-2.6.26-rc3/include/linux/kernel.h
@@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * within - check whether address is within a start-and-end address range
+ * @val: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+#define within(val, start, end) ({			\
+	unsigned long __val = (unsigned long) (val);	\
+	unsigned long __start = (unsigned long) (start);	\
+	unsigned long __end = (unsigned long) (end);	\
+	(__val >= __start) && (__val < __end); })
+
+/**
+ * within_len - check whether address is within a start-and-length address range
+ * @val: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+#define within_len(val, start, len) ({				\
+	unsigned long __val = (unsigned long) (val);		\
+	unsigned long __start = (unsigned long) (start);	\
+	unsigned long __end = __start + (unsigned long) (len);	\
+	(__val >= __start) && (__val < __end); })
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/lockdep.c
+++ linux-2.6.26-rc3/kernel/lockdep.c
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;
@@ -2956,9 +2952,9 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (within_len(class->key, start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (within_len(class->name, start, size))
 				zap_class(class);
 		}
 	}
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/module.c
+++ linux-2.6.26-rc3/kernel/module.c
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found
@@ -2287,7 +2282,7 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (within_len(addr, mod->module_init, mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;
@@ -2335,8 +2330,8 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size)
+		    || within_len(addr, mod->module_core, mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);
@@ -2358,8 +2353,8 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size) ||
+		    within_len(addr, mod->module_core, mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);
@@ -2382,8 +2377,8 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_init, mod->init_size) ||
+		    within_len(addr, mod->module_core, mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);
@@ -2579,7 +2574,7 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (within_len(addr, mod->module_core, mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}
@@ -2597,8 +2592,8 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (within_len(addr, mod->module_init, mod->init_text_size)
+		    || within_len(addr, mod->module_core, mod->core_text_size))
 			return mod;
 	return NULL;
 }
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
--- linux-2.6.26-rc3.orig/arch/x86/mm/pageattr.c
+++ linux-2.6.26-rc3/arch/x86/mm/pageattr.c
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>
@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */

Re: [PATCH] consolidate all within() implementations

[Harvey Harrison]

On Mon, 2008-05-19 at 10:45 +0200, Peter Oberparleiter wrote:
> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> 
> This patch consolidates a number of different implementations of the
> within() function which checks whether an address is within a specified
> address range. Apart from parameter typing, existing implementations can
> be classified in two categories which differ in the way the range is
> specified:
> 
>   1) by start and end address
>   2) by start and size
> 
> These categories are covered by the within() macro (case 1) and the
> within_len() macro (case 2). Both macros can be used with any pointer
> or pointer-equivalent type as parameter.

Would it be that hard to just make them static inlines taking unsigned
longs?

> 
> Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> ---
>  
>  /**
> + * within - check whether address is within a start-and-end address range
> + * @val: address

@addr perhaps

> + * @start: start address (included in range)
> + * @end: end address (excluded from range)
> + */
> +#define within(val, start, end) ({			\

How about:
static inline int addr_within(unsigned long addr, unsigned long start,
			      unsigned long end)

> +	unsigned long __val = (unsigned long) (val);	\
> +	unsigned long __start = (unsigned long) (start);	\
> +	unsigned long __end = (unsigned long) (end);	\
> +	(__val >= __start) && (__val < __end); })
> +
> +/**
> + * within_len - check whether address is within a start-and-length address range
> + * @val: address

@addr
> + * @start: start of range
> + * @len: number of bytes in range
> + */
> +#define within_len(val, start, len) ({				\
static inline int addr_within_len(unsigned long addr, unsigned long start,
				  unsigned long len)

Just a thought.

Harvey

Re: [PATCH] consolidate all within() implementations

[Peter Oberparleiter]

Harvey Harrison wrote:
> On Mon, 2008-05-19 at 10:45 +0200, Peter Oberparleiter wrote:
>> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
>> 
>> This patch consolidates a number of different implementations of the
>> within() function which checks whether an address is within a specified
>> address range.
> 
> Would it be that hard to just make them static inlines taking unsigned
> longs?

I was hoping to get by without the spray of unsigned long casts that
entails the enforcement of this specific parameter type, seeing that
each implementation had it's own combination of longs and void *.

On the other hand, an inline function would of course be the cleaner
approach, so if the code owners agree, here goes take #2:

--

From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>

This patch consolidates a number of different implementations of the
within() function which checks whether an address is within a specified
address range. Apart from parameter typing, existing implementations can
be classified in two categories which differ in the way the range is
specified:

  1) by start and end address
  2) by start and size

Case 1) is covered by addr_within() while 2) is covered by
addr_within_len().

Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
---
 arch/x86/mm/pageattr.c |   20 ++++++++------------
 include/linux/kernel.h |   24 ++++++++++++++++++++++++
 kernel/lockdep.c       |   12 +++++-------
 kernel/module.c        |   35 ++++++++++++++++++++---------------
 4 files changed, 57 insertions(+), 34 deletions(-)

Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
--- linux-2.6.26-rc3.orig/include/linux/kernel.h
+++ linux-2.6.26-rc3/include/linux/kernel.h
@@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+static inline int addr_within(unsigned long addr, unsigned long start,
+			      unsigned long end)
+{
+	return (addr >= start) && (addr < end);
+}
+
+/**
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(unsigned long addr, unsigned long start,
+				  unsigned long len)
+{
+	return (addr >= start) && (addr < (start + len));
+}
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/lockdep.c
+++ linux-2.6.26-rc3/kernel/lockdep.c
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;
@@ -2956,9 +2952,11 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (addr_within_len((unsigned long) class->key,
+					    (unsigned long) start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (addr_within_len((unsigned long) class->name,
+						 (unsigned long) start, size))
 				zap_class(class);
 		}
 	}
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/module.c
+++ linux-2.6.26-rc3/kernel/module.c
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found
@@ -2287,7 +2282,8 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (addr_within_len(addr, (unsigned long) mod->module_init,
+			    mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;
@@ -2335,8 +2331,10 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size)
+		    || addr_within_len(addr, (unsigned long) mod->module_core,
+				       mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);
@@ -2358,8 +2356,10 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);
@@ -2382,8 +2382,10 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);
@@ -2579,7 +2581,8 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len(addr, (unsigned long) mod->module_core,
+				    mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}
@@ -2597,8 +2600,10 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (addr_within_len(addr, (unsigned long) mod->module_init,
+				    mod->init_text_size)
+		    || addr_within_len(addr, (unsigned long) mod->module_core,
+				       mod->core_text_size))
 			return mod;
 	return NULL;
 }
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
--- linux-2.6.26-rc3.orig/arch/x86/mm/pageattr.c
+++ linux-2.6.26-rc3/arch/x86/mm/pageattr.c
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>
@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */
@@ -164,7 +159,7 @@ static inline pgprot_t static_protection
 	 * The BIOS area between 640k and 1Mb needs to be executable for
 	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
 	 */
-	if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
+	if (addr_within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
@@ -172,14 +167,14 @@ static inline pgprot_t static_protection
 	 * Does not cover __inittext since that is gone later on. On
 	 * 64bit we do not enforce !NX on the low mapping
 	 */
-	if (within(address, (unsigned long)_text, (unsigned long)_etext))
+	if (addr_within(address, (unsigned long)_text, (unsigned long)_etext))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
 	 * The .rodata section needs to be read-only. Using the pfn
 	 * catches all aliases.
 	 */
-	if (within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
+	if (addr_within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
 		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_RW;
 
@@ -620,7 +615,7 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (!within(cpa->vaddr, PAGE_OFFSET,
+	if (!addr_within(cpa->vaddr, PAGE_OFFSET,
 		    PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT))) {
 
 		alias_cpa = *cpa;
@@ -636,14 +631,15 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the high
 	 * mapping already:
 	 */
-	if (within(cpa->vaddr, (unsigned long) _text, (unsigned long) _end))
+	if (addr_within(cpa->vaddr, (unsigned long) _text,
+			(unsigned long) _end))
 		return 0;
 
 	/*
 	 * If the physical address is inside the kernel map, we need
 	 * to touch the high mapped kernel as well:
 	 */
-	if (!within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
+	if (!addr_within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
 		return 0;
 
 	alias_cpa = *cpa;

Re: [PATCH] consolidate all within() implementations

[Andrew Morton]

On Tue, 20 May 2008 10:08:32 +0200 Peter Oberparleiter <oberparleiter@googlemail.com> wrote:

> Harvey Harrison wrote:
> > On Mon, 2008-05-19 at 10:45 +0200, Peter Oberparleiter wrote:
> >> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> >> 
> >> This patch consolidates a number of different implementations of the
> >> within() function which checks whether an address is within a specified
> >> address range.
> > 
> > Would it be that hard to just make them static inlines taking unsigned
> > longs?
> 
> I was hoping to get by without the spray of unsigned long casts that
> entails the enforcement of this specific parameter type, seeing that
> each implementation had it's own combination of longs and void *.
> 
> On the other hand, an inline function would of course be the cleaner
> approach, so if the code owners agree, here goes take #2:
> 
> --
> 
> From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> 
> This patch consolidates a number of different implementations of the
> within() function which checks whether an address is within a specified
> address range. Apart from parameter typing, existing implementations can
> be classified in two categories which differ in the way the range is
> specified:
> 
>   1) by start and end address
>   2) by start and size
> 
> Case 1) is covered by addr_within() while 2) is covered by
> addr_within_len().
> 
> Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
> ---
>  arch/x86/mm/pageattr.c |   20 ++++++++------------
>  include/linux/kernel.h |   24 ++++++++++++++++++++++++
>  kernel/lockdep.c       |   12 +++++-------
>  kernel/module.c        |   35 ++++++++++++++++++++---------------
>  4 files changed, 57 insertions(+), 34 deletions(-)
> 
> Index: linux-2.6.26-rc3/include/linux/kernel.h
> ===================================================================
> --- linux-2.6.26-rc3.orig/include/linux/kernel.h
> +++ linux-2.6.26-rc3/include/linux/kernel.h
> @@ -434,6 +434,30 @@ static inline char *pack_hex_byte(char *
>  	__val > __max ? __max: __val; })
>  
>  /**
> + * addr_within - check whether address is in start-and-end address range
> + * @addr: address
> + * @start: start address (included in range)
> + * @end: end address (excluded from range)
> + */
> +static inline int addr_within(unsigned long addr, unsigned long start,
> +			      unsigned long end)
> +{
> +	return (addr >= start) && (addr < end);
> +}
> +
> +/**
> + * addr_within_len - check whether address is in start-and-length address range
> + * @addr: address
> + * @start: start of range
> + * @len: number of bytes in range
> + */
> +static inline int addr_within_len(unsigned long addr, unsigned long start,
> +				  unsigned long len)
> +{
> +	return (addr >= start) && (addr < (start + len));
> +}

The kernel's use of unsigned long to represent pointers sometimes makes
sense, but often gets us into a mess.  It's OK in bootup code which
fiddles with memory map layout because there is no reason why such
code will ever dereference any of the addresses.

But I don't think we can assume this usage pattern when creating a
kernel-wide facility in kernel.h.

So yes, I do think that an address-comparison tool like this should
operate on void*'s.  (They will need to be const void*'s).

> +			if (addr_within_len((unsigned long) class->key,
> +					    (unsigned long) start, size))
> +			else if (addr_within_len((unsigned long) class->name,
> +						 (unsigned long) start, size))
> +	if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    || addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_core,
> +		if (addr_within_len(addr, (unsigned long) mod->module_init,
> +		    || addr_within_len(addr, (unsigned long) mod->module_core,

And you've had to add a great pile of casts anwyay?

Re: [PATCH] consolidate all within() implementations

[Peter Oberparleiter]

Andrew Morton wrote:
> On Tue, 20 May 2008 10:08:32 +0200 Peter Oberparleiter <oberparleiter@googlemail.com> wrote:
>> +/**
>> + * addr_within_len - check whether address is in start-and-length address range
>> + * @addr: address
>> + * @start: start of range
>> + * @len: number of bytes in range
>> + */
>> +static inline int addr_within_len(unsigned long addr, unsigned long start,
>> +				  unsigned long len)
>> +{
>> +	return (addr >= start) && (addr < (start + len));
>> +}
> 
> The kernel's use of unsigned long to represent pointers sometimes makes
> sense, but often gets us into a mess.  It's OK in bootup code which
> fiddles with memory map layout because there is no reason why such
> code will ever dereference any of the addresses.
> 
> But I don't think we can assume this usage pattern when creating a
> kernel-wide facility in kernel.h.
> 
> So yes, I do think that an address-comparison tool like this should
> operate on void*'s.  (They will need to be const void*'s).

Now that every line using a within() has to be changed anyway, it could
as well be done right, yes.

> And you've had to add a great pile of casts anwyay?

Well, between the unsigned long and the void * version, there's not that
much of a difference in the number of casts. So here goes #3:

--

From: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>

This patch consolidates a number of different implementations of the
within() function which checks whether an address is within a specified
address range. Apart from parameter typing, existing implementations can
be classified in two categories which differ in the way the range is
specified:

  1) by start and end address
  2) by start and size

Case 1) is covered by addr_within() while 2) is covered by
addr_within_len().

Signed-off-by: Peter Oberparleiter <peter.oberparleiter@de.ibm.com>
---
 arch/x86/mm/pageattr.c |   26 ++++++++++++--------------
 include/linux/kernel.h |   26 ++++++++++++++++++++++++++
 kernel/lockdep.c       |   10 +++-------
 kernel/module.c        |   34 +++++++++++++++++++---------------
 4 files changed, 60 insertions(+), 36 deletions(-)

Index: linux-2.6.26-rc3/include/linux/kernel.h
===================================================================
--- linux-2.6.26-rc3.orig/include/linux/kernel.h
+++ linux-2.6.26-rc3/include/linux/kernel.h
@@ -434,6 +434,32 @@ static inline char *pack_hex_byte(char *
 	__val > __max ? __max: __val; })
 
 /**
+ * addr_within - check whether address is in start-and-end address range
+ * @addr: address
+ * @start: start address (included in range)
+ * @end: end address (excluded from range)
+ */
+static inline int addr_within(const void *addr, const void *start,
+			      const void *end)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < (unsigned long) end);
+}
+
+/**
+ * addr_within_len - check whether address is in start-and-length address range
+ * @addr: address
+ * @start: start of range
+ * @len: number of bytes in range
+ */
+static inline int addr_within_len(const void *addr, const void *start,
+				  size_t len)
+{
+	return ((unsigned long) addr >= (unsigned long) start) &&
+	       ((unsigned long) addr < ((unsigned long) start + len));
+}
+
+/**
  * container_of - cast a member of a structure out to the containing structure
  * @ptr:	the pointer to the member.
  * @type:	the type of the container struct this is embedded in.
Index: linux-2.6.26-rc3/kernel/lockdep.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/lockdep.c
+++ linux-2.6.26-rc3/kernel/lockdep.c
@@ -25,6 +25,7 @@
  * Thanks to Arjan van de Ven for coming up with the initial idea of
  * mapping lock dependencies runtime.
  */
+#include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
@@ -2932,11 +2933,6 @@ static void zap_class(struct lock_class 
 
 }
 
-static inline int within(const void *addr, void *start, unsigned long size)
-{
-	return addr >= start && addr < start + size;
-}
-
 void lockdep_free_key_range(void *start, unsigned long size)
 {
 	struct lock_class *class, *next;
@@ -2956,9 +2952,9 @@ void lockdep_free_key_range(void *start,
 		if (list_empty(head))
 			continue;
 		list_for_each_entry_safe(class, next, head, hash_entry) {
-			if (within(class->key, start, size))
+			if (addr_within_len(class->key, start, size))
 				zap_class(class);
-			else if (within(class->name, start, size))
+			else if (addr_within_len(class->name, start, size))
 				zap_class(class);
 		}
 	}
Index: linux-2.6.26-rc3/kernel/module.c
===================================================================
--- linux-2.6.26-rc3.orig/kernel/module.c
+++ linux-2.6.26-rc3/kernel/module.c
@@ -2262,11 +2262,6 @@ sys_init_module(void __user *umod,
 	return 0;
 }
 
-static inline int within(unsigned long addr, void *start, unsigned long size)
-{
-	return ((void *)addr >= start && (void *)addr < start + size);
-}
-
 #ifdef CONFIG_KALLSYMS
 /*
  * This ignores the intensely annoying "mapping symbols" found
@@ -2287,7 +2282,7 @@ static const char *get_ksymbol(struct mo
 	unsigned long nextval;
 
 	/* At worse, next value is at end of module */
-	if (within(addr, mod->module_init, mod->init_size))
+	if (addr_within_len((void *) addr, mod->module_init, mod->init_size))
 		nextval = (unsigned long)mod->module_init+mod->init_text_size;
 	else
 		nextval = (unsigned long)mod->module_core+mod->core_text_size;
@@ -2335,8 +2330,10 @@ const char *module_address_lookup(unsign
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size)
-		    || within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size)
+		    || addr_within_len((void *) addr, mod->module_core,
+				       mod->core_size)) {
 			if (modname)
 				*modname = mod->name;
 			ret = get_ksymbol(mod, addr, size, offset);
@@ -2358,8 +2355,10 @@ int lookup_module_symbol_name(unsigned l
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, NULL, NULL);
@@ -2382,8 +2381,10 @@ int lookup_module_symbol_attrs(unsigned 
 
 	preempt_disable();
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_init, mod->init_size) ||
-		    within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_size) ||
+		    addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			const char *sym;
 
 			sym = get_ksymbol(mod, addr, size, offset);
@@ -2579,7 +2580,8 @@ int is_module_address(unsigned long addr
 	preempt_disable();
 
 	list_for_each_entry(mod, &modules, list) {
-		if (within(addr, mod->module_core, mod->core_size)) {
+		if (addr_within_len((void *) addr, mod->module_core,
+				    mod->core_size)) {
 			preempt_enable();
 			return 1;
 		}
@@ -2597,8 +2599,10 @@ struct module *__module_text_address(uns
 	struct module *mod;
 
 	list_for_each_entry(mod, &modules, list)
-		if (within(addr, mod->module_init, mod->init_text_size)
-		    || within(addr, mod->module_core, mod->core_text_size))
+		if (addr_within_len((void *) addr, mod->module_init,
+				    mod->init_text_size)
+		    || addr_within_len((void *) addr, mod->module_core,
+				       mod->core_text_size))
 			return mod;
 	return NULL;
 }
Index: linux-2.6.26-rc3/arch/x86/mm/pageattr.c
===================================================================
--- linux-2.6.26-rc3.orig/arch/x86/mm/pageattr.c
+++ linux-2.6.26-rc3/arch/x86/mm/pageattr.c
@@ -2,6 +2,7 @@
  * Copyright 2002 Andi Kleen, SuSE Labs.
  * Thanks to Ben LaHaise for precious feedback.
  */
+#include <linux/kernel.h>
 #include <linux/highmem.h>
 #include <linux/bootmem.h>
 #include <linux/module.h>
@@ -54,12 +55,6 @@ static inline unsigned long highmap_end_
 # define debug_pagealloc 0
 #endif
 
-static inline int
-within(unsigned long addr, unsigned long start, unsigned long end)
-{
-	return addr >= start && addr < end;
-}
-
 /*
  * Flushing functions
  */
@@ -164,7 +159,8 @@ static inline pgprot_t static_protection
 	 * The BIOS area between 640k and 1Mb needs to be executable for
 	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
 	 */
-	if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
+	if (addr_within((void *) pfn, (void *) (BIOS_BEGIN >> PAGE_SHIFT),
+			(void *) (BIOS_END >> PAGE_SHIFT)))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
@@ -172,15 +168,16 @@ static inline pgprot_t static_protection
 	 * Does not cover __inittext since that is gone later on. On
 	 * 64bit we do not enforce !NX on the low mapping
 	 */
-	if (within(address, (unsigned long)_text, (unsigned long)_etext))
+	if (addr_within((void *) address, _text, _etext))
 		pgprot_val(forbidden) |= _PAGE_NX;
 
 	/*
 	 * The .rodata section needs to be read-only. Using the pfn
 	 * catches all aliases.
 	 */
-	if (within(pfn, __pa((unsigned long)__start_rodata) >> PAGE_SHIFT,
-		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
+	if (addr_within((void *) pfn,
+			(void *) (__pa(__start_rodata) >> PAGE_SHIFT),
+			(void *) (__pa(__end_rodata) >> PAGE_SHIFT)))
 		pgprot_val(forbidden) |= _PAGE_RW;
 
 	prot = __pgprot(pgprot_val(prot) & ~pgprot_val(forbidden));
@@ -620,8 +617,8 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the direct
 	 * mapping already:
 	 */
-	if (!within(cpa->vaddr, PAGE_OFFSET,
-		    PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT))) {
+	if (!addr_within((void *) cpa->vaddr, (void *) PAGE_OFFSET,
+		    (void *) (PAGE_OFFSET + (max_pfn_mapped << PAGE_SHIFT)))) {
 
 		alias_cpa = *cpa;
 		alias_cpa.vaddr = (unsigned long) __va(cpa->pfn << PAGE_SHIFT);
@@ -636,14 +633,15 @@ static int cpa_process_alias(struct cpa_
 	 * No need to redo, when the primary call touched the high
 	 * mapping already:
 	 */
-	if (within(cpa->vaddr, (unsigned long) _text, (unsigned long) _end))
+	if (addr_within((void *) cpa->vaddr, _text, _end))
 		return 0;
 
 	/*
 	 * If the physical address is inside the kernel map, we need
 	 * to touch the high mapped kernel as well:
 	 */
-	if (!within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn()))
+	if (!addr_within((void *) cpa->pfn, (void *) highmap_start_pfn(),
+			 (void *) highmap_end_pfn()))
 		return 0;
 
 	alias_cpa = *cpa;

Re: [PATCH] consolidate all within() implementations

[Peter Zijlstra]

>  
>  /**
> + * addr_within - check whether address is in start-and-end address range
> + * @addr: address
> + * @start: start address (included in range)
> + * @end: end address (excluded from range)
> + */
> +static inline int addr_within(const void *addr, const void *start,
> +			      const void *end)
> +{
> +	return ((unsigned long) addr >= (unsigned long) start) &&
> +	       ((unsigned long) addr < (unsigned long) end);
> +}
> +
> +/**
> + * addr_within_len - check whether address is in start-and-length address range
> + * @addr: address
> + * @start: start of range
> + * @len: number of bytes in range
> + */
> +static inline int addr_within_len(const void *addr, const void *start,
> +				  size_t len)
> +{
> +	return ((unsigned long) addr >= (unsigned long) start) &&
> +	       ((unsigned long) addr < ((unsigned long) start + len));
> +}

might be my braindamage, but I'd have written it like:

static inline int 
addr_within_len(const void *addr, const void *start, size_t len)
{
	return (unsigned long)addr - (unsigned long)start < len;
}

static inline int
addr_within(const void *add, const void *start, const void *end)
{
	return addr_within_len(addr, start, 
			(unsigned long)end - (unsigned long)start);
}

Re: [PATCH] consolidate all within() implementations

[Peter 1 Oberparleiter]

Peter Zijlstra <a.p.zijlstra@chello.nl> wrote on 21.05.2008 12:04:26:
> > +static inline int addr_within_len(const void *addr, const void 
*start,
> > +              size_t len)
> > +{
> > +   return ((unsigned long) addr >= (unsigned long) start) &&
> > +          ((unsigned long) addr < ((unsigned long) start + len));
> > +}
> 
> might be my braindamage, but I'd have written it like:
> 
> static inline int 
> addr_within_len(const void *addr, const void *start, size_t len)
> {
>    return (unsigned long)addr - (unsigned long)start < len;
> }

Definitely another way to put it. In my opinion the intention of the
implementation is more easily understood though when spelling it out
as (a>=b) && (a<c).

> static inline int
> addr_within(const void *add, const void *start, const void *end)
> {
>    return addr_within_len(addr, start, 
>          (unsigned long)end - (unsigned long)start);
> }

For empty ranges (start > end), this produces different (less expected)
results than the previous version.


Regards,
  Peter

Re: [PATCH] consolidate all within() implementations

[Peter Zijlstra]

On Wed, 2008-05-21 at 12:33 +0200, Peter 1 Oberparleiter wrote:
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote on 21.05.2008 12:04:26:
> > > +static inline int addr_within_len(const void *addr, const void 
> *start,
> > > +              size_t len)
> > > +{
> > > +   return ((unsigned long) addr >= (unsigned long) start) &&
> > > +          ((unsigned long) addr < ((unsigned long) start + len));
> > > +}
> > 
> > might be my braindamage, but I'd have written it like:
> > 
> > static inline int 
> > addr_within_len(const void *addr, const void *start, size_t len)
> > {
> >    return (unsigned long)addr - (unsigned long)start < len;
> > }
> 
> Definitely another way to put it. In my opinion the intention of the
> implementation is more easily understood though when spelling it out
> as (a>=b) && (a<c).

peter@lappy:~/tmp$ cat cmp.c

int within_len1(const void *addr, const void *start, unsigned long len)
{
        return (unsigned long)addr - (unsigned long)start < len;
}

int within1(const void *addr, const void *start, const void *end)
{
        return within_len1(addr, start,
                        (unsigned long)end - (unsigned long)start);
}
peter@lappy:~/tmp$ cat cmp2.c
int within_len2(const void *addr, const void *start, unsigned long len)
{
        return ((unsigned long) addr >= (unsigned long) start) &&
                ((unsigned long) addr < ((unsigned long) start + len));
}

int within2(const void *addr, const void *start, const void *end)
{
        return ((unsigned long) addr >= (unsigned long) start) &&
                ((unsigned long) addr < ((unsigned long) end));
}
peter@lappy:~/tmp$ gcc -S -Os cmp*.c
peter@lappy:~/tmp$ ls -la cmp*.o
-rw-r--r-- 1 peter peter 752 2008-05-21 12:43 cmp2.o
-rw-r--r-- 1 peter peter 743 2008-05-21 12:43 cmp.o


Also look at the .s output and notice mine doesn't have any additional
branches ;-)

> > static inline int
> > addr_within(const void *add, const void *start, const void *end)
> > {
> >    return addr_within_len(addr, start, 
> >          (unsigned long)end - (unsigned long)start);
> > }
> 
> For empty ranges (start > end), this produces different (less expected)
> results than the previous version.

agreed, do we care about those?

Re: [PATCH] consolidate all within() implementations

[Peter 1 Oberparleiter]

Peter Zijlstra <a.p.zijlstra@chello.nl> wrote on 21.05.2008 12:48:52:

> On Wed, 2008-05-21 at 12:33 +0200, Peter 1 Oberparleiter wrote:
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote on 21.05.2008 12:04:26:

> peter@lappy:~/tmp$ gcc -S -Os cmp*.c
> peter@lappy:~/tmp$ ls -la cmp*.o
> -rw-r--r-- 1 peter peter 752 2008-05-21 12:43 cmp2.o
> -rw-r--r-- 1 peter peter 743 2008-05-21 12:43 cmp.o

Yeah, but!

[oberpar@local cmp]$ gcc -c -O2 cmp*.c
[oberpar@local cmp]$ ls -la cmp*.o
-rw-r--r-- 1 oberpar oberpar 1352 May 21 15:40 cmp2.o
-rw-r--r-- 1 oberpar oberpar 1408 May 21 15:40 cmp.o

:)

> Also look at the .s output and notice mine doesn't have any additional
> branches ;-)

It really boils down to the question whether you want to trade
a bit of obviousness for a bit of efficiency/clever programming.
I vote for keeping the former.

> > > static inline int
> > > addr_within(const void *add, const void *start, const void *end)
> > > {
> > >    return addr_within_len(addr, start, 
> > >          (unsigned long)end - (unsigned long)start);
> > > }
> > 
> > For empty ranges (start > end), this produces different (less 
expected)
> > results than the previous version.
> 
> agreed, do we care about those?

Why not plan for the unexpected when it comes at little cost?


Regards,
  Peter