Re: [PATCHv4 bpf-next 2/4] xdp: extend xdp_redirect_map with broadcast support

[John Fastabend <john.fastabend@gmail.com>]

Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@gmail.com> writes:
> 
> > Toke Høiland-Jørgensen wrote:
> >> John Fastabend <john.fastabend@gmail.com> writes:
> >> 
> >> > Toke Høiland-Jørgensen wrote:
> >> >> Hangbin Liu <liuhangbin@gmail.com> writes:
> >> >> 
> >> >> > 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.

[...]

> >> > Have we consider doing something like the batch lookup ops over hashtab?
> >> > I don't mind "missing" values so if we just walk the list?
> >> >
> >> >      head = dev_map_index_hash(dtab, i)
> >> >      // collect all my devs and get ready to send multicast
> >> >      hlist_nulls_for_each_entry_safe(dev, next, head, index_hlist) {
> >> > 		enqueue(dev, skb)
> >> >      }
> >> >      // submit the queue of entries and do all the work to actually xmit
> >> >      submit_enqueued();
> >> >
> >> > We don't have to care about keys just walk the hash list?
> >> 
> >> So you'd wrap that in a loop like:
> >> 
> >> for (i = 0; i < dtab->n_buckets; i++) {
> >> 	head = dev_map_index_hash(dtab, i);
> >> 	hlist_nulls_for_each_entry_safe(dev, next, head, index_hlist) {
> >> 		bq_enqueue(dev, xdpf, dev_rx, obj->xdp_prog);
> >> 	}
> >> }
> >> 
> >> or? Yeah, I guess that would work!
> >
> > Nice. Thanks for sticking with this Hangbin its taking us a bit, but
> > I think above works on my side at least.
> >
> >> 
> >> It would mean that dev_map_enqueue_multi() would need more in-depth
> >> knowledge into the map type, so would likely need to be two different
> >> functions for the two different map types, living in devmap.c - but
> >> that's probably acceptable.
> >
> > Yeah, I think thats fine.
> >
> >> 
> >> And while we're doing that, the array-map version can also loop over all
> >> indexes up to max_entries, instead of stopping at the first index that
> >> doesn't have an entry like it does now (right now, it looks like if you
> >> populate entries 0 and 2 in an array-map only one copy of the packet
> >> will be sent, to index 0).
> >
> > Right, this is likely needed anyways. At least when I was doing prototypes
> > of using array maps I often ended up with holes in the map. Just imagine
> > adding a set of devs and then removing one, its not likely to be the
> > last one you insert.
> 
> Yeah, totally. Would have pointed it out if I'd noticed before, but I
> was too trusting in the abstraction of get_next_key() etc :)
 
Hangbin,

If possible please try to capture some of the design discussion in
the commit message on the next rev along with the tradeoffs we are making
so we don't lose these important details. Some of these points are fairly
subtle calling them out will surely save (for me at least) some thinking
when I pick this up when it lands in a released kernel.

Thanks!