Re: [PATCH 3/7] bpf: Rename BPF_XADD and prepare to encode other atomics in .imm

[Yonghong Song <yhs@fb.com>]

On 11/24/20 3:02 AM, Brendan Jackman wrote:
> On Mon, Nov 23, 2020 at 03:54:38PM -0800, Yonghong Song wrote:
>>
>>
>> On 11/23/20 9:31 AM, Brendan Jackman wrote:
> ...
>>> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
>>> index 0207b6ea6e8a..897634d0a67c 100644
>>> --- a/arch/arm/net/bpf_jit_32.c
>>> +++ b/arch/arm/net/bpf_jit_32.c
>>> @@ -1620,10 +1620,9 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
>>>    		}
>>>    		emit_str_r(dst_lo, tmp2, off, ctx, BPF_SIZE(code));
>>>    		break;
>>> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_W:
>>> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_DW:
>>> +	/* Atomic ops */
>>> +	case BPF_STX | BPF_ATOMIC | BPF_W:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
>>>    		goto notyet;
>>>    	/* STX: *(size *)(dst + off) = src */
>>>    	case BPF_STX | BPF_MEM | BPF_W:
>>> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
>>> index ef9f1d5e989d..f7b194878a99 100644
>>> --- a/arch/arm64/net/bpf_jit_comp.c
>>> +++ b/arch/arm64/net/bpf_jit_comp.c
>>> @@ -875,10 +875,18 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx,
>>>    		}
>>>    		break;
>>> -	/* STX XADD: lock *(u32 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_W:
>>> -	/* STX XADD: lock *(u64 *)(dst + off) += src */
>>> -	case BPF_STX | BPF_XADD | BPF_DW:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_W:
>>> +	case BPF_STX | BPF_ATOMIC | BPF_DW:
>>> +		if (insn->imm != BPF_ADD) {
>>
>> Currently BPF_ADD (although it is 0) is encoded at bit 4-7 of imm.
>> Do you think we should encode it in 0-3 to make such a comparision
>> and subsequent insn->imm = BPF_ADD making more sense?
> 
> Sorry not quite sure what you mean by this... I think encoding in 4-7 is
> nice because it lets us use BPF_OP. In this patchset wherever we have
> (insn->imm == BPF_ADD) meaning "this is a traditional XADD without
> fetch" and (BPF_OP(insn->imm) == BPF_ADD) meaning "this is an atomic
> add, either with or without a fetch".
> 
> Does that answer the question...?

Yes, thanks for explanation. It is a little bit odd but certainly 
acceptable.

> 
>>> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> index 0a721f6e8676..0767d7b579e9 100644
>>> --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
>>> @@ -3109,13 +3109,19 @@ mem_xadd(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta, bool is64)
>>>    	return 0;
>>>    }
>>> -static int mem_xadd4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>> +static int mem_atm4(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>>    {
>>> +	if (meta->insn.off != BPF_ADD)
>>> +		return -EOPNOTSUPP;
>>
>> meta->insn.imm?
>>
>>> +
>>>    	return mem_xadd(nfp_prog, meta, false);
>>>    }
>>> -static int mem_xadd8(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>> +static int mem_atm(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
>>>    {
>>> +	if (meta->insn.off != BPF_ADD)
>>
>> meta->insn.imm?
> 
> Yikes, thanks for spotting these! Apparently I wasn't even compiling
> this code.
>