Hey David, Rusty, Quentin
This commit:
------------------------------
commit d945b697d0eea5a811ec299c5f1a25889bb0242b
From: David Woodhouse <[email protected]>
Date: Tue, 16 Sep 2008 16:23:28 -0700
Subject: [PATCH] Automatic MODULE_ALIAS() for DMI match tables.
This makes modpost handle MODULE_DEVICE_TABLE(dmi, xxxx).
I had to change the string pointers in the match table to char arrays,
and picked a size of 79 bytes almost at random -- do we need to make it
bigger than that? I was a bit concerned about the 'bloat' this
introduces into the match tables, but they should all be __initdata so
it shouldn't matter too much.
(Actually, modpost does go through the relocations and look at most of
them; it wouldn't be impossible to make it handle string pointers -- but
doesn't seem to be worth the effort, since they're __initdata).
------------------------------
changed dmi_strmatch.substr from char * to char[79];
Changing it back to const char * would shrink an x86-64
defconfig more than 100KB.
$ size vmlinux.old vmlinux.new
text data bss dec hex filename
11941725 1825624 1085440 14852789 e2a2b5 vmlinux.old
11921172 1730648 1085440 14737260 e0df6c vmlinux.new
modpost has changed a bit since 2008, is it's time to change it back?
---
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 7ab00d6..66c4309 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -462,7 +462,7 @@ enum dmi_field {
struct dmi_strmatch {
unsigned char slot:7;
unsigned char exact_match:1;
- char substr[79];
+ const char *substr;
};
struct dmi_system_id {
On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
>
> changed dmi_strmatch.substr from char * to char[79];
>
> Changing it back to const char * would shrink an x86-64
> defconfig more than 100KB.
>
> $ size vmlinux.old vmlinux.new
> text data bss dec hex filename
> 11941725 1825624 1085440 14852789 e2a2b5 vmlinux.old
> 11921172 1730648 1085440 14737260 e0df6c vmlinux.new
>
> modpost has changed a bit since 2008, is it's time to change it back?
Does the match table stuff still work if you do that? I thought the
point in changing to an array was to make the table extraction do the
right thing because it can't follow pointers...
--
dwmw2
On Tue, 19 May 2015 07:46:58 +0100
David Woodhouse <[email protected]> wrote:
> On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
> >
> > changed dmi_strmatch.substr from char * to char[79];
> >
> > Changing it back to const char * would shrink an x86-64
> > defconfig more than 100KB.
> >
> > $ size vmlinux.old vmlinux.new
> > text data bss dec hex filename
> > 11941725 1825624 1085440 14852789 e2a2b5 vmlinux.old
> > 11921172 1730648 1085440 14737260 e0df6c vmlinux.new
What percentage of those are __initdata ?
> > modpost has changed a bit since 2008, is it's time to change it back?
>
> Does the match table stuff still work if you do that? I thought the
> point in changing to an array was to make the table extraction do the
> right thing because it can't follow pointers...
Can't follow, or can't currently follow. All the data needed appears to
exist.
If you are trying to build a compact kernel with no modules then you can
certainly make them pointers. A more serious and invasive fix would be to
make substr a u32 and replace the text in the match with a Rabin-Karp or
other rolling hash value, then do a rolling hash for the match. As we've
got zlib in the kernel I assume we have the hash to hand ?
The same could be applied to a lot of the other matchers although with
drastically reduced results as they don't have 320 bytes of match
bloating each single entry.
It does however mean something would have to do the substitutions at
compile time, either by writing the rolling hash as a recursive
preprocessor macro or having a tool in the path which is ugly.
Alan
On Tue, 2015-05-19 at 16:56 +0100, One Thousand Gnomes wrote:
> On Tue, 19 May 2015 07:46:58 +0100 David Woodhouse <[email protected]> wrote:
> > On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
> > > changed dmi_strmatch.substr from char * to char[79];
> > >
> > > Changing it back to const char * would shrink an x86-64
> > > defconfig more than 100KB.
> > >
> > > $ size vmlinux.old vmlinux.new
> > > text data bss dec hex filename
> > > 11941725 1825624 1085440 14852789 e2a2b5 vmlinux.old
> > > 11921172 1730648 1085440 14737260 e0df6c vmlinux.new
>
> What percentage of those are __initdata ?
old: 17 .init.data 000876b0 ffffffff81f9e000 0000000001f9e000 0139e000 2**12
new: 17 .init.data 000711b0 ffffffff81f9d000 0000000001f9d000 0139d000 2**12
.init.data: 0x876b0 - 0x711b0 = 0x16500 (91392)
vmlinux: 14852789 - 14737260 = 115529
so there's ~25KB delta that's not .init.data.
The longest DMI_MATCH substr I found was 40 chars, so
there's some value in reducing the substr size of 79
to something shorter like 47 to reduce 80*4=320 to
48*4=192 per use.
$ grep-2.5.4 -rP --include=*.[ch] "\bDMI_(?:EXACT_)?MATCH\s*\(\s*\w+\s*,\s*\"[^\"]*" *| \
grep -oh '"[^"]*"' | \
sed 's/"//g' | sort | uniq | \
awk '{print length($0), " ", $0}'| \
sort -rn | head -10
39 Matsushita Electric Industrial Co.,Ltd.
35 ASUS PR-DLS ACPI BIOS Revision 1010
34 Micro-Star International Co., Ltd.
34 Award Software International, Inc.
34 900X3C/900X3D/900X3E/900X4C/900X4D
34 370R4E/370R4V/370R5E/3570RE/370R5V
33 MICRO-STAR INTERNATIONAL CO., LTD
32 MV85010A.86A.0016.P07.0201251536
32 MO81010A.86A.0008.P04.0004170800
32 ASUS A7V ACPI BIOS Revision 1007
Changing substr from 79 to 47 reduces .init.data a bit
(different -next version)
old: 17 .init.data 00081718 ffffffff81f84000 0000000001f84000 01384000 2**12
new: 17 .init.data 00076598 ffffffff81f84000 0000000001f84000 01384000 2**12
.init.data: 0x81718 - 0x76598 = 0xb180 (45440)
Unless/until modpost is updated, maybe this patch is OK:
---
include/linux/mod_devicetable.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 3bfd567..279f1be 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -462,7 +462,11 @@ enum dmi_field {
struct dmi_strmatch {
unsigned char slot:7;
unsigned char exact_match:1;
- char substr[79];
+#ifdef CONFIG_MODULES
+ char substr[47];
+#else
+ const char *substr;
+#endif
};
struct dmi_system_id {
Joe Perches <[email protected]> writes:
> On Tue, 2015-05-19 at 16:56 +0100, One Thousand Gnomes wrote:
>> On Tue, 19 May 2015 07:46:58 +0100 David Woodhouse <[email protected]> wrote:
>> > On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
>> > > changed dmi_strmatch.substr from char * to char[79];
>> > >
>> > > Changing it back to const char * would shrink an x86-64
>> > > defconfig more than 100KB.
>> > >
>> > > $ size vmlinux.old vmlinux.new
>> > > text data bss dec hex filename
>> > > 11941725 1825624 1085440 14852789 e2a2b5 vmlinux.old
>> > > 11921172 1730648 1085440 14737260 e0df6c vmlinux.new
>>
>> What percentage of those are __initdata ?
>
> old: 17 .init.data 000876b0 ffffffff81f9e000 0000000001f9e000 0139e000 2**12
> new: 17 .init.data 000711b0 ffffffff81f9d000 0000000001f9d000 0139d000 2**12
>
> .init.data: 0x876b0 - 0x711b0 = 0x16500 (91392)
> vmlinux: 14852789 - 14737260 = 115529
>
> so there's ~25KB delta that's not .init.data.
>
> The longest DMI_MATCH substr I found was 40 chars, so
> there's some value in reducing the substr size of 79
> to something shorter like 47 to reduce 80*4=320 to
> 48*4=192 per use.
This patch is nice and trivial.
But it seems the file2alias code was rewritten in 2013 by Andreas Schwab
<[email protected]>, and SOB Michal Marek <[email protected]>, without
going through me. Annoying, since they had to hack it because people
screwed up mod_devicetable.h with arch-dependent layouts :(
I guess that means Michal is the maintainer now, so I've CC'd him.
Cheers,
Rusty.
> $ grep-2.5.4 -rP --include=*.[ch] "\bDMI_(?:EXACT_)?MATCH\s*\(\s*\w+\s*,\s*\"[^\"]*" *| \
> grep -oh '"[^"]*"' | \
> sed 's/"//g' | sort | uniq | \
> awk '{print length($0), " ", $0}'| \
> sort -rn | head -10
> 39 Matsushita Electric Industrial Co.,Ltd.
> 35 ASUS PR-DLS ACPI BIOS Revision 1010
> 34 Micro-Star International Co., Ltd.
> 34 Award Software International, Inc.
> 34 900X3C/900X3D/900X3E/900X4C/900X4D
> 34 370R4E/370R4V/370R5E/3570RE/370R5V
> 33 MICRO-STAR INTERNATIONAL CO., LTD
> 32 MV85010A.86A.0016.P07.0201251536
> 32 MO81010A.86A.0008.P04.0004170800
> 32 ASUS A7V ACPI BIOS Revision 1007
>
> Changing substr from 79 to 47 reduces .init.data a bit
> (different -next version)
>
> old: 17 .init.data 00081718 ffffffff81f84000 0000000001f84000 01384000 2**12
> new: 17 .init.data 00076598 ffffffff81f84000 0000000001f84000 01384000 2**12
>
> .init.data: 0x81718 - 0x76598 = 0xb180 (45440)
>
> Unless/until modpost is updated, maybe this patch is OK:
> ---
> include/linux/mod_devicetable.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 3bfd567..279f1be 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -462,7 +462,11 @@ enum dmi_field {
> struct dmi_strmatch {
> unsigned char slot:7;
> unsigned char exact_match:1;
> - char substr[79];
> +#ifdef CONFIG_MODULES
> + char substr[47];
> +#else
> + const char *substr;
> +#endif
> };
>
> struct dmi_system_id {
Dne 20.5.2015 v 13:19 Rusty Russell napsal(a):
> Joe Perches <[email protected]> writes:
>> On Tue, 2015-05-19 at 16:56 +0100, One Thousand Gnomes wrote:
>>> On Tue, 19 May 2015 07:46:58 +0100 David Woodhouse <[email protected]> wrote:
>>>> On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
>>>>> changed dmi_strmatch.substr from char * to char[79];
>>>>>
>>>>> Changing it back to const char * would shrink an x86-64
>>>>> defconfig more than 100KB.
As David already pointed out, this breaks modpost. Also, what makes the
dmi tables special? We use character arrays in other tables as well, to
make them self-contained for modpost.
> But it seems the file2alias code was rewritten in 2013 by Andreas Schwab
> <[email protected]>, and SOB Michal Marek <[email protected]>, without
> going through me. Annoying, since they had to hack it because people
> screwed up mod_devicetable.h with arch-dependent layouts :(
Oh, sorry about it.
> I guess that means Michal is the maintainer now, so I've CC'd him.
OK, fine, I can carry modpost patches in kbuid.git. I think I have
merged a few more besides the file2alias rework.
Michal
On Wed, 2015-05-20 at 15:25 +0800, Michal Marek wrote:
> what makes the dmi tables special?
size.
Michal Marek <[email protected]> writes:
> Dne 20.5.2015 v 13:19 Rusty Russell napsal(a):
>> Joe Perches <[email protected]> writes:
>>> On Tue, 2015-05-19 at 16:56 +0100, One Thousand Gnomes wrote:
>>>> On Tue, 19 May 2015 07:46:58 +0100 David Woodhouse <[email protected]> wrote:
>>>>> On Mon, 2015-05-18 at 17:07 -0700, Joe Perches wrote:
>>>>>> changed dmi_strmatch.substr from char * to char[79];
>>>>>>
>>>>>> Changing it back to const char * would shrink an x86-64
>>>>>> defconfig more than 100KB.
>
> As David already pointed out, this breaks modpost. Also, what makes
> the dmi tables special? We use character arrays in other tables as
> well, to make them self-contained for modpost.
Yes, but the patch I referred to merely shrunk it, or changed
it to a pointer in the !CONFIG_MODULES case.
modpost has gotten far more sophisticated, thanks mainly to the
init section detection code. So it now knows about relocations;
it would be possible to use the same infrastructure to decode
char *, and I think it might be worth it.
It'd be a nice trick if someone were to code it :)
>> But it seems the file2alias code was rewritten in 2013 by Andreas Schwab
>> <[email protected]>, and SOB Michal Marek <[email protected]>, without
>> going through me. Annoying, since they had to hack it because people
>> screwed up mod_devicetable.h with arch-dependent layouts :(
>
> Oh, sorry about it.
>
>> I guess that means Michal is the maintainer now, so I've CC'd him.
>
> OK, fine, I can carry modpost patches in kbuid.git. I think I have
> merged a few more besides the file2alias rework.
Sure, let's do that from now on; I was a bit surprised but I'm always
happy for others to do my work for me :)
Thanks,
Rusty.