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

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

On Sat, May 29, 2021 at 11:36:08PM -0700, Cong Wang wrote:
> 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."

That was in the context of doing auto-cancel of timers.
There is only one choice to make. Either auto-cancel or not.
That quote was during the time when auto-cancel felt as it would fit
the FD model better.
We auto-detach on close(link_fd) and auto-unload on close(prog_fd).
The armed timer would prevent that and that promise felt
necessary to keep. But disappearing timer is a bigger surprise
to users than not auto-unloading progs.
Hence this patch is doing prog_refcnt++ in bpf_timer_start.
Please see other emails threads in v1 patch set.

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

The global data is a map.
When spin_lock was introduced there was no global data concept.

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

Sounds like your goal is to replicate existing kernel conntrack
as bpf program by doing exactly the same algorithm and repeating
the same mistakes. Then add kernel conntrack functions to allow list
of kfuncs (unstable helpers) and call them from your bpf progs.