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;
Date:   Fri, 15 Jul 2022 15:12:17 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Song Liu <songliubraving@fb.com>
Cc:     Song Liu <song@kernel.org>, Networking <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "jolsa@kernel.org" <jolsa@kernel.org>,
        "mhiramat@kernel.org" <mhiramat@kernel.org>
Subject: Re: [PATCH v2 bpf-next 3/5] ftrace: introduce
 FTRACE_OPS_FL_SHARE_IPMODIFY
Message-ID: <20220715151217.141dc98f@gandalf.local.home>
In-Reply-To: <0CE9BF90-B8CE-40F6-A431-459936157B78@fb.com>
References: <20220602193706.2607681-1-song@kernel.org>
        <20220602193706.2607681-4-song@kernel.org>
        <20220713203343.4997eb71@rorschach.local.home>
        <AA1D9833-DF67-4AFD-815C-DD89AB57B3A2@fb.com>
        <20220714204817.2889e280@rorschach.local.home>
        <6A7EF1C7-471B-4652-99C1-87C72C223C59@fb.com>
        <20220714224646.62d49e36@rorschach.local.home>
        <170BE89A-101C-4B25-A664-5E47A902DB83@fb.com>
        <0CE9BF90-B8CE-40F6-A431-459936157B78@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 15 Jul 2022 17:42:55 +0000
Song Liu <songliubraving@fb.com> wrote:


> A quick update and ask for feedback/clarification.
> 
> Based on my understanding, you recommended calling ops_func() from 
> __ftrace_hash_update_ipmodify() and in ops_func() the direct trampoline
> may make changes to the trampoline. Did I get this right?
> 
> 
> I am going towards this direction, but hit some issue. Specifically, in 
> __ftrace_hash_update_ipmodify(), ftrace_lock is already locked, so the 
> direct trampoline cannot easily make changes with 
> modify_ftrace_direct_multi(), which locks both direct_mutex and 
> ftrace_mutex. 
> 
> One solution would be have no-lock version of all the functions called
> by modify_ftrace_direct_multi(), but that's a lot of functions and the
> code will be pretty ugly. The alternative would be the logic in v2: 
> __ftrace_hash_update_ipmodify() returns -EAGAIN, and we make changes to 
> the direct trampoline in other places: 
> 
> 1) if DIRECT ops attached first, the trampoline is updated in 
>    prepare_direct_functions_for_ipmodify(), see 3/5 of v2;
> 
> 2) if IPMODIFY ops attached first, the trampoline is updated in
>    bpf_trampoline_update(), see "goto again" path in 5/5 of v2. 
> 
> Overall, I think this way is still cleaner. What do you think about this?

What about if we release the lock when doing the callback?

Then we just need to make sure things are the same after reacquiring the
lock, and if they are different, we release the lock again and do the
callback with the new update. Wash, rinse, repeat, until the state is the
same before and after the callback with locks acquired?

This is a common way to handle callbacks that need to do something that
takes the lock held before doing a callback.

The reason I say this, is because the more we can keep the accounting
inside of ftrace the better.

Wouldn't this need to be done anyway if BPF was first and live kernel
patching needed the update? An -EAGAIN would not suffice.

-- Steve