On Wed, Jul 22, 2020 at 8:31 AM Maciej Fijalkowski
<maciej.fijalkowski@xxxxxxxxx> wrote:
>
> On Tue, Jul 21, 2020 at 11:45:55PM -0700, Andrii Nakryiko wrote:
> > Instead of delegating to drivers, maintain information about which BPF
> > programs are attached in which XDP modes (generic/skb, driver, or hardware)
> > locally in net_device. This effectively obsoletes XDP_QUERY_PROG command.
> >
> > Such re-organization simplifies existing code already. But it also allows to
> > further add bpf_link-based XDP attachments without drivers having to know
> > about any of this at all, which seems like a good setup.
> > XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to
> > install/uninstall active BPF program. All the higher-level concerns about
> > prog/link interaction will be contained within generic driver-agnostic logic.
> >
> > All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed.
> > It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags
> > when resetting installed programs. That seems unnecessary, plus most drivers
> > don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW
> > should be enough of an indicator of what is required of driver to correctly
> > reset active BPF program. dev_xdp_uninstall() is also generalized as an
> > iteration over all three supported mode.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> > ---
> > include/linux/netdevice.h | 17 +++-
> > net/core/dev.c | 158 +++++++++++++++++++++-----------------
> > net/core/rtnetlink.c | 5 +-
> > 3 files changed, 105 insertions(+), 75 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index ac2cd3f49aba..cad44b40c776 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -889,6 +889,17 @@ struct netlink_ext_ack;
> > struct xdp_umem;
> > struct xdp_dev_bulk_queue;
> >
> > +enum bpf_xdp_mode {
> > + XDP_MODE_SKB = 0,
> > + XDP_MODE_DRV = 1,
> > + XDP_MODE_HW = 2,
> > + __MAX_XDP_MODE
> > +};
> > +
> > +struct bpf_xdp_entity {
> > + struct bpf_prog *prog;
> > +};
> > +
[...]
> >
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > - enum bpf_netdev_command cmd)
> > +static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
> > {
> > - struct netdev_bpf xdp;
> > + if (flags & XDP_FLAGS_HW_MODE)
> > + return XDP_MODE_HW;
> > + if (flags & XDP_FLAGS_DRV_MODE)
> > + return XDP_MODE_DRV;
> > + return XDP_MODE_SKB;
> > +}
> >
> > - if (!bpf_op)
> > - return 0;
> > +static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
> > +{
> > + switch (mode) {
> > + case XDP_MODE_SKB:
> > + return generic_xdp_install;
> > + case XDP_MODE_DRV:
> > + case XDP_MODE_HW:
> > + return dev->netdev_ops->ndo_bpf;
> > + default:
> > + return NULL;
> > + };
> > +}
> >
[...]
> >
> > static void dev_xdp_uninstall(struct net_device *dev)
> > {
> > - struct netdev_bpf xdp;
> > - bpf_op_t ndo_bpf;
> > + struct bpf_prog *prog;
> > + enum bpf_xdp_mode mode;
> > + bpf_op_t bpf_op;
> >
> > - /* Remove generic XDP */
> > - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > + ASSERT_RTNL();
> >
> > - /* Remove from the driver */
> > - ndo_bpf = dev->netdev_ops->ndo_bpf;
> > - if (!ndo_bpf)
> > - return;
> > + for (mode = XDP_MODE_SKB; mode < __MAX_XDP_MODE; mode++) {
> > + prog = dev_xdp_prog(dev, mode);
> > + if (!prog)
> > + continue;
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = XDP_QUERY_PROG;
> > - WARN_ON(ndo_bpf(dev, &xdp));
> > - if (xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > - NULL));
> > + bpf_op = dev_xdp_bpf_op(dev, mode);
> > + if (!bpf_op)
> > + continue;
>
> could we assume that we are iterating over the defined XDP modes so bpf_op
> will always be a valid function pointer so that we could use directly
> dev_xdp_bpf_op() in dev_xdp_install() ?
>
> Just a nit, however current state is probably less error-prone when in
> future we might be introducing new XDP mode.
No we can't because for DRV and HW modes, dev->netdev_ops->ndo_bpf can
be NULL. So even though there are 3 possible XDP modes, only one (SKB,
using generic_xdp_install handler) might be supported for a given
net_device.
>
> >
> > - /* Remove HW offload */
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = XDP_QUERY_PROG_HW;
> > - if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > - NULL));
> > + WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));
> > +
> > + bpf_prog_put(prog);
> > + dev_xdp_set_prog(dev, mode, NULL);
> > + }
> > }
> >
> > /**
> > @@ -8810,29 +8829,22 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, int expected_fd, u32 flags)
> > {
> > const struct net_device_ops *ops = dev->netdev_ops;
> > - enum bpf_netdev_command query;
> > + enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > + bool offload = mode == XDP_MODE_HW;
> > u32 prog_id, expected_id = 0;
> > - bpf_op_t bpf_op, bpf_chk;
> > struct bpf_prog *prog;
> > - bool offload;
> > + bpf_op_t bpf_op;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > - offload = flags & XDP_FLAGS_HW_MODE;
> > - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> > -
> > - bpf_op = bpf_chk = ops->ndo_bpf;
> > - if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
> > + bpf_op = dev_xdp_bpf_op(dev, mode);
> > + if (!bpf_op) {
> > NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> > return -EOPNOTSUPP;
> > }
>
> Seems that we won't ever hit this case with this patch?
> If flags are not drv/hw, then dev_xdp_mode() will always spit out the
> XDP_MODE_SKB which then passed to dev_xdp_bpf_op() will in turn give the
> generic_xdp_install().
>
But flags can be drv/hw, in which case dev_xdp_bpf_op() will return
dev->netdev_ops->ndo_bpf, which is NULL, so at that time we should
emit error that driver doesn't support native mode.
> I think this check was against the situation where user wanted to go with
> native mode but underlying HW was not supporting it, right?
right, and it's still the case.
>
> So right now we will always be silently going with generic XDP?
No, please check dev_xdp_bpf_op() implementation again, it returns
generic_xdp_install for SKB mode only.
>
> I haven't followed previous revisions so I might be missing something.
>
> > - if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> > - bpf_op = generic_xdp_install;
> > - if (bpf_op == bpf_chk)
> > - bpf_chk = generic_xdp_install;
> >
> > - prog_id = __dev_xdp_query(dev, bpf_op, query);
> > + prog_id = dev_xdp_prog_id(dev, mode);
> > if (flags & XDP_FLAGS_REPLACE) {
> > if (expected_fd >= 0) {
> > prog = bpf_prog_get_type_dev(expected_fd,
[...]
