Hi Toke,
Thanks for your review, please see replies below.
On Fri, Apr 24, 2020 at 04:34:49PM +0200, Toke Høiland-Jørgensen wrote:
> >
> > The general data path is kept in net/core/filter.c. The native data
> > path is in kernel/bpf/devmap.c so we can use direct calls to
> > get better performace.
>
> Got any performance numbers? :)
No, I haven't test the performance. Do you have any suggestions about how
to test it? I'd like to try forwarding pkts to 10+ ports. But I don't know
how to test the throughput. I don't think netperf or iperf supports this.
>
> > + * int bpf_redirect_map_multi(struct bpf_map *map, struct bpf_map *ex_map,
> > u64 flags)
> > + * Description
> > + * Redirect the packet to all the interfaces in *map*, and
> > + * exclude the interfaces that in *ex_map*. The *ex_map*
> > could
> > + * be NULL.
> > + *
> > + * Currently the *flags* only supports
> > *BPF_F_EXCLUDE_INGRESS*,
> > + * which could exlcude redirect to the ingress device.
>
> I'd suggest rewording this to:
>
> * Redirect the packet to ALL the interfaces in *map*, but
> * exclude the interfaces in *ex_map* (which may be NULL).
> *
> * Currently the *flags* only supports *BPF_F_EXCLUDE_INGRESS*,
> * which additionally excludes the current ingress device.
Thanks, I will update it
> > +
> > +bool dev_in_exclude_map(struct bpf_dtab_netdev *obj, struct bpf_map *map,
> > + int exclude_ifindex)
> > +{
> > + struct bpf_dtab_netdev *in_obj = NULL;
> > + u32 key, next_key;
> > + int err;
> > +
> > + if (!map)
> > + return false;
> > +
> > + if (obj->dev->ifindex == exclude_ifindex)
> > + return true;
>
> We probably want the EXCLUDE_INGRESS flag to work even if ex_map is
> NULL, right? In that case you want to switch the order of the two checks
> above.
Yes, will fix it.
>
> > + devmap_get_next_key(map, NULL, &key);
> > +
> > + for (;;) {
>
> I wonder if we should require DEVMAP_HASH maps to be indexed by ifindex
> to avoid the loop?
I guess it's not easy to force user to index the map by ifindex.
> > + xdpf = convert_to_xdp_frame(xdp);
> > + if (unlikely(!xdpf))
> > + return -EOVERFLOW;
>
> You do a clone for each map entry below, so I think you end up leaking
> this initial xdpf? Also, you'll end up with one clone more than
> necessary - redirecting to two interfaces should only require 1 clone,
> you're doing 2.
We don't know which is the latest one. So we need to keep the initial
for clone. Is it enough to call xdp_release_frame() after the for loop?
>
> > + for (;;) {
> > + switch (map->map_type) {
> > + case BPF_MAP_TYPE_DEVMAP:
> > + obj = __dev_map_lookup_elem(map, key);
> > + break;
> > + case BPF_MAP_TYPE_DEVMAP_HASH:
> > + obj = __dev_map_hash_lookup_elem(map, key);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + if (!obj || dev_in_exclude_map(obj, ex_map,
> > + exclude_ingress ?
> > dev_rx->ifindex : 0))
> > + goto find_next;
> > +
> > + dev = obj->dev;
> > +
> > + if (!dev->netdev_ops->ndo_xdp_xmit)
> > + return -EOPNOTSUPP;
> > +
> > + err = xdp_ok_fwd_dev(dev, xdp->data_end - xdp->data);
> > + if (unlikely(err))
> > + return err;
>
> These abort the whole operation midway through the loop if any error
> occurs. That is probably not what we want? I think the right thing to do
> is just continue the loop and only return an error if *all* of the
> forwarding attempts failed. Maybe we need a tracepoint to catch
> individual errors?
Makes sense. I will see if we can add a tracepoint here.
> >
> > +static int dev_map_redirect_multi(struct net_device *dev, struct sk_buff
> > *skb,
> > + struct bpf_prog *xdp_prog,
> > + struct bpf_map *map, struct bpf_map *ex_map,
> > + bool exclude_ingress)
> > +
> > +{
> > + struct bpf_dtab_netdev *dst;
> > + struct sk_buff *nskb;
> > + u32 key, next_key;
> > + int err;
> > + void *fwd;
> > +
> > + /* Get first key from forward map */
> > + map->ops->map_get_next_key(map, NULL, &key);
> > +
> > + for (;;) {
> > + fwd = __xdp_map_lookup_elem(map, key);
> > + if (fwd) {
> > + dst = (struct bpf_dtab_netdev *)fwd;
> > + if (dev_in_exclude_map(dst, ex_map,
> > + exclude_ingress ? dev->ifindex :
> > 0))
> > + goto find_next;
> > +
> > + nskb = skb_clone(skb, GFP_ATOMIC);
> > + if (!nskb)
> > + return -EOVERFLOW;
> > +
> > + err = dev_map_generic_redirect(dst, nskb, xdp_prog);
> > + if (unlikely(err))
> > + return err;
> > + }
> > +
> > +find_next:
> > + err = map->ops->map_get_next_key(map, &key, &next_key);
> > + if (err)
> > + break;
> > +
> > + key = next_key;
> > + }
> > +
> > + return 0;
> > +}
>
> This duplication bugs me; maybe we should try to consolidate the generic
> and native XDP code paths?
Yes, I have tried to combine these two functions together. But one is generic
code path and another is XDP code patch. One use skb_clone and another
use xdpf_clone(). There are also some extra checks for XDP code. So maybe
we'd better just keep it as it is.
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 2e29a671d67e..1dbe42290223 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
>
> Updates to tools/include should generally go into a separate patch.
Will fix it, thanks.
Best Regards
Hangbin
