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

Hangbin Liu [off-list ref] writes:

This patch adds two flags BPF_F_BROADCAST and BPF_F_EXCLUDE_INGRESS to
extend xdp_redirect_map for broadcast support.

With BPF_F_BROADCAST the packet will be broadcasted to all the interfaces
in the map. with BPF_F_EXCLUDE_INGRESS the ingress interface will be
excluded when do broadcasting.

When getting the devices in dev hash map via dev_map_hash_get_next_key(),
there is a possibility that we fall back to the first key when a device
was removed. This will duplicate packets on some interfaces. So just walk
the whole buckets to avoid this issue. For dev array map, we also walk the
whole map to find valid interfaces.

Function bpf_clear_redirect_map() was removed in
commit ee75aef23afe ("bpf, xdp: Restructure redirect actions").
Add it back as we need to use ri->map again.

Here is the performance result by using 10Gb i40e NIC, do XDP_DROP on
veth peer, run 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 rc4         | redirect_map        i40e->i40e      |    1.9M |  9.6M
5.12 rc4         | redirect_map        i40e->veth      |    1.7M | 11.7M
5.12 rc4 + patch | redirect_map        i40e->i40e      |    1.9M |  9.3M
5.12 rc4 + patch | redirect_map        i40e->veth      |    1.7M | 11.4M
5.12 rc4 + patch | redirect_map multi  i40e->i40e      |    1.9M |  8.9M
5.12 rc4 + patch | redirect_map multi  i40e->veth      |    1.7M | 10.9M
5.12 rc4 + patch | redirect_map multi  i40e->mlx4+veth |    1.2M |  3.8M

Signed-off-by: Hangbin Liu <redacted>

---
v6:
Fix a skb leak in the error path for generic XDP

That's better, thanks! When checking this I at first thought you were
missing a free; turns out I was wrong, the caller of
xdp_do_generic_redirect() will free the skb on error.

However, this is also the case for the native path: the driver is
supposed to free/recycle the frame if xdp_do_redirect() fails. Which
means that this:

[...]

+static int dev_map_enqueue_clone(struct bpf_dtab_netdev *obj,
+				 struct net_device *dev_rx,
+				 struct xdp_frame *xdpf)
+{
+	struct xdp_frame *nxdpf;
+
+	nxdpf = xdpf_clone(xdpf);
+	if (unlikely(!nxdpf)) {
+		xdp_return_frame_rx_napi(xdpf);
+		return -ENOMEM;
+	}

is wrong; the ENOMEM return gets propagated up to the caller of
xdp_do_redirect() which will take care of freeing the frame, so this
code shouldn't also be freeing it.

Sorry for not spotting this on the last round - these error conditions
are a bit confusing to me as well. Making the generic and native paths
more similar like you did in this round made it more obvious, though,
which was the point; so yay! :)

-Toke