Ilya Leoshkevich <iii@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> Closes: https://lore.kernel.org/bpf/mb61p34qvq3wf.fsf@xxxxxxxxxx/
> Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> ---
> 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
