diff mbox series

[v2,bpf-next,13/13] bpf: Convert bpf_cpumask to bpf_mem_cache_free_rcu.

Message ID 20230624031333.96597-14-alexei.starovoitov@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Introduce bpf_mem_cache_free_rcu(). | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for veristat
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 34 this patch: 34
netdev/cc_maintainers warning 8 maintainers not CCed: yhs@fb.com kpsingh@kernel.org martin.lau@linux.dev john.fastabend@gmail.com song@kernel.org sdf@google.com jolsa@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 34 this patch: 34
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain_full }}
bpf/vmtest-bpf-next-VM_Test-2 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for veristat

Commit Message

Alexei Starovoitov June 24, 2023, 3:13 a.m. UTC 
From: Alexei Starovoitov <ast@kernel.org>

Convert bpf_cpumask to bpf_mem_cache_free_rcu.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/cpumask.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

David Vernet June 26, 2023, 3:42 p.m. UTC | #1 
On Fri, Jun 23, 2023 at 08:13:33PM -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Convert bpf_cpumask to bpf_mem_cache_free_rcu.
> 
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Acked-by: David Vernet <void@manifault.com>

LGTM, thanks for cleaning this up. I left one drive-by comment /
observation below, but it's not a blocker for this patch / series.

> ---
>  kernel/bpf/cpumask.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
> index 938a60ff4295..6983af8e093c 100644
> --- a/kernel/bpf/cpumask.c
> +++ b/kernel/bpf/cpumask.c
> @@ -9,7 +9,6 @@
>  /**
>   * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
>   * @cpumask:	The actual cpumask embedded in the struct.
> - * @rcu:	The RCU head used to free the cpumask with RCU safety.
>   * @usage:	Object reference counter. When the refcount goes to 0, the
>   *		memory is released back to the BPF allocator, which provides
>   *		RCU safety.
> @@ -25,7 +24,6 @@
>   */
>  struct bpf_cpumask {
>  	cpumask_t cpumask;
> -	struct rcu_head rcu;
>  	refcount_t usage;
>  };
>  
> @@ -82,16 +80,6 @@ __bpf_kfunc struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask)
>  	return cpumask;
>  }
>  
> -static void cpumask_free_cb(struct rcu_head *head)
> -{
> -	struct bpf_cpumask *cpumask;
> -
> -	cpumask = container_of(head, struct bpf_cpumask, rcu);
> -	migrate_disable();
> -	bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
> -	migrate_enable();
> -}
> -
>  /**
>   * bpf_cpumask_release() - Release a previously acquired BPF cpumask.
>   * @cpumask: The cpumask being released.
> @@ -102,8 +90,12 @@ static void cpumask_free_cb(struct rcu_head *head)
>   */
>  __bpf_kfunc void bpf_cpumask_release(struct bpf_cpumask *cpumask)
>  {
> -	if (refcount_dec_and_test(&cpumask->usage))
> -		call_rcu(&cpumask->rcu, cpumask_free_cb);
> +	if (!refcount_dec_and_test(&cpumask->usage))
> +		return;
> +
> +	migrate_disable();
> +	bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
> +	migrate_enable();

The fact that callers have to disable migration like this in order to
safely free the memory feels a bit leaky. Is there any reason we can't
move this into bpf_mem_{cache_}free_rcu()?
Alexei Starovoitov June 26, 2023, 4:09 p.m. UTC | #2 
On Mon, Jun 26, 2023 at 8:42 AM David Vernet <void@manifault.com> wrote:
>
> On Fri, Jun 23, 2023 at 08:13:33PM -0700, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Convert bpf_cpumask to bpf_mem_cache_free_rcu.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> Acked-by: David Vernet <void@manifault.com>
>
> LGTM, thanks for cleaning this up. I left one drive-by comment /
> observation below, but it's not a blocker for this patch / series.
>
> > ---
> >  kernel/bpf/cpumask.c | 20 ++++++--------------
> >  1 file changed, 6 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
> > index 938a60ff4295..6983af8e093c 100644
> > --- a/kernel/bpf/cpumask.c
> > +++ b/kernel/bpf/cpumask.c
> > @@ -9,7 +9,6 @@
> >  /**
> >   * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
> >   * @cpumask: The actual cpumask embedded in the struct.
> > - * @rcu:     The RCU head used to free the cpumask with RCU safety.
> >   * @usage:   Object reference counter. When the refcount goes to 0, the
> >   *           memory is released back to the BPF allocator, which provides
> >   *           RCU safety.
> > @@ -25,7 +24,6 @@
> >   */
> >  struct bpf_cpumask {
> >       cpumask_t cpumask;
> > -     struct rcu_head rcu;
> >       refcount_t usage;
> >  };
> >
> > @@ -82,16 +80,6 @@ __bpf_kfunc struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask)
> >       return cpumask;
> >  }
> >
> > -static void cpumask_free_cb(struct rcu_head *head)
> > -{
> > -     struct bpf_cpumask *cpumask;
> > -
> > -     cpumask = container_of(head, struct bpf_cpumask, rcu);
> > -     migrate_disable();
> > -     bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
> > -     migrate_enable();
> > -}
> > -
> >  /**
> >   * bpf_cpumask_release() - Release a previously acquired BPF cpumask.
> >   * @cpumask: The cpumask being released.
> > @@ -102,8 +90,12 @@ static void cpumask_free_cb(struct rcu_head *head)
> >   */
> >  __bpf_kfunc void bpf_cpumask_release(struct bpf_cpumask *cpumask)
> >  {
> > -     if (refcount_dec_and_test(&cpumask->usage))
> > -             call_rcu(&cpumask->rcu, cpumask_free_cb);
> > +     if (!refcount_dec_and_test(&cpumask->usage))
> > +             return;
> > +
> > +     migrate_disable();
> > +     bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
> > +     migrate_enable();
>
> The fact that callers have to disable migration like this in order to
> safely free the memory feels a bit leaky. Is there any reason we can't
> move this into bpf_mem_{cache_}free_rcu()?

migrate_disable/enable() are actually not necessary here.
We can call bpf_mem_cache_free_rcu() directly from any kfunc.
Explicit migrate_disable() is only necessary from syscall.
I believe rcu callbacks also cannot migrate, so the existing
code probably doesn't need them either.
David Vernet June 26, 2023, 5:55 p.m. UTC | #3 
On Mon, Jun 26, 2023 at 09:09:20AM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 26, 2023 at 8:42 AM David Vernet <void@manifault.com> wrote:
> >
> > On Fri, Jun 23, 2023 at 08:13:33PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > Convert bpf_cpumask to bpf_mem_cache_free_rcu.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >
> > Acked-by: David Vernet <void@manifault.com>
> >
> > LGTM, thanks for cleaning this up. I left one drive-by comment /
> > observation below, but it's not a blocker for this patch / series.
> >
> > > ---
> > >  kernel/bpf/cpumask.c | 20 ++++++--------------
> > >  1 file changed, 6 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
> > > index 938a60ff4295..6983af8e093c 100644
> > > --- a/kernel/bpf/cpumask.c
> > > +++ b/kernel/bpf/cpumask.c
> > > @@ -9,7 +9,6 @@
> > >  /**
> > >   * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
> > >   * @cpumask: The actual cpumask embedded in the struct.
> > > - * @rcu:     The RCU head used to free the cpumask with RCU safety.
> > >   * @usage:   Object reference counter. When the refcount goes to 0, the
> > >   *           memory is released back to the BPF allocator, which provides
> > >   *           RCU safety.
> > > @@ -25,7 +24,6 @@
> > >   */
> > >  struct bpf_cpumask {
> > >       cpumask_t cpumask;
> > > -     struct rcu_head rcu;
> > >       refcount_t usage;
> > >  };
> > >
> > > @@ -82,16 +80,6 @@ __bpf_kfunc struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask)
> > >       return cpumask;
> > >  }
> > >
> > > -static void cpumask_free_cb(struct rcu_head *head)
> > > -{
> > > -     struct bpf_cpumask *cpumask;
> > > -
> > > -     cpumask = container_of(head, struct bpf_cpumask, rcu);
> > > -     migrate_disable();
> > > -     bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
> > > -     migrate_enable();
> > > -}
> > > -
> > >  /**
> > >   * bpf_cpumask_release() - Release a previously acquired BPF cpumask.
> > >   * @cpumask: The cpumask being released.
> > > @@ -102,8 +90,12 @@ static void cpumask_free_cb(struct rcu_head *head)
> > >   */
> > >  __bpf_kfunc void bpf_cpumask_release(struct bpf_cpumask *cpumask)
> > >  {
> > > -     if (refcount_dec_and_test(&cpumask->usage))
> > > -             call_rcu(&cpumask->rcu, cpumask_free_cb);
> > > +     if (!refcount_dec_and_test(&cpumask->usage))
> > > +             return;
> > > +
> > > +     migrate_disable();
> > > +     bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
> > > +     migrate_enable();
> >
> > The fact that callers have to disable migration like this in order to
> > safely free the memory feels a bit leaky. Is there any reason we can't
> > move this into bpf_mem_{cache_}free_rcu()?
> 
> migrate_disable/enable() are actually not necessary here.
> We can call bpf_mem_cache_free_rcu() directly from any kfunc.

Could you please clarify why? Can't we migrate if the kfunc is called
from a sleepable struct_ops callback? If migration is always disabled
for any kfunc then I agree these migrate_{en,dis}able() calls can be
removed. Otherwise from my reading of the code we'd race between calling
this_cpu_ptr() and the local_irq_save() in unit_free().

Thanks,
David

> Explicit migrate_disable() is only necessary from syscall.
>
> I believe rcu callbacks also cannot migrate, so the existing
> code probably doesn't need them either.
Alexei Starovoitov June 26, 2023, 5:59 p.m. UTC | #4 
On Mon, Jun 26, 2023 at 10:55 AM David Vernet <void@manifault.com> wrote:
>
> > > > +
> > > > +     migrate_disable();
> > > > +     bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
> > > > +     migrate_enable();
> > >
> > > The fact that callers have to disable migration like this in order to
> > > safely free the memory feels a bit leaky. Is there any reason we can't
> > > move this into bpf_mem_{cache_}free_rcu()?
> >
> > migrate_disable/enable() are actually not necessary here.
> > We can call bpf_mem_cache_free_rcu() directly from any kfunc.
>
> Could you please clarify why? Can't we migrate if the kfunc is called
> from a sleepable struct_ops callback?

migration is disabled for all bpf progs including sleepable.

> If migration is always disabled
> for any kfunc then I agree these migrate_{en,dis}able() calls can be
> removed. Otherwise from my reading of the code we'd race between calling
> this_cpu_ptr() and the local_irq_save() in unit_free().
>
> Thanks,
> David
>
> > Explicit migrate_disable() is only necessary from syscall.
> >
> > I believe rcu callbacks also cannot migrate, so the existing
> > code probably doesn't need them either.
David Vernet June 26, 2023, 6:01 p.m. UTC | #5 
On Mon, Jun 26, 2023 at 10:59:40AM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 26, 2023 at 10:55 AM David Vernet <void@manifault.com> wrote:
> >
> > > > > +
> > > > > +     migrate_disable();
> > > > > +     bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
> > > > > +     migrate_enable();
> > > >
> > > > The fact that callers have to disable migration like this in order to
> > > > safely free the memory feels a bit leaky. Is there any reason we can't
> > > > move this into bpf_mem_{cache_}free_rcu()?
> > >
> > > migrate_disable/enable() are actually not necessary here.
> > > We can call bpf_mem_cache_free_rcu() directly from any kfunc.
> >
> > Could you please clarify why? Can't we migrate if the kfunc is called
> > from a sleepable struct_ops callback?
> 
> migration is disabled for all bpf progs including sleepable.

Fair enough :-) Then yep, let's remove these. Feel free to do so when
you send v2, and keep my Ack. Otherwise I'm happy to send a follow-on
patch after this series is merged.
diff mbox series

