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

[Menglong Dong]

On 2026/2/23 20:14 Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> >   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)
> 
>