On Mon, Nov 23, 2020 at 05:32:01PM +0000, Brendan Jackman wrote:
> These are the operations that implement atomic exchange and
> compare-exchange.
>
> They are peculiarly named because of the presence of the separate
> FETCH field that tells you whether the instruction writes the value
> back to the src register. Neither operation is supported without
> BPF_FETCH:
>
> - BPF_CMPSET without BPF_FETCH (i.e. an atomic compare-and-set
> without knowing whether the write was successfully) isn't implemented
> by the kernel, x86, or ARM. It would be a burden on the JIT and it's
> hard to imagine a use for this operation, so it's not supported.
>
> - BPF_SET without BPF_FETCH would be bpf_set, which has pretty
> limited use: all it really lets you do is atomically set 64-bit
> values on 32-bit CPUs. It doesn't imply any barriers.
...
> - if (insn->imm & BPF_FETCH) {
> + switch (insn->imm) {
> + case BPF_SET | BPF_FETCH:
> + /* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
> + EMIT1(0x87);
> + break;
> + case BPF_CMPSET | BPF_FETCH:
> + /* r0 = atomic_cmpxchg(*(u32/u64*)(dst_reg + off), r0, src_reg); */
> + EMIT2(0x0F, 0xB1);
> + break;
...
> /* atomic op type fields (stored in immediate) */
> +#define BPF_SET 0xe0 /* atomic write */
> +#define BPF_CMPSET 0xf0 /* atomic compare-and-write */
> +
> #define BPF_FETCH 0x01 /* fetch previous value into src reg */
I think SET in the name looks odd.
I understand that you picked this name so that SET|FETCH together would form
more meaningful combination of words, but we're not planning to support SET
alone. There is no such instruction in a cpu. If we ever do test_and_set it
would be something different.
How about the following instead:
+#define BPF_XCHG 0xe1 /* atomic exchange */
+#define BPF_CMPXCHG 0xf1 /* atomic compare exchange */
In other words get that fetch bit right away into the encoding.
Then the switch statement above could be:
+ switch (insn->imm) {
+ case BPF_XCHG:
+ /* src_reg = atomic_chg(*(u32/u64*)(dst_reg + off), src_reg); */
+ EMIT1(0x87);
...
+ case BPF_ADD | BPF_FETCH:
...
+ case BPF_ADD:
