modules, "modules" and CONFIG_LIST_SORT

modules, "modules" and CONFIG_LIST_SORT

[Alexey Dobriyan]

There is unhealthy tendency lately which manifests itself as ifdefing
parts of core kernel code which are only used by modular code.
Example:

	commit a069c266ae5fdfbf5b4aecf2c672413aa33b2504
	lib: build list_sort() only if needed

	Build list_sort() only for configs that need it -- those that don't save
	~581 bytes (i386).

	+obj-$(CONFIG_LIST_SORT) += list_sort.o

Usually it's done to save tiny amount of code.

Unpleasant side effect of the change is that some modules stop being
true modules, i. e. admin is unable to start using them without reboot
if kernel was compiled without that tiny amount of core kernel.

Having used this feature several times, I think it'd be correct
to preserve this behaviour, at least not regress for those modules
which benefitted from it. For modules which were always "modules" (ipv6)
it's fine to continue.

Can we declare some policy about it?

And revert LIST_SORT commit if yes.

Regardless, can list_sort people fix this:

	+ make ARCH=s390 CROSS_COMPILE=s390-ibm-linux-gnu- O=../obj/s390-32-smp-n-debug-n KCONFIG_ALLCONFIG=/src/config/s390-32-smp-n-debug-n KBUILD_MODPOST_NOFINAL=1
	GEN     /src/obj/s390-32-smp-n-debug-n/Makefile
	scripts/kconfig/conf -s arch/s390/Kconfig
	Using /src/linux-1 as source for kernel
	GEN     /src/obj/s390-32-smp-n-debug-n/Makefile
	CHK     include/linux/version.h
	CHK     include/generated/utsrelease.h
	CALL    /src/linux-1/scripts/checksyscalls.sh
	CHK     include/generated/compile.h
	GZIP    kernel/config_data.gz
	IKCFG   kernel/config_data.h
	CC [M]  kernel/configs.o
	CHK     include/linux/version.h
	make[3]: `scripts/unifdef' is up to date.
	make[2]: `arch/s390/boot/image' is up to date.
	make[3]: `arch/s390/boot/compressed/vmlinux' is up to date.
	Building modules, stage 2.
	MODPOST 672 modules
	ERROR: "list_sort" [fs/xfs/xfs.ko] undefined!

commit a069c266ae5fdfbf5b4aecf2c672413aa33b2504
Author: Don Mullis <don.mullis@gmail.com>
Date:   Fri Mar 5 13:43:16 2010 -0800

    lib: build list_sort() only if needed
    
    Build list_sort() only for configs that need it -- those that don't save
    ~581 bytes (i386).
    
    Signed-off-by: Don Mullis <don.mullis@gmail.com>
    Cc: Dave Airlie <airlied@redhat.com>
    Cc: Andi Kleen <andi@firstfloor.org>
    Cc: Dave Chinner <david@fromorbit.com>
    Cc: Artem Bityutskiy <dedekind@infradead.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 305c590..3d2ab03 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -9,6 +9,7 @@ menuconfig DRM
 	depends on (AGP || AGP=n) && PCI && !EMULATED_CMPXCHG && MMU
 	select I2C
 	select I2C_ALGOBIT
+	select LIST_SORT
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/fs/ubifs/Kconfig b/fs/ubifs/Kconfig
index 830e3f7..430c69f 100644
--- a/fs/ubifs/Kconfig
+++ b/fs/ubifs/Kconfig
@@ -7,6 +7,7 @@ config UBIFS_FS
 	select CRYPTO if UBIFS_FS_ZLIB
 	select CRYPTO_LZO if UBIFS_FS_LZO
 	select CRYPTO_DEFLATE if UBIFS_FS_ZLIB
+	select LIST_SORT
 	depends on MTD_UBI
 	help
 	  UBIFS is a file system for flash devices which works on top of UBI.
diff --git a/lib/Kconfig b/lib/Kconfig
index 97b136f..8034c46 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -160,6 +160,9 @@ config TEXTSEARCH_BM
 config TEXTSEARCH_FSM
 	tristate
 
+config LIST_SORT
+	boolean
+
 config HAS_IOMEM
 	boolean
 	depends on !NO_IOMEM
diff --git a/lib/Makefile b/lib/Makefile
index 3b0b4a6..e39c361 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -21,7 +21,7 @@ lib-y	+= kobject.o kref.o klist.o
 
 obj-y += bcd.o div64.o sort.o parser.o halfmd4.o debug_locks.o random32.o \
 	 bust_spinlocks.o hexdump.o kasprintf.o bitmap.o scatterlist.o \
-	 string_helpers.o gcd.o list_sort.o
+	 string_helpers.o gcd.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
@@ -40,6 +40,7 @@ lib-$(CONFIG_GENERIC_FIND_FIRST_BIT) += find_next_bit.o
 lib-$(CONFIG_GENERIC_FIND_NEXT_BIT) += find_next_bit.o
 obj-$(CONFIG_GENERIC_FIND_LAST_BIT) += find_last_bit.o
 obj-$(CONFIG_GENERIC_HWEIGHT) += hweight.o
+obj-$(CONFIG_LIST_SORT) += list_sort.o
 obj-$(CONFIG_LOCK_KERNEL) += kernel_lock.o
 obj-$(CONFIG_DEBUG_PREEMPT) += smp_processor_id.o
 obj-$(CONFIG_DEBUG_LIST) += list_debug.o

Re: modules, "modules" and CONFIG_LIST_SORT

[Linus Torvalds]

On Sun, 7 Mar 2010, Alexey Dobriyan wrote:
> 
> Unpleasant side effect of the change is that some modules stop being
> true modules, i. e. admin is unable to start using them without reboot
> if kernel was compiled without that tiny amount of core kernel.
> 
> Having used this feature several times, I think it'd be correct
> to preserve this behaviour, at least not regress for those modules
> which benefitted from it. For modules which were always "modules" (ipv6)
> it's fine to continue.
> 
> Can we declare some policy about it?
> 
> And revert LIST_SORT commit if yes.

Yeah, I think that in cases like this, you have a very good argument: 
LIST_SORT enables code that isn't that large, and is clearly very generic.

And changing the config later and trying to compile and install a module 
is rather sane. And if that new module needs LIST_SORT, you're screwed 
because it didn't get compiled in originally.

Honestly, personally I'd rather have a real library that modules can link 
to _before_ even loading into kernel space, but that's not how we've 
traditionally done things. So I guess we should just revert that commit.

		Linus

Re: modules, "modules" and CONFIG_LIST_SORT

[Randy Dunlap]

On 03/07/10 03:23, Linus Torvalds wrote:
> 
> 
> On Sun, 7 Mar 2010, Alexey Dobriyan wrote:
>>
>> Unpleasant side effect of the change is that some modules stop being
>> true modules, i. e. admin is unable to start using them without reboot
>> if kernel was compiled without that tiny amount of core kernel.
>>
>> Having used this feature several times, I think it'd be correct
>> to preserve this behaviour, at least not regress for those modules
>> which benefitted from it. For modules which were always "modules" (ipv6)
>> it's fine to continue.
>>
>> Can we declare some policy about it?
>>
>> And revert LIST_SORT commit if yes.
> 
> Yeah, I think that in cases like this, you have a very good argument: 
> LIST_SORT enables code that isn't that large, and is clearly very generic.
> 
> And changing the config later and trying to compile and install a module 
> is rather sane. And if that new module needs LIST_SORT, you're screwed 
> because it didn't get compiled in originally.
> 
> Honestly, personally I'd rather have a real library that modules can link 
> to _before_ even loading into kernel space, but that's not how we've 
> traditionally done things. So I guess we should just revert that commit.

xfs also needs "select LIST_SORT".  I posted a patch for that a few days
ago and now Christoph Hellwig has asked me to send the patch directly to Linus,
but if Linus is going to revert the 'config LIST_SORT' patch, I'll skip it.

-- 
~Randy

Re: modules, "modules" and CONFIG_LIST_SORT

[Linus Torvalds]

On Sun, 7 Mar 2010, Randy Dunlap wrote:
> On 03/07/10 03:23, Linus Torvalds wrote:
> > 
> > Honestly, personally I'd rather have a real library that modules can link 
> > to _before_ even loading into kernel space, but that's not how we've 
> > traditionally done things. So I guess we should just revert that commit.
> 
> xfs also needs "select LIST_SORT".  I posted a patch for that a few days
> ago and now Christoph Hellwig has asked me to send the patch directly to Linus,
> but if Linus is going to revert the 'config LIST_SORT' patch, I'll skip it.

Ok, it's reverted in my tree now, I'll push out soon.

That said, I was serious about the "real library" thing. I do wonder if we 
should add a "link with standard kernel libraries" phase to the final 
module link, so that we can stop exporting silly things, and handle cases 
like this sanely without forcing it on everybody just because some module 
_might_ need it.

For example, I've always hated how we export the 'libgcc' kind of symbols 
too. I think we'd generally be _much_ better off just linking them into 
the module directly, even if that means that we'll have multiple copies. I 
hate the things like

	EXPORT_SYMBOL(__ashldi3);

that various architectures use. They have _nothing_ to do with real kernel 
interfaces, and are usually really small. And on several architectures a 
direct-linked call can be optimized by the linker in ways that external 
linkage cannot, so it might even improve code in some cases.

				Linus