Re: [PATCH bpf-next 7/8] xdp: remove map_to_flush and map swap detection

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On Wed, 18 Dec 2019 11:53:59 +0100
Björn Töpel <bjorn.topel@gmail.com> wrote:

> From: Björn Töpel <bjorn.topel@intel.com>
> 
> Now that all XDP maps that can be used with bpf_redirect_map() tracks
> entries to be flushed in a global fashion, there is not need to track
> that the map has changed and flush from xdp_do_generic_map()
> anymore. All entries will be flushed in xdp_do_flush_map().
> 
> This means that the map_to_flush can be removed, and the corresponding
> checks. Moving the flush logic to one place, xdp_do_flush_map(), give
> a bulking behavior and performance boost.
> 
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
>  include/linux/filter.h |  1 -
>  net/core/filter.c      | 27 +++------------------------
>  2 files changed, 3 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 37ac7025031d..69d6706fc889 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -592,7 +592,6 @@ struct bpf_redirect_info {
>  	u32 tgt_index;
>  	void *tgt_value;
>  	struct bpf_map *map;
> -	struct bpf_map *map_to_flush;
>  	u32 kern_flags;
>  };
>  
> diff --git a/net/core/filter.c b/net/core/filter.c
> index c706325b3e66..d9caa3e57ea1 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3547,26 +3547,9 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
>  
>  void xdp_do_flush_map(void)
>  {
> -	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
> -	struct bpf_map *map = ri->map_to_flush;
> -
> -	ri->map_to_flush = NULL;
> -	if (map) {
> -		switch (map->map_type) {
> -		case BPF_MAP_TYPE_DEVMAP:
> -		case BPF_MAP_TYPE_DEVMAP_HASH:
> -			__dev_map_flush();
> -			break;
> -		case BPF_MAP_TYPE_CPUMAP:
> -			__cpu_map_flush();
> -			break;
> -		case BPF_MAP_TYPE_XSKMAP:
> -			__xsk_map_flush();
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> +	__dev_map_flush();
> +	__cpu_map_flush();
> +	__xsk_map_flush();
>  }
>  EXPORT_SYMBOL_GPL(xdp_do_flush_map);
>  
> @@ -3615,14 +3598,10 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
>  	ri->tgt_value = NULL;
>  	WRITE_ONCE(ri->map, NULL);
>  
> -	if (ri->map_to_flush && unlikely(ri->map_to_flush != map))
> -		xdp_do_flush_map();
> -

I guess, I need to read the other patches to understand why this is valid.

The idea here is to detect if the BPF-prog are using several different
redirect maps, and do the flush if the program uses another map.  I
assume you handle this?


>  	err = __bpf_tx_xdp_map(dev, fwd, map, xdp);
>  	if (unlikely(err))
>  		goto err;
>  
> -	ri->map_to_flush = map;
>  	_trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
>  	return 0;
>  err:



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer