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

[Cong Wang <xiyou.wangcong@gmail.com>]

On Tue, May 25, 2021 at 11:21 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, May 24, 2021 at 9:59 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Mon, May 24, 2021 at 8:16 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sun, May 23, 2021 at 9:01 AM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > On Fri, May 21, 2021 at 2:37 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > Hi, Alexei
> > > > >
> > > > > On Thu, May 20, 2021 at 11:52 PM Alexei Starovoitov
> > > > > <alexei.starovoitov@gmail.com> wrote:
> > > > > >
> > > > > > 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/


>
> > 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.