diff mbox series

[v2,bpf-next,6/7] bpf: Allows per-cpu maps and map-in-map in sleepable programs

Message ID 20210206170344.78399-7-alexei.starovoitov@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Misc improvements | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 8 maintainers not CCed: songliubraving@fb.com kafai@fb.com john.fastabend@gmail.com yhs@fb.com ast@kernel.org netdev@vger.kernel.org andrii@kernel.org kpsingh@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 33 this patch: 33
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 31 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 33 this patch: 33
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Alexei Starovoitov Feb. 6, 2021, 5:03 p.m. UTC 
From: Alexei Starovoitov <ast@kernel.org>

Since sleepable programs are now executing under migrate_disable
the per-cpu maps are safe to use.
The map-in-map were ok to use in sleepable from the time sleepable
progs were introduced.

Note that non-preallocated maps are still not safe, since there is
no rcu_read_lock yet in sleepable programs and dynamically allocated
map elements are relying on rcu protection. The sleepable programs
have rcu_read_lock_trace instead. That limitation will be addresses
in the future.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/hashtab.c  | 4 ++--
 kernel/bpf/verifier.c | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Andrii Nakryiko Feb. 8, 2021, 9 p.m. UTC | #1 
On Sat, Feb 6, 2021 at 9:06 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Since sleepable programs are now executing under migrate_disable
> the per-cpu maps are safe to use.
> The map-in-map were ok to use in sleepable from the time sleepable
> progs were introduced.
>
> Note that non-preallocated maps are still not safe, since there is
> no rcu_read_lock yet in sleepable programs and dynamically allocated
> map elements are relying on rcu protection. The sleepable programs
> have rcu_read_lock_trace instead. That limitation will be addresses
> in the future.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

