[bpf-next,17/18] bpf: add bpf_wq_start

Commit Message

Benjamin Tissoires April 16, 2024, 2:08 p.m. UTC

again, copy/paste from bpf_timer_start().

Signed-off-by: Benjamin Tissoires <[e-mail]>
---
 kernel/bpf/helpers.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Alexei Starovoitov April 19, 2024, 6:18 a.m. UTC | #1

On Tue, Apr 16, 2024 at 04:08:30PM +0200, Benjamin Tissoires wrote:
> again, copy/paste from bpf_timer_start().
> 
> Signed-off-by: Benjamin Tissoires <[e-mail]>
> ---
>  kernel/bpf/helpers.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index e5c8adc44619..ed5309a37eda 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2728,6 +2728,29 @@ __bpf_kfunc int bpf_wq_init(struct bpf_wq *wq, void *map, unsigned int flags)
>  	return __bpf_async_init(async, map, flags, BPF_ASYNC_TYPE_WQ);
>  }
>  
> +__bpf_kfunc int bpf_wq_start(struct bpf_wq *wq, unsigned int flags)
> +{
> +	struct bpf_async_kern *async = (struct bpf_async_kern *)wq;
> +	struct bpf_work *w;
> +	int ret = 0;
> +
> +	if (in_nmi())
> +		return -EOPNOTSUPP;
> +	if (flags)
> +		return -EINVAL;
> +	__bpf_spin_lock_irqsave(&async->lock);
> +	w = async->work;
> +	if (!w || !w->cb.prog) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	schedule_work(&w->work);
> +out:
> +	__bpf_spin_unlock_irqrestore(&async->lock);

Looks like you're not adding wq_cancel kfunc in this patch set and
it's probably a good thing not to expose async cancel to bpf users,
since it's a foot gun.
Even when we eventually add wq_cancel_sync kfunc it will not be
removing a callback.
So we can drop spinlock here.
READ_ONCE of w and cb would be enough.
Since they cannot get back to NULL once init-ed and cb is set.

Benjamin Tissoires April 19, 2024, 3:14 p.m. UTC | #2

On Apr 18 2024, Alexei Starovoitov wrote:
> On Tue, Apr 16, 2024 at 04:08:30PM +0200, Benjamin Tissoires wrote:
> > again, copy/paste from bpf_timer_start().
> > 
> > Signed-off-by: Benjamin Tissoires <[e-mail]>
> > ---
> >  kernel/bpf/helpers.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index e5c8adc44619..ed5309a37eda 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -2728,6 +2728,29 @@ __bpf_kfunc int bpf_wq_init(struct bpf_wq *wq, void *map, unsigned int flags)
> >  	return __bpf_async_init(async, map, flags, BPF_ASYNC_TYPE_WQ);
> >  }
> >  
> > +__bpf_kfunc int bpf_wq_start(struct bpf_wq *wq, unsigned int flags)
> > +{
> > +	struct bpf_async_kern *async = (struct bpf_async_kern *)wq;
> > +	struct bpf_work *w;
> > +	int ret = 0;
> > +
> > +	if (in_nmi())
> > +		return -EOPNOTSUPP;
> > +	if (flags)
> > +		return -EINVAL;
> > +	__bpf_spin_lock_irqsave(&async->lock);
> > +	w = async->work;
> > +	if (!w || !w->cb.prog) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	schedule_work(&w->work);
> > +out:
> > +	__bpf_spin_unlock_irqrestore(&async->lock);
> 
> Looks like you're not adding wq_cancel kfunc in this patch set and
> it's probably a good thing not to expose async cancel to bpf users,
> since it's a foot gun.

Honestly I just felt the patch series was big enough for a PoC and
comparison with sleepable bpf_timer. But if we think this needs not to
be added, I guess that works too :)

> Even when we eventually add wq_cancel_sync kfunc it will not be
> removing a callback.

Yeah, I understood that bit :)

> So we can drop spinlock here.
> READ_ONCE of w and cb would be enough.
> Since they cannot get back to NULL once init-ed and cb is set.

Great, thanks for the review (and the other patches).

I'll work toward v2.

Cheers,
Benjamin

Alexei Starovoitov April 19, 2024, 3:49 p.m. UTC | #3

