Re: [PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 22, 2020 at 12:35:36PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 8:31 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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@fb.com>
> > > ---
> > >  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.

Thanks, I missed the fact about ndo_bpf being NULL. Comments below can be
ignored.

> 
> >
> > >
> > > -     /* 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,
> 
> [...]