[patch] 2.6.1-mm5 compile do not use shared extable code for ia64

[patch] 2.6.1-mm5 compile do not use shared extable code for ia64

[Jes Sorensen]

Hi,

The new sort_extable and shares search_extable code doesn't work on
ia64. I have introduced two new #defines that archs can define to avoid
the common code being built. ARCH_HAS_SEARCH_EXTABLE and
ARCH_HAS_SORT_EXTABLE.

With this patch, 2.6.1-mm5 builds again on ia64.

Cheers,
Jes

--- orig/linux-2.6.1-mm5/lib/extable.c	Tue Jan 20 02:53:26 2004
+++ linux-2.6.1-mm5/lib/extable.c	Tue Jan 20 04:08:37 2004
@@ -18,6 +18,7 @@
 extern struct exception_table_entry __start___ex_table[];
 extern struct exception_table_entry __stop___ex_table[];
 
+#ifndef ARCH_HAS_SORT_EXTABLE
 /*
  * The exception table needs to be sorted so that the binary
  * search that we use to find entries in it works properly.
@@ -45,7 +46,9 @@
 		}
 	}
 }
+#endif
 
+#ifndef ARCH_HAS_SEARCH_EXTABLE
 /*
  * Search one exception table for an entry corresponding to the
  * given instruction address, and return the address of the entry,
@@ -75,3 +78,4 @@
         }
         return NULL;
 }
+#endif
--- orig/linux-2.6.1-mm5/include/asm-ia64/uaccess.h	Sun Jan 11 07:00:36 2004
+++ linux-2.6.1-mm5/include/asm-ia64/uaccess.h	Tue Jan 20 04:09:13 2004
@@ -281,6 +281,9 @@
 	__su_ret;						\
 })
 
+#define ARCH_HAS_SORT_EXTABLE
+#define ARCH_HAS_SEARCH_EXTABLE
+
 struct exception_table_entry {
 	int addr;	/* gp-relative address of insn this fixup is for */
 	int cont;	/* gp-relative continuation address; if bit 2 is set, r9 is set to 0 */

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[Andrew Morton]

Jes Sorensen <jes@trained-monkey.org> wrote:
>
> The new sort_extable and shares search_extable code doesn't work on
>  ia64.

hm, OK.  It would be nice if ia64 could use the generic code at some stage,
of course.

One wonders why the linker dragged lib/extable.c in at all.  Or does it fail at
compile time?

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for ia64

[David Mosberger]

>>>>> On Tue, 20 Jan 2004 07:23:49 -0500, Jes Sorensen <jes@trained-monkey.org> said:

  Jes> Hi,
  Jes> The new sort_extable and shares search_extable code doesn't work on
  Jes> ia64. I have introduced two new #defines that archs can define to avoid
  Jes> the common code being built. ARCH_HAS_SEARCH_EXTABLE and
  Jes> ARCH_HAS_SORT_EXTABLE.

  Jes> With this patch, 2.6.1-mm5 builds again on ia64.

What's this about?  Isn't the only reason this doesn't work because
the "insn" member is called "addr" on ia64?  If so, surely it would
make more sense to do a little renaming?

	--david

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[David Mosberger]

>>>>> On Tue, 20 Jan 2004 09:00:04 -0800, Andrew Morton <akpm@osdl.org> said:

  Andrew> Jes Sorensen <jes@trained-monkey.org> wrote:
  >>  The new sort_extable and shares search_extable code doesn't work
  >> on ia64.

  Andrew> hm, OK.  It would be nice if ia64 could use the generic code
  Andrew> at some stage, of course.

How about something along these lines?  If you want to standardize on
a single instruction-address format, I'd strongly favor using the
location-relative addresses used on Alpha and ia64 (it makes no sense
to uses a full 64-bit address for those members).

	--david

=== lib/extable.c 1.3 vs edited ==--- 1.3/lib/extable.c	Tue Jan 20 17:58:55 2004
+++ edited/lib/extable.c	Fri Jan 23 18:10:10 2004
@@ -19,6 +19,12 @@
 extern struct exception_table_entry __stop___ex_table[];
 
 #ifndef ARCH_HAS_SORT_EXTABLE
