Re: [RFC PATCH bpf-next] bpf: Introduce bpf_timer

On Tue, May 25, 2021 at 11:21 AM Alexei Starovoitov
[off-list ref] wrote:

On Mon, May 24, 2021 at 9:59 PM Cong Wang [off-list ref] wrote:

quoted

On Mon, May 24, 2021 at 8:16 PM Cong Wang [off-list ref] wrote:

quoted

On Sun, May 23, 2021 at 9:01 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Fri, May 21, 2021 at 2:37 PM Cong Wang [off-list ref] wrote:

quoted

Hi, Alexei

On Thu, May 20, 2021 at 11:52 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

Introduce 'struct bpf_timer' that can be embedded in most BPF map types
and helpers to operate on it:
long bpf_timer_init(struct bpf_timer *timer, void *callback, int flags)
long bpf_timer_mod(struct bpf_timer *timer, u64 msecs)
long bpf_timer_del(struct bpf_timer *timer)

Like we discussed, this approach would make the timer harder
to be independent of other eBPF programs, which is a must-have
for both of our use cases (mine and Jamal's). Like you explained,
this requires at least another program array, a tail call, a mandatory
prog pinning to work.

That is simply not true.

Which part is not true? The above is what I got from your explanation.

I tried to write some code sketches to use your timer to implement
our conntrack logic, below shows how difficult it is to use,

Was it difficult because you've used tail_call and over complicated
the progs for no good reason?

Using tail call is what I got from you, here is the quote:

"Sure. That's trivially achieved with pinning.
One can have an ingress prog that tailcalls into another prog
that arms the timer with one of its subprogs.
Egress prog can tailcall into the same prog as well.
The ingress and egress progs can be replaced one by one
or removed both together and middle prog can stay alive
if it's pinned in bpffs or held alive by FD."

Here is the link:
https://lore.kernel.org/bpf/CAADnVQK9BgguVorziWgpMktLHuPCgEaKa4fz-KCfhcZtT46teQ@mail.gmail.com/ (local)

quoted

SEC("ingress")
void ingress(struct __sk_buff *skb)
{
        struct tuple tuple;
        // extract tuple from skb

        if (bpf_map_lookup_elem(&timers, &key) == NULL)
                bpf_tail_call(NULL, &jmp_table, 0);
                // here is not reachable unless failure
        val = bpf_map_lookup_elem(&conntrack, &tuple);
        if (val && val->expires < now) {
                bpf_tail_call(NULL, &jmp_table, 1);
                // here is not reachable unless failure
        }
}

SEC("egress")
void egress(struct __sk_buff *skb)
{
        struct tuple tuple;
        // extract tuple from skb

        if (bpf_map_lookup_elem(&timers, &key) == NULL)
                bpf_tail_call(NULL, &jmp_table, 0);
                // here is not reachable unless failure
        val = bpf_map_lookup_elem(&conntrack, &tuple);
        if (val && val->expires < now) {
                bpf_tail_call(NULL, &jmp_table, 1);
                // here is not reachable unless failure

tail_calls are unnecessary. Just call the funcs directly.
All lookups and maps are unnecessary as well.
Looks like a single global timer will be enough for this use case.

Hmm? With your design, a timer has to be embedded into a map
value, you said this is to mimic bpf spinlock.

In general the garbage collection in any form doesn't scale.
The conntrack logic doesn't need it. The cillium conntrack is a great
example of how to implement a conntrack without GC.

That is simply not a conntrack. We expire connections based on
its time, not based on the size of the map where it residents.

Thanks.