| Message ID | 1580011133-17784-2-git-send-email-john.fastabend@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | XDP flush cleanups | expand |
On Sat, 25 Jan 2020 19:58:51 -0800 John Fastabend <john.fastabend@gmail.com> wrote: > Now that we rely on synchronize_rcu and call_rcu waiting to > exit perempt-disable regions (NAPI) lets update the comments > to reflect this. > > Fixes: 0536b85239b84 ("xdp: Simplify devmap cleanup") > Acked-by: Björn Töpel <bjorn.topel@intel.com> > Acked-by: Song Liu <songliubraving@fb.com> > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- > kernel/bpf/devmap.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c > index da9c832..f0bf525 100644 > --- a/kernel/bpf/devmap.c > +++ b/kernel/bpf/devmap.c > @@ -193,10 +193,12 @@ static void dev_map_free(struct bpf_map *map) > > /* At this point bpf_prog->aux->refcnt == 0 and this map->refcnt == 0, > * so the programs (can be more than one that used this map) were > - * disconnected from events. Wait for outstanding critical sections in > - * these programs to complete. The rcu critical section only guarantees > - * no further reads against netdev_map. It does __not__ ensure pending > - * flush operations (if any) are complete. > + * disconnected from events. The following synchronize_rcu() guarantees > + * both rcu read critical sections complete and waits for > + * preempt-disable regions (NAPI being the relavent context here) so we ^^^^^^^^ Spelling: relevant I would hate to block the patch this close to the release deadline, so maybe DaveM can just adjust this before applying? > + * are certain there will be no further reads against the netdev_map and > + * all flush operations are complete. Flush operations can only be done > + * from NAPI context for this reason. > */ > > spin_lock(&dev_map_lock); > @@ -498,12 +500,11 @@ static int dev_map_delete_elem(struct bpf_map *map, void *key) > return -EINVAL; > > /* Use call_rcu() here to ensure any rcu critical sections have > - * completed, but this does not guarantee a flush has happened > - * yet. Because driver side rcu_read_lock/unlock only protects the > - * running XDP program. However, for pending flush operations the > - * dev and ctx are stored in another per cpu map. And additionally, > - * the driver tear down ensures all soft irqs are complete before > - * removing the net device in the case of dev_put equals zero. > + * completed as well as any flush operations because call_rcu > + * will wait for preempt-disable region to complete, NAPI in this > + * context. And additionally, the driver tear down ensures all > + * soft irqs are complete before removing the net device in the > + * case of dev_put equals zero. > */ > old_dev = xchg(&dtab->netdev_map[k], NULL); > if (old_dev)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index da9c832..f0bf525 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -193,10 +193,12 @@ static void dev_map_free(struct bpf_map *map) /* At this point bpf_prog->aux->refcnt == 0 and this map->refcnt == 0, * so the programs (can be more than one that used this map) were - * disconnected from events. Wait for outstanding critical sections in - * these programs to complete. The rcu critical section only guarantees - * no further reads against netdev_map. It does __not__ ensure pending - * flush operations (if any) are complete. + * disconnected from events. The following synchronize_rcu() guarantees + * both rcu read critical sections complete and waits for + * preempt-disable regions (NAPI being the relavent context here) so we + * are certain there will be no further reads against the netdev_map and + * all flush operations are complete. Flush operations can only be done + * from NAPI context for this reason. */ spin_lock(&dev_map_lock); @@ -498,12 +500,11 @@ static int dev_map_delete_elem(struct bpf_map *map, void *key) return -EINVAL; /* Use call_rcu() here to ensure any rcu critical sections have - * completed, but this does not guarantee a flush has happened - * yet. Because driver side rcu_read_lock/unlock only protects the - * running XDP program. However, for pending flush operations the - * dev and ctx are stored in another per cpu map. And additionally, - * the driver tear down ensures all soft irqs are complete before - * removing the net device in the case of dev_put equals zero. + * completed as well as any flush operations because call_rcu + * will wait for preempt-disable region to complete, NAPI in this + * context. And additionally, the driver tear down ensures all + * soft irqs are complete before removing the net device in the + * case of dev_put equals zero. */ old_dev = xchg(&dtab->netdev_map[k], NULL); if (old_dev)
Now that we rely on synchronize_rcu and call_rcu waiting to exit perempt-disable regions (NAPI) lets update the comments to reflect this. Fixes: 0536b85239b84 ("xdp: Simplify devmap cleanup") Acked-by: Björn Töpel <bjorn.topel@intel.com> Acked-by: Song Liu <songliubraving@fb.com> Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- kernel/bpf/devmap.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)