| Message ID | 20191218105400.2895-8-bjorn.topel@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Simplify xdp_do_redirect_map()/xdp_do_flush_map() and XDP maps | expand |
Björn Töpel <bjorn.topel@gmail.com> writes: > 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> Acked-by: Toke Høiland-Jørgensen <toke@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:
On Wed, 18 Dec 2019 at 14:03, Jesper Dangaard Brouer <jbrouer@redhat.com> wrote: > > 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. > Please do; Review would be very much appreciated! > 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? > Yes, the highlevel idea is that since the maps are dealing partly with this already (but swaps map internal), let the map code deal with map swaps as well.
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(); - 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: