[PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Rasmus Villemoes]

A typical kernel image has hundreds of static struct of_device_id
instances (a lot of which are sentinel all-zeroes), each occupying
~200 bytes. Nobody initializes the .compatible member with strings
anywhere near 128 bytes, so a lot of that memory is simply wasted.

To verify, I first had the 0day bot chew on a patch adding a dummy
extremely long .compatible string, and I did get an email saying that
the patch resulted in lots of new
"warning:initializer-string-for-array-of-chars-is-too-long"
warnings. Then I had it chew on a version of this patch reducing to 46
(because gcc unfortunately does not warn when the literal sans the
terminating nul just fits), and got a SUCCESS mail listing 107
config/arch combinations.

For an arm imx_v6_v7_defconfig kernel, .rodata becomes 70K smaller;
.init.data shrinks by another ~13K, making the whole kernel image
about 83K, or 0.3%, smaller.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/mod_devicetable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 448621c32e4d..502629bdac2e 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -241,7 +241,7 @@ struct sdw_device_id {
 struct of_device_id {
 	char	name[32];
 	char	type[32];
-	char	compatible[128];
+	char	compatible[48];
 	const void *data;
 };
 
-- 
2.20.1

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Arnd Bergmann]

On Thu, Apr 25, 2019 at 10:31 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> A typical kernel image has hundreds of static struct of_device_id
> instances (a lot of which are sentinel all-zeroes), each occupying
> ~200 bytes. Nobody initializes the .compatible member with strings
> anywhere near 128 bytes, so a lot of that memory is simply wasted.
>
> To verify, I first had the 0day bot chew on a patch adding a dummy
> extremely long .compatible string, and I did get an email saying that
> the patch resulted in lots of new
> "warning:initializer-string-for-array-of-chars-is-too-long"
> warnings. Then I had it chew on a version of this patch reducing to 46
> (because gcc unfortunately does not warn when the literal sans the
> terminating nul just fits), and got a SUCCESS mail listing 107
> config/arch combinations.
>
> For an arm imx_v6_v7_defconfig kernel, .rodata becomes 70K smaller;
> .init.data shrinks by another ~13K, making the whole kernel image
> about 83K, or 0.3%, smaller.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

The space savings are nice, but I wonder if the format of these
structures is part of the ABI or not. I have some vague recollection
of that, but it's possible that it's no longer true in this century.

scripts/mod/file2alias.c processes the structures into a different
format and seems to be written specifically to avoid problems
with changes like the one you did. Can anyone confirm that
this is true before we apply the patch?

      Arnd

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Rasmus Villemoes]

On 26/04/2019 11.27, Arnd Bergmann wrote:
> On Thu, Apr 25, 2019 at 10:31 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> For an arm imx_v6_v7_defconfig kernel, .rodata becomes 70K smaller;
>> .init.data shrinks by another ~13K, making the whole kernel image
>> about 83K, or 0.3%, smaller.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> The space savings are nice, but I wonder if the format of these
> structures is part of the ABI or not. I have some vague recollection
> of that, but it's possible that it's no longer true in this century.
> 
> scripts/mod/file2alias.c processes the structures into a different
> format and seems to be written specifically to avoid problems
> with changes like the one you did. Can anyone confirm that
> this is true before we apply the patch?

I can't confirm it, of course, but I did do some digging around and
couldn't find anything other than file2alias, which as you mention is
prepared for such a change. I also couldn't find any specific reason for
the 128 (it's not a #define, so at least originally it didn't seem to be
tied to some external consumer) - Jeff, do you remember why you chose
that back when you did 5e6557722e69?

But we cannot really know whether there is some userspace tool that
parses the .ko ELF objects the same way that file2alias does, doing
pattern matching on the symbol names etc. I cannot see why anybody would
_do_ that (the in-tree infrastructure already generates the
MODULE_ALIAS() from which modules.alias gets generated), but the only
way of knowing, I think, is to try to apply the patch and see if anybody
complains.

Rasmus

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Jeff Mahoney]

On 5/2/19 5:41 AM, Rasmus Villemoes wrote:
> On 26/04/2019 11.27, Arnd Bergmann wrote:
>> On Thu, Apr 25, 2019 at 10:31 PM Rasmus Villemoes
>> <linux@rasmusvillemoes.dk> wrote:
>>>
>>> For an arm imx_v6_v7_defconfig kernel, .rodata becomes 70K smaller;
>>> .init.data shrinks by another ~13K, making the whole kernel image
>>> about 83K, or 0.3%, smaller.
>>>
>>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
>> The space savings are nice, but I wonder if the format of these
>> structures is part of the ABI or not. I have some vague recollection
>> of that, but it's possible that it's no longer true in this century.
>>
>> scripts/mod/file2alias.c processes the structures into a different
>> format and seems to be written specifically to avoid problems
>> with changes like the one you did. Can anyone confirm that
>> this is true before we apply the patch?
> 
> I can't confirm it, of course, but I did do some digging around and
> couldn't find anything other than file2alias, which as you mention is
> prepared for such a change. I also couldn't find any specific reason for
> the 128 (it's not a #define, so at least originally it didn't seem to be
> tied to some external consumer) - Jeff, do you remember why you chose
> that back when you did 5e6557722e69?

I had been wondering why I'd been included on this thread.  I completely 
forgot that I wrote this code nearly 15 years ago. :)

It was probably as simple as there not being a real limit for how long 
the compatible string could be and wanting to make it flexible.  I was 
targetting a powerpc mac notebook I had at the time -- not tight memory 
embedded systems, so sorry for that.

> But we cannot really know whether there is some userspace tool that
> parses the .ko ELF objects the same way that file2alias does, doing
> pattern matching on the symbol names etc. I cannot see why anybody would
> _do_ that (the in-tree infrastructure already generates the
> MODULE_ALIAS() from which modules.alias gets generated), but the only
> way of knowing, I think, is to try to apply the patch and see if anybody
> complains.

The size is part of the ABI, though.  module-init-tools has a copy of 
the same struct and uses that size to walk an array of of_device_id when 
a module as more than one.  If you shrink it, that will certainly break.

file2alias does the right things only because it's tightly coupled to 
the kernel version it's being used with.  It still directly accesses the 
structure definitions in the headers.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Rasmus Villemoes]

On 02/05/2019 14.29, Jeff Mahoney wrote:
> On 5/2/19 5:41 AM, Rasmus Villemoes wrote:

>> But we cannot really know whether there is some userspace tool that
>> parses the .ko ELF objects the same way that file2alias does, doing
>> pattern matching on the symbol names etc. I cannot see why anybody would
>> _do_ that (the in-tree infrastructure already generates the
>> MODULE_ALIAS() from which modules.alias gets generated), but the only
>> way of knowing, I think, is to try to apply the patch and see if anybody
>> complains.
> 
> The size is part of the ABI, though.  module-init-tools has a copy of
> the same struct and uses that size to walk an array of of_device_id when
> a module as more than one.  If you shrink it, that will certainly break.

Urgh, yes, didn't know about module-init-tools. But it seems to be
abandoned? So does anybody actually use that with a modern kernel (there
seems to be many new structs in mod_devicetable.h that the last release
of module-init-tools doesn't know about)?

Oh well. If it's not deemed worth the risk to do a release with this
patch applied, I can always just add this to the list of trivial things
to do when asked to trim a custom kernel.

Rasmus

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Enrico Weigelt, metux IT consult]

On 25.04.19 22:31, Rasmus Villemoes wrote:

Hi,

> A typical kernel image has hundreds of static struct of_device_id
> instances (a lot of which are sentinel all-zeroes), each occupying
> ~200 bytes. Nobody initializes the .compatible member with strings
> anywhere near 128 bytes, so a lot of that memory is simply wasted.

I just wonder whether it has to be a fixed size array at all, instead
of an const char* pointer. Using a pointer should, IMHO, offer even
more savings while not having the size limit at all.

Is that struct copied as-is somewhere ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH] mod_devicetable.h: reduce sizeof(struct of_device_id) by 80 bytes

[Rasmus Villemoes]

On 26/04/2019 13.05, Enrico Weigelt, metux IT consult wrote:
> On 25.04.19 22:31, Rasmus Villemoes wrote:
> 
> Hi,
> 
>> A typical kernel image has hundreds of static struct of_device_id
>> instances (a lot of which are sentinel all-zeroes), each occupying
>> ~200 bytes. Nobody initializes the .compatible member with strings
>> anywhere near 128 bytes, so a lot of that memory is simply wasted.
> 
> I just wonder whether it has to be a fixed size array at all, instead
> of an const char* pointer. Using a pointer should, IMHO, offer even
> more savings while not having the size limit at all.

I did consider that, but file2alias reads the struct directly from the
object files, so it would have to be taught to apply relocations and
find the string literal somewhere else. Also, I think there's in-tree
code that does "foo->compatible[0]" to test if the .compatible member
was initialized with something; that would obviously crash if it's a
pointer rather than an embedded array.

One thing that might be doable, though requiring quite a bit of work, is
getting rid of the silly use of sentinels (quite a lot of the
of_device_id arrays have just two elements, so the sentinels are
responsible for perhaps 40% of the total memory use) - file2alias
already reads the st_size of the symbol and does a "is that a multiple
of sizeof(foo)", and "are the last sizeof(foo) bytes all-zeroes" - it's
obviously trivial to drop the last test, and process all elements
(though ignoring an all-zero element, so we can delete sentinels slowly
throughout the tree). That leaves in-tree uses of the of_device_id
arrays, where one would need a way to forward the ARRAY_SIZE(), yet
continue to stop at an all-zeroes element.

Rasmus