Re: [PATCH bpf-next 1/3] bpf: add bpf queue map

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Aug 09, 2018 at 09:51:49AM -0500, Mauricio Vasquez wrote:
> 
> > Agree that existing ops are not the right alias, but deferring to user
> > space as inline function also doesn't really seem like a good fit, imho,
> > so I'd prefer rather to have something native. (Aside from that, the
> > above inline bpf_pop() would also race between CPUs.)
> 
> I think we should have push/pop/peek syscalls as well, having a bpf_pop()
> that is race prone would create problems. Users expected maps operations to
> be safe, so having one that is not will confuse them.

agree the races are not acceptable.
How about a mixed solution:
- introduce bpf_push/pop/peak helpers that programs will use, so
  they don't need to pass useless key=NULL
- introduce map->ops->lookup_and_delete and map->ops->lookup_or_init
  that prog-side helpers can use and syscall has 1-1 mapping for

Native lookup_or_init() helper for programs and syscall is badly missing.
Most of the bcc scripts use it and bcc has a racy workaround.
Similarly lookup_and_delete() syscall is 1-1 to pop() for stack/queue
and useful for regular hash maps.

At the end for stack/queue map the programs will use:
int bpf_push(map, value);
value_or_null = bpf_pop(map); // guaranteed non-racy for multi-cpu
value_or_null = bpf_peak(map); // racy if 2+ cpus doing it

from syscall:
bpf_map_lookup_elem(map, NULL, &value); // returns top of stack
bpf_map_lookup_and_delete_elem(map, NULL, &value); // returns top and deletes top atomically
bpf_map_update_elem(map, NULL, &value); // pushes new value into stack atomically

Eventually hash and other maps will implement bpf_map_lookup_and_delete()
for both bpf progs and syscall.

The main point that prog-side api doesn't have to match 1-1 to syscall-side,
since they're different enough already.
Like lookup_or_init() is badly needed for programs, but unnecessary for syscall.

Thoughts?