Hi Kazu,
On Fri, Oct 15, 2021 at 8:19 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> -----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;
> }
> }
>
Agreed, thanks for the suggestion! I have integrated the fix in the v6 patch.
Thanks,
Tao Liu
> Thanks,
> Kazu
>
> >
> > >
> > > Signed-off-by: Tao Liu <ltao@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx
> > https://listman.redhat.com/mailman/listinfo/crash-utility
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
