Re: [PATCH bpf-next 2/3] bpf,s390: add fsession support for trampolines

Menglong Dong
2026-02-24T02:53:51Z

On 2026/2/23 20:14 Ilya Leoshkevich <iii@linux.ibm.com> write:
>
> On 2/23/26 09:40, Menglong Dong wrote:
> > Implement BPF_TRACE_FSESSION support for s390. The logic here is similar
> > to what we did in x86_64.
> >
> > In order to simply the logic, we factor out the function invoke_bpf() for
> > fentry and fexit.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > ---
> > arch/s390/net/bpf_jit_comp.c | 68 +++++++++++++++++++++++++++++++-----
> > 1 file changed, 59 insertions(+), 9 deletions(-)
>
>
> Thank you for contributing this series!
>
>
> In general this all looks very reasonable; I believe I found a few nits,
> please take a look at my comments below.
>
>
> >
> > diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> > index 763d2491dfa3..ea0c81f18ece 100644
[...]
> >
> > - for (i = 0; i < fentry->nr_links; i++)
> > - if (invoke_bpf_prog(tjit, m, fentry->links[i],
> > - flags & BPF_TRAMP_F_RET_FENTRY_RET))
> > + if (fsession_cnt) {
> > + /* clear all the session cookies' value */
>
> Here and below: please use the existing style for single-line comments:
> full sentence starting with a capital letter and ending with a dot.
> Unfortunately the JIT is somewhat inconsistent in this area, but let's
> at least not introduce a new comment style here.

OK, I'll fix up the comments style in the next version.

>
> > + for (i = 0; i < cookie_cnt; i++)
> > + emit_store_stack_imm64(jit, REG_0, cookie_off + 8 * i, 0);
> > + /* clear the return value to make sure fentry always gets 0 */
> > + emit_store_stack_imm64(jit, REG_0, tjit->retval_off, 0);
> > + }
>
> Would it make sense to clear cookies right after invoke_bpf_prog() and
> only if bpf_prog_calls_session_cookie() is true?

I think it doesn't make much sense. We only reserve session cookies for
the fsession progs that called bpf_session_cookie(), which means that
all the cookies that we reserved here will be used. Right?

>
> Going one step further and reducing the size of cookies array would
> probably be ideal, but I guess this will complicate things
> significantly, so I'm not suggesting to do this.
>
> > +
> > + if (fentry->nr_links) {
>
> I think it's okay to drop this condition, invoke_bpf() is most likely
> inlined and the loop will automatically have 0 iterations in this case.

OK, I'll drop this checking.

>
> > + if (invoke_bpf(tjit, m, fentry, flags & BPF_TRAMP_F_RET_FENTRY_RET,
> > + func_meta, cookie_off))
> > return -EINVAL;
> > + }
> >
> > if (fmod_ret->nr_links) {
> > /*
> > @@ -2842,11 +2880,18 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
> > EMIT6_PCREL_RILC(0xc0040000, 0, (u64)im->ip_epilogue);
> > }
> >
> > + /* set the "is_return" flag for fsession */
> > + func_meta |= (1ULL << BPF_TRAMP_IS_RETURN_SHIFT);
> > + if (fsession_cnt)
> > + emit_store_stack_imm64(jit, REG_W0, tjit->func_meta_off,
> > + func_meta);
> > +
> > /* do_fexit: */
> > tjit->do_fexit = jit->prg;
> > - for (i = 0; i < fexit->nr_links; i++)
> > - if (invoke_bpf_prog(tjit, m, fexit->links[i], false))
> > + if (fexit->nr_links) {
> Same as for fentry.

ACK.

Thanks!
Menglong Dong

> > + if (invoke_bpf(tjit, m, fexit, false, func_meta, cookie_off))
> > return -EINVAL;
> > + }
> >
> > if (flags & BPF_TRAMP_F_CALL_ORIG) {
> > im->ip_epilogue = jit->prg_buf + jit->prg;
> > @@ -2951,6 +2996,11 @@ bool bpf_jit_supports_arena(void)
> > return true;
> > }
> >
> > +bool bpf_jit_supports_fsession(void)
> > +{
> > + return true;
> > +}
> > +
> > bool bpf_jit_supports_insn(struct bpf_insn *insn, bool in_arena)
> > {
> > if (!in_arena)
>
>