On Mon, Apr 05, 2021 at 05:24:48PM -0700, John Fastabend wrote:
> Hangbin Liu wrote:
> > This patch add two flags BPF_F_BROADCAST and BPF_F_EXCLUDE_INGRESS to extend
> > xdp_redirect_map for broadcast support.
> >
> > Keep the general data path in net/core/filter.c and the native data
> > path in kernel/bpf/devmap.c so we can use direct calls to get better
> > performace.
> >
> > Here is the performance result by using xdp_redirect_{map, map_multi} in
> > sample/bpf and send pkts via pktgen cmd:
> > ./pktgen_sample03_burst_single_flow.sh -i eno1 -d $dst_ip -m $dst_mac -t 10
> > -s 64
> >
> > There are some drop back as we need to loop the map and get each interface.
> >
> > Version | Test | Generic | Native
> > 5.12 rc2 | redirect_map i40e->i40e | 2.0M | 9.8M
> > 5.12 rc2 | redirect_map i40e->veth | 1.8M | 12.0M
>
> Are these are 10gbps i40e ports? Sorry if I asked this earlier, maybe
> add a note in the commit if another respin is needed.
Yes, I will add it if there is an update.
> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> > index 3980fb3bfb09..c8452c5f40f8 100644
> > --- a/kernel/bpf/devmap.c
> > +++ b/kernel/bpf/devmap.c
> > @@ -198,6 +198,7 @@ static void dev_map_free(struct bpf_map *map)
> > list_del_rcu(&dtab->list);
> > spin_unlock(&dev_map_lock);
> >
> > + bpf_clear_redirect_map(map);
>
> Is this a bugfix? If its needed here wouldn't we also need it in the
> devmap case.
No, in ee75aef23afe ("bpf, xdp: Restructure redirect actions") this function
was removed. I added it back as we use ri->map again.
What devmap case you mean?
>
> > synchronize_rcu();
> >
> > /* Make sure prior __dev_map_entry_free() have completed. */
>
> [...]
>
> > +
> > +static struct bpf_dtab_netdev *devmap_get_next_obj(struct xdp_buff *xdp,
> > + struct bpf_map *map,
> > + u32 *key, u32 *next_key,
> > + int ex_ifindex)
> > +{
> > + struct bpf_dtab_netdev *obj;
> > + struct net_device *dev;
> > + u32 index;
> > + int err;
> > +
> > + err = devmap_get_next_key(map, key, next_key);
> > + if (err)
> > + return NULL;
> > +
> > + /* When using dev map hash, we could restart the hashtab traversal
> > + * in case the key has been updated/removed in the mean time.
> > + * So we may end up potentially looping due to traversal restarts
> > + * from first elem.
> > + *
> > + * Let's use map's max_entries to limit the loop number.
> > + */
> > + for (index = 0; index < map->max_entries; index++) {
> > + obj = devmap_lookup_elem(map, *next_key);
> > + if (!obj || dst_dev_is_ingress(obj, ex_ifindex))
> > + goto find_next;
> > +
> > + dev = obj->dev;
> > +
> > + if (!dev->netdev_ops->ndo_xdp_xmit)
> > + goto find_next;
> > +
> > + err = xdp_ok_fwd_dev(dev, xdp->data_end - xdp->data);
> > + if (unlikely(err))
> > + goto find_next;
> > +
> > + return obj;
> > +
> > +find_next:
> > + key = next_key;
> > + err = devmap_get_next_key(map, key, next_key);
> > + if (err)
> > + break;
> > + }
>
> I'm missing something. Either an elaborated commit message or comment
> is probably needed. I've been looking at this block for 30 minutes and
> can't see how we avoid sending duplicate frames on a single interface?
> Can you check this code flow,
>
> dev_map_enqueue_multi()
> for (;;) {
> next_obj = devmap_get_next_obj(...)
> for (index = 0; index < map->max_entries; index++) {
> obj = devmap_lookup_elem();
> if (!obj) goto find_next
> key = next_key;
> err = devmap_get_next_key()
> if (!key) goto find_first
> for (i = 0; i < dtab->n_buckets; i++)
> return *next <- now *next_key is point back
> at first entry
> // loop back through and find first obj and return that
devmap_get_next_key() will loop to find the first one if there is no
key or dev. In normal time it will stop after the latest one.
> }
> bq_enqueue(...) // enqueue original obj
> obj = next_obj;
> key = next_key;
> ... // we are going to enqueue first obj, but how do we know
> // this hasn't already been sent? Presumably if we have
> // a delete in the hash table in the middle of a multicast
> // operation this might happen?
> }
And yes, there is an corner case that if we removed a dev during multicast,
there is an possibility that restart from the first key. But given that
this is an unlikely case, and in normal internet there is also a possibility
of duplicate/lost packet. This should also be acceptable?
For the loop limit, Daniel suggested to add it:
https://lore.kernel.org/bpf/609c2fdf-09b7-b86e-26c0-ad386770a...@iogearbox.net/
> > +
> > + return NULL;
> > +}
> > +
> > +int dev_map_enqueue_multi(struct xdp_buff *xdp, struct net_device *dev_rx,
> > + struct bpf_map *map, bool exclude_ingress)
> > +{
> > + struct bpf_dtab_netdev *obj = NULL, *next_obj = NULL;
> > + struct xdp_frame *xdpf, *nxdpf;
> > + u32 key, next_key;
> > + int ex_ifindex;
> > +
> > + ex_ifindex = exclude_ingress ? dev_rx->ifindex : 0;
> > +
> > + /* Find first available obj */
> > + obj = devmap_get_next_obj(xdp, map, NULL, &key, ex_ifindex);
> > + if (!obj)
> > + return -ENOENT;
> > +
> > + xdpf = xdp_convert_buff_to_frame(xdp);
> > + if (unlikely(!xdpf))
> > + return -EOVERFLOW;
> > +
> > + for (;;) {
>
> A nit take it or not. These for (;;) loops always seem a bit odd to me
> when we really don't want it to run forever. I prefer
>
> while (!next_obj)
>
> but a matter of style I guess.
OK, I will do it if there is an respin.
Thank
Hangbin
