RE: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Maninder Singh]

Hi All,

Thanks for your inputs.

> On Wed 2022-02-09 15:02:06, Luis Chamberlain wrote:
> > On Wed, Feb 09, 2022 at 12:40:38PM +0100, Petr Mladek wrote:
> > > > --- a/include/linux/kallsyms.h
> > > > +++ b/include/linux/kallsyms.h
> > > > @@ -163,6 +163,33 @@ static inline bool kallsyms_show_value(const struct cred *cred)
> > > >          return false;
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_MODULES
> > > > +static inline int fill_minimal_module_info(char *sym, int size, unsigned long value)
> > > > +{
> > > > +        struct module *mod;
> > > > +        unsigned long offset;
> > > > +        int ret = 0;
> > > > +
> > > > +        preempt_disable();
> > > > +        mod = __module_address(value);
> > > > +        if (mod) {
> > > > +                offset = value - (unsigned long)mod->core_layout.base;
> > > > +                snprintf(sym, size - 1, "0x%lx+0x%lx [%s]",
> > > > +                                (unsigned long)mod->core_layout.base, offset, mod->name);
> > > > +
> > > > +                sym[size - 1] = '\0';
> > > > +                ret = 1;
> > > > +        }
> > > > +
> > > > +        preempt_enable();
> > > > +        return ret;
> > > > +}
> > > 
> > > It looks too big for an inlined function. Anyway, we will need
> > > something even more complex, see below.
> > 
> > Interesting, these observations might apply to Vimal's work as well [0].
> > 
> > [0] https://lkml.kernel.org/r/YgKyC4ZRud0JW1PF@xxxxxxxxxxxxxxxxxxxxxx
>  
> Honestly, I am not sure what is the best practice. My understanding is
> that inlined functions are used primary for speed up at runtime.
>  

Main reason of making it inline was:
(1) kallsysm.c was not getting compiled(with disabled config), so could not add defination there.
(2) lib/vsnprintf.c was not correct place to define new function of kallsyms(fill_minimal_module_info)
(3) I thought static int will be part of each .c file which includes kallsyms.h and compiler can make noise for unused functions,
and also increase code size, where as static inline will be added only if some code is calling that function otherwise will be discarded.

But as peter said better version will be to make a new defination of __sprint_symbol (probably in kernel/module.c)
to handle all cases of %ps/S/B/b when KALLSYSMS is disabled.

I will try to prepare changes and share V2 patch.

Thanks,
Maninder Singh

Attachment: rcptInfo.txt
Description: Binary data