+
+# ifndef exception_table_entry_insn
+   /* Return the instruction address to which exception tabl entry E applies.  */
+#  define exception_table_entry_insn(e)	((e)->insn)
+# endif
+
 /*
  * The exception table needs to be sorted so that the binary
  * search that we use to find entries in it works properly.
@@ -33,7 +39,7 @@
 	/* insertion sort */
 	for (p = start + 1; p < finish; ++p) {
 		/* start .. p-1 is sorted */
-		if (p[0].insn < p[-1].insn) {
+		if (exception_table_entry_insn(&p[0]) < exception_table_entry_insn(&p[-1])) {
 			/* move element p down to its right place */
 			el = *p;
 			q = p;
@@ -41,7 +47,8 @@
 				/* el comes before q[-1], move q[-1] up one */
 				q[0] = q[-1];
 				--q;
-			} while (q > start && el.insn < q[-1].insn);
+			} while (q > start && (exception_table_entry_insn(&el)
+					       < exception_table_entry_insn(&q[-1])));
 			*q = el;
 		}
 	}
=== include/asm-ia64/uaccess.h 1.16 vs edited ==--- 1.16/include/asm-ia64/uaccess.h	Fri Jan 23 16:43:32 2004
+++ edited/include/asm-ia64/uaccess.h	Fri Jan 23 18:06:38 2004
@@ -283,13 +283,18 @@
 	__su_ret;						\
 })
 
-#define ARCH_HAS_SORT_EXTABLE
 #define ARCH_HAS_SEARCH_EXTABLE
 
 struct exception_table_entry {
-	int addr;	/* gp-relative address of insn this fixup is for */
-	int cont;	/* gp-relative continuation address; if bit 2 is set, r9 is set to 0 */
+	int addr;	/* loc-relative address of insn this fixup is for */
+	int cont;	/* loc-relative continuation address; if bit 2 is set, r9 is set to 0 */
 };
+
+#define exception_table_entry_insn(e)				\
+({								\
+	const struct exception_table_entry *_etei_e = (e);	\
+	(u64) &(_etei_e)->addr + (_etei_e)->addr;		\
+})
 
 extern void handle_exception (struct pt_regs *regs, const struct exception_table_entry *e);
 extern const struct exception_table_entry *search_exception_tables (unsigned long addr);

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[Paul Mackerras]

David Mosberger writes:

> How about something along these lines?  If you want to standardize on
> a single instruction-address format, I'd strongly favor using the
> location-relative addresses used on Alpha and ia64 (it makes no sense
> to uses a full 64-bit address for those members).

Won't you have to change the offset when you move the entry, if the
value you store is relative to the address of the entry?  You could
get around that by storing the offset relative to the start of the
exception table, I guess.

Paul.

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[David Mosberger]

>>>>> On Sat, 24 Jan 2004 21:49:26 +1100, Paul Mackerras <paulus@samba.org> said:

  Paul> David Mosberger writes:
  >> How about something along these lines?  If you want to
  >> standardize on a single instruction-address format, I'd strongly
  >> favor using the location-relative addresses used on Alpha and
  >> ia64 (it makes no sense to uses a full 64-bit address for those
  >> members).

  Paul> Won't you have to change the offset when you move the entry,
  Paul> if the value you store is relative to the address of the
  Paul> entry?

Details, details!

How about the attached one?  It will touch memory more when moving an
element down, but we're talking about exception tables here, and I
don't think module loading time would be affected in any noticable
fashion.

	--david

=== arch/ia64/mm/extable.c 1.7 vs edited ==--- 1.7/arch/ia64/mm/extable.c	Sun Jan 18 03:36:30 2004
+++ edited/arch/ia64/mm/extable.c	Fri Jan 23 18:04:24 2004
@@ -10,11 +10,6 @@
 #include <asm/uaccess.h>
 #include <asm/module.h>
 
-void sort_extable(struct exception_table_entry *start,
-		  struct exception_table_entry *finish)
-{
-}
-
 const struct exception_table_entry *
 search_extable (const struct exception_table_entry *first,
 		const struct exception_table_entry *last,
=== include/asm-ia64/uaccess.h 1.16 vs edited ==--- 1.16/include/asm-ia64/uaccess.h	Fri Jan 23 16:43:32 2004
+++ edited/include/asm-ia64/uaccess.h	Mon Jan 26 15:15:28 2004
@@ -283,13 +283,42 @@
 	__su_ret;						\
 })
 
-#define ARCH_HAS_SORT_EXTABLE
 #define ARCH_HAS_SEARCH_EXTABLE
 
 struct exception_table_entry {
-	int addr;	/* gp-relative address of insn this fixup is for */
-	int cont;	/* gp-relative continuation address; if bit 2 is set, r9 is set to 0 */
+	int addr;	/* loc-relative address of insn this fixup is for */
+	int cont;	/* loc-relative continuation address; if bit 2 is set, r9 is set to 0 */
 };
