Re: [PATCH] Restore module support.

Re: [PATCH] Restore module support.

[Luck, Tony]

> (2) has the disadvantage that its touching non-architecture specific
> code, but this is the option I'd prefer due to the obvious performance
> advantage.  However, I'm afraid that it isn't worth the effort to fix
> up vmalloc and /proc/kcore.  vmalloc fix appears simple, but /proc/kcore
> has issues (anyone know what KCORE_BASE is all about?)

KCORE_BASE is my fault ... it was an attempt to fix the "modules
below PAGE_OFFSET" problem for the ia64 port.  For a few nanoseconds
the code just here looked like this:

#if VMALLOC_START < PAGE_OFFSET
#define	KCORE_BASE	VMALLOC_START
#else
#define	KCORE_BASE	PAGE_OFFSET
#endif

Which worked great for ia64, but failed to even compile on i386
(because on i386 VMALLOC_START isn't a simple constant that cpp
can compare against).

Linus kept the bulk of my patch and just replaced the above code with
the "#define	KCORE_BASE	PAGE_OFFSET" that is there today, maybe
in the hope that I'd come back with a workable #ifdef ... but the only
one I've come up with so far is "#ifdef CONFIG_IA64" which can't be
right as ia64 isn't the only architecture with this issue.

There was some discussion on a better way to do this, by adding the
kernel itself to the vmlist, and eliminating all the special case code.
I took a brief look at this, but realised that there were all sorts
of ugly race conditions with /proc/kcore if a module is loaded/unloaded
after some process has read the Elf header.

-Tony Luck

Re: [PATCH] Restore module support.

[Russell King]

On Fri, Feb 07, 2003 at 10:43:19AM -0800, Luck, Tony wrote:
> > (2) has the disadvantage that its touching non-architecture specific
> > code, but this is the option I'd prefer due to the obvious performance
> > advantage.  However, I'm afraid that it isn't worth the effort to fix
> > up vmalloc and /proc/kcore.  vmalloc fix appears simple, but /proc/kcore
> > has issues (anyone know what KCORE_BASE is all about?)
> 
> KCORE_BASE is my fault ... it was an attempt to fix the "modules
> below PAGE_OFFSET" problem for the ia64 port.  For a few nanoseconds
> the code just here looked like this:
> 
> #if VMALLOC_START < PAGE_OFFSET
> #define	KCORE_BASE	VMALLOC_START
> #else
> #define	KCORE_BASE	PAGE_OFFSET
> #endif

Ah, ok.  What I'm thinking of is something like the following (untested
and probably improperly thought out patch...):

--- orig/fs/proc/kcore.c	Sat Nov  2 18:58:18 2002
+++ linux/fs/proc/kcore.c	Fri Feb  7 19:48:35 2003
@@ -99,7 +99,10 @@
 }
 #else /* CONFIG_KCORE_AOUT */
 
+#ifndef KCORE_BASE
 #define	KCORE_BASE	PAGE_OFFSET
+#define in_vmlist_region(x) ((x) >= VMALLOC_START && (x) < VMALLOC_END)
+#endif
 
 #define roundup(x, y)  ((((x)+((y)-1))/(y))*(y))
 
