Re: [PATCH bpf-next 1/4] xdp: Support specifying expected existing program when attaching XDP

Jakub Kicinski
2020-03-20T23:35:18Z

On Fri, 20 Mar 2020 22:55:43 +0100 Daniel Borkmann wrote:
> >> Another aspect that falls into this atomic replacement is also that the programs can
> >> actually be atomically replaced at runtime. Last time I looked, some drivers still do
> >> a down/up cycle on replacement and hence traffic would be interrupted. I would argue
> >> that such /atomic/ swap operation on bpf_link would cover a guarantee of not having to
> >> perform this as well (workaround today would be a simple tail call map as entry point).
> >
> > I don't think that's the case. Drivers generally have a fast path
> > for the active-active replace.
> >
> > Up/Down is only done to remap DMA buffers and change RX buffer
> > allocation scheme. That's when program is installed or removed,
> > not replaced.
>
> I know; though it seems not all adhere to that scheme sadly. I don't have that HW so can
> only judge on the code, but one example that looked suspicious enough to me is qede_xdp().
> It calls qede_xdp_set(), which does a qede_reload() for /every/ prog update. The latter
> basically does ...
>
> if (edev->state == QEDE_STATE_OPEN) {
> qede_unload(edev, QEDE_UNLOAD_NORMAL, true);
> if (args)
> args->func(edev, args); <-- prog replace here
> qede_load(edev, QEDE_LOAD_RELOAD, true);
> [...]
> }

Ack, one day maybe we can restructure things enough so that drivers
don't have to copy/paste this dance :(

> ... now that is one driver. I haven't checked all the others (aside from i40e/ixgbe/mlx4/
> mlx5/nfp), but in any case it's also fixable in the driver w/o the extra need for bpf_link.

Agreed