Great.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  kernel/bpf/hashtab.c  | 4 ++--
>  kernel/bpf/verifier.c | 7 ++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index c1ac7f964bc9..d63912e73ad9 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1148,7 +1148,7 @@ static int __htab_percpu_map_update_elem(struct bpf_map *map, void *key,
>                 /* unknown flags */
>                 return -EINVAL;
>
> -       WARN_ON_ONCE(!rcu_read_lock_held());
> +       WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
>
>         key_size = map->key_size;
>
> @@ -1202,7 +1202,7 @@ static int __htab_lru_percpu_map_update_elem(struct bpf_map *map, void *key,
>                 /* unknown flags */
>                 return -EINVAL;
>
> -       WARN_ON_ONCE(!rcu_read_lock_held());
> +       WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
>
>         key_size = map->key_size;
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4189edb41b73..9561f2af7710 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10020,9 +10020,14 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>                 case BPF_MAP_TYPE_HASH:
>                 case BPF_MAP_TYPE_LRU_HASH:
>                 case BPF_MAP_TYPE_ARRAY:
> +               case BPF_MAP_TYPE_PERCPU_HASH:
> +               case BPF_MAP_TYPE_PERCPU_ARRAY:
> +               case BPF_MAP_TYPE_LRU_PERCPU_HASH:
> +               case BPF_MAP_TYPE_ARRAY_OF_MAPS:
> +               case BPF_MAP_TYPE_HASH_OF_MAPS:
>                         if (!is_preallocated_map(map)) {
>                                 verbose(env,
> -                                       "Sleepable programs can only use preallocated hash maps\n");
> +                                       "Sleepable programs can only use preallocated maps\n");
>                                 return -EINVAL;
>                         }
>                         break;
> --
> 2.24.1
>
Andrii Nakryiko Feb. 8, 2021, 9:01 p.m. UTC | #2 
On Mon, Feb 8, 2021 at 1:00 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Feb 6, 2021 at 9:06 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Since sleepable programs are now executing under migrate_disable
> > the per-cpu maps are safe to use.

Also made me wonder if PERF_EVENT_ARRAY map is usable in sleepable now?

> > The map-in-map were ok to use in sleepable from the time sleepable
> > progs were introduced.
> >
> > Note that non-preallocated maps are still not safe, since there is
> > no rcu_read_lock yet in sleepable programs and dynamically allocated
> > map elements are relying on rcu protection. The sleepable programs
> > have rcu_read_lock_trace instead. That limitation will be addresses
> > in the future.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
>
> Great.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >  kernel/bpf/hashtab.c  | 4 ++--
> >  kernel/bpf/verifier.c | 7 ++++++-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index c1ac7f964bc9..d63912e73ad9 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -1148,7 +1148,7 @@ static int __htab_percpu_map_update_elem(struct bpf_map *map, void *key,
> >                 /* unknown flags */
> >                 return -EINVAL;
> >
> > -       WARN_ON_ONCE(!rcu_read_lock_held());
> > +       WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
> >
> >         key_size = map->key_size;
> >
> > @@ -1202,7 +1202,7 @@ static int __htab_lru_percpu_map_update_elem(struct bpf_map *map, void *key,
> >                 /* unknown flags */
> >                 return -EINVAL;
> >
> > -       WARN_ON_ONCE(!rcu_read_lock_held());
> > +       WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
> >
> >         key_size = map->key_size;
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 4189edb41b73..9561f2af7710 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -10020,9 +10020,14 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> >                 case BPF_MAP_TYPE_HASH:
> >                 case BPF_MAP_TYPE_LRU_HASH:
> >                 case BPF_MAP_TYPE_ARRAY:
> > +               case BPF_MAP_TYPE_PERCPU_HASH:
> > +               case BPF_MAP_TYPE_PERCPU_ARRAY:
> > +               case BPF_MAP_TYPE_LRU_PERCPU_HASH:
> > +               case BPF_MAP_TYPE_ARRAY_OF_MAPS:
> > +               case BPF_MAP_TYPE_HASH_OF_MAPS:
> >                         if (!is_preallocated_map(map)) {
> >                                 verbose(env,
> > -                                       "Sleepable programs can only use preallocated hash maps\n");
> > +                                       "Sleepable programs can only use preallocated maps\n");
> >                                 return -EINVAL;
> >                         }
> >                         break;
> > --
> > 2.24.1
> >
Alexei Starovoitov Feb. 8, 2021, 11:20 p.m. UTC | #3 
On 2/8/21 1:01 PM, Andrii Nakryiko wrote:
> On Mon, Feb 8, 2021 at 1:00 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Sat, Feb 6, 2021 at 9:06 AM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> From: Alexei Starovoitov <ast@kernel.org>
>>>
>>> Since sleepable programs are now executing under migrate_disable
>>> the per-cpu maps are safe to use.
> 
> Also made me wonder if PERF_EVENT_ARRAY map is usable in sleepable now?

maybe. Probably would need to add explicit preempt_disable to 
__bpf_perf_event_output around perf_event_output.
It needs more analysis.
diff mbox series

Patch

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index c1ac7f964bc9..d63912e73ad9 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -1148,7 +1148,7 @@  static int __htab_percpu_map_update_elem(struct bpf_map *map, void *key,
 		/* unknown flags */
 		return -EINVAL;
 
-	WARN_ON_ONCE(!rcu_read_lock_held());
+	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
 
 	key_size = map->key_size;
 
@@ -1202,7 +1202,7 @@  static int __htab_lru_percpu_map_update_elem(struct bpf_map *map, void *key,
 		/* unknown flags */
 		return -EINVAL;
 
-	WARN_ON_ONCE(!rcu_read_lock_held());
+	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held());
 
 	key_size = map->key_size;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4189edb41b73..9561f2af7710 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10020,9 +10020,14 @@  static int check_map_prog_compatibility(struct bpf_verifier_env *env,
 		case BPF_MAP_TYPE_HASH:
 		case BPF_MAP_TYPE_LRU_HASH:
 		case BPF_MAP_TYPE_ARRAY:
+		case BPF_MAP_TYPE_PERCPU_HASH:
+		case BPF_MAP_TYPE_PERCPU_ARRAY:
+		case BPF_MAP_TYPE_LRU_PERCPU_HASH:
+		case BPF_MAP_TYPE_ARRAY_OF_MAPS:
+		case BPF_MAP_TYPE_HASH_OF_MAPS:
 			if (!is_preallocated_map(map)) {
 				verbose(env,
-					"Sleepable programs can only use preallocated hash maps\n");
+					"Sleepable programs can only use preallocated maps\n");
 				return -EINVAL;
 			}
 			break;