@@ -394,7 +397,7 @@
 		tsz = buflen;
 		
 	while (buflen) {
-		if ((start >= VMALLOC_START) && (start < VMALLOC_END)) {
+		if (in_vmlist_region(start)) {
 			char * elf_buf;
 			struct vm_struct *m;
 			unsigned long curstart = start;

An architecture could then define KCORE_BASE and in_vmlist_region()
alongside their VMALLOC_START definition if they needed to change
them.

> There was some discussion on a better way to do this, by adding the
> kernel itself to the vmlist, and eliminating all the special case code.
> I took a brief look at this, but realised that there were all sorts
> of ugly race conditions with /proc/kcore if a module is loaded/unloaded
> after some process has read the Elf header.

Well, only root can debug using /proc/kcore, and I'd suggest the best
answer to that problem is "if it hurts, don't do that."  I don't think
you should prevent modules from being unloaded just because you have
/proc/kcore open.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

RE: [PATCH] Restore module support.

[Luck, Tony]

Russell King wrote:
> On Fri, Feb 07, 2003 at 10:43:19AM -0800, Luck, Tony wrote:
> > > (2) has the disadvantage that its touching 
> non-architecture specific
> > > code, but this is the option I'd prefer due to the 
> obvious performance
> > > advantage.  However, I'm afraid that it isn't worth the 
> effort to fix
> > > up vmalloc and /proc/kcore.  vmalloc fix appears simple, 
> but /proc/kcore
> > > has issues (anyone know what KCORE_BASE is all about?)
> > 
> > KCORE_BASE is my fault ... it was an attempt to fix the "modules
> > below PAGE_OFFSET" problem for the ia64 port.  For a few nanoseconds
> > the code just here looked like this:
> > 
> > #if VMALLOC_START < PAGE_OFFSET
> > #define	KCORE_BASE	VMALLOC_START
> > #else
> > #define	KCORE_BASE	PAGE_OFFSET
> > #endif
> 
> Ah, ok.  What I'm thinking of is something like the following 
> (untested
> and probably improperly thought out patch...):
> 
> --- orig/fs/proc/kcore.c	Sat Nov  2 18:58:18 2002
> +++ linux/fs/proc/kcore.c	Fri Feb  7 19:48:35 2003
> @@ -99,7 +99,10 @@
>  }
>  #else /* CONFIG_KCORE_AOUT */
>  
> +#ifndef KCORE_BASE
>  #define	KCORE_BASE	PAGE_OFFSET
> +) < #define in_vmlist_region(x) ((x) >= VMALLOC_START && (x
> VMALLOC_END)
> +#endif
>  
>  #define roundup(x, y)  ((((x)+((y)-1))/(y))*(y))
>  
> @@ -394,7 +397,7 @@
>  		tsz = buflen;
>  		
>  	while (buflen) {
> -		if ((start >= VMALLOC_START) && (start < VMALLOC_END)) {
> +		if (in_vmlist_region(start)) {
>  			char * elf_buf;
>  			struct vm_struct *m;
>  			unsigned long curstart = start;
> 
> An architecture could then define KCORE_BASE and in_vmlist_region()
> alongside their VMALLOC_START definition if they needed to change
> them.

Looks pretty good.  What's the motivation for the in_vmlist_region()?
I don't think that I need that for ia64 ... so it might be better to
have separate #ifdefs:

#ifndef KCORE_BASE
#define	KCORE_BASE	PAGE_OFFSET
endif
#ifndef in_vmlist_region
#define in_vmlist_region(x) ((x) >= VMALLOC_START && (x < VMALLOC_END))
#endif

-Tony

Re: [PATCH] Restore module support.

[Rusty Russell]

> In message <20030207001006.A19306@flint.arm.linux.org.uk> you write:
> > And I'll promptly provide you with the other view.  I'm still trying to
> > sort out the best thing to do for ARM.  We have the choice of:

Actually, I must be really confused.  I thought ARM was already
complete.

Anyway, here's a version which simply does what the usermode one did,
if you decide to take the "fix it later" approach.

Cheers!
Rusty.
PS.  I did this in the usermode test framework, so not live tested.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/arch/arm/kernel/module.c working-2.5.59-armmodule/arch/arm/kernel/module.c
--- linux-2.5.59/arch/arm/kernel/module.c	2003-02-07 19:21:51.000000000 +1100
+++ working-2.5.59-armmodule/arch/arm/kernel/module.c	2003-02-07 19:04:12.000000000 +1100
@@ -2,6 +2,7 @@
  *  linux/arch/arm/kernel/module.c
  *
  *  Copyright (C) 2002 Russell King.
+ *  Dumbed down by Rusty Russell.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -17,54 +18,74 @@
 #include <linux/fs.h>
 #include <linux/string.h>
 
-#include <asm/pgtable.h>
+#if 0
+#define DEBUGP printk
+#else
+#define DEBUGP(fmt , ...)
+#endif
 
-void *module_alloc(unsigned long size)
+/* This is a beautiful architecture. --RR */
+struct arm_plt_entry
 {
-	struct vm_struct *area;
-	struct page **pages;
-	unsigned int array_size, i;
-
-	size = PAGE_ALIGN(size);
-	if (!size)
-		goto out_null;
+	u32 ldr_pc;			/* ldr pc,[pc,#-4] */
+	u32 location;			/* sym@ */
+};
 
-	area = __get_vm_area(size, VM_ALLOC, MODULE_START, MODULE_END);
-	if (!area)
-		goto out_null;
+void *module_alloc(unsigned long size)
+{
+	if (size == 0)
+		return NULL;
+	return vmalloc(size);
+}
 
-	area->nr_pages = size >> PAGE_SHIFT;
-	array_size = area->nr_pages * sizeof(struct page *);
-	area->pages = pages = kmalloc(array_size, GFP_KERNEL);
-	if (!area->pages) {
-		remove_vm_area(area->addr);
-		kfree(area);
-		goto out_null;
-	}
+/* Free memory returned from module_alloc */
+void module_free(struct module *mod, void *module_region)
+{
+	vfree(module_region);
+}
 
-	memset(pages, 0, array_size);
+/* Count how many different PC24 relocations (different symbol) */
+static unsigned int count_relocs(const Elf32_Rel *rel, unsigned int num)
+{
+	unsigned int i, j, ret = 0;
 
-	for (i = 0; i < area->nr_pages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (unlikely(!pages[i])) {
-			area->nr_pages = i;
-			goto out_no_pages;
+	/* Sure, this is order(n^2), but it's usually short, and not
+           time critical */
+	for (i = 0; i < num; i++) {
+		if (ELF32_R_TYPE(rel[i].r_info) != R_ARM_PC24)
+			continue;
+		for (j = 0; j < i; j++) {
+			if (ELF32_R_TYPE(rel[j].r_info) != R_ARM_PC24)
+				continue;
+			/* If this addend appeared before, it's
+                           already been counted */
+			if (ELF32_R_SYM(rel[i].r_info)
+			    == ELF32_R_SYM(rel[j].r_info))
+				break;
 		}
+		if (j == i) ret++;
 	}
-
-	if (map_vm_area(area, PAGE_KERNEL, &pages))
-		goto out_no_pages;
-	return area->addr;
-
- out_no_pages:
-	vfree(area->addr);
- out_null:
-	return NULL;
+	return ret;
 }
 
-void module_free(struct module *module, void *region)
+/* Get the potential trampolines size required sections */
+static unsigned long get_plt_size(const Elf32_Ehdr *hdr,
+				  const Elf32_Shdr *sechdrs,
+				  const char *secstrings)
 {
-	vfree(region);
+	unsigned long ret = 0;
+	unsigned i;
+
+	/* Everything marked ALLOC (this includes the exported
+           symbols) */
+	for (i = 1; i < hdr->e_shnum; i++) {
+		if (sechdrs[i].sh_type != SHT_REL)
+			continue;
+		ret += count_relocs((void *)hdr + sechdrs[i].sh_offset,
+				    sechdrs[i].sh_size / sizeof(Elf32_Rel));
+	}
+
+	return ret * sizeof(struct arm_plt_entry);
 }
 
 int module_frob_arch_sections(Elf_Ehdr *hdr,
@@ -72,9 +93,55 @@ int module_frob_arch_sections(Elf_Ehdr *
 			      char *secstrings,
 			      struct module *mod)
 {
+	unsigned int i;
+	char *p;
+
+	/* Find .plt section, and rename .init sections, which we
+           don't handle */
+	for (i = 1; i < hdr->e_shnum; i++) {
+		if (strcmp(secstrings + sechdrs[i].sh_name, ".plt") == 0)
+			mod->arch.plt_section = i;
+		while ((p = strstr(secstrings + sechdrs[i].sh_name, ".init")))
+			p[0] = '_';
+	}
+	if (!mod->arch.plt_section) {
+		printk("Module doesn't contain .plt section.\n");
+		return -ENOEXEC;
+	}
+
+	/* Override its size */
+	sechdrs[mod->arch.plt_section].sh_size
+		= get_plt_size(hdr, sechdrs, secstrings);
+	/* Override its type and flags: in asm statement doesn't work 8( */
+	sechdrs[mod->arch.plt_section].sh_type = SHT_NOBITS;
+	sechdrs[mod->arch.plt_section].sh_flags = (SHF_EXECINSTR | SHF_ALLOC);
 	return 0;
 }
 
+/* Allocate (or find) the PLT entry for this function. */
+static u32 make_plt(Elf32_Shdr *sechdrs, struct module *module, u32 funcaddr)
+{
+	struct arm_plt_entry *plt;
+	unsigned int i, num_plts;
+
+	plt = (void *)sechdrs[module->arch.plt_section].sh_addr;
+	num_plts = sechdrs[module->arch.plt_section].sh_size / sizeof(*plt);
+
+	for (i = 0; i < num_plts; i++) {
+		if (!plt[i].ldr_pc) {
+			/* New one.  Fill in. */
+			plt[i].ldr_pc = 0xe51ff004;
+			plt[i].location = funcaddr;
+		}
+		if (plt[i].location == funcaddr) {
+			DEBUGP("Made plt %u for %p at %p\n",
+			       i, (void *)funcaddr, &plt[i]);
+			return (u32)&plt[i];
+		}
+	}
+	BUG();
+}
+
 int
 apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,
 	       unsigned int relindex, struct module *module)
@@ -86,7 +153,7 @@ apply_relocate(Elf32_Shdr *sechdrs, cons
 	unsigned int i;
 
 	for (i = 0; i < relsec->sh_size / sizeof(Elf32_Rel); i++, rel++) {
-		unsigned long loc;
+		unsigned long loc, addend;
 		Elf32_Sym *sym;
 		s32 offset;
 
@@ -98,6 +165,11 @@ apply_relocate(Elf32_Shdr *sechdrs, cons
 		}
 
 		sym = ((Elf32_Sym *)symsec->sh_addr) + offset;
+		if (!sym->st_value) {
+			printk(KERN_WARNING "%s: unknown symbol %s\n",
+				module->name, strtab + sym->st_name);
+			return -ENOENT;
+		}
 
 		if (rel->r_offset < 0 || rel->r_offset > dstsec->sh_size - sizeof(u32)) {
 			printk(KERN_ERR "%s: out of bounds relocation, "
@@ -115,24 +187,26 @@ apply_relocate(Elf32_Shdr *sechdrs, cons
 			break;
 
 		case R_ARM_PC24:
-			offset = (*(u32 *)loc & 0x00ffffff) << 2;
-			if (offset & 0x02000000)
-				offset -= 0x04000000;
+			/* Pull addend from location */
+			addend = (*(u32 *)loc & 0x00ffffff) << 2;
+			if (addend & 0x02000000)
+				addend -= 0x04000000;
+			offset = sym->st_value + addend - loc;
 
-			offset += sym->st_value - loc;
-			if (offset & 3 ||
-			    offset <= (s32)0xfc000000 ||
-			    offset >= (s32)0x04000000) {
+			/* if the target is too far away, use plt. */
+			if (offset < -0x02000000 || offset >= 0x02000000)
+				offset = make_plt(sechdrs,module,sym->st_value)
+					+ addend - loc;
+
+			if (offset & 3) {
 				printk(KERN_ERR "%s: unable to fixup "
-				       "relocation: out of range\n",
-				       module->name);
+				       "relocation: %u out of range\n",
+				       module->name, offset);
 				return -ENOEXEC;
 			}
 
-			offset >>= 2;
-
 			*(u32 *)loc &= 0xff000000;
-			*(u32 *)loc |= offset & 0x00ffffff;
+			*(u32 *)loc |= (offset >> 2) & 0x00ffffff;
 			break;
 
 		default:
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/include/asm-arm/module.h working-2.5.59-armmodule/include/asm-arm/module.h
--- linux-2.5.59/include/asm-arm/module.h	2003-02-07 19:16:25.000000000 +1100
+++ working-2.5.59-armmodule/include/asm-arm/module.h	2003-02-07 19:04:03.000000000 +1100
@@ -3,11 +3,16 @@
 
 struct mod_arch_specific
 {
-	int foo;
+	/* Index of PLT section within module. */
+	unsigned int plt_section;
 };
 
 #define Elf_Shdr	Elf32_Shdr
 #define Elf_Sym		Elf32_Sym
 #define Elf_Ehdr	Elf32_Ehdr
 
+/* Make empty sections for module_frob_arch_sections to expand. */
+#ifdef MODULE
+asm(".section .plt; .align 3; .previous");
+#endif
 #endif /* _ASM_ARM_MODULE_H */

Re: [PATCH] Restore module support.

[Russell King]

On Fri, Feb 07, 2003 at 07:26:50PM +1100, Rusty Russell wrote:
> Actually, I must be really confused.  I thought ARM was already
> complete.
> 
> Anyway, here's a version which simply does what the usermode one did,
> if you decide to take the "fix it later" approach.

Rusty, as I said, I already have a patch for this approach.  Its the
second approach that I'd prefer to get working.

Also, if you see the message-id I posted in my mail just 5 minutes ago,
you'll see why the existing code does not work.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Restore module support.

[Rusty Russell]

In message <20030207100540.B23442@flint.arm.linux.org.uk> you write:
> On Fri, Feb 07, 2003 at 07:26:50PM +1100, Rusty Russell wrote:
> > Actually, I must be really confused.  I thought ARM was already
> > complete.
> > 
> > Anyway, here's a version which simply does what the usermode one did,
> > if you decide to take the "fix it later" approach.
> 
> Rusty, as I said, I already have a patch for this approach.  Its the
> second approach that I'd prefer to get working.

Sure, but you complained that I hadn't made life easier for the arch
maintainers.  I'm sorry if you feel this way, but I felt that the
least I could do, at the first complaint I was aware of, was to
provide you with a solution.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Restore module support.

[Adam J. Richter]

On 2003-02-06, Greg KH wrote:
>On Fri, Feb 07, 2003 at 12:09:27AM +0100, Roman Zippel wrote:
>> Hi,
>> 
>> On Tue, 4 Feb 2003, Rusty Russell wrote:
>> 
>> > I'm going to stop here, since I don't think you understand what I am
>> > proposing, nor how the current system works: this makes is extremely
>> > difficult to describe changes, and time consuming.
>> 
>> Rusty, if you continue to ignore criticism, I have only one answer left:
>> 
>> http://www.xs4all.nl/~zippel/restore-modules-2.5.59.diff
>
>But what are the modutils numbers? :)
>
>Come on, what Rusty did was the "right thing to do" and has made life
>easier for all of the arch maintainers (or so says the ones that I've
>talked to), and has made my life easier with regards to
>MODULE_DEVICE_TABLE() logic, which will enable the /sbin/hotplug
>scripts/binary to shrink a _lot_.

	I'd be interested in some elaboration on these two points.

	I'd like to understand what problems were solved for other
architectures by putting the module loader into the kernel, so I could
compare what would be involved to delivering the same benefit with a
user-level module loader.

	I think the MODULE_DEVICE_TABLE stuff is largely independent
of whether the module loading is done inside the kernel or from user
level, but if this is due to some misunderstanding on my part, please
set me straight.

	Although I write this in response to a message by Greg KH, I
would welcome answers from anyone.

Adam J. Richter     __     ______________   575 Oroville Road
adam@yggdrasil.com     \ /                  Milpitas, California 95035
+1 408 309-6081         | g g d r a s i l   United States of America
                         "Free Software For The Rest Of Us."

Re: [PATCH] Module alias and device table support.

[Rusty Russell]

In message <200302040805.h1485lhI002898@eeyore.valparaiso.cl> you write:
> > It's undefined for the "in the config file case" (they curently *do*
> > override, but that's an implementation detail).  It'd be clearer to
> > explicitly say "you can't override module names with "alias", use
> > "install" instead, IMHO.
> 
> Urgh. What is "alias" then for? It has been used for ages as a way of "call
> module foo by name bar, possibly with this further arguments". Why change
> that gratuitously?

I'm going to stop here, since I don't think you understand what I am
proposing, nor how the current system works: this makes is extremely
difficult to describe changes, and time consuming.

Sorry,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

[PATCH] Restore module support.

[Roman Zippel]

Hi,

On Tue, 4 Feb 2003, Rusty Russell wrote:

> I'm going to stop here, since I don't think you understand what I am
> proposing, nor how the current system works: this makes is extremely
> difficult to describe changes, and time consuming.

Rusty, if you continue to ignore criticism, I have only one answer left:

http://www.xs4all.nl/~zippel/restore-modules-2.5.59.diff

These numbers are quite interesting:

$ diffstat restore-modules-2.5.59.diff
 arch/i386/kernel/Makefile          |    1
 arch/i386/kernel/cpu/mtrr/if.c     |    1
 arch/i386/kernel/entry.S           |    6
 arch/i386/kernel/module.c          |  111 -
 arch/i386/mm/extable.c             |    1
 drivers/char/agp/Makefile          |    2
 drivers/char/misc.c                |    1                                                                                                                  
 drivers/eisa/eisa-bus.c            |    1
 drivers/input/serio/serport.c      |    1                                                                                                                  
 fs/filesystems.c                   |   27
 fs/proc/proc_misc.c                |   12
 include/asm-generic/percpu.h       |    1
 include/asm-generic/vmlinux.lds.h  |   15
 include/asm-i386/module.h          |   57
 include/linux/init.h               |  130 -
 include/linux/module.h             |  817 ++++++------
 include/linux/moduleloader.h       |   43
 include/linux/moduleparam.h        |  126 -
 init/Kconfig                       |   40
 init/main.c                        |  109 +
 kernel/Makefile                    |    8
 kernel/extable.c                   |   41
 kernel/intermodule.c               |  182 --
 kernel/kallsyms.c                  |    5
 kernel/kmod.c                      |    2
 kernel/ksyms.c                     |    7
 kernel/module.c                    | 2448 +++++++++++++++++--------------------
 kernel/params.c                    |  336 -----
 net/ipv4/netfilter/ip_nat_helper.c |    2
 scripts/Makefile.modinst           |    8
 sound/sound_core.c                 |    1
 31 files changed, 1805 insertions(+), 2737 deletions(-)

$ size linux-2.5.59-org/vmlinux linux-2.5.59-mod/vmlinux
   text    data     bss     dec     hex filename
3403915  864229  338052 4606196  4648f4 linux-2.5.59-org/vmlinux
3361448  863393  342020 4566861  45af4d linux-2.5.59-mod/vmlinux

This patch still has modversion disabled, when Kai finishes the new 
modversion support, I'll add the support for it to modutils.

bye, Roman

Re: [PATCH] Restore module support.

[Greg KH]

On Fri, Feb 07, 2003 at 12:09:27AM +0100, Roman Zippel wrote:
> Hi,
> 
> On Tue, 4 Feb 2003, Rusty Russell wrote:
> 
> > I'm going to stop here, since I don't think you understand what I am
> > proposing, nor how the current system works: this makes is extremely
> > difficult to describe changes, and time consuming.
> 
> Rusty, if you continue to ignore criticism, I have only one answer left:
> 
> http://www.xs4all.nl/~zippel/restore-modules-2.5.59.diff

But what are the modutils numbers? :)

Come on, what Rusty did was the "right thing to do" and has made life
easier for all of the arch maintainers (or so says the ones that I've
talked to), and has made my life easier with regards to
MODULE_DEVICE_TABLE() logic, which will enable the /sbin/hotplug
scripts/binary to shrink a _lot_.

thanks,

gre gk-h

Re: [PATCH] Restore module support.

[Roman Zippel]

Hi,

On Thu, 6 Feb 2003, Greg KH wrote:

> But what are the modutils numbers? :)

There should be no real difference as I'd like to integrate Kai's patch too.

> Come on, what Rusty did was the "right thing to do" and has made life
> easier for all of the arch maintainers (or so says the ones that I've
> talked to), and has made my life easier with regards to
> MODULE_DEVICE_TABLE() logic, which will enable the /sbin/hotplug
> scripts/binary to shrink a _lot_.

What was the "right thing to do"?
There were certainly a few interesting changes, but I'd like discuss them 
first. For example there is more than one solution to improve the 
MODULE_DEVICE_TABLE() logic (*), so how is Rusty's better?

bye, Roman

(*) http://marc.theaimsgroup.com/?l=linux-kernel&m=104405265719327&w=2
    http://marc.theaimsgroup.com/?l=linux-kernel&m=104437966220610&w=2

Re: [PATCH] Restore module support.

[Greg KH]

On Fri, Feb 07, 2003 at 01:01:01AM +0100, Roman Zippel wrote:
> Hi,
> 
> On Thu, 6 Feb 2003, Greg KH wrote:
> 
> > But what are the modutils numbers? :)
> 
> There should be no real difference as I'd like to integrate Kai's patch too.

Ok, I'm confused, you're advocating putting back the old modutils
interface, but somehow not using the old modutils code?  I don't
understand.


> > Come on, what Rusty did was the "right thing to do" and has made life
> > easier for all of the arch maintainers (or so says the ones that I've
> > talked to), and has made my life easier with regards to
> > MODULE_DEVICE_TABLE() logic, which will enable the /sbin/hotplug
> > scripts/binary to shrink a _lot_.
> 
> What was the "right thing to do"?
> There were certainly a few interesting changes, but I'd like discuss them 
> first. For example there is more than one solution to improve the 
> MODULE_DEVICE_TABLE() logic (*), so how is Rusty's better?

Neither one of those proposals, no any others, were backed with working
examples.  Rusty had the only working example of getting rid of the
userspace knowledge of the kernel data structures that I know of so far.

thanks,

greg k-h

Re: [PATCH] Restore module support.

[Roman Zippel]

Hi,

On Thu, 6 Feb 2003, Greg KH wrote:

> > There should be no real difference as I'd like to integrate Kai's patch too.
> 
> Ok, I'm confused, you're advocating putting back the old modutils
> interface, but somehow not using the old modutils code?  I don't
> understand.

No, I'm advocating to break as little as possible. I'm certainly willing 
to port any interesting feature from Rusty's patches. If one feature 
requires changes to modutils, that's fine.

> Neither one of those proposals, no any others, were backed with working
> examples.  Rusty had the only working example of getting rid of the
> userspace knowledge of the kernel data structures that I know of so far.

1. In the past I posted enough example code, which was pretty much 
ignored, why should I think it would be any different this time?
2. Is hotplug that broken, that it wouldn't survive 2.6, so it required a 
complete new implementation? If that should be case I herewith volunteer 
to add module alias support to modutils.

bye, Roman

Re: [PATCH] Restore module support.

[Roman Zippel]

Hi,

On Thu, 6 Feb 2003, Greg KH wrote:

> Neither one of those proposals, no any others, were backed with working
> examples.  Rusty had the only working example of getting rid of the
> userspace knowledge of the kernel data structures that I know of so far.

3. Somehow I hoped we could discuss this on a technical base, I didn't 
know we've reached already the "first post" level.
SCNR :)

bye, Roman

Re: [PATCH] Restore module support.

[Russell King]

On Thu, Feb 06, 2003 at 03:25:15PM -0800, Greg KH wrote:
> Come on, what Rusty did was the "right thing to do" and has made life
> easier for all of the arch maintainers (or so says the ones that I've
> talked to)

And I'll promptly provide you with the other view.  I'm still trying to
sort out the best thing to do for ARM.  We have the choice of:

1. load modules in the vmalloc region and build two jump tables, one for
   the init text and one for the core text.

2. fix vmalloc and /proc/kcore to be able to cope with a separate module
   region located below PAGE_OFFSET.  Currently, neither play well with
   this option.

(1) has the advantage that it's all architecture code, its what we've
done with the old modutils, and I've finally managed to implement it.
However, it introduces an extra instruction and data cache line fetch
to branches from modules into the kernel text.

(2) has the disadvantage that its touching non-architecture specific
code, but this is the option I'd prefer due to the obvious performance
advantage.  However, I'm afraid that it isn't worth the effort to fix
up vmalloc and /proc/kcore.  vmalloc fix appears simple, but /proc/kcore
has issues (anyone know what KCORE_BASE is all about?)

I've not made up my mind which option I'm going to take.  If I don't get
around to fixing /proc/kcore by this weekend, I'll probably just throw
option (1) at Linus, which bring Linus' tree back to a buildable state
for some ARM targets again.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Restore module support.

[Rusty Russell]

In message <20030207001006.A19306@flint.arm.linux.org.uk> you write:
> And I'll promptly provide you with the other view.  I'm still trying to
> sort out the best thing to do for ARM.  We have the choice of:
> 
> 1. load modules in the vmalloc region and build two jump tables, one for
>    the init text and one for the core text.

Yes.  PPC and PPC64 have the same issues: currently this is done by
(1) putting nothing in the .init sections (on PPC64), and (2) with
stubs when jumping outside the module code.

This gives the same effect as the previous userspace loader: for PPC64
noone cares about discarding init stuff, so it's firmly on the TODO
list.  ARM's priorities are obviously different.

> 2. fix vmalloc and /proc/kcore to be able to cope with a separate module
>    region located below PAGE_OFFSET.  Currently, neither play well with
>    this option.

x86_64 has this, as does sparc64: they do their own allocation.  Does
ARM require something special in this regard?  I'd love to see what
you've got...

Thanks!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] Restore module support.

[Russell King]

On Fri, Feb 07, 2003 at 03:53:44PM +1100, Rusty Russell wrote:
> Yes.  PPC and PPC64 have the same issues: currently this is done by
> (1) putting nothing in the .init sections (on PPC64), and (2) with
> stubs when jumping outside the module code.
> 
> This gives the same effect as the previous userspace loader: for PPC64
> noone cares about discarding init stuff, so it's firmly on the TODO
> list.  ARM's priorities are obviously different.

As I say, I have this solution working, but its suboptimal, and I'll
probably push this Linus-wards if I can't resolve (2) soon.

> > 2. fix vmalloc and /proc/kcore to be able to cope with a separate module
> >    region located below PAGE_OFFSET.  Currently, neither play well with
> >    this option.
> 
> x86_64 has this, as does sparc64: they do their own allocation.  Does
> ARM require something special in this regard?  I'd love to see what
> you've got...

There are two problems - one I mentioned during on LKML recently:

  Message-ID: <20030131102013.A19646@flint.arm.linux.org.uk>

This seems simple to resolve.  We just need to make get_vm_area() ignore
mappings for invalid areas:

--- orig/mm/vmalloc.c	Tue Nov  5 12:51:41 2002
+++ linux/mm/vmalloc.c	Fri Feb  7 09:48:42 2003
@@ -210,6 +210,8 @@
 
 	write_lock(&vmlist_lock);
 	for (p = &vmlist; (tmp = *p) ;p = &tmp->next) {
+		if (tmp->addr < addr)
+			continue;
 		if ((size + addr) < addr)
 			goto out;
 		if (size + addr <= (unsigned long)tmp->addr)

Since the vmlist is an ordered list, and we place the modules below
VMALLOC_START, this change ensures that we will completely ignore any
vmlist entries below the current minimum address (addr) we're looking
for.

/proc/kcore currently assumes that:

1. all vmlist mappings are above PAGE_OFFSET.
2. all vmlist mappings are within VMALLOC_START to VMALLOC_END

Looking at fs/proc/kcore.c this morning, I have a couple of ideas to
solve this problem.  Patch will follow later today, hopefully without
any ifdefs.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] Restore module support.

[Kai Germaschewski]

On Fri, 7 Feb 2003, Russell King wrote:

> On Thu, Feb 06, 2003 at 03:25:15PM -0800, Greg KH wrote:
> > Come on, what Rusty did was the "right thing to do" and has made life
> > easier for all of the arch maintainers (or so says the ones that I've
> > talked to)
> 
> And I'll promptly provide you with the other view.  I'm still trying to
> sort out the best thing to do for ARM.  We have the choice of:
> 
> 1. load modules in the vmalloc region and build two jump tables, one for
>    the init text and one for the core text.
> 
> 2. fix vmalloc and /proc/kcore to be able to cope with a separate module
>    region located below PAGE_OFFSET.  Currently, neither play well with
>    this option.

So you have the choice of either sticking to the solution which was 
previously used (only that it's now done in the kernel, not in modutils), 
or doing something new and more efficient.

Now, what's the reason you're not happy with that? You've got more
flexibility than before, and you can even switch between different ways
without having to teach an external package about it, so you avoid the
compatibility issues when kernel and modutils are not in sync.

--Kai

Re: [PATCH] Restore module support.

[Roman Zippel]

Hi,

On Fri, 7 Feb 2003, Kai Germaschewski wrote:

> So you have the choice of either sticking to the solution which was 
> previously used (only that it's now done in the kernel, not in modutils), 
> or doing something new and more efficient.

Where is the problem to do the "new and more efficient" in modutils?

> Now, what's the reason you're not happy with that? You've got more
> flexibility than before, and you can even switch between different ways
> without having to teach an external package about it, so you avoid the
> compatibility issues when kernel and modutils are not in sync.

Where is the problem with updating user space tools? We should certainly 
reduce dependencies, but moving everything into the kernel source can't be 
the answer either.

bye, Roman