Patch

diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index 938a60ff4295..6983af8e093c 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -9,7 +9,6 @@ 
 /**
  * struct bpf_cpumask - refcounted BPF cpumask wrapper structure
  * @cpumask:	The actual cpumask embedded in the struct.
- * @rcu:	The RCU head used to free the cpumask with RCU safety.
  * @usage:	Object reference counter. When the refcount goes to 0, the
  *		memory is released back to the BPF allocator, which provides
  *		RCU safety.
@@ -25,7 +24,6 @@ 
  */
 struct bpf_cpumask {
 	cpumask_t cpumask;
-	struct rcu_head rcu;
 	refcount_t usage;
 };
 
@@ -82,16 +80,6 @@  __bpf_kfunc struct bpf_cpumask *bpf_cpumask_acquire(struct bpf_cpumask *cpumask)
 	return cpumask;
 }
 
-static void cpumask_free_cb(struct rcu_head *head)
-{
-	struct bpf_cpumask *cpumask;
-
-	cpumask = container_of(head, struct bpf_cpumask, rcu);
-	migrate_disable();
-	bpf_mem_cache_free(&bpf_cpumask_ma, cpumask);
-	migrate_enable();
-}
-
 /**
  * bpf_cpumask_release() - Release a previously acquired BPF cpumask.
  * @cpumask: The cpumask being released.
@@ -102,8 +90,12 @@  static void cpumask_free_cb(struct rcu_head *head)
  */
 __bpf_kfunc void bpf_cpumask_release(struct bpf_cpumask *cpumask)
 {
-	if (refcount_dec_and_test(&cpumask->usage))
-		call_rcu(&cpumask->rcu, cpumask_free_cb);
+	if (!refcount_dec_and_test(&cpumask->usage))
+		return;
+
+	migrate_disable();
+	bpf_mem_cache_free_rcu(&bpf_cpumask_ma, cpumask);
+	migrate_enable();
 }
 
 /**