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 = ...
[ message continues ]

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


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

static inline int addr_within_len(unsigned long addr, unsigned long start,
				  unsigned long len)

Just a thought.

Harvey

--

[ message continues ]

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 ...
[ message continues ]

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

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

[ message continues ]

Now that every line using a within() has to be changed anyway, it could

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 ...
[ message continues ]

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);
}


--

[ message continues ]

Definitely another way to put it. In my opinion the intention of the
implementation is more easily understood though when spelling it out

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


Regards,
  Peter
--

[ message continues ]

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

agreed, do we care about those?

--

[ message continues ]

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

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

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


Regards,
  Peter
--

[ message continues ]