RE: [PATCH bpf-next v2 5/8] xdp: make devmap flush_list common for all map instances

John Fastabend
2020-01-07T17:58:19Z

Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> The devmap flush list is used to track entries that need to flushed
> from via the xdp_do_flush_map() function. This list used to be
> per-map, but there is really no reason for that. Instead make the
> flush list global for all devmaps, which simplifies __dev_map_flush()
> and dev_map_init_map().
>
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/bpf.h | 4 ++--
> kernel/bpf/devmap.c | 35 +++++++++++++----------------------
> net/core/filter.c | 2 +-
> 3 files changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d467983e61bb..31191804ca09 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -959,7 +959,7 @@ struct sk_buff;
>
> struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
> struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key);
> -void __dev_map_flush(struct bpf_map *map);
> +void __dev_map_flush(void);
> int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
> struct net_device *dev_rx);
> int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
> @@ -1068,7 +1068,7 @@ static inline struct net_device *__dev_map_hash_lookup_elem(struct bpf_map *map
> return NULL;
> }
>
> -static inline void __dev_map_flush(struct bpf_map *map)
> +static inline void __dev_map_flush(void)

How about __dev_flush(void) then sense its not map specific anymore?
Probably same in patch 4/5.

> {
> }
>
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index b7595de6a91a..da9c832fc5c8 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c

[...]

> @@ -384,10 +371,9 @@ static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags)
> * net device can be torn down. On devmap tear down we ensure the flush list
> * is empty before completing to ensure all flush operations have completed.
> */
> -void __dev_map_flush(struct bpf_map *map)
> +void __dev_map_flush(void)

__dev_flush()?

> {
> - struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
> - struct list_head *flush_list = this_cpu_ptr(dtab->flush_list);
> + struct list_head *flush_list = this_cpu_ptr(&dev_map_flush_list);
> struct xdp_bulk_queue *bq, *tmp;
>
> rcu_read_lock();

[...]

Looks good changing the function name would make things a bit cleaner IMO.

Acked-by: John Fastabend <john.fastabend@gmail.com>