Re: [Crash-utility] [PATCH v5 1/6] Implement install and remove operations for mod_symname_hash

-----Original Message----- > Hi Tao Liu, > > Thanks for the update. > > -----Original Message----- > > 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. > > > > Install spn previous to sp: > > > > If sp is the start of bucket, and > > 1) spn->value is smaller than sp->value, or > > 2) spn->value equals sp->value and spn is smaller than sp. > > > > Install spn next to sp: > > > > 1) spn->value is larger than sp->value, or > > 2) spn->value equals to sp->value and spn is larger than sp, > > spn will stay behind of sp. And if > > 1) sp->name_hash_next is NULL or > > 2) sp->name_hash_next->value larger than spn->value > > spn will be next to sp. > > > > 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, so we use spn > > and sp to determine the sequence. > > If sp->value equals to spn->value, they are in the same module, and > spn should be larger than sp, according to mod_symtable_hash_install_range(). > Then is it OK to insert spn when sp->value becomes larger than spn->value > like this? > > sp = st->mod_symname_hash[index]; > if (!sp) { > st->mod_symname_hash[index] = spn; > spn->name_hash_next = NULL; > return; > } > for (; sp; sp = sp->name_hash_next) { > if (sp->value > spn->value || !sp->name_hash_next) { > spn->name_hash_next = sp->name_hash_next; > sp->name_hash_next = spn; > return; > } > } sorry, I was confused, this is a fixed one: sp = st->mod_symname_hash[index]; if (!sp || sp->value > spn->value) { spn->name_hash_next = sp; st->mod_symname_hash[index] = spn; return; } for (; sp; sp = sp->name_hash_next) { if (!sp->name_hash_next || sp->name_hash_next->value > spn->value) { spn->name_hash_next = sp->name_hash_next; sp->name_hash_next = spn; return; } }

Thanks, Kazu > > > > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > --- > > defs.h | 1 + > > symbols.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 82 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..7fb2df7 100644 > > --- a/symbols.c > > +++ b/symbols.c > > @@ -1157,6 +1157,87 @@ 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, *tmp; > > + int index; > > + > > + if (!spn) > > + return; > > + > > + index = SYMNAME_HASH_INDEX(spn->name); > > + > > + sp = st->mod_symname_hash[index]; > > + > > + if (!sp || > > + (spn->value < sp->value) || > > + (spn->value == sp->value && spn < sp)) { > > + /* > > + * Install spn to the previous position of sp. > > + */ > > + st->mod_symname_hash[index] = spn; > > + spn->name_hash_next = sp; > > + } else { > > + for (; sp; sp = sp->name_hash_next) { > > + if ((spn->value > sp->value) || > > + (spn->value == sp->value && spn > sp)) { > > + if (!sp->name_hash_next || > > + spn->value < sp->name_hash_next->value) { > > + /* > > + * Install spn to the next position of sp. > > + */ > > + tmp = sp->name_hash_next; > > + sp->name_hash_next = spn; > > + spn->name_hash_next = tmp; > > + break; > > + } > > + } > > + } > > + } > > +} > > + > > +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; > > I think now we can return here, > > > + > > + 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; > > and here, too. > > For the other patches, please wait for a while. > > Thanks, > Kazu > > > > + } > > +} > > + > > +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 > > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://listman.redhat.com/mailman/listinfo/crash-utility