+
+static inline int
+extable_compare_entries (struct exception_table_entry *l, struct exception_table_entry *r)
+{
+	u64 lip = (u64) &l->addr + l->addr;
+	u64 rip = (u64) &r->addr + r->addr;
+
+	if (lip < rip)
+		return -1;
+	if (lip = rip)
+		return 0;
+	else
+		return 1;
+}
+
+static inline void
+extable_swap_entries (struct exception_table_entry *l, struct exception_table_entry *r)
+{
+	u64 delta = (u64) r - (u64) l;
+	struct exception_table_entry tmp;
+
+	tmp = *l;
+	l->addr = r->addr + delta;
+	l->cont = r->cont + delta;
+	r->addr = tmp.addr - delta;
+	r->cont = tmp.cont - delta;
+}
+
+#define extable_compare_entries	extable_compare_entries
+#define extable_swap_entries	extable_swap_entries
 
 extern void handle_exception (struct pt_regs *regs, const struct exception_table_entry *e);
 extern const struct exception_table_entry *search_exception_tables (unsigned long addr);
=== lib/extable.c 1.3 vs edited ==--- 1.3/lib/extable.c	Tue Jan 20 17:58:55 2004
+++ edited/lib/extable.c	Mon Jan 26 15:23:12 2004
@@ -18,7 +18,25 @@
 extern struct exception_table_entry __start___ex_table[];
 extern struct exception_table_entry __stop___ex_table[];
 
-#ifndef ARCH_HAS_SORT_EXTABLE
+#ifndef extable_compare_entries
+
+/*
+ * Compare exception-table entries L and R and return <0 if L is smaller, 0 if L and R are
+ * equal and >0 if L is bigger.
+ */
+# define extable_compare_entries(l,r)	((l)->insn - (r)->insn)
+
+static inline void
+extable_swap_entries (struct exception_table_entry *l, struct exception_table_entry *r)
+{
+	struct exception_table_entry tmp;
+
+	tmp = *l;
+	*l = *r;
+	*r = tmp;
+}
+#endif /* !extable_compare_entries */
+
 /*
  * The exception table needs to be sorted so that the binary
  * search that we use to find entries in it works properly.
@@ -28,25 +46,14 @@
 void sort_extable(struct exception_table_entry *start,
 		  struct exception_table_entry *finish)
 {
-	struct exception_table_entry el, *p, *q;
+	struct exception_table_entry *p, *q;
 
 	/* insertion sort */
-	for (p = start + 1; p < finish; ++p) {
-		/* start .. p-1 is sorted */
-		if (p[0].insn < p[-1].insn) {
-			/* move element p down to its right place */
-			el = *p;
-			q = p;
-			do {
-				/* el comes before q[-1], move q[-1] up one */
-				q[0] = q[-1];
-				--q;
-			} while (q > start && el.insn < q[-1].insn);
-			*q = el;
-		}
-	}
+	for (p = start + 1; p < finish; ++p)
+		/* start .. p-1 is sorted; push p down to it's proper place */
+		for (q = p; q > start && extable_compare_entries(&q[0], &q[-1]) < 0; --q)
+			extable_swap_entries(&q[0], &q[-1]);
 }
-#endif
 
 #ifndef ARCH_HAS_SEARCH_EXTABLE
 /*

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for ia64

[Jes Sorensen]

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

David,

I am just nitpicking here, but wouldn't it be better to stick to the
convention of all upper case defines for the #ifdef check?

Maybe use something like?
#define ARCH_EXTABLE_COMPARE_ENTRIES ia64_extable_compare_entries

Cheers,
Jes


@@ -18,7 +18,25 @@
 extern struct exception_table_entry __start___ex_table[];
 extern struct exception_table_entry __stop___ex_table[];
 
-#ifndef ARCH_HAS_SORT_EXTABLE
+#ifndef extable_compare_entries
+
+/*
+ * Compare exception-table entries L and R and return <0 if L is smaller, 0 if L and R are
+ * equal and >0 if L is bigger.
+ */

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[Paul Mackerras]

David Mosberger writes:

> How about the attached one?  It will touch memory more when moving an
> element down, but we're talking about exception tables here, and I
> don't think module loading time would be affected in any noticable
> fashion.

Hmmm...  Stylistically I much prefer to pick up the new element,
move the others up and just drop the new element in where it should
go, rather than doing swap, swap, swap down the list.

Also, I don't think there is enough code there to be worth the bother
of trying to abstract the generic routine so you can plug in different
compare and move-element routines.  The whole sort routine is only 16
lines of code, after all.  Why not just have an ia64-specific version
of sort_extable?  That's what I thought you would do.

Regards,
Paul.

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[David Mosberger]

