netdev - RE: [PATCH bpf-next v2 5/8] xdp: make devmap flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Jan 2020 09:58:08 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Björn Töpel <bjorn.topel@...il.com>,
        netdev@...r.kernel.org, ast@...nel.org, daniel@...earbox.net
Cc:     Björn Töpel <bjorn.topel@...el.com>,
        bpf@...r.kernel.org, davem@...emloft.net,
        jakub.kicinski@...ronome.com, hawk@...nel.org,
        john.fastabend@...il.com, magnus.karlsson@...el.com,
        jonathan.lemon@...il.com,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: RE: [PATCH bpf-next v2 5/8] xdp: make devmap flush_list common for
 all map instances

Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@...el.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@...hat.com>
> Signed-off-by: Björn Töpel <bjorn.topel@...el.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@...il.com>