12:21 a.m.
Currently the sequence for symbol_search to search a symbol is: 1) kernel
symname hash table, 2) iterate all kernel symbols, 3) iterate all kernel
modules and their symbols. In the worst case, if a non-exist symbol been
searched, all 3 stages will be went through. The time consuming status for
each stage is like:
stage 1 stage 2 stage 3
0.007000(ms) 0.593000(ms) 2.421000(ms)
stage 3 takes too much time when comparing to stage 1. This patch introduces
a symname hash table for kernel modules, to improve the performance of symbol
searching.
Functions symbol_search and symbol_exists are fundamental and widely used by
other crash functions, thus the benefit of performance improvement can
get accumulated. For example, "ps -m" and "irq" commands, which call
the functions many times, will become faster with the patch.
v5 -> v6:
1) Add mod_symname_hash table dump to help -s
2) Modified mod_symname_hash install/remove based on Kazu's suggestion.
Tao Liu (7):
Implement install and remove operations for mod_symname_hash
Integrate symbol_search with mod_symname_hash search
Extend symname_hash_search with hash table select
Intergrate symbol_exists with mod_symname_hash search
Sync module symbols into mod_symtable whenever module symbols change
Refactor SYMNAME_HASH_INDEX macro to be a function
Add mod_symname_hash table dump to help -s
defs.h | 3 +-
kernel.c | 1 +
symbols.c | 261 +++++++++++++++++++++++++++++++++---------------------
3 files changed, 164 insertions(+), 101 deletions(-)
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 1/7] Implement install and remove operations for mod_symname_hash
This patch indroduces mod_symname_hash, and its install/remove operations.
Since symbol_search() has to return the lowest address symbol and
symbol_search_next() returns the next lowest symbol, thus the installation
should be sorted ascendingly.
In mod_symname_hash_install_range scenario, spn are already arranged
ascendingly, so for mod_symname_hash_install:
Install spn previous to sp:
If sp is the start of bucket, and
1) spn->value is smaller than sp->value.
Install spn next to sp:
1) sp->name_hash_next is NULL or
2) sp->name_hash_next->value larger than spn->value
spn->value is the kernel address of the symbol and will not change.
So we use it mainly to determine the sequence. When spn->value equals
sp->value, they must be symbols within a kernel module.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
defs.h | 1 +
symbols.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/defs.h b/defs.h
index cbd45e5..bbdca79 100644
--- a/defs.h
+++ b/defs.h
@@ -2755,6 +2755,7 @@ struct symbol_table_data {
double val_hash_searches;
double val_hash_iterations;
struct syment *symname_hash[SYMNAME_HASH];
+ struct syment *mod_symname_hash[SYMNAME_HASH];
struct symbol_namespace kernel_namespace;
struct syment *ext_module_symtable;
struct syment *ext_module_symend;
diff --git a/symbols.c b/symbols.c
index 69dccdb..ad12d1c 100644
--- a/symbols.c
+++ b/symbols.c
@@ -1157,6 +1157,79 @@ symname_hash_install(struct syment *spn)
}
}
+/*
+ * Install a single kernel module symbol into the mod_symname_hash.
+ */
+static void
+mod_symname_hash_install(struct syment *spn)
+{
+ struct syment *sp;
+ int index;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+
+ sp = st->mod_symname_hash[index];
+
+ if (!sp || (spn->value < sp->value)) {
+ st->mod_symname_hash[index] = spn;
+ spn->name_hash_next = sp;
+ return;
+ }
+ for (; sp; sp = sp->name_hash_next) {
+ if (!sp->name_hash_next ||
+ spn->value < sp->name_hash_next->value) {
+ spn->name_hash_next = sp->name_hash_next;
+ sp->name_hash_next = spn;
+ return;
+ }
+ }
+}
+
+static void
+mod_symname_hash_remove(struct syment *spn)
+{
+ struct syment *sp;
+ int index;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+
+ if (st->mod_symname_hash[index] == spn) {
+ st->mod_symname_hash[index] = spn->name_hash_next;
+ return;
+ }
+
+ for (sp = st->mod_symname_hash[index]; sp; sp = sp->name_hash_next) {
+ if (sp->name_hash_next == spn) {
+ sp->name_hash_next = spn->name_hash_next;
+ return;
+ }
+ }
+}
+
+static void
+mod_symtable_hash_install_range(struct syment *from, struct syment *to)
+{
+ struct syment *sp;
+
+ for (sp = from; sp <= to; sp++)
+ mod_symname_hash_install(sp);
+}
+
+static void
+mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
+{
+ struct syment *sp;
+
+ for (sp = from; sp <= to; sp++)
+ mod_symname_hash_remove(sp);
+}
+
/*
* Static kernel symbol value search
*/
--
2.29.2
3:46 a.m.
New subject: [PATCH v6 1/7] Implement install and remove operations for mod_symname_hash
Hi, Tao
Thank you for the update.
On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
This patch indroduces mod_symname_hash, and its install/remove operations.
Since symbol_search() has to return the lowest address symbol and
symbol_search_next() returns the next lowest symbol, thus the installation
should be sorted ascendingly.
In mod_symname_hash_install_range scenario, spn are already arranged
ascendingly, so for mod_symname_hash_install:
Install spn previous to sp:
If sp is the start of bucket, and
1) spn->value is smaller than sp->value.
Install spn next to sp:
1) sp->name_hash_next is NULL or
2) sp->name_hash_next->value larger than spn->value
spn->value is the kernel address of the symbol and will not change.
So we use it mainly to determine the sequence. When spn->value equals
sp->value, they must be symbols within a kernel module.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
defs.h | 1 +
symbols.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/defs.h b/defs.h
index cbd45e5..bbdca79 100644
--- a/defs.h
+++ b/defs.h
@@ -2755,6 +2755,7 @@ struct symbol_table_data {
double val_hash_searches;
double val_hash_iterations;
struct syment *symname_hash[SYMNAME_HASH];
+ struct syment *mod_symname_hash[SYMNAME_HASH];
struct symbol_namespace kernel_namespace;
struct syment *ext_module_symtable;
struct syment *ext_module_symend;
diff --git a/symbols.c b/symbols.c
index 69dccdb..ad12d1c 100644
--- a/symbols.c
+++ b/symbols.c
@@ -1157,6 +1157,79 @@ symname_hash_install(struct syment *spn)
}
}
+/*
+ * Install a single kernel module symbol into the mod_symname_hash.
+ */
+static void
+mod_symname_hash_install(struct syment *spn)
+{
+ struct syment *sp;
+ int index;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+
+ sp = st->mod_symname_hash[index];
+
+ if (!sp || (spn->value < sp->value)) {
+ st->mod_symname_hash[index] = spn;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This could overwrite the existing syment if (sp && (spn->value <
sp->value)), right?
+ spn->name_hash_next = sp;
+ return;
+ }
+ for (; sp; sp = sp->name_hash_next) {
+ if (!sp->name_hash_next ||
+ spn->value < sp->name_hash_next->value) {
+ spn->name_hash_next = sp->name_hash_next;
+ sp->name_hash_next = spn;
+ return;
+ }
+ }
+}
+
+static void
+mod_symname_hash_remove(struct syment *spn)
+{
+ struct syment *sp;
+ int index;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+
+ if (st->mod_symname_hash[index] == spn) {
+ st->mod_symname_hash[index] = spn->name_hash_next;
+ return;
+ }
+
+ for (sp = st->mod_symname_hash[index]; sp; sp = sp->name_hash_next) {
+ if (sp->name_hash_next == spn) {
+ sp->name_hash_next = spn->name_hash_next;
+ return;
+ }
+ }
+}
Can the above mod_symname_hash_remove() be simplified into the
following implementation? The code may become more readable, and I
didn't see any obvious performance issues as below.
+static void
+mod_symname_hash_remove(struct syment *spn)
+{
+ int index;
+ struct syment *sp;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+ sp = st->mod_symname_hash[index];
+
+ while (sp) {
+ if (sp == spn) {
+ sp = spn->name_hash_next;
+ spn->name_hash_next = NULL;
+ return;
+ }
+ sp = sp->name_hash_next;
+ }
+}
+
+static void
+mod_symtable_hash_install_range(struct syment *from, struct syment *to)
+{
+ struct syment *sp;
+
+ for (sp = from; sp <= to; sp++)
+ mod_symname_hash_install(sp);
+}
+
+static void
+mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
+{
+ struct syment *sp;
+
+ for (sp = from; sp <= to; sp++)
+ mod_symname_hash_remove(sp);
+}
+
/*
* Static kernel symbol value search
*/
--
2.29.2
5:21 a.m.
New subject: [PATCH v6 1/7] Implement install and remove operations for mod_symname_hash
Hello lijiang,
Thanks for reviewing the patch!
On Wed, Oct 20, 2021 at 4:46 PM lijiang <lijiang(a)redhat.com> wrote:
Hi, Tao
Thank you for the update.
On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
>
> This patch indroduces mod_symname_hash, and its install/remove operations.
> Since symbol_search() has to return the lowest address symbol and
> symbol_search_next() returns the next lowest symbol, thus the installation
> should be sorted ascendingly.
>
> In mod_symname_hash_install_range scenario, spn are already arranged
> ascendingly, so for mod_symname_hash_install:
>
> Install spn previous to sp:
>
> If sp is the start of bucket, and
> 1) spn->value is smaller than sp->value.
>
> Install spn next to sp:
>
> 1) sp->name_hash_next is NULL or
> 2) sp->name_hash_next->value larger than spn->value
>
> spn->value is the kernel address of the symbol and will not change.
> So we use it mainly to determine the sequence. When spn->value equals
> sp->value, they must be symbols within a kernel module.
>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> defs.h | 1 +
> symbols.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/defs.h b/defs.h
> index cbd45e5..bbdca79 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -2755,6 +2755,7 @@ struct symbol_table_data {
> double val_hash_searches;
> double val_hash_iterations;
> struct syment *symname_hash[SYMNAME_HASH];
> + struct syment *mod_symname_hash[SYMNAME_HASH];
> struct symbol_namespace kernel_namespace;
> struct syment *ext_module_symtable;
> struct syment *ext_module_symend;
> diff --git a/symbols.c b/symbols.c
> index 69dccdb..ad12d1c 100644
> --- a/symbols.c
> +++ b/symbols.c
> @@ -1157,6 +1157,79 @@ symname_hash_install(struct syment *spn)
> }
> }
>
> +/*
> + * Install a single kernel module symbol into the mod_symname_hash.
> + */
> +static void
> +mod_symname_hash_install(struct syment *spn)
> +{
> + struct syment *sp;
> + int index;
> +
> + if (!spn)
> + return;
> +
> + index = SYMNAME_HASH_INDEX(spn->name);
> +
> + sp = st->mod_symname_hash[index];
> +
> + if (!sp || (spn->value < sp->value)) {
> + st->mod_symname_hash[index] = spn;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This could overwrite the existing syment if (sp && (spn->value <
sp->value)), right?
I think it won't overwrite existing syment. if (sp && (spn->value
<
sp->value)), then what we want to do is inserting spn as the start
of the bucket, and making sp to be the 2nd one. So st->mod_symname_hash[index] = spn,
making spn to be the start of the bucket, then spn->name_hash_next = sp, making sp
right after spn.
> + spn->name_hash_next = sp;
> + return;
> + }
> + for (; sp; sp = sp->name_hash_next) {
> + if (!sp->name_hash_next ||
> + spn->value < sp->name_hash_next->value) {
> + spn->name_hash_next = sp->name_hash_next;
> + sp->name_hash_next = spn;
> + return;
> + }
> + }
> +}
> +
> +static void
> +mod_symname_hash_remove(struct syment *spn)
> +{
> + struct syment *sp;
> + int index;
> +
> + if (!spn)
> + return;
> +
> + index = SYMNAME_HASH_INDEX(spn->name);
> +
> + if (st->mod_symname_hash[index] == spn) {
> + st->mod_symname_hash[index] = spn->name_hash_next;
> + return;
> + }
> +
> + for (sp = st->mod_symname_hash[index]; sp; sp = sp->name_hash_next) {
> + if (sp->name_hash_next == spn) {
> + sp->name_hash_next = spn->name_hash_next;
> + return;
> + }
> + }
> +}
Can the above mod_symname_hash_remove() be simplified into the
following implementation? The code may become more readable, and I
didn't see any obvious performance issues as below.
+static void
+mod_symname_hash_remove(struct syment *spn)
+{
+ int index;
+ struct syment *sp;
+
+ if (!spn)
+ return;
+
+ index = SYMNAME_HASH_INDEX(spn->name);
+ sp = st->mod_symname_hash[index];
+
+ while (sp) {
+ if (sp == spn) {
+ sp = spn->name_hash_next;
+ spn->name_hash_next = NULL;
+ return;
+ }
+ sp = sp->name_hash_next;
+ }
+}
I don't think it can work. if (sp == spn), then sp should be removed
from the hash table. Since it is a singly linked list, the
name_hash_next field of the one which is prior to sp should be
updated. But in the code it is not.
Thanks,
Tao Liu
> +
> +static void
> +mod_symtable_hash_install_range(struct syment *from, struct syment *to)
> +{
> + struct syment *sp;
> +
> + for (sp = from; sp <= to; sp++)
> + mod_symname_hash_install(sp);
> +}
> +
> +static void
> +mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
> +{
> + struct syment *sp;
> +
> + for (sp = from; sp <= to; sp++)
> + mod_symname_hash_remove(sp);
> +}
> +
> /*
> * Static kernel symbol value search
> */
> --
> 2.29.2
>
5:44 a.m.
New subject: [PATCH v6 1/7] Implement install and remove operations for mod_symname_hash
On Wed, Oct 20, 2021 at 6:22 PM Tao Liu <ltao(a)redhat.com> wrote:
Hello lijiang,
Thanks for reviewing the patch!
On Wed, Oct 20, 2021 at 4:46 PM lijiang <lijiang(a)redhat.com> wrote:
>
> Hi, Tao
> Thank you for the update.
>
> On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
> >
> > This patch indroduces mod_symname_hash, and its install/remove operations.
> > Since symbol_search() has to return the lowest address symbol and
> > symbol_search_next() returns the next lowest symbol, thus the installation
> > should be sorted ascendingly.
> >
> > In mod_symname_hash_install_range scenario, spn are already arranged
> > ascendingly, so for mod_symname_hash_install:
> >
> > Install spn previous to sp:
> >
> > If sp is the start of bucket, and
> > 1) spn->value is smaller than sp->value.
> >
> > Install spn next to sp:
> >
> > 1) sp->name_hash_next is NULL or
> > 2) sp->name_hash_next->value larger than spn->value
> >
> > spn->value is the kernel address of the symbol and will not change.
> > So we use it mainly to determine the sequence. When spn->value equals
> > sp->value, they must be symbols within a kernel module.
> >
> > Signed-off-by: Tao Liu <ltao(a)redhat.com>
> > ---
> > defs.h | 1 +
> > symbols.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 74 insertions(+)
> >
> > diff --git a/defs.h b/defs.h
> > index cbd45e5..bbdca79 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -2755,6 +2755,7 @@ struct symbol_table_data {
> > double val_hash_searches;
> > double val_hash_iterations;
> > struct syment *symname_hash[SYMNAME_HASH];
> > + struct syment *mod_symname_hash[SYMNAME_HASH];
> > struct symbol_namespace kernel_namespace;
> > struct syment *ext_module_symtable;
> > struct syment *ext_module_symend;
> > diff --git a/symbols.c b/symbols.c
> > index 69dccdb..ad12d1c 100644
> > --- a/symbols.c
> > +++ b/symbols.c
> > @@ -1157,6 +1157,79 @@ symname_hash_install(struct syment *spn)
> > }
> > }
> >
> > +/*
> > + * Install a single kernel module symbol into the mod_symname_hash.
> > + */
> > +static void
> > +mod_symname_hash_install(struct syment *spn)
> > +{
> > + struct syment *sp;
> > + int index;
> > +
> > + if (!spn)
> > + return;
> > +
> > + index = SYMNAME_HASH_INDEX(spn->name);
> > +
> > + sp = st->mod_symname_hash[index];
> > +
> > + if (!sp || (spn->value < sp->value)) {
> > + st->mod_symname_hash[index] = spn;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This could overwrite the existing syment if (sp && (spn->value <
> sp->value)), right?
>
I think it won't overwrite existing syment. if (sp && (spn->value <
> sp->value)), then what we want to do is inserting spn as the start of the bucket,
and making sp to be the 2nd one. So st->mod_symname_hash[index] = spn, making spn to be
the start of the bucket, then spn->name_hash_next = sp, making sp right after spn.
> > + spn->name_hash_next = sp;
> > + return;
> > + }
> > + for (; sp; sp = sp->name_hash_next) {
> > + if (!sp->name_hash_next ||
> > + spn->value < sp->name_hash_next->value) {
> > + spn->name_hash_next = sp->name_hash_next;
> > + sp->name_hash_next = spn;
> > + return;
> > + }
> > + }
> > +}
> > +
> > +static void
> > +mod_symname_hash_remove(struct syment *spn)
> > +{
> > + struct syment *sp;
> > + int index;
> > +
> > + if (!spn)
> > + return;
> > +
> > + index = SYMNAME_HASH_INDEX(spn->name);
> > +
> > + if (st->mod_symname_hash[index] == spn) {
> > + st->mod_symname_hash[index] = spn->name_hash_next;
> > + return;
> > + }
> > +
> > + for (sp = st->mod_symname_hash[index]; sp; sp =
sp->name_hash_next) {
> > + if (sp->name_hash_next == spn) {
> > + sp->name_hash_next = spn->name_hash_next;
> > + return;
> > + }
> > + }
> > +}
>
> Can the above mod_symname_hash_remove() be simplified into the
> following implementation? The code may become more readable, and I
> didn't see any obvious performance issues as below.
>
> +static void
> +mod_symname_hash_remove(struct syment *spn)
> +{
> + int index;
> + struct syment *sp;
> +
> + if (!spn)
> + return;
> +
> + index = SYMNAME_HASH_INDEX(spn->name);
> + sp = st->mod_symname_hash[index];
> +
> + while (sp) {
> + if (sp == spn) {
> + sp = spn->name_hash_next;
> + spn->name_hash_next = NULL;
> + return;
> + }
> + sp = sp->name_hash_next;
> + }
> +}
>
I don't think it can work. if (sp == spn), then sp should be removed
from the hash table. Since it is a singly linked list, the
name_hash_next field of the one which is prior to sp should be
updated. But in the code it is not.
I haven't debugged it, just an idea. I will debug it later. Thanks.
Thanks,
Tao Liu
>
> > +
> > +static void
> > +mod_symtable_hash_install_range(struct syment *from, struct syment *to)
> > +{
> > + struct syment *sp;
> > +
> > + for (sp = from; sp <= to; sp++)
> > + mod_symname_hash_install(sp);
> > +}
> > +
> > +static void
> > +mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
> > +{
> > + struct syment *sp;
> > +
> > + for (sp = from; sp <= to; sp++)
> > + mod_symname_hash_remove(sp);
> > +}
> > +
> > /*
> > * Static kernel symbol value search
> > */
> > --
> > 2.29.2
> >
>
8:17 p.m.
New subject: [PATCH v6 1/7] Implement install and remove operations for mod_symname_hash
On Wed, Oct 20, 2021 at 6:44 PM lijiang <lijiang(a)redhat.com> wrote:
On Wed, Oct 20, 2021 at 6:22 PM Tao Liu <ltao(a)redhat.com> wrote:
>
> Hello lijiang,
>
> Thanks for reviewing the patch!
>
> On Wed, Oct 20, 2021 at 4:46 PM lijiang <lijiang(a)redhat.com> wrote:
> >
> > Hi, Tao
> > Thank you for the update.
> >
> > On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
> > >
> > > This patch indroduces mod_symname_hash, and its install/remove
operations.
> > > Since symbol_search() has to return the lowest address symbol and
> > > symbol_search_next() returns the next lowest symbol, thus the
installation
> > > should be sorted ascendingly.
> > >
> > > In mod_symname_hash_install_range scenario, spn are already arranged
> > > ascendingly, so for mod_symname_hash_install:
> > >
> > > Install spn previous to sp:
> > >
> > > If sp is the start of bucket, and
> > > 1) spn->value is smaller than sp->value.
> > >
> > > Install spn next to sp:
> > >
> > > 1) sp->name_hash_next is NULL or
> > > 2) sp->name_hash_next->value larger than spn->value
> > >
> > > spn->value is the kernel address of the symbol and will not change.
> > > So we use it mainly to determine the sequence. When spn->value equals
> > > sp->value, they must be symbols within a kernel module.
> > >
> > > Signed-off-by: Tao Liu <ltao(a)redhat.com>
> > > ---
> > > defs.h | 1 +
> > > symbols.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 74 insertions(+)
> > >
> > > diff --git a/defs.h b/defs.h
> > > index cbd45e5..bbdca79 100644
> > > --- a/defs.h
> > > +++ b/defs.h
> > > @@ -2755,6 +2755,7 @@ struct symbol_table_data {
> > > double val_hash_searches;
> > > double val_hash_iterations;
> > > struct syment *symname_hash[SYMNAME_HASH];
> > > + struct syment *mod_symname_hash[SYMNAME_HASH];
> > > struct symbol_namespace kernel_namespace;
> > > struct syment *ext_module_symtable;
> > > struct syment *ext_module_symend;
> > > diff --git a/symbols.c b/symbols.c
> > > index 69dccdb..ad12d1c 100644
> > > --- a/symbols.c
> > > +++ b/symbols.c
> > > @@ -1157,6 +1157,79 @@ symname_hash_install(struct syment *spn)
> > > }
> > > }
> > >
> > > +/*
> > > + * Install a single kernel module symbol into the mod_symname_hash.
> > > + */
> > > +static void
> > > +mod_symname_hash_install(struct syment *spn)
> > > +{
> > > + struct syment *sp;
> > > + int index;
> > > +
> > > + if (!spn)
> > > + return;
> > > +
> > > + index = SYMNAME_HASH_INDEX(spn->name);
> > > +
> > > + sp = st->mod_symname_hash[index];
> > > +
> > > + if (!sp || (spn->value < sp->value)) {
> > > + st->mod_symname_hash[index] = spn;
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > This could overwrite the existing syment if (sp && (spn->value <
> > sp->value)), right?
> >
> I think it won't overwrite existing syment. if (sp && (spn->value
<
> > sp->value)), then what we want to do is inserting spn as the start of the
bucket, and making sp to be the 2nd one. So st->mod_symname_hash[index] = spn, making
spn to be the start of the bucket, then spn->name_hash_next = sp, making sp right after
spn.
>
> > > + spn->name_hash_next = sp;
> > > + return;
> > > + }
> > > + for (; sp; sp = sp->name_hash_next) {
> > > + if (!sp->name_hash_next ||
> > > + spn->value < sp->name_hash_next->value) {
> > > + spn->name_hash_next = sp->name_hash_next;
> > > + sp->name_hash_next = spn;
> > > + return;
> > > + }
> > > + }
> > > +}
> > > +
> > > +static void
> > > +mod_symname_hash_remove(struct syment *spn)
> > > +{
> > > + struct syment *sp;
> > > + int index;
> > > +
> > > + if (!spn)
> > > + return;
> > > +
> > > + index = SYMNAME_HASH_INDEX(spn->name);
> > > +
> > > + if (st->mod_symname_hash[index] == spn) {
> > > + st->mod_symname_hash[index] = spn->name_hash_next;
> > > + return;
> > > + }
> > > +
> > > + for (sp = st->mod_symname_hash[index]; sp; sp =
sp->name_hash_next) {
> > > + if (sp->name_hash_next == spn) {
> > > + sp->name_hash_next = spn->name_hash_next;
> > > + return;
> > > + }
> > > + }
> > > +}
> >
> > Can the above mod_symname_hash_remove() be simplified into the
> > following implementation? The code may become more readable, and I
> > didn't see any obvious performance issues as below.
> >
> > +static void
> > +mod_symname_hash_remove(struct syment *spn)
> > +{
> > + int index;
> > + struct syment *sp;
> > +
> > + if (!spn)
> > + return;
> > +
> > + index = SYMNAME_HASH_INDEX(spn->name);
> > + sp = st->mod_symname_hash[index];
> > +
> > + while (sp) {
> > + if (sp == spn) {
> > + sp = spn->name_hash_next;
> > + spn->name_hash_next = NULL;
> > + return;
> > + }
> > + sp = sp->name_hash_next;
> > + }
> > +}
> >
>
> I don't think it can work. if (sp == spn), then sp should be removed
> from the hash table. Since it is a singly linked list, the
> name_hash_next field of the one which is prior to sp should be
> updated. But in the code it is not.
>
I haven't debugged it, just an idea. I will debug it later. Thanks.
You are right, the first node needs to be treated specially in a
single linked list without a specific head node. I have no other
issues.
Thanks.
Lianbo
> Thanks,
> Tao Liu
> >
> > > +
> > > +static void
> > > +mod_symtable_hash_install_range(struct syment *from, struct syment *to)
> > > +{
> > > + struct syment *sp;
> > > +
> > > + for (sp = from; sp <= to; sp++)
> > > + mod_symname_hash_install(sp);
> > > +}
> > > +
> > > +static void
> > > +mod_symtable_hash_remove_range(struct syment *from, struct syment *to)
> > > +{
> > > + struct syment *sp;
> > > +
> > > + for (sp = from; sp <= to; sp++)
> > > + mod_symname_hash_remove(sp);
> > > +}
> > > +
> > > /*
> > > * Static kernel symbol value search
> > > */
> > > --
> > > 2.29.2
> > >
> >
>
12:21 a.m.
New subject: [PATCH v6 2/7] Integrate symbol_search with mod_symname_hash search
This patch introduces mod_symname_hash search to symbol_search,
to get a better searching performance.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
---
symbols.c | 58 ++++++++++++++++++-------------------------------------
1 file changed, 19 insertions(+), 39 deletions(-)
diff --git a/symbols.c b/symbols.c
index ad12d1c..c4ad31f 100644
--- a/symbols.c
+++ b/symbols.c
@@ -4551,6 +4551,17 @@ symbol_query(char *s, char *print_pad, struct syment **spp)
return(cnt);
}
+static int
+skip_symbols(struct syment *sp, char *s)
+{
+ int pseudos, skip = FALSE;
+
+ pseudos = (strstr(s, "_MODULE_START_") || strstr(s, "_MODULE_END_")
||
+ strstr(s, "_MODULE_INIT_START_") || strstr(s,
"_MODULE_INIT_END_"));
+ if (!pseudos && MODULE_PSEUDO_SYMBOL(sp))
+ skip = TRUE;
+ return skip;
+}
/*
* Return the syment of a symbol.
@@ -4558,10 +4569,7 @@ symbol_query(char *s, char *print_pad, struct syment **spp)
struct syment *
symbol_search(char *s)
{
- int i;
- struct syment *sp_hashed, *sp, *sp_end;
- struct load_module *lm;
- int pseudos, search_init;
+ struct syment *sp_hashed, *sp;
sp_hashed = symname_hash_search(s);
@@ -4570,43 +4578,15 @@ symbol_search(char *s)
return(sp);
}
- pseudos = (strstr(s, "_MODULE_START_") || strstr(s,
"_MODULE_END_"));
- search_init = FALSE;
-
- for (i = 0; i < st->mods_installed; i++) {
- lm = &st->load_modules[i];
- if (lm->mod_flags & MOD_INIT)
- search_init = TRUE;
- sp = lm->mod_symtable;
- sp_end = lm->mod_symend;
-
- for ( ; sp <= sp_end; sp++) {
- if (!pseudos && MODULE_PSEUDO_SYMBOL(sp))
- continue;
- if (STREQ(s, sp->name))
- return(sp);
- }
- }
-
- if (!search_init)
- return((struct syment *)NULL);
-
- pseudos = (strstr(s, "_MODULE_INIT_START_") || strstr(s,
"_MODULE_INIT_END_"));
-
- for (i = 0; i < st->mods_installed; i++) {
- lm = &st->load_modules[i];
- if (!lm->mod_init_symtable)
+ sp = st->mod_symname_hash[SYMNAME_HASH_INDEX(s)];
+ while (sp) {
+ if (skip_symbols(sp, s)) {
+ sp = sp->name_hash_next;
continue;
- sp = lm->mod_init_symtable;
- sp_end = lm->mod_init_symend;
-
- for ( ; sp < sp_end; sp++) {
- if (!pseudos && MODULE_PSEUDO_SYMBOL(sp))
- continue;
-
- if (STREQ(s, sp->name))
- return(sp);
}
+ if (STREQ(sp->name, s))
+ return sp;
+ sp = sp->name_hash_next;
}
return((struct syment *)NULL);
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 3/7] Extend symname_hash_search with hash table select
Previously symname_hash_search can only search symbols from kernel's
symname_hash. This patch add hash table pointer as parameter for
symname_hash_search. Thus symname_hash_search can be used both for
symname_hash and mod_symname_hash searching.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
symbols.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/symbols.c b/symbols.c
index c4ad31f..83393f0 100644
--- a/symbols.c
+++ b/symbols.c
@@ -65,7 +65,7 @@ static void symval_hash_init(void);
static struct syment *symval_hash_search(ulong);
static void symname_hash_init(void);
static void symname_hash_install(struct syment *);
-static struct syment *symname_hash_search(char *);
+static struct syment *symname_hash_search(struct syment *[], char *);
static void gnu_qsort(bfd *, void *, long, unsigned int, asymbol *, asymbol *);
static int check_gnu_debuglink(bfd *);
static int separate_debug_file_exists(const char *, unsigned long, int *);
@@ -1234,11 +1234,11 @@ mod_symtable_hash_remove_range(struct syment *from, struct syment
*to)
* Static kernel symbol value search
*/
static struct syment *
-symname_hash_search(char *name)
+symname_hash_search(struct syment *table[], char *name)
{
struct syment *sp;
- sp = st->symname_hash[SYMNAME_HASH_INDEX(name)];
+ sp = table[SYMNAME_HASH_INDEX(name)];
while (sp) {
if (STREQ(sp->name, name))
@@ -4571,7 +4571,7 @@ symbol_search(char *s)
{
struct syment *sp_hashed, *sp;
- sp_hashed = symname_hash_search(s);
+ sp_hashed = symname_hash_search(st->symname_hash, s);
for (sp = sp_hashed ? sp_hashed : st->symtable; sp < st->symend; sp++)
{
if (STREQ(s, sp->name))
@@ -5485,7 +5485,7 @@ symbol_exists(char *symbol)
struct syment *sp, *sp_end;
struct load_module *lm;
- if ((sp = symname_hash_search(symbol)))
+ if ((sp = symname_hash_search(st->symname_hash, symbol)))
return TRUE;
for (i = 0; i < st->mods_installed; i++) {
@@ -5564,7 +5564,7 @@ kernel_symbol_exists(char *symbol)
{
struct syment *sp;
- if ((sp = symname_hash_search(symbol)))
+ if ((sp = symname_hash_search(st->symname_hash, symbol)))
return TRUE;
else
return FALSE;
@@ -5576,7 +5576,7 @@ kernel_symbol_exists(char *symbol)
struct syment *
kernel_symbol_search(char *symbol)
{
- return symname_hash_search(symbol);
+ return symname_hash_search(st->symname_hash, symbol);
}
/*
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 4/7] Intergrate symbol_exists with mod_symname_hash search
This patch introduces mod_symname_hash search to symbol_exists
to improve its performance. And code refactoring for
kernel_symbol_exists.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
---
symbols.c | 35 ++++-------------------------------
1 file changed, 4 insertions(+), 31 deletions(-)
diff --git a/symbols.c b/symbols.c
index 83393f0..8d3dd95 100644
--- a/symbols.c
+++ b/symbols.c
@@ -5481,33 +5481,11 @@ value_symbol(ulong value)
int
symbol_exists(char *symbol)
{
- int i;
- struct syment *sp, *sp_end;
- struct load_module *lm;
-
- if ((sp = symname_hash_search(st->symname_hash, symbol)))
+ if (symname_hash_search(st->symname_hash, symbol))
return TRUE;
- for (i = 0; i < st->mods_installed; i++) {
- lm = &st->load_modules[i];
- sp = lm->mod_symtable;
- sp_end = lm->mod_symend;
-
- for ( ; sp < sp_end; sp++) {
- if (STREQ(symbol, sp->name))
- return(TRUE);
- }
-
- if (lm->mod_init_symtable) {
- sp = lm->mod_init_symtable;
- sp_end = lm->mod_init_symend;
-
- for ( ; sp < sp_end; sp++) {
- if (STREQ(symbol, sp->name))
- return(TRUE);
- }
- }
- }
+ if (symname_hash_search(st->mod_symname_hash, symbol))
+ return TRUE;
return(FALSE);
}
@@ -5562,12 +5540,7 @@ per_cpu_symbol_search(char *symbol)
int
kernel_symbol_exists(char *symbol)
{
- struct syment *sp;
-
- if ((sp = symname_hash_search(st->symname_hash, symbol)))
- return TRUE;
- else
- return FALSE;
+ return !!symname_hash_search(st->symname_hash, symbol);
}
/*
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 5/7] Sync module symbols into mod_symtable whenever module symbols change
Signed-off-by: Tao Liu <ltao(a)redhat.com>
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
---
kernel.c | 1 +
symbols.c | 9 +++++++++
2 files changed, 10 insertions(+)
diff --git a/kernel.c b/kernel.c
index 3ead4bb..f10b8b2 100644
--- a/kernel.c
+++ b/kernel.c
@@ -4661,6 +4661,7 @@ reinit_modules(void)
st->ext_module_symtable = NULL;
st->load_modules = NULL;
kt->mods_installed = 0;
+ memset(st->mod_symname_hash, 0, sizeof(st->mod_symname_hash));
module_init();
}
diff --git a/symbols.c b/symbols.c
index 8d3dd95..5603a2e 100644
--- a/symbols.c
+++ b/symbols.c
@@ -1668,6 +1668,7 @@ store_module_symbols_v1(ulong total, int mods_installed)
lm->mod_symend = sp;
}
}
+ mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
}
st->flags |= MODULE_SYMS;
@@ -2148,6 +2149,8 @@ store_module_symbols_v2(ulong total, int mods_installed)
lm->mod_init_symend = sp;
}
}
+ mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
+ mod_symtable_hash_install_range(lm->mod_init_symtable, lm->mod_init_symend);
}
st->flags |= MODULE_SYMS;
@@ -12478,8 +12481,10 @@ store_load_module_symbols(bfd *bfd, int dynamic, void *minisyms,
error(INFO, "%s: last symbol: %s is not _MODULE_END_%s?\n",
lm->mod_name, lm->mod_load_symend->name, lm->mod_name);
+ mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
lm->mod_symtable = lm->mod_load_symtable;
lm->mod_symend = lm->mod_load_symend;
+ mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
lm->mod_flags &= ~MOD_EXT_SYMS;
lm->mod_flags |= MOD_LOAD_SYMS;
@@ -12509,6 +12514,7 @@ delete_load_module(ulong base_addr)
req->name = lm->mod_namelist;
gdb_interface(req);
}
+ mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
if (lm->mod_load_symtable) {
free(lm->mod_load_symtable);
namespace_ctl(NAMESPACE_FREE,
@@ -12518,6 +12524,7 @@ delete_load_module(ulong base_addr)
unlink_module(lm);
lm->mod_symtable = lm->mod_ext_symtable;
lm->mod_symend = lm->mod_ext_symend;
+ mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH);
lm->mod_flags |= MOD_EXT_SYMS;
lm->mod_load_symtable = NULL;
@@ -12546,6 +12553,7 @@ delete_load_module(ulong base_addr)
req->name = lm->mod_namelist;
gdb_interface(req);
}
+ mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend);
if (lm->mod_load_symtable) {
free(lm->mod_load_symtable);
namespace_ctl(NAMESPACE_FREE,
@@ -12555,6 +12563,7 @@ delete_load_module(ulong base_addr)
unlink_module(lm);
lm->mod_symtable = lm->mod_ext_symtable;
lm->mod_symend = lm->mod_ext_symend;
+ mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend);
lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH);
lm->mod_flags |= MOD_EXT_SYMS;
lm->mod_load_symtable = NULL;
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 6/7] Refactor SYMNAME_HASH_INDEX macro to be a function
SYMNAME_HASH_INDEX is used as the index of symname hash table. It will
be out of range if SYMNAME_HASH_INDEX is negative. This patch avoids
the risk by changing the marco into a function, and casting and calculating
the numbers as unsigned.
Suggested-by: Lianbo Jiang <lijiang(a)redhat.com>
Suggested-by: Philipp Rudo <prudo(a)redhat.com>
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
defs.h | 2 --
symbols.c | 31 +++++++++++++++++++++++--------
2 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/defs.h b/defs.h
index bbdca79..8b356d5 100644
--- a/defs.h
+++ b/defs.h
@@ -2728,8 +2728,6 @@ struct downsized {
(((vaddr) >> machdep->pageshift) % SYMVAL_HASH)
#define SYMNAME_HASH (512)
-#define SYMNAME_HASH_INDEX(name) \
- ((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH)
#define PATCH_KERNEL_SYMBOLS_START ((char *)(1))
#define PATCH_KERNEL_SYMBOLS_STOP ((char *)(2))
diff --git a/symbols.c b/symbols.c
index 5603a2e..67b7014 100644
--- a/symbols.c
+++ b/symbols.c
@@ -1127,6 +1127,21 @@ symname_hash_init(void)
st->__per_cpu_end = sp->value;
}
+static unsigned int
+symname_hash_index(char *name)
+{
+ unsigned int len, value;
+ unsigned char *array = (unsigned char *)name;
+
+ len = strlen(name);
+ if (!len)
+ error(FATAL, "The length of the symbol name is zero!\n");
+
+ value = array[len - 1] * array[len / 2];
+
+ return (array[0] ^ value) % SYMNAME_HASH;
+}
+
/*
* Install a single static kernel symbol into the symname_hash.
*/
@@ -1134,9 +1149,9 @@ static void
symname_hash_install(struct syment *spn)
{
struct syment *sp;
- int index;
+ unsigned int index;
- index = SYMNAME_HASH_INDEX(spn->name);
+ index = symname_hash_index(spn->name);
spn->cnt = 1;
if ((sp = st->symname_hash[index]) == NULL)
@@ -1164,12 +1179,12 @@ static void
mod_symname_hash_install(struct syment *spn)
{
struct syment *sp;
- int index;
+ unsigned int index;
if (!spn)
return;
- index = SYMNAME_HASH_INDEX(spn->name);
+ index = symname_hash_index(spn->name);
sp = st->mod_symname_hash[index];
@@ -1192,12 +1207,12 @@ static void
mod_symname_hash_remove(struct syment *spn)
{
struct syment *sp;
- int index;
+ unsigned int index;
if (!spn)
return;
- index = SYMNAME_HASH_INDEX(spn->name);
+ index = symname_hash_index(spn->name);
if (st->mod_symname_hash[index] == spn) {
st->mod_symname_hash[index] = spn->name_hash_next;
@@ -1238,7 +1253,7 @@ symname_hash_search(struct syment *table[], char *name)
{
struct syment *sp;
- sp = table[SYMNAME_HASH_INDEX(name)];
+ sp = table[symname_hash_index(name)];
while (sp) {
if (STREQ(sp->name, name))
@@ -4581,7 +4596,7 @@ symbol_search(char *s)
return(sp);
}
- sp = st->mod_symname_hash[SYMNAME_HASH_INDEX(s)];
+ sp = st->mod_symname_hash[symname_hash_index(s)];
while (sp) {
if (skip_symbols(sp, s)) {
sp = sp->name_hash_next;
--
2.29.2
12:21 a.m.
New subject: [PATCH v6 7/7] Add mod_symname_hash table dump to help -s
Previously, help -s only print out the dump status of symname_hash
table. Since we have mod_symname_hash table introduced, let's print
out mod_symname_hash in help -s as well.
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
symbols.c | 57 ++++++++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git a/symbols.c b/symbols.c
index 67b7014..3e6655f 100644
--- a/symbols.c
+++ b/symbols.c
@@ -3219,13 +3219,40 @@ kallsyms_module_function_size(struct syment *sp, struct
load_module *lm, ulong *
return FALSE;
}
+static void
+dump_symname_hash_table(struct syment *table[])
+{
+ int i, cnt, tot;
+ struct syment *sp;
+
+ fprintf(fp, " ");
+ for (i = tot = 0; i < SYMNAME_HASH; i++) {
+ fprintf(fp, "[%3d]: ", i);
+ if ((sp = table[i]) == NULL)
+ fprintf(fp, "%3d ", 0);
+ else {
+ cnt = 1;
+ while (sp->name_hash_next) {
+ cnt++;
+ sp = sp->name_hash_next;
+ }
+ fprintf(fp, "%3d ", cnt);
+ tot += cnt;
+ }
+ if (i && (((i+1) % 6) == 0))
+ fprintf(fp, "\n ");
+ }
+ if (SYMNAME_HASH % 6)
+ fprintf(fp, "\n");
+}
+
/*
* "help -s" output
*/
void
dump_symbol_table(void)
{
- int i, s, cnt, tot;
+ int i, s, cnt;
struct load_module *lm;
struct syment *sp;
struct downsized *ds;
@@ -3355,28 +3382,14 @@ dump_symbol_table(void)
fprintf(fp, " symname_hash[%d]: %lx\n", SYMNAME_HASH,
(ulong)&st->symname_hash[0]);
+ if (CRASHDEBUG(1))
+ dump_symname_hash_table(st->symname_hash);
+
+ fprintf(fp, "mod_symname_hash[%d]: %lx\n", SYMNAME_HASH,
+ (ulong)&st->mod_symname_hash[0]);
+ if (CRASHDEBUG(1))
+ dump_symname_hash_table(st->mod_symname_hash);
- if (CRASHDEBUG(1)) {
- fprintf(fp, " ");
- for (i = tot = 0; i < SYMNAME_HASH; i++) {
- fprintf(fp, "[%3d]: ", i);
- if ((sp = st->symname_hash[i]) == NULL)
- fprintf(fp, "%3d ", 0);
- else {
- cnt = 1;
- while (sp->name_hash_next) {
- cnt++;
- sp = sp->name_hash_next;
- }
- fprintf(fp, "%3d ", cnt);
- tot += cnt;
- }
- if (i && (((i+1) % 6) == 0))
- fprintf(fp, "\n ");
- }
- if (SYMNAME_HASH % 6)
- fprintf(fp, "\n");
- }
fprintf(fp, " symbol_namespace: ");
fprintf(fp, "address: %lx ", (ulong)st->kernel_namespace.address);
fprintf(fp, "index: %ld ", st->kernel_namespace.index);
--
2.29.2
8:26 p.m.
On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
Currently the sequence for symbol_search to search a symbol is: 1) kernel
symname hash table, 2) iterate all kernel symbols, 3) iterate all kernel
modules and their symbols. In the worst case, if a non-exist symbol been
searched, all 3 stages will be went through. The time consuming status for
each stage is like:
stage 1 stage 2 stage 3
0.007000(ms) 0.593000(ms) 2.421000(ms)
stage 3 takes too much time when comparing to stage 1. This patch introduces
a symname hash table for kernel modules, to improve the performance of symbol
searching.
Functions symbol_search and symbol_exists are fundamental and widely used by
other crash functions, thus the benefit of performance improvement can
get accumulated. For example, "ps -m" and "irq" commands, which call
the functions many times, will become faster with the patch.
v5 -> v6:
1) Add mod_symname_hash table dump to help -s
2) Modified mod_symname_hash install/remove based on Kazu's suggestion.
Tao Liu (7):
Implement install and remove operations for mod_symname_hash
Integrate symbol_search with mod_symname_hash search
Extend symname_hash_search with hash table select
Intergrate symbol_exists with mod_symname_hash search
Sync module symbols into mod_symtable whenever module symbols change
Refactor SYMNAME_HASH_INDEX macro to be a function
Add mod_symname_hash table dump to help -s
defs.h | 3 +-
kernel.c | 1 +
symbols.c | 261 +++++++++++++++++++++++++++++++++---------------------
3 files changed, 164 insertions(+), 101 deletions(-)
For the V6: Acked-by: Lianbo Jiang <lijiang(a)redhat.com>
Thanks.
--
2.29.2
2:28 a.m.
Hi Lijiang,
On Sat, Oct 23, 2021 at 9:27 AM lijiang <lijiang(a)redhat.com> wrote:
On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
>
> Currently the sequence for symbol_search to search a symbol is: 1) kernel
> symname hash table, 2) iterate all kernel symbols, 3) iterate all kernel
> modules and their symbols. In the worst case, if a non-exist symbol been
> searched, all 3 stages will be went through. The time consuming status for
> each stage is like:
>
> stage 1 stage 2 stage 3
> 0.007000(ms) 0.593000(ms) 2.421000(ms)
>
> stage 3 takes too much time when comparing to stage 1. This patch introduces
> a symname hash table for kernel modules, to improve the performance of symbol
> searching.
>
> Functions symbol_search and symbol_exists are fundamental and widely used by
> other crash functions, thus the benefit of performance improvement can
> get accumulated. For example, "ps -m" and "irq" commands, which
call
> the functions many times, will become faster with the patch.
>
> v5 -> v6:
>
> 1) Add mod_symname_hash table dump to help -s
> 2) Modified mod_symname_hash install/remove based on Kazu's suggestion.
>
> Tao Liu (7):
> Implement install and remove operations for mod_symname_hash
> Integrate symbol_search with mod_symname_hash search
> Extend symname_hash_search with hash table select
> Intergrate symbol_exists with mod_symname_hash search
> Sync module symbols into mod_symtable whenever module symbols change
> Refactor SYMNAME_HASH_INDEX macro to be a function
> Add mod_symname_hash table dump to help -s
>
> defs.h | 3 +-
> kernel.c | 1 +
> symbols.c | 261 +++++++++++++++++++++++++++++++++---------------------
> 3 files changed, 164 insertions(+), 101 deletions(-)
>
For the V6: Acked-by: Lianbo Jiang <lijiang(a)redhat.com>
Thanks.
Thanks for reviewing the patch set and ack!
Thanks,
Tao Liu
> --
> 2.29.2
>
1:29 a.m.
-----Original Message-----
Hi Lijiang,
On Sat, Oct 23, 2021 at 9:27 AM lijiang <lijiang(a)redhat.com> wrote:
>
> On Sat, Oct 16, 2021 at 1:21 PM Tao Liu <ltao(a)redhat.com> wrote:
> >
> > Currently the sequence for symbol_search to search a symbol is: 1) kernel
> > symname hash table, 2) iterate all kernel symbols, 3) iterate all kernel
> > modules and their symbols. In the worst case, if a non-exist symbol been
> > searched, all 3 stages will be went through. The time consuming status for
> > each stage is like:
> >
> > stage 1 stage 2 stage 3
> > 0.007000(ms) 0.593000(ms) 2.421000(ms)
> >
> > stage 3 takes too much time when comparing to stage 1. This patch introduces
> > a symname hash table for kernel modules, to improve the performance of symbol
> > searching.
> >
> > Functions symbol_search and symbol_exists are fundamental and widely used by
> > other crash functions, thus the benefit of performance improvement can
> > get accumulated. For example, "ps -m" and "irq" commands,
which call
> > the functions many times, will become faster with the patch.
> >
> > v5 -> v6:
> >
> > 1) Add mod_symname_hash table dump to help -s
> > 2) Modified mod_symname_hash install/remove based on Kazu's suggestion.
> >
> > Tao Liu (7):
> > Implement install and remove operations for mod_symname_hash
> > Integrate symbol_search with mod_symname_hash search
> > Extend symname_hash_search with hash table select
> > Intergrate symbol_exists with mod_symname_hash search
> > Sync module symbols into mod_symtable whenever module symbols change
> > Refactor SYMNAME_HASH_INDEX macro to be a function
> > Add mod_symname_hash table dump to help -s
> >
> > defs.h | 3 +-
> > kernel.c | 1 +
> > symbols.c | 261 +++++++++++++++++++++++++++++++++---------------------
> > 3 files changed, 164 insertions(+), 101 deletions(-)
> >
>
> For the V6: Acked-by: Lianbo Jiang <lijiang(a)redhat.com>
> Thanks.
Thank you Tao Liu, Philipp and Lianbo for the improvements.
Applied to the both branches with some minor adjustments.
Thanks,
Kazu
982
days inactive
991
days old
devel@lists.crash-utility.osci.io
14 comments
3 participants
participants (3)
-
HAGIO KAZUHITO(萩尾 一仁) -
lijiang -
Tao Liu