>>>>> On Tue, 27 Jan 2004 19:56:26 +1100, Paul Mackerras <paulus@samba.org> said:

  Paul> David Mosberger writes:
  >> How about the attached one?  It will touch memory more when
  >> moving an element down, but we're talking about exception tables
  >> here, and I don't think module loading time would be affected in
  >> any noticable fashion.

  Paul> Hmmm...  Stylistically I much prefer to pick up the new
  Paul> element, move the others up and just drop the new element in
  Paul> where it should go, rather than doing swap, swap, swap down
  Paul> the list.

Sure, the latter can be done, too.

  Paul> Also, I don't think there is enough code there to be worth the
  Paul> bother of trying to abstract the generic routine so you can
  Paul> plug in different compare and move-element routines.  The
  Paul> whole sort routine is only 16 lines of code, after all.  Why
  Paul> not just have an ia64-specific version of sort_extable?
  Paul> That's what I thought you would do.

Because the Alpha needs exactly the same code.

	--david

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for ia64

[David Mosberger]

>>>>> On 27 Jan 2004 03:11:03 -0500, Jes Sorensen <jes@wildopensource.com> said:

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:
  Jes> David,

  Jes> I am just nitpicking here, but wouldn't it be better to stick
  Jes> to the convention of all upper case defines for the #ifdef
  Jes> check?

Yeah, it is nitpicking! ;-)

  Jes> Maybe use something like?  #define ARCH_EXTABLE_COMPARE_ENTRIES
  Jes> ia64_extable_compare_entries

I'd rather have ARCH_HAS_EXTABLE_COMPARE_ENTRIES or something like
that, in that case.

	--david

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[David Mosberger]

>>>>> On Tue, 27 Jan 2004 19:56:26 +1100, Paul Mackerras <paulus@samba.org> said:

  Paul> David Mosberger writes:
  >> How about the attached one?  It will touch memory more when
  >> moving an element down, but we're talking about exception tables
  >> here, and I don't think module loading time would be affected in
  >> any noticable fashion.

  Paul> Hmmm...  Stylistically I much prefer to pick up the new
  Paul> element, move the others up and just drop the new element in
  Paul> where it should go, rather than doing swap, swap, swap down
  Paul> the list.

The original code may be slightly faster, but who cares?  From a
readability point of view, I think my version is easier to understand.

  Paul> Also, I don't think there is enough code there to be worth the
  Paul> bother of trying to abstract the generic routine so you can
  Paul> plug in different compare and move-element routines.  The
  Paul> whole sort routine is only 16 lines of code, after all.  Why
  Paul> not just have an ia64-specific version of sort_extable?
  Paul> That's what I thought you would do.

That's certainly an option.  It was Andrew who called for a generic
version.  I tend to agree with him because even though it's just a
little sort routine, it's one of those things where stupid errors tend
to creep in.  And like I mentioned earlier, Alpha needs the exact same
code (and frankly, I'm surprised there are 64-bit platforms that do
NOT use the location-relative format that Richard invented).

	--david

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[Paul Mackerras]

David Mosberger writes:

>   Paul> Also, I don't think there is enough code there to be worth the
>   Paul> bother of trying to abstract the generic routine so you can
>   Paul> plug in different compare and move-element routines.  The
>   Paul> whole sort routine is only 16 lines of code, after all.  Why
>   Paul> not just have an ia64-specific version of sort_extable?
>   Paul> That's what I thought you would do.
> 
> Because the Alpha needs exactly the same code.

I really don't like the uglification of lib/extable.c.  I think it is
much better to have ~20 lines of code in each of arch/ia64/mm and
arch/alpha/mm than to try to generalize lib/extable.c.  Once you add
all the extra definitions you need for your version, I doubt that the
overall savings would be more than a dozen lines or so.

The point is that with lib/extable.c as it is, you can look at one
page of code, and everything you need to understand that code is
there.  With your change, I have to hunt around to check what the
macros are doing on each arch, and flip backwards and forwards to
check side effects, calling environment etc.  With an ia64-specific
extable.c, you should be able to look at one page of code there and
see that the ia64 version is also correct.

Paul.

Re: [patch] 2.6.1-mm5 compile do not use shared extable code for

[David Mosberger]

>>>>> On Wed, 28 Jan 2004 10:49:10 +1100, Paul Mackerras <paulus@samba.org> said:

  Paul> I really don't like the uglification of lib/extable.c.

I disagree about this being an uglification.  But beauty is obviously
in the eye of the beholder...

Anyhow, you clearly feel _much_ stronger about this particular issue
than I do and I haven't heard much from Andrew, so I'll make a local
version of sort_extable() for now.  If someone cares about
resurrecting a generic version, they can do that later on.

	--david