On Sat, Jan 11, 2020 at 06:37:21PM -0800, John Fastabend wrote:
Small nits for typos, can be ignored.
> Now that we rely on synchronize_rcu and call_rcu waiting to
> exit perempt-disable regions (NAPI) lets update the comments
s/perempt/preempt
> to reflect this.
>
> Fixes: 0536b85239b84 ("xdp: Simplify devmap cleanup")
> Acked-by: Björn Töpel <bjorn.topel@xxxxxxxxx>
> Acked-by: Song Liu <songliubraving@xxxxxx>
> Signed-off-by: John Fastabend <john.fastabend@xxxxxxxxx>
> ---
> 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
s/relavent/relevant
> + * 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)
>
