Re: [PATCH bpf-next] s390/bpf: Fully order atomic "add", "and", "or" and "xor"

[Puranjay Mohan <puranjay12@gmail.com>]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> BPF_ATOMIC_OP() macro documentation states that "BPF_ADD | BPF_FETCH"
> should be the same as atomic_fetch_add(), which is currently not the
> case on s390x: the synchronization instruction "bcr 14,0" is missing.
>
> This should not be a problem in practice, because s390x is allowed to
> reorder only stores with subsequent fetches from different addresses.
> Still, just to be on the safe side, and also for consistency, emit the
> synchronization instruction.
>
> Note that it's not required to do this for BPF_XCHG and BPF_CMPXCHG,
> because COMPARE AND SWAP performs serialization itself.
>
> Fixes: ba3b86b9cef0 ("s390/bpf: Implement new atomic ops")
> Reported-by: Puranjay Mohan <puranjay12@gmail.com>
> Closes: https://lore.kernel.org/bpf/mb61p34qvq3wf.fsf@kernel.org/
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  arch/s390/net/bpf_jit_comp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
> index fa2f824e3b06..a0dfb3f665ab 100644
> --- a/arch/s390/net/bpf_jit_comp.c
> +++ b/arch/s390/net/bpf_jit_comp.c
> @@ -1427,6 +1427,8 @@ static noinline int bpf_jit_insn(struct bpf_jit *jit, struct bpf_prog *fp,
>  	EMIT6_DISP_LH(0xeb000000, is32 ? (op32) : (op64),		\
>  		      (insn->imm & BPF_FETCH) ? src_reg : REG_W0,	\
>  		      src_reg, dst_reg, off);				\
> +	/* bcr 14,0 - see atomic_fetch_{add,and,or,xor}() */		\
> +	_EMIT2(0x07e0);							\

Shouldn't this be:

        /* bcr 14,0 - see atomic_fetch_{add,and,or,xor}() */ 
        if (insn->imm & BPF_FETCH)
                _EMIT2(0x07e0); 

The barrier is only needed for the BPF_FETCH varient and I am assuming
that the barrier has some overhead. So we should not emit it for all
operations.


Thanks,
Puranjay