On Fri, Apr 19, 2024 at 8:14 AM Benjamin Tissoires <[e-mail]> wrote:
>
>
> Honestly I just felt the patch series was big enough for a PoC and
> comparison with sleepable bpf_timer. But if we think this needs not to
> be added, I guess that works too :)

It certainly did its job to compare the two and imo bpf_wq with kfunc approach
looks cleaner overall and will be easier to extend in the long term.

I mean that we'll be adding 3 kfuncs initially:
bpf_wq_init, bpf_wq_start, bpf_wq_set_callback.

imo that's good enough to land it and get some exposure.
I'll be using it right away to refactor bpf_arena_alloc.h into
actual arena allocator for bpf progs that is not just a selftest.

I'm currently working on locks for bpf_arena.
Kumar has a patch set that adds bpf_preempt_disble kfunc and
coupled with bpf_wq we'll have all mechanisms to build
arbitrary data structures/algorithms as bpf programs.

Benjamin Tissoires April 19, 2024, 4:01 p.m. UTC | #4

On Apr 19 2024, Alexei Starovoitov wrote:
> On Fri, Apr 19, 2024 at 8:14 AM Benjamin Tissoires <[e-mail]> wrote:
> >
> >
> > Honestly I just felt the patch series was big enough for a PoC and
> > comparison with sleepable bpf_timer. But if we think this needs not to
> > be added, I guess that works too :)
> 
> It certainly did its job to compare the two and imo bpf_wq with kfunc approach
> looks cleaner overall and will be easier to extend in the long term.

Yeah. I agree. I'm also glad we pick the bpf_wq approach as I gave it a
lot more care :)

Talking about extending, I think I'll need delayed_work soon enough.
Most of the time when I receive an input event, the device is preventing
any communication with it, and with plain bpf_wq, it's likely that when
the code kicks in the device won't have processed the current input,
meaning to a useless retry. With delayed_works, I can schedule it
slightly later, and have a higher chance of not having to retry.

I've got a quick hack locally that I can submit once this series get
merged.

> 
> I mean that we'll be adding 3 kfuncs initially:
> bpf_wq_init, bpf_wq_start, bpf_wq_set_callback.
> 
> imo that's good enough to land it and get some exposure.

sounds good to me.

> I'll be using it right away to refactor bpf_arena_alloc.h into
> actual arena allocator for bpf progs that is not just a selftest.
> 
> I'm currently working on locks for bpf_arena.
> Kumar has a patch set that adds bpf_preempt_disble kfunc and
> coupled with bpf_wq we'll have all mechanisms to build
> arbitrary data structures/algorithms as bpf programs.

Oh. I did not realize that it was that needed for outside of my
playground. That's good to hear :)

Cheers,
Benjamin

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index e5c8adc44619..ed5309a37eda 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2728,6 +2728,29 @@  __bpf_kfunc int bpf_wq_init(struct bpf_wq *wq, void *map, unsigned int flags)
 	return __bpf_async_init(async, map, flags, BPF_ASYNC_TYPE_WQ);
 }
 
+__bpf_kfunc int bpf_wq_start(struct bpf_wq *wq, unsigned int flags)
+{
+	struct bpf_async_kern *async = (struct bpf_async_kern *)wq;
+	struct bpf_work *w;
+	int ret = 0;
+
+	if (in_nmi())
+		return -EOPNOTSUPP;
+	if (flags)
+		return -EINVAL;
+	__bpf_spin_lock_irqsave(&async->lock);
+	w = async->work;
+	if (!w || !w->cb.prog) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	schedule_work(&w->work);
+out:
+	__bpf_spin_unlock_irqrestore(&async->lock);
+	return ret;
+}
+
 __bpf_kfunc int bpf_wq_set_callback_impl(struct bpf_wq *wq,
 					 int (callback_fn)(void *map, int *key, struct bpf_wq *wq),
 					 unsigned int flags__k,
@@ -2821,6 +2844,7 @@  BTF_ID_FLAGS(func, bpf_dynptr_clone)
 BTF_ID_FLAGS(func, bpf_modify_return_test_tp)
 BTF_ID_FLAGS(func, bpf_wq_init)
 BTF_ID_FLAGS(func, bpf_wq_set_callback_impl)
+BTF_ID_FLAGS(func, bpf_wq_start)
 BTF_KFUNCS_END(common_btf_ids)
 
 static const struct btf_kfunc_id_set common_kfunc_set = {