On Wed, 22 May 2019 at 20:32, Jakub Kicinski
<jakub.kicinski@xxxxxxxxxxxxx> wrote:
>
> On Wed, 22 May 2019 14:53:51 +0200, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@xxxxxxxxx>
> >
> > All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> > command of ndo_bpf. The query code is fairly generic. This commit
> > refactors the query code up from the drivers to the netdev level.
> >
> > The struct net_device has gained four new members tracking the XDP
> > program, the corresponding program flags, and which programs
> > (SKB_MODE, DRV_MODE or HW_MODE) that are activated.
> >
> > The xdp_prog member, previously only used for SKB_MODE, is shared with
> > DRV_MODE, since they are mutually exclusive.
> >
> > The program query operations is all done under the rtnl lock. However,
> > the xdp_prog member is __rcu annotated, and used in a lock-less manner
> > for the SKB_MODE. This is because the xdp_prog member is shared from a
> > query program perspective (again, SKB and DRV are mutual exclusive),
> > RCU read and assignments functions are still used when altering
> > xdp_prog, even when only for queries in DRV_MODE. This is for
> > sparse/lockdep correctness.
> >
> > A minor behavioral change was done during this refactorization; When
> > passing a negative file descriptor to a netdev to disable XDP, the
> > same program flag as the running program is required. An example.
> >
> > The eth0 is DRV_DRV capable. Previously, this was OK, but confusing:
> >
> > # ip link set dev eth0 xdp obj foo.o sec main
> > # ip link set dev eth0 xdpgeneric off
> >
> > Now, the same commands give:
> >
> > # ip link set dev eth0 xdp obj foo.o sec main
> > # ip link set dev eth0 xdpgeneric off
> > Error: native and generic XDP can't be active at the same time.
>
> I'm not clear why this change is necessary? It is a change in
> behaviour, and if anything returning ENOENT would seem cleaner
> in this case.
>
To me, the existing behavior was non-intuitive. If most people *don't*
agree, I'll remove this change. So, what do people think about this?
:-)
ENOENT does make more sense.
> > Signed-off-by: Björn Töpel <bjorn.topel@xxxxxxxxx>
> > ---
> > include/linux/netdevice.h | 13 +++--
> > net/core/dev.c | 120 ++++++++++++++++++++------------------
> > net/core/rtnetlink.c | 33 ++---------
> > 3 files changed, 76 insertions(+), 90 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..bdcb20a3946c 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1940,6 +1940,11 @@ struct net_device {
> > #endif
> > struct hlist_node index_hlist;
> >
> > + struct bpf_prog *xdp_prog_hw;
> > + unsigned int xdp_flags;
> > + u32 xdp_prog_flags;
> > + u32 xdp_prog_hw_flags;
>
> Do we really need 3 xdp flags variables? Offloaded programs
> realistically must have only the HW mode flag set (not sure if
> netdevsim still pretends we can do offload of code after rewrites,
> but if it does it can be changed :)). Non-offloaded programs need
> flags, but we don't need the "all ORed" flags either, AFAICT. No?
>
Good point, I'll try to reduce the netdev bloat.
> > +
> > /*
> > * Cache lines mostly used on transmit path
> > */
> > @@ -2045,9 +2050,8 @@ struct net_device {
> >
> > static inline bool netif_elide_gro(const struct net_device *dev)
> > {
> > - if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> > - return true;
> > - return false;
> > + return !(dev->features & NETIF_F_GRO) ||
> > + dev->xdp_flags & XDP_FLAGS_SKB_MODE;
> > }
> >
> > #define NETDEV_ALIGN 32
> > @@ -3684,8 +3688,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> > typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, u32 flags);
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> > - enum bpf_netdev_command cmd);
> > +u32 dev_xdp_query(struct net_device *dev, unsigned int flags);
> > int xdp_umem_query(struct net_device *dev, u16 queue_id);
> >
> > int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index b6b8505cfb3e..ead16c3f955c 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8005,31 +8005,31 @@ int dev_change_proto_down_generic(struct net_device *dev, bool proto_down)
> > }
> > EXPORT_SYMBOL(dev_change_proto_down_generic);
> >
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > - enum bpf_netdev_command cmd)
> > +u32 dev_xdp_query(struct net_device *dev, unsigned int flags)
>
> You only pass the mode here, so perhaps rename the flags argument to
> mode?
>
> > {
> > - struct netdev_bpf xdp;
> > + struct bpf_prog *prog = NULL;
> >
> > - if (!bpf_op)
> > + if (hweight32(flags) != 1)
> > return 0;
>
> This looks superfluous, given callers will always pass mode, right?
>
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = cmd;
> > -
> > - /* Query must always succeed. */
> > - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> > + if (flags & (XDP_FLAGS_SKB_MODE | XDP_FLAGS_DRV_MODE))
> > + prog = rtnl_dereference(dev->xdp_prog);
> > + else if (flags & XDP_FLAGS_HW_MODE)
> > + prog = dev->xdp_prog_hw;
>
> Perhaps use a switch statement here?
>
> > - return xdp.prog_id;
> > + return prog ? prog->aux->id : 0;
> > }
> >
> > -static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > +static int dev_xdp_install(struct net_device *dev, unsigned int target,
>
> Could you say mode instead of target everywhere?
>
> > struct netlink_ext_ack *extack, u32 flags,
> > struct bpf_prog *prog)
> > {
> > + bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
>
> The point of passing bpf_op around is to have the right one (generic or
> driver) this is lost with the ternary statement below :S
>
> > struct netdev_bpf xdp;
> > + int err;
> >
> > memset(&xdp, 0, sizeof(xdp));
> > - if (flags & XDP_FLAGS_HW_MODE)
> > + if (target == XDP_FLAGS_HW_MODE)
>
> Is this change necessary?
>
> > xdp.command = XDP_SETUP_PROG_HW;
> > else
> > xdp.command = XDP_SETUP_PROG;
> > @@ -8037,35 +8037,41 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > xdp.flags = flags;
> > xdp.prog = prog;
> >
> > - return bpf_op(dev, &xdp);
> > + err = (target == XDP_FLAGS_SKB_MODE) ?
>
> Brackets unnecessary.
>
> > + generic_xdp_install(dev, &xdp) :
> > + bpf_op(dev, &xdp);
> > + if (!err) {
>
> Keep success path unindented, save indentation.
>
> if (err)
> return err;
>
> bla bla
>
> return 0;
>
> > + if (prog)
> > + dev->xdp_flags |= target;
> > + else
> > + dev->xdp_flags &= ~target;
>
> These "all ORed" flags are not needed, AFAICT, as mentioned above.
>
> > + if (target == XDP_FLAGS_HW_MODE) {
> > + dev->xdp_prog_hw = prog;
> > + dev->xdp_prog_hw_flags = flags;
> > + } else if (target == XDP_FLAGS_DRV_MODE) {
> > + rcu_assign_pointer(dev->xdp_prog, prog);
> > + dev->xdp_prog_flags = flags;
> > + }
> > + }
> > +
> > + return err;
> > }
> >
> > static void dev_xdp_uninstall(struct net_device *dev)
> > {
> > - struct netdev_bpf xdp;
> > - bpf_op_t ndo_bpf;
> > -
> > - /* Remove generic XDP */
> > - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > -
> > - /* Remove from the driver */
> > - ndo_bpf = dev->netdev_ops->ndo_bpf;
> > - if (!ndo_bpf)
> > - return;
> > -
> > - 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));
> > -
> > - /* 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));
> > + if (dev->xdp_flags & XDP_FLAGS_SKB_MODE) {
> > + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_SKB_MODE,
> > + NULL, 0, NULL));
> > + }
>
> Brackets unnecessary.
>
> > + if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> > + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> > + NULL, dev->xdp_prog_flags, NULL));
> > + }
>
> You should be able to just call install with the original flags, and
> install handler should do the right maths again to direct it either to
> drv or generic, no?
>
> > + if (dev->xdp_flags & XDP_FLAGS_HW_MODE) {
> > + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_HW_MODE,
> > + NULL, dev->xdp_prog_hw_flags, NULL));
> > + }
> > }
> >
> > /**
> > @@ -8080,41 +8086,41 @@ static void dev_xdp_uninstall(struct net_device *dev)
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, u32 flags)
> > {
> > - const struct net_device_ops *ops = dev->netdev_ops;
> > - enum bpf_netdev_command query;
> > + bool offload, drv_supp = !!dev->netdev_ops->ndo_bpf;
>
> Please avoid calculations in init. If the function gets complicated
> this just ends up as a weirdly indented code, which is hard to read.
>
> > struct bpf_prog *prog = NULL;
> > - bpf_op_t bpf_op, bpf_chk;
> > - bool offload;
> > + unsigned int target;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > offload = flags & XDP_FLAGS_HW_MODE;
> > - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> > + target = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
> >
> > - bpf_op = bpf_chk = ops->ndo_bpf;
> > - if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
> > + if (!drv_supp && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
>
> Here you have a bracket for & inside an &&..
>
> > NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> > return -EOPNOTSUPP;
> > }
> > - if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> > - bpf_op = generic_xdp_install;
> > - if (bpf_op == bpf_chk)
> > - bpf_chk = generic_xdp_install;
> > +
> > + if (!drv_supp || (flags & XDP_FLAGS_SKB_MODE))
> > + target = XDP_FLAGS_SKB_MODE;
> > +
> > + if ((mode == XDP_FLAGS_SKB_MODE &&
> > + dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
>
> .. and here you don't :)
>
> > + (target == XDP_FLAGS_DRV_MODE &&
> > + dev->xdp_flags & XDP_FLAGS_SKB_MODE)) {
>
> I think this condition can be shortened. You can't get a conflict if
> the driver has no support. So the only conflicting case is:
>
> rcu_access_pointer(dev->xdp_prog) && drv_supp &&
> (flags ^ dev->xdp_flags) & XDP_FLAGS_SKB_MODE
>
> > + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> > + return -EEXIST;
> > + }
> >
> > if (fd >= 0) {
> > - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> > - NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> > - return -EEXIST;
> > - }
> > - if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) &&
> > - __dev_xdp_query(dev, bpf_op, query)) {
> > + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> > + dev_xdp_query(dev, target)) {
> > NL_SET_ERR_MSG(extack, "XDP program already attached");
> > return -EBUSY;
> > }
> >
> > - prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> > - bpf_op == ops->ndo_bpf);
> > + prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, drv_supp);
> > +
>
> Extra new line.
>
> > if (IS_ERR(prog))
> > return PTR_ERR(prog);
> >
> > @@ -8125,7 +8131,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > }
> > }
> >
> > - err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> > + err = dev_xdp_install(dev, target, extack, flags, prog);
> > if (err < 0 && prog)
> > bpf_prog_put(prog);
> >
>
> I think apart from returning the new error it looks functionally
> correct :)
Thanks for the review, Jakub. Very valuable. I'll address all your
comments in the v3!
Cheers,
Björn
