Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Mon, 14 Nov 2022 14:26:19 +0100
To:     "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com>
Cc:     Jiri Olsa <olsajiri@gmail.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Jiri Kosina <jikos@kernel.org>,
        Miroslav Benes <mbenes@suse.cz>,
        Petr Mladek <pmladek@suse.com>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Masahiro Yamada <masahiroy@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        linux-modules@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        David Laight <David.Laight@aculab.com>,
        Stephen Rothwell <sfr@canb.auug.org.au>
Subject: Re: [PATCH v8 7/9] livepatch: Improve the search performance of
 module_kallsyms_on_each_symbol()
Message-ID: <Y3JB++KOXxMWWX35@krava>
References: <20221102084921.1615-1-thunder.leizhen@huawei.com>
 <20221102084921.1615-8-thunder.leizhen@huawei.com>
 <Y3HyrIwlZPYM8zYd@krava>
 <050b7513-4a20-75c7-0574-185004770329@huawei.com>
 <Y3IJ5GjrXBYDbfnA@krava>
 <ad637488-930e-33c1-558c-fc03d848afa8@huawei.com>
 <Y3IY6gzDtk1ze3u7@krava>
 <955eebae-0b36-d13f-0199-2f1b32af7da6@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <955eebae-0b36-d13f-0199-2f1b32af7da6@huawei.com>
Precedence: bulk

On Mon, Nov 14, 2022 at 07:30:16PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/11/14 18:31, Jiri Olsa wrote:
> > On Mon, Nov 14, 2022 at 06:00:38PM +0800, Leizhen (ThunderTown) wrote:
> >>
> >>
> >> On 2022/11/14 17:27, Jiri Olsa wrote:
> >>> On Mon, Nov 14, 2022 at 04:50:25PM +0800, Leizhen (ThunderTown) wrote:
> >>>>
> >>>> On 2022/11/14 15:47, Jiri Olsa wrote:
> >>>>> On Wed, Nov 02, 2022 at 04:49:19PM +0800, Zhen Lei wrote:
> >>>>>> Currently we traverse all symbols of all modules to find the specified
> >>>>>> function for the specified module. But in reality, we just need to find
> >>>>>> the given module and then traverse all the symbols in it.
> >>>>> hi,
> >>>>> sorry for delayed answer, I did not notice this until Stephen's email
> >>>>> about merge issue with recent bpf change [1]
> >>>>>
> >>>>>> Let's add a new parameter 'const char *modname' to function
> >>>>>> module_kallsyms_on_each_symbol(), then we can compare the module names
> >>>>> we have use case for iterating all modules and their symbols when we
> >>>>> want to resolve passed addresses for tracing
> >>>>>
> >>>>> we don't have 'modname' that we could pass, we need to iterate all modules
> >>>>>
> >>>>> so perhaps this could be made optional like with passing NULL for modname?
> >>>> The deletion of modname was suggested by Petr Mladek. The reason is that
> >>>> everyone passes modname as NULL, there was no actual demand at the time.
> >>>> https://lkml.org/lkml/2022/9/20/682
> >>>>
> >>>>>> directly in this function and call hook 'fn' after matching. And the
> >>>>>> parameter 'struct module *' in the hook 'fn' can also be deleted.
> >>>>> we need 'struct module *' argument in the callback as well because we are
> >>>>> taking the module reference if we trace function in it, so it wont get
> >>>>> unloaded
> >>>>>
> >>>>> please let me know if I should do the change or can help in any way
> >>>> It seems that we should take the module reference before invoking callback
> >>>> and put it after it is called, without passing modname.
> >>> we take the module ref only if we (callback) find the traced address in
> >>> the module, we don't have the module object before
> >>>
> >>> jirka
> >>>
> >>
> >> Do it in function module_kallsyms_on_each_symbol()?
> >>
> >> But I just saw that mutex_lock(&module_mutex) protection is already
> >> provided in this function. So reference counting protection may not
> >> be required.
> > 
> > we take the module ref so it won't unload even outside of the
> > module_kallsyms_on_each_symbol function
> 
> There's another way to do it, but it's more time consuming.
> 
> struct module *__module_text_address(unsigned long addr);
> struct module *__module_address(unsigned long addr);
> 
> Which way do you think is more appropriate?

I think it'd be best to keep the module argument in the callback,
I don't see the harm.. AFAICS it was removed because you thought
nobody was using it, but that's not the case

using __module_text_address/__module_address might be doable and
it might be fast enough, because it's using rbtree

I'll check on that, meanwhile if we could keep the module argument,
that'd be great

thanks,
jirka