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.

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

depmod: Minor performance improvements #158

Closed
wants to merge 6 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 16 additions & 19 deletions tools/depmod.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,10 +265,9 @@ static int index_insert(struct index_node *node, const char *key, const char *va
struct index_node *n;

/* New child is copy of node with prefix[j+1..N] */
n = calloc(1, sizeof(struct index_node));
n = memdup(node, sizeof(struct index_node));
if (n == NULL)
fatal_oom();
memcpy(n, node, sizeof(struct index_node));
n->prefix = strdup(&prefix[j + 1]);
if (n->prefix == NULL)
fatal_oom();
Expand Down Expand Up @@ -1067,6 +1066,11 @@ static int depmod_module_add(struct depmod *depmod, struct kmod_module *kmod)
size_t uncrelpathlen = lastslash - mod->relpath + modnamesz +
strlen(KMOD_EXTENSION_UNCOMPRESSED);
mod->uncrelpath = memdup(mod->relpath, uncrelpathlen + 1);
if (mod->uncrelpath == NULL) {
err = -ENOMEM;
lucasdemarchi marked this conversation as resolved.
Show resolved Hide resolved
hash_del(depmod->modules_by_name, mod->modname);
goto fail;
}
mod->uncrelpath[uncrelpathlen] = '\0';
err = hash_add_unique(depmod->modules_by_uncrelpath, mod->uncrelpath, mod);
if (err < 0) {
Expand Down Expand Up @@ -1424,6 +1428,8 @@ static int depmod_modules_build_array(struct depmod *depmod)
if (err < 0)
return err;
}
if (depmod->modules.count >= UINT16_MAX)
return -ERANGE;

return 0;
}
Expand Down Expand Up @@ -1459,39 +1465,30 @@ static void depmod_modules_sort(struct depmod *depmod)
char line[PATH_MAX];
const char *order_file = "modules.order";
FILE *fp;
unsigned idx = 0, total = 0;
size_t idx = 0;
// all sorted modules shall have precedence
int i = -(int)depmod->modules.count;

fp = dfdopen(depmod->cfg->dirname, order_file, O_RDONLY, "r");
if (fp == NULL)
return;

while (fgets(line, sizeof(line), fp) != NULL) {
struct mod *mod;
size_t len = strlen(line);
idx++;
if (len == 0)
continue;
if (line[len - 1] != '\n') {
ERR("%s/%s:%u corrupted line misses '\\n'\n",
ERR("%s/%s:%zu corrupted line misses '\\n'\n",
depmod->cfg->dirname, order_file, idx);
goto corrupted;
}
}
total = idx + 1;
idx = 0;
fseek(fp, 0, SEEK_SET);
while (fgets(line, sizeof(line), fp) != NULL) {
size_t len = strlen(line);
struct mod *mod;

idx++;
if (len == 0)
continue;
line[len - 1] = '\0';

mod = hash_find(depmod->modules_by_uncrelpath, line);
if (mod == NULL)
if (mod == NULL || mod->sort_idx < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can the sort index be negative? Should we split that into a separate patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's negative if we encounter the line twice, i.e. someone entered the same line multiple times in input file. This could also allow to overflow any kind of counter.

In general, this value is supposed to be negative to be in front of all other indices written so far. So "unsigned int" makes a rather wrong impression on what's going on here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like a pre existing bug, which ideally should be a separate patch.

Copy link
Contributor Author

@stoeckmann stoeckmann Sep 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can strip it out of this commit, but now that you've mentioned it: I didn't notice that it was a bug before as well. It occurred to me because I was trying to think about how to break my new approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep it as it is, because otherwise we would have to keep track which line we have seen so far., bloating code which is removed with the next commit. So ... it's a nice bonus due to rewrite. ;)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't follow - what tracking/bloat are you referring to... Having the below as prep commit should be enough, no?

- if (mod == NULL)
+ if (mod == NULL || mod->sort_idx < 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, adding this AFTER my commit makes sense. I thought about adding this check to existing code, which would have been very complicated. Sorry for the noise. Adjusted.

continue;
mod->sort_idx = idx - total;
mod->sort_idx = i++;
}

array_sort(&depmod->modules, mod_cmp);
Expand Down Expand Up @@ -2661,7 +2658,7 @@ static int depmod_output(struct depmod *depmod, FILE *out)

static void depmod_add_fake_syms(struct depmod *depmod)
{
/* __this_module is magic inserted by kernel loader. */
/* __this_module is magically inserted by kernel loader. */
depmod_symbol_add(depmod, "__this_module", true, 0, NULL);
/* On S390, this is faked up too */
depmod_symbol_add(depmod, "_GLOBAL_OFFSET_TABLE_", true, 0, NULL);
Expand Down