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

libkmod: Improve index dump performance #250

Closed
wants to merge 1 commit into from
Closed

libkmod: Improve index dump performance #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 16 additions & 16 deletions libkmod/libkmod-index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@
return NULL;
}

static void index_dump_node(struct index_node_f *node, struct strbuf *buf, int fd)
static void index_dump_node(struct index_node_f *node, struct strbuf *buf, FILE *fp)

Check warning on line 363 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L363 

Added line #L363 was not covered by tests
{
struct index_value *v;
size_t pushed;
Expand All @@ -369,10 +369,10 @@
pushed = strbuf_pushchars(buf, node->prefix);

for (v = node->values; v != NULL; v = v->next) {
write_str_safe(fd, buf->bytes, buf->used);
write_str_safe(fd, " ", 1);
write_str_safe(fd, v->value, strlen(v->value));
write_str_safe(fd, "\n", 1);
fwrite(buf->bytes, 1, buf->used, fp);
fputc(' ', fp);
fputs(v->value, fp);
fputc('\n', fp);

Check warning on line 375 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L372-L375 

Added lines #L372 - L375 were not covered by tests
}

for (ch = node->first; ch <= node->last; ch++) {
Expand All @@ -382,7 +382,7 @@
continue;

if (strbuf_pushchar(buf, ch)) {
index_dump_node(child, buf, fd);
index_dump_node(child, buf, fp);

Check warning on line 385 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L385 

Added line #L385 was not covered by tests
strbuf_popchar(buf);
}
}
Expand All @@ -391,7 +391,7 @@
index_close(node);
}

void index_dump(struct index_file *in, int fd, bool alias_prefix)
void index_dump(struct index_file *in, FILE *fp, bool alias_prefix)

Check warning on line 394 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L394 

Added line #L394 was not covered by tests
{
struct index_node_f *root;
struct strbuf buf;
Expand All @@ -402,7 +402,7 @@

strbuf_init(&buf);
if (!alias_prefix || strbuf_pushchars(&buf, "alias "))
index_dump_node(root, &buf, fd);
index_dump_node(root, &buf, fp);

Check warning on line 405 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L405 

Added line #L405 was not covered by tests
strbuf_release(&buf);
}

Expand Down Expand Up @@ -821,7 +821,7 @@
return NULL;
}

static void index_mm_dump_node(struct index_mm_node *node, struct strbuf *buf, int fd)
static void index_mm_dump_node(struct index_mm_node *node, struct strbuf *buf, FILE *fp)

Check warning on line 824 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L824 

Added line #L824 was not covered by tests
{
const void *p;
size_t i, pushed;
Expand All @@ -833,10 +833,10 @@
struct index_mm_value v;

read_value_mm(&p, &v);
write_str_safe(fd, buf->bytes, buf->used);
write_str_safe(fd, " ", 1);
write_str_safe(fd, v.value, v.len);
write_str_safe(fd, "\n", 1);
fwrite(buf->bytes, 1, buf->used, fp);
fputc(' ', fp);
fwrite(v.value, 1, v.len, fp);
fputc('\n', fp);

Check warning on line 839 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L836-L839 

Added lines #L836 - L839 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity: have you tried placing the contents into a buffer (on the stack) and doing a single write for given value instead of 4? Does it make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This already helps by almost 50 %, which is nice. At first I tried scratchbuf (and soon strbuf), but we could also keep a buffer around for longer. See #252 as an example.

}

for (ch = node->first; ch <= node->last; ch++) {
Expand All @@ -847,15 +847,15 @@
continue;

if (strbuf_pushchar(buf, ch)) {
index_mm_dump_node(child, buf, fd);
index_mm_dump_node(child, buf, fp);

Check warning on line 850 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L850 

Added line #L850 was not covered by tests
strbuf_popchar(buf);
}
}

strbuf_popchars(buf, pushed);
}

void index_mm_dump(const struct index_mm *idx, int fd, bool alias_prefix)
void index_mm_dump(const struct index_mm *idx, FILE *fp, bool alias_prefix)

Check warning on line 858 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L858 

Added line #L858 was not covered by tests
{
struct index_mm_node nbuf, *root;
struct strbuf buf;
Expand All @@ -866,7 +866,7 @@

strbuf_init(&buf);
if (!alias_prefix || strbuf_pushchars(&buf, "alias "))
index_mm_dump_node(root, &buf, fd);
index_mm_dump_node(root, &buf, fp);

Check warning on line 869 in libkmod/libkmod-index.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod-index.c#L869 

Added line #L869 was not covered by tests
strbuf_release(&buf);
}

Expand Down
4 changes: 2 additions & 2 deletions libkmod/libkmod-index.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ struct index_file;
struct index_file *index_file_open(const char *filename);
void index_file_close(struct index_file *idx);
char *index_search(struct index_file *idx, const char *key);
void index_dump(struct index_file *in, int fd, bool alias_prefix);
void index_dump(struct index_file *in, FILE *fp, bool alias_prefix);
struct index_value *index_searchwild(struct index_file *idx, const char *key);

void index_values_free(struct index_value *values);
Expand All @@ -31,4 +31,4 @@ int index_mm_open(const struct kmod_ctx *ctx, const char *filename,
void index_mm_close(struct index_mm *index);
char *index_mm_search(const struct index_mm *idx, const char *key);
struct index_value *index_mm_searchwild(const struct index_mm *idx, const char *key);
void index_mm_dump(const struct index_mm *idx, int fd, bool alias_prefix);
void index_mm_dump(const struct index_mm *idx, FILE *fp, bool alias_prefix);
33 changes: 26 additions & 7 deletions libkmod/libkmod.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,22 +810,37 @@

KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type, int fd)
{
int err = 0, fd2;
FILE *fp;

if (ctx == NULL)
return -ENOSYS;

fd2 = dup(fd);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT libraries should really be using CLOEXEC, aka fcntl(fd, F_DUPFD_CLOEXEC...) or alike.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fdopen would set ´O_CLOEXEC. Unfortunately I cannot simply use dup3`, which would do it atomically for us, because then I would have to figure out a free fd first (or open one just to reserve it).

if (fd2 == -1)
return -errno;

Check warning on line 821 in libkmod/libkmod.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod.c#L821 

Added line #L821 was not covered by tests
fp = fdopen(fd2, "ae");
if (fp == NULL) {
err = -errno;
close(fd2);
return err;

Check warning on line 826 in libkmod/libkmod.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod.c#L824-L826 

Added lines #L824 - L826 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move all the whole dup/fdopen hunk after all the input validation - aka after the type check below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would adjust it if we decide if it's even worth it:

  • FILE loses EINTR handling (modprobe -c shouldn't really care, since it first uses FILE and then passes a file descriptor to library, but let's not break API and whatever other users might rely on)
  • wbuf speeds up processing, but introduces custom code, not improving the error handling

So ... let's see. I wouldn't mind keeping everything as it is right now and, if we ever adjust the API in a release, reconsider turning it FILE-based.

Copy link
Collaborator

Choose a reason for hiding this comment

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

FILE loses EINTR handling

My slight inclination towards a wbuf-like approach got a whole lot stronger.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that. What we could have is one additional API that receives a FILE* as argument.... we are not really going to change the API and break users because of this.

However if we add the new API, then we don't drop the old code since converting it to use FILE* would mean to lose EINTR handling. So.... I'm leaning towards the wbuf approach.

}

#if defined(__clang__)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wtautological-unsigned-enum-zero-compare"
#endif
if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
return -ENOENT;
if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE) {
err = -ENOENT;
goto done;

Check warning on line 835 in libkmod/libkmod.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod.c#L834-L835 

Added lines #L834 - L835 were not covered by tests
}
#if defined(__clang__)
#pragma clang diagnostic pop
#endif

if (ctx->indexes[type] != NULL) {
DBG(ctx, "use mmapped index '%s'\n", index_files[type].fn);
index_mm_dump(ctx->indexes[type], fd, index_files[type].alias_prefix);
index_mm_dump(ctx->indexes[type], fp, index_files[type].alias_prefix);

Check warning on line 843 in libkmod/libkmod.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod.c#L843 

Added line #L843 was not covered by tests
} else {
char fn[PATH_MAX];
struct index_file *idx;
Expand All @@ -835,14 +850,18 @@
DBG(ctx, "file=%s\n", fn);

idx = index_file_open(fn);
if (idx == NULL)
return -ENOSYS;
if (idx == NULL) {
err = -ENOSYS;
goto done;
}

index_dump(idx, fd, index_files[type].alias_prefix);
index_dump(idx, fp, index_files[type].alias_prefix);

Check warning on line 858 in libkmod/libkmod.c

View check run for this annotation

Codecov / codecov/patch

libkmod/libkmod.c#L858 

Added line #L858 was not covered by tests
index_file_close(idx);
}

return 0;
done:
fclose(fp);
return err;
}

const struct kmod_config *kmod_get_config(const struct kmod_ctx *ctx)
Expand Down
26 changes: 0 additions & 26 deletions shared/util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,32 +237,6 @@ ssize_t read_str_safe(int fd, char *buf, size_t buflen)
return done;
}

ssize_t write_str_safe(int fd, const char *buf, size_t buflen)
{
size_t todo = buflen;
size_t done = 0;

assert_cc(EAGAIN == EWOULDBLOCK);

do {
ssize_t r = write(fd, buf + done, todo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did we remove the helper instead of adapting it to use fwrite()? AFAICT all the safety handling it does is still applicable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with FILE output is that fclose itself could still write data. And this is a one-shot approach. We cannot call it multiple times until the remaining data is successfully written.

I mistakenly believed that FILE itself handles short writes, but I was wrong. So if we want to keep interrupt-safe writes, we cannot use FILE here. At best, we would adjust the API, but ... Not in a simple PR. :)


if (r == 0)
break;
else if (r > 0) {
todo -= r;
done += r;
} else {
if (errno == EAGAIN || errno == EINTR)
continue;
else
return -errno;
}
} while (todo > 0);

return done;
}

int read_str_long(int fd, long *value, int base)
{
char buf[32], *end;
Expand Down
1 change: 0 additions & 1 deletion shared/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ _nonnull_all_ bool path_ends_with_kmod_ext(const char *path, size_t len);
_must_check_ _nonnull_(2) ssize_t pread_str_safe(int fd, char *buf, size_t buflen,
off_t off);
_must_check_ _nonnull_(2) ssize_t read_str_safe(int fd, char *buf, size_t buflen);
_nonnull_(2) ssize_t write_str_safe(int fd, const char *buf, size_t buflen);
_must_check_ _nonnull_(2) int read_str_long(int fd, long *value, int base);
_must_check_ _nonnull_(2) int read_str_ulong(int fd, unsigned long *value, int base);
_nonnull_(1) char *freadline_wrapped(FILE *fp, unsigned int *linenum);
Expand Down
29 changes: 0 additions & 29 deletions testsuite/test-util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,35 +162,6 @@ static int test_path_ends_with_kmod_ext(const struct test *t)
DEFINE_TEST(test_path_ends_with_kmod_ext,
.description = "check implementation of path_ends_with_kmod_ext()")

#define TEST_WRITE_STR_SAFE_FILE "/write-str-safe"
#define TEST_WRITE_STR_SAFE_PATH TESTSUITE_ROOTFS "test-util2/" TEST_WRITE_STR_SAFE_FILE
static int test_write_str_safe(const struct test *t)
{
const char *s = "test";
int fd;

fd = open(TEST_WRITE_STR_SAFE_FILE ".txt", O_CREAT | O_TRUNC | O_WRONLY, 0644);
assert_return(fd >= 0, EXIT_FAILURE);

write_str_safe(fd, s, strlen(s));
close(fd);

return EXIT_SUCCESS;
}
DEFINE_TEST(test_write_str_safe,
.description = "check implementation of write_str_safe()",
.config = {
[TC_ROOTFS] = TESTSUITE_ROOTFS "test-util2/",
},
.need_spawn = true,
.output = {
.files = (const struct keyval[]) {
{ TEST_WRITE_STR_SAFE_PATH ".txt",
TEST_WRITE_STR_SAFE_PATH "-correct.txt" },
{ },
},
});

static int test_uadd32_overflow(const struct test *t)
{
uint32_t res;
Expand Down