Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid copying elf symbol names, cache elf_get_section_info results #210

Closed
wants to merge 8 commits into from
Closed

Avoid copying elf symbol names, cache elf_get_section_info results #210

wants to merge 8 commits into from

Conversation

evelikov
Copy link
Collaborator

Couple of tiny tweaks, which shave a ~4MB of memory and ~5% CPU cycles off a depmod run, although the actual perf difference is within the noise margin.

Grab whichever commit make sense.

OOC: any ideas when all .strtab started getting only __crc_ symbols? Going back to ~2011 and it seems to have always been the case.

stoeckmann
stoeckmann suggested changes Oct 24, 2024
View reviewed changes
libkmod/libkmod-elf.c Outdated Show resolved Hide resolved
libkmod/libkmod-elf.c Outdated Show resolved Hide resolved
stoeckmann
stoeckmann suggested changes Oct 25, 2024
View reviewed changes
libkmod/libkmod-elf.c Show resolved Hide resolved
@evelikov
Copy link
Collaborator Author

OOC: any ideas when all .strtab started getting only _crc symbols? Going back to ~2011 and it seems to have always been the case.

Grabbed a random 3.10 kernel and it rarely hits the fallback path, in contrary to my 5.15 one... Should probably check if we can swap the order, although my gut feeling is that it'll cause breakage.

The generated index files do vary across the two kernels. At a glance an extra crc16 module is inserted in a few more places and/or it's order is different. Such old kernels lack modules.order so I suspect this is intentional. Will add to the TODO of things to check properly.

@evelikov evelikov mentioned this pull request Oct 25, 2024
Open
@evelikov evelikov marked this pull request as draft November 6, 2024 14:56
@evelikov
libkmod: const annotate kmod_modversion::symbol
04d0528 
Signed-off-by: Emil Velikov <[email protected]>
@evelikov
libkmod: stop copying symbol names in kmod_elf_get_modversions()
490b96b 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
@evelikov
libkmod: check for trailing \0 in __ksymtab_strings
a5889d3 
As per the documentation (man 5 elf) the section must be null
terminated. Move the check further up and remove the no longer needed
code trying to workaround non-compliant instances.

Note: drop the erroneous +1 in the overflow (malloc size) calculation

Signed-off-by: Emil Velikov <[email protected]>
@evelikov
libkmod: stop copying symbol names in kmod_elf_get_symbols_symtab()
bfaeb40 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
@evelikov
libkmod: stop copying symbol names in kmod_elf_get_symbols()
b86b98b 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
@codecov Codecov
Copy link

codecov bot commented Nov 11, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 66.66667% with 25 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libkmod/libkmod-elf.c 66.66% 16 Missing and 9 partials ⚠️
Files with missing lines Coverage Δ
libkmod/libkmod-module.c 53.66% <ø> (ø)
libkmod/libkmod-elf.c 51.01% <66.66%> (ø)

@evelikov
libkmod: stop copying symbol names in kmod_elf_get_dependency_symbols()
3a91021 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
@evelikov
libkmod: rename kmod_elf_get_strings()
022a8b2 
Rename kmod_elf_get_strings() to kmod_elf_get_modinfo_strings() and fold
the section name within, instead of passing it as an argument.

This aligns better with the other kmod_elf function names and will allow
us to tweak the kmod_elf_get_section() handling with later commit.

Signed-off-by: Emil Velikov <[email protected]>
@evelikov evelikov marked this pull request as ready for review November 11, 2024 14:03
@evelikov
Copy link
Collaborator Author

v2: quit looping when found, ELFDBG print missing sections

Seemingly only ~20% of modules have __ksymtab_strings and most modules have dozen+ sections. So the extra check helps a tiny bit there. As a bonus the malloc -> calloc change is no longer needed.

stoeckmann
stoeckmann suggested changes Nov 11, 2024
View reviewed changes
libkmod/libkmod-elf.c Outdated Show resolved Hide resolved
@evelikov
libkmod: store common section off/size and use them
4a3d7dd 
Currently, we repeatedly loop over the elf headers looking for five well
known sections. Just do it once in kmod_elf_new() and reuse the data as
needed.

Note that not all sections are guaranteed to be available, so check and
ELFDBG print the ones which are missing.

v2: quit looping when found, ELFDBG print missing sections
v3: match the first section name, use a loop

Signed-off-by: Emil Velikov <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: const annotate kmod_modversion::symbol
b5c9f24 
Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: stop copying symbol names in kmod_elf_get_modversions()
069d314 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: check for trailing \0 in __ksymtab_strings
e5ef157 
As per the documentation (man 5 elf) the section must be null
terminated. Move the check further up and remove the no longer needed
code trying to workaround non-compliant instances.

Note: drop the erroneous +1 in the overflow (malloc size) calculation

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: stop copying symbol names in kmod_elf_get_symbols_symtab()
63e1039 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: stop copying symbol names in kmod_elf_get_symbols()
4c74e4d 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: stop copying symbol names in kmod_elf_get_dependency_symbols()
18567a8 
Since the caller already copies the strings as needed, we can just
point to the elf data directly.

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: rename kmod_elf_get_strings()
9099346 
Rename kmod_elf_get_strings() to kmod_elf_get_modinfo_strings() and fold
the section name within, instead of passing it as an argument.

This aligns better with the other kmod_elf function names and will allow
us to tweak the kmod_elf_get_section() handling with later commit.

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
lucasdemarchi pushed a commit that referenced this pull request Nov 15, 2024
@evelikov @lucasdemarchi
libkmod: store common section off/size and use them
16ea23b 
Currently, we repeatedly loop over the elf headers looking for five well
known sections. Just do it once in kmod_elf_new() and reuse the data as
needed.

Note that not all sections are guaranteed to be available, so check and
ELFDBG print the ones which are missing.

v2: quit looping when found, ELFDBG print missing sections
v3: match the first section name, use a loop

Signed-off-by: Emil Velikov <[email protected]>
Reviewed-by: Tobias Stoeckmann <[email protected]>
Link: #210
Signed-off-by: Lucas De Marchi <[email protected]>
@lucasdemarchi
Copy link
Contributor

Applied, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stoeckmann stoeckmann stoeckmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evelikov @lucasdemarchi @stoeckmann