* [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code
@ 2019-05-22 12:53 Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 12:53 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, bjorn.topel, brouer, bpf,
jakub.kicinski
Here's another attempt (the first, horribly broken one, here [1]) to
move the XDP_QUERY_PROG{,_HW} code out from the drivers to generic
netdev code.
The first patch in the series move the XDP query functionality, and
the second remove XDP_QUERY_PROG{,_HW} from all drivers.
Please refer to the individual commit messages for more details.
Shout out to all XDP driver hackers to check that the second patch
doesn't break anything (especially Jakub). I've only been able to test
on the Intel NICs.
Thanks,
Björn
[1] https://lore.kernel.org/netdev/CAJ+HfNjfcGW=b_Ckox4jXf7od7yr+Sk2dXxFyO8Qpr-WJX0q7A@mail.gmail.com/
Björn Töpel (2):
net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
net: xdp: remove XDP_QUERY_PROG{,_HW}
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 -
.../net/ethernet/cavium/thunder/nicvf_main.c | 3 -
.../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 3 -
drivers/net/ethernet/intel/i40e/i40e_main.c | 3 -
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 -
.../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 -
.../net/ethernet/mellanox/mlx4/en_netdev.c | 24 ----
.../net/ethernet/mellanox/mlx5/core/en_main.c | 18 ---
.../ethernet/netronome/nfp/nfp_net_common.c | 4 -
.../net/ethernet/qlogic/qede/qede_filter.c | 3 -
drivers/net/netdevsim/bpf.c | 4 -
drivers/net/netdevsim/netdevsim.h | 2 +-
drivers/net/tun.c | 15 ---
drivers/net/veth.c | 15 ---
drivers/net/virtio_net.c | 17 ---
include/linux/netdevice.h | 21 ++-
include/net/xdp.h | 2 -
net/core/dev.c | 125 +++++++++---------
net/core/rtnetlink.c | 33 +----
net/core/xdp.c | 9 --
20 files changed, 77 insertions(+), 236 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 12:53 [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
@ 2019-05-22 12:53 ` Björn Töpel
2019-05-22 13:02 ` Toke Høiland-Jørgensen
` (2 more replies)
2019-05-22 12:53 ` [PATCH bpf-next 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
2019-05-22 17:41 ` [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Jakub Kicinski
2 siblings, 3 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 12:53 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, brouer, bpf, jakub.kicinski
From: Björn Töpel <bjorn.topel@intel.com>
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.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
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;
+
/*
* 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)
{
- struct netdev_bpf xdp;
+ struct bpf_prog *prog = NULL;
- if (!bpf_op)
+ if (hweight32(flags) != 1)
return 0;
- 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;
- 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,
struct netlink_ext_ack *extack, u32 flags,
struct bpf_prog *prog)
{
+ bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
struct netdev_bpf xdp;
+ int err;
memset(&xdp, 0, sizeof(xdp));
- if (flags & XDP_FLAGS_HW_MODE)
+ if (target == XDP_FLAGS_HW_MODE)
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) ?
+ generic_xdp_install(dev, &xdp) :
+ bpf_op(dev, &xdp);
+ if (!err) {
+ if (prog)
+ dev->xdp_flags |= target;
+ else
+ dev->xdp_flags &= ~target;
+
+ 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));
+ }
+ if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
+ WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
+ NULL, dev->xdp_prog_flags, NULL));
+ }
+ 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;
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))) {
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 ((target == XDP_FLAGS_SKB_MODE &&
+ dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
+ (target == XDP_FLAGS_DRV_MODE &&
+ 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);
+
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);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index adcc045952c2..99ca58db297f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1360,37 +1360,14 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
return 0;
}
-static u32 rtnl_xdp_prog_skb(struct net_device *dev)
-{
- const struct bpf_prog *generic_xdp_prog;
-
- ASSERT_RTNL();
-
- generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
- if (!generic_xdp_prog)
- return 0;
- return generic_xdp_prog->aux->id;
-}
-
-static u32 rtnl_xdp_prog_drv(struct net_device *dev)
-{
- return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
-}
-
-static u32 rtnl_xdp_prog_hw(struct net_device *dev)
-{
- return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
- XDP_QUERY_PROG_HW);
-}
-
static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
- u32 (*get_prog_id)(struct net_device *dev))
+ unsigned int tgt_flags)
{
u32 curr_id;
int err;
- curr_id = get_prog_id(dev);
+ curr_id = dev_xdp_query(dev, tgt_flags);
if (!curr_id)
return 0;
@@ -1421,15 +1398,15 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
prog_id = 0;
mode = XDP_ATTACHED_NONE;
err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
- IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
+ IFLA_XDP_SKB_PROG_ID, XDP_FLAGS_SKB_MODE);
if (err)
goto err_cancel;
err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
- IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
+ IFLA_XDP_DRV_PROG_ID, XDP_FLAGS_DRV_MODE);
if (err)
goto err_cancel;
err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
- IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
+ IFLA_XDP_HW_PROG_ID, XDP_FLAGS_HW_MODE);
if (err)
goto err_cancel;
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH bpf-next 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW}
2019-05-22 12:53 [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
@ 2019-05-22 12:53 ` Björn Töpel
2019-05-22 17:41 ` [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Jakub Kicinski
2 siblings, 0 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 12:53 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, brouer, bpf, jakub.kicinski
From: Björn Töpel <bjorn.topel@intel.com>
Remove all use of XDP_QUERY_PROG{,_HW}, since it was moved to the
generic code path.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 4 ----
.../net/ethernet/cavium/thunder/nicvf_main.c | 3 ---
.../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 3 ---
drivers/net/ethernet/intel/i40e/i40e_main.c | 3 ---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ----
.../net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ----
.../net/ethernet/mellanox/mlx4/en_netdev.c | 24 -------------------
.../net/ethernet/mellanox/mlx5/core/en_main.c | 18 --------------
.../ethernet/netronome/nfp/nfp_net_common.c | 4 ----
.../net/ethernet/qlogic/qede/qede_filter.c | 3 ---
drivers/net/netdevsim/bpf.c | 4 ----
drivers/net/netdevsim/netdevsim.h | 2 +-
drivers/net/tun.c | 15 ------------
drivers/net/veth.c | 15 ------------
drivers/net/virtio_net.c | 17 -------------
include/linux/netdevice.h | 8 -------
include/net/xdp.h | 2 --
net/core/dev.c | 5 ----
net/core/xdp.c | 9 -------
19 files changed, 1 insertion(+), 146 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 0184ef6f05a7..8b1e77522e18 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -217,10 +217,6 @@ int bnxt_xdp(struct net_device *dev, struct netdev_bpf *xdp)
case XDP_SETUP_PROG:
rc = bnxt_xdp_set(bp, xdp->prog);
break;
- case XDP_QUERY_PROG:
- xdp->prog_id = bp->xdp_prog ? bp->xdp_prog->aux->id : 0;
- rc = 0;
- break;
default:
rc = -EINVAL;
break;
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index c032bef1b776..14c079538cb5 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1914,9 +1914,6 @@ static int nicvf_xdp(struct net_device *netdev, struct netdev_bpf *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return nicvf_xdp_setup(nic, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = nic->xdp_prog ? nic->xdp_prog->aux->id : 0;
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 63b1ecc18c26..a00cf674b8c8 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1763,9 +1763,6 @@ static int dpaa2_eth_xdp(struct net_device *dev, struct netdev_bpf *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return setup_xdp(dev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = priv->xdp_prog ? priv->xdp_prog->aux->id : 0;
- break;
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 320562b39686..3dd591068591 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12302,9 +12302,6 @@ static int i40e_xdp(struct net_device *dev,
switch (xdp->command) {
case XDP_SETUP_PROG:
return i40e_xdp_setup(vsi, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = vsi->xdp_prog ? vsi->xdp_prog->aux->id : 0;
- return 0;
case XDP_SETUP_XSK_UMEM:
return i40e_xsk_umem_setup(vsi, xdp->xsk.umem,
xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 57fd9ee6de66..59dc82c71f9c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10284,10 +10284,6 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return ixgbe_xdp_setup(dev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = adapter->xdp_prog ?
- adapter->xdp_prog->aux->id : 0;
- return 0;
case XDP_SETUP_XSK_UMEM:
return ixgbe_xsk_umem_setup(adapter, xdp->xsk.umem,
xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index d189ed247665..80a22d638734 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -4485,10 +4485,6 @@ static int ixgbevf_xdp(struct net_device *dev, struct netdev_bpf *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return ixgbevf_xdp_setup(dev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = adapter->xdp_prog ?
- adapter->xdp_prog->aux->id : 0;
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index c1438ae52a11..8850fc35510a 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2883,35 +2883,11 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
return err;
}
-static u32 mlx4_xdp_query(struct net_device *dev)
-{
- struct mlx4_en_priv *priv = netdev_priv(dev);
- struct mlx4_en_dev *mdev = priv->mdev;
- const struct bpf_prog *xdp_prog;
- u32 prog_id = 0;
-
- if (!priv->tx_ring_num[TX_XDP])
- return prog_id;
-
- mutex_lock(&mdev->state_lock);
- xdp_prog = rcu_dereference_protected(
- priv->rx_ring[0]->xdp_prog,
- lockdep_is_held(&mdev->state_lock));
- if (xdp_prog)
- prog_id = xdp_prog->aux->id;
- mutex_unlock(&mdev->state_lock);
-
- return prog_id;
-}
-
static int mlx4_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
return mlx4_xdp_set(dev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = mlx4_xdp_query(dev);
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 457cc39423f2..f1b80c1d9ca2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4237,29 +4237,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
return err;
}
-static u32 mlx5e_xdp_query(struct net_device *dev)
-{
- struct mlx5e_priv *priv = netdev_priv(dev);
- const struct bpf_prog *xdp_prog;
- u32 prog_id = 0;
-
- mutex_lock(&priv->state_lock);
- xdp_prog = priv->channels.params.xdp_prog;
- if (xdp_prog)
- prog_id = xdp_prog->aux->id;
- mutex_unlock(&priv->state_lock);
-
- return prog_id;
-}
-
static int mlx5e_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
return mlx5e_xdp_set(dev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = mlx5e_xdp_query(dev);
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index b82b684f52ce..2a9683db54e5 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3548,10 +3548,6 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_bpf *xdp)
return nfp_net_xdp_setup_drv(nn, xdp);
case XDP_SETUP_PROG_HW:
return nfp_net_xdp_setup_hw(nn, xdp);
- case XDP_QUERY_PROG:
- return xdp_attachment_query(&nn->xdp, xdp);
- case XDP_QUERY_PROG_HW:
- return xdp_attachment_query(&nn->xdp_hw, xdp);
default:
return nfp_app_bpf(nn->app, nn, xdp);
}
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index add922b93d2c..69f4e7d37d01 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1118,9 +1118,6 @@ int qede_xdp(struct net_device *dev, struct netdev_bpf *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
return qede_xdp_set(edev, xdp->prog);
- case XDP_QUERY_PROG:
- xdp->prog_id = edev->xdp_prog ? edev->xdp_prog->aux->id : 0;
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 2b74425822ab..d03d31721e38 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -549,10 +549,6 @@ int nsim_bpf(struct net_device *dev, struct netdev_bpf *bpf)
ASSERT_RTNL();
switch (bpf->command) {
- case XDP_QUERY_PROG:
- return xdp_attachment_query(&ns->xdp, bpf);
- case XDP_QUERY_PROG_HW:
- return xdp_attachment_query(&ns->xdp_hw, bpf);
case XDP_SETUP_PROG:
err = nsim_setup_prog_checks(ns, bpf);
if (err)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 3f398797c2bc..3f75f50b3250 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -108,7 +108,7 @@ static inline void nsim_bpf_uninit(struct netdevsim *ns)
static inline int nsim_bpf(struct net_device *dev, struct netdev_bpf *bpf)
{
- return bpf->command == XDP_QUERY_PROG ? 0 : -EOPNOTSUPP;
+ return -EOPNOTSUPP;
}
static inline int nsim_bpf_disable_tc(struct netdevsim *ns)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index abae165dcca5..dbf115aa5c11 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1236,26 +1236,11 @@ static int tun_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return 0;
}
-static u32 tun_xdp_query(struct net_device *dev)
-{
- struct tun_struct *tun = netdev_priv(dev);
- const struct bpf_prog *xdp_prog;
-
- xdp_prog = rtnl_dereference(tun->xdp_prog);
- if (xdp_prog)
- return xdp_prog->aux->id;
-
- return 0;
-}
-
static int tun_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
return tun_xdp_set(dev, xdp->prog, xdp->extack);
- case XDP_QUERY_PROG:
- xdp->prog_id = tun_xdp_query(dev);
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 09a1433b0833..8fde5f319bfb 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1096,26 +1096,11 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return err;
}
-static u32 veth_xdp_query(struct net_device *dev)
-{
- struct veth_priv *priv = netdev_priv(dev);
- const struct bpf_prog *xdp_prog;
-
- xdp_prog = priv->_xdp_prog;
- if (xdp_prog)
- return xdp_prog->aux->id;
-
- return 0;
-}
-
static int veth_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
return veth_xdp_set(dev, xdp->prog, xdp->extack);
- case XDP_QUERY_PROG:
- xdp->prog_id = veth_xdp_query(dev);
- return 0;
default:
return -EINVAL;
}
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 559c48e66afc..f201d5a19cd3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2525,28 +2525,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return err;
}
-static u32 virtnet_xdp_query(struct net_device *dev)
-{
- struct virtnet_info *vi = netdev_priv(dev);
- const struct bpf_prog *xdp_prog;
- int i;
-
- for (i = 0; i < vi->max_queue_pairs; i++) {
- xdp_prog = rtnl_dereference(vi->rq[i].xdp_prog);
- if (xdp_prog)
- return xdp_prog->aux->id;
- }
- return 0;
-}
-
static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
return virtnet_xdp_set(dev, xdp->prog, xdp->extack);
- case XDP_QUERY_PROG:
- xdp->prog_id = virtnet_xdp_query(dev);
- return 0;
default:
return -EINVAL;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index bdcb20a3946c..fa944bc507cd 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -864,8 +864,6 @@ enum bpf_netdev_command {
*/
XDP_SETUP_PROG,
XDP_SETUP_PROG_HW,
- XDP_QUERY_PROG,
- XDP_QUERY_PROG_HW,
/* BPF program for offload callbacks, invoked at program load time. */
BPF_OFFLOAD_MAP_ALLOC,
BPF_OFFLOAD_MAP_FREE,
@@ -885,12 +883,6 @@ struct netdev_bpf {
struct bpf_prog *prog;
struct netlink_ext_ack *extack;
};
- /* XDP_QUERY_PROG, XDP_QUERY_PROG_HW */
- struct {
- u32 prog_id;
- /* flags with which program was installed */
- u32 prog_flags;
- };
/* BPF_OFFLOAD_MAP_ALLOC, BPF_OFFLOAD_MAP_FREE */
struct {
struct bpf_offloaded_map *offmap;
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 0f25b3675c5c..4ad4b20fe2c0 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -159,8 +159,6 @@ struct xdp_attachment_info {
};
struct netdev_bpf;
-int xdp_attachment_query(struct xdp_attachment_info *info,
- struct netdev_bpf *bpf);
bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
struct netdev_bpf *bpf);
void xdp_attachment_setup(struct xdp_attachment_info *info,
diff --git a/net/core/dev.c b/net/core/dev.c
index ead16c3f955c..33792b495ce4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5156,11 +5156,6 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
dev_disable_gro_hw(dev);
}
break;
-
- case XDP_QUERY_PROG:
- xdp->prog_id = old ? old->aux->id : 0;
- break;
-
default:
ret = -EINVAL;
break;
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 4b2b194f4f1f..6f76ad995fef 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -379,15 +379,6 @@ void xdp_return_buff(struct xdp_buff *xdp)
}
EXPORT_SYMBOL_GPL(xdp_return_buff);
-int xdp_attachment_query(struct xdp_attachment_info *info,
- struct netdev_bpf *bpf)
-{
- bpf->prog_id = info->prog ? info->prog->aux->id : 0;
- bpf->prog_flags = info->prog ? info->flags : 0;
- return 0;
-}
-EXPORT_SYMBOL_GPL(xdp_attachment_query);
-
bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
struct netdev_bpf *bpf)
{
--
2.20.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
@ 2019-05-22 13:02 ` Toke Høiland-Jørgensen
2019-05-22 18:32 ` Jakub Kicinski
2019-05-23 5:47 ` Saeed Mahameed
2 siblings, 0 replies; 15+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-05-22 13:02 UTC (permalink / raw)
To: Björn Töpel, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, brouer, bpf, jakub.kicinski
Björn Töpel <bjorn.topel@gmail.com> writes:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> 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.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Nice work!
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> 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;
> +
> /*
> * 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)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = NULL;
>
> - if (!bpf_op)
> + if (hweight32(flags) != 1)
> return 0;
>
> - 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;
>
> - 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,
> struct netlink_ext_ack *extack, u32 flags,
> struct bpf_prog *prog)
> {
> + bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (target == XDP_FLAGS_HW_MODE)
> 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) ?
> + generic_xdp_install(dev, &xdp) :
> + bpf_op(dev, &xdp);
> + if (!err) {
> + if (prog)
> + dev->xdp_flags |= target;
> + else
> + dev->xdp_flags &= ~target;
> +
> + 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));
> + }
> + if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> + NULL, dev->xdp_prog_flags, NULL));
> + }
> + 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;
> 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))) {
> 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 ((target == XDP_FLAGS_SKB_MODE &&
> + dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
> + (target == XDP_FLAGS_DRV_MODE &&
> + 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);
> +
> 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);
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..99ca58db297f 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,37 +1360,14 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
> return 0;
> }
>
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> -{
> - const struct bpf_prog *generic_xdp_prog;
> -
> - ASSERT_RTNL();
> -
> - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> - if (!generic_xdp_prog)
> - return 0;
> - return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> -}
> -
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> -}
> -
> static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> - u32 (*get_prog_id)(struct net_device *dev))
> + unsigned int tgt_flags)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, tgt_flags);
> if (!curr_id)
> return 0;
>
> @@ -1421,15 +1398,15 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
> prog_id = 0;
> mode = XDP_ATTACHED_NONE;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
> - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> + IFLA_XDP_SKB_PROG_ID, XDP_FLAGS_SKB_MODE);
> if (err)
> goto err_cancel;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
> - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> + IFLA_XDP_DRV_PROG_ID, XDP_FLAGS_DRV_MODE);
> if (err)
> goto err_cancel;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
> - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> + IFLA_XDP_HW_PROG_ID, XDP_FLAGS_HW_MODE);
> if (err)
> goto err_cancel;
>
> --
> 2.20.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code
2019-05-22 12:53 [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
@ 2019-05-22 17:41 ` Jakub Kicinski
2019-05-22 20:48 ` Björn Töpel
2 siblings, 1 reply; 15+ messages in thread
From: Jakub Kicinski @ 2019-05-22 17:41 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, magnus.karlsson, bjorn.topel, brouer, bpf
On Wed, 22 May 2019 14:53:50 +0200, Björn Töpel wrote:
> Shout out to all XDP driver hackers to check that the second patch
> doesn't break anything (especially Jakub). I've only been able to test
> on the Intel NICs.
Please test XDP offload on netdevsim, that's why we have it! :)
At the minimum please run tools/testing/selftests/bpf/test_offload.py
Now let me look at the code :)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-22 13:02 ` Toke Høiland-Jørgensen
@ 2019-05-22 18:32 ` Jakub Kicinski
2019-05-22 20:54 ` Björn Töpel
2019-05-28 17:06 ` Björn Töpel
2019-05-23 5:47 ` Saeed Mahameed
2 siblings, 2 replies; 15+ messages in thread
From: Jakub Kicinski @ 2019-05-22 18:32 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, Björn Töpel,
magnus.karlsson, brouer, bpf
On Wed, 22 May 2019 14:53:51 +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> 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.
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> 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?
> +
> /*
> * 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 :)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code
2019-05-22 17:41 ` [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Jakub Kicinski
@ 2019-05-22 20:48 ` Björn Töpel
0 siblings, 0 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 20:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Karlsson, Magnus, Björn Töpel,
Jesper Dangaard Brouer, bpf
On Wed, 22 May 2019 at 19:41, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 22 May 2019 14:53:50 +0200, Björn Töpel wrote:
> > Shout out to all XDP driver hackers to check that the second patch
> > doesn't break anything (especially Jakub). I've only been able to test
> > on the Intel NICs.
>
> Please test XDP offload on netdevsim, that's why we have it! :)
> At the minimum please run tools/testing/selftests/bpf/test_offload.py
>
Yikes, I did not know about this. Thanks for pointing it out! I'll do
that from now on!
Thanks,
Björn
> Now let me look at the code :)
Thanks! :-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 18:32 ` Jakub Kicinski
@ 2019-05-22 20:54 ` Björn Töpel
2019-05-22 21:04 ` Jakub Kicinski
2019-05-28 17:06 ` Björn Töpel
1 sibling, 1 reply; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 20:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf
On Wed, 22 May 2019 at 20:32, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 22 May 2019 14:53:51 +0200, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > 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@intel.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 20:54 ` Björn Töpel
@ 2019-05-22 21:04 ` Jakub Kicinski
2019-05-22 21:12 ` Björn Töpel
2019-05-23 8:55 ` Toke Høiland-Jørgensen
0 siblings, 2 replies; 15+ messages in thread
From: Jakub Kicinski @ 2019-05-22 21:04 UTC (permalink / raw)
To: Björn Töpel
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf
On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
> > > 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?
> :-)
Having things start to fail after they were successful/ignored
is one of those ABI breakage types Linux and netdev usually takes
pretty seriously, unfortunately. Especially when motivation is
"it's more intuitive" :)
If nobody chimes in please break out this behaviour change into
a commit of its own.
> ENOENT does make more sense.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 21:04 ` Jakub Kicinski
@ 2019-05-22 21:12 ` Björn Töpel
2019-05-23 8:55 ` Toke Høiland-Jørgensen
1 sibling, 0 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-22 21:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf
On Wed, 22 May 2019 at 23:04, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
> > > > 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?
> > :-)
>
> Having things start to fail after they were successful/ignored
> is one of those ABI breakage types Linux and netdev usually takes
> pretty seriously, unfortunately. Especially when motivation is
> "it's more intuitive" :)
>
Hey, intuition is a great thing! :-D
> If nobody chimes in please break out this behaviour change into
> a commit of its own.
>
Will do.
Björn
> > ENOENT does make more sense.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-22 13:02 ` Toke Høiland-Jørgensen
2019-05-22 18:32 ` Jakub Kicinski
@ 2019-05-23 5:47 ` Saeed Mahameed
2019-05-23 6:35 ` Björn Töpel
2 siblings, 1 reply; 15+ messages in thread
From: Saeed Mahameed @ 2019-05-23 5:47 UTC (permalink / raw)
To: toke, daniel, ast, netdev, bjorn.topel
Cc: magnus.karlsson, bjorn.topel, jakub.kicinski, brouer, bpf
On Wed, 2019-05-22 at 14:53 +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> 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.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> 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;
> +
Maybe it will be nicer if you wrap all xdp related data in one struct,
This is good as a weak enforcement so future xdp extensions will go to
the same place and not spread all across the net_device struct.
this is going to be handy when we start introducing XDP offloads.
> /*
> * 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)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = NULL;
>
> - if (!bpf_op)
> + if (hweight32(flags) != 1)
> return 0;
>
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = cmd;
> -
> - /* Query must always succeed. */
> - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
since now this function can only query PROG or PROG_HW it is a good
time to reflect that in the function name, maybe dev_xdp_query_prog is
a better name ?
> + 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;
>
> - 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,
> struct netlink_ext_ack *extack, u32 flags,
> struct bpf_prog *prog)
> {
> + bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
I would assing bpf_op below :
bpf_op = (target == XDP_FLAGS_SKB_MODE) ?
generic_xdp_install :
dev->netdev_ops->ndo_bpf;
or just drop bpf_op and call dev->netdev_ops->ndo_bpf in the one place
it bpf_op is being called.
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (target == XDP_FLAGS_HW_MODE)
> 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;
Are you sure no one is using flags
>
> - return bpf_op(dev, &xdp);
> + err = (target == XDP_FLAGS_SKB_MODE) ?
> + generic_xdp_install(dev, &xdp) :
> + bpf_op(dev, &xdp);
> + if (!err) {
> + if (prog)
> + dev->xdp_flags |= target;
> + else
> + dev->xdp_flags &= ~target;
> +
> + 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));
> + }
> + if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> + NULL, dev->xdp_prog_flags,
> NULL));
> + }
> + 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));
> + }
instead this long "if" dance, why don't you just use xdp->flags inside
dev_xdp_install when prog is NULL ?
I would even consider introducing an uninstall function, which doesn't
receive flags, and will use xdp->flags directly..
uninstall can be just a two liner function
dev_xdp_uninstall() {
xdp.prog = NULL;
(dev->xdp_flags == XDP_FLAGS_SKB_MODE) ?
generic_xdp_install(dev, &xdp) :
dev->netdev_ops->ndo_bpf(dev, &xdp);
dev->xdp_flags &= ~(XDP_FLAGS_SKB_MODE | XDP_FLAGS_HW_MODE |
XDP_FLAGS_DRV_MODE);
}
> }
>
> /**
> @@ -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;
> 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;
I don't think you really need this target parameter it only complicates
things for no reason, all information you need are already in flags and
dev->xdp_flags.
>
> - 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))) {
> 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;
> +
since dev_xdp_install handles all cases now, you don't need this logic
here, I would move this into dev_xdp_install, maybe introduce a helper
dedicated function to choose the mode to be used.
> + if ((target == XDP_FLAGS_SKB_MODE &&
> + dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
> + (target == XDP_FLAGS_DRV_MODE &&
> + 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);
> +
> 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);
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..99ca58db297f 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,37 +1360,14 @@ static int rtnl_fill_link_ifmap(struct
> sk_buff *skb, struct net_device *dev)
> return 0;
> }
>
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> -{
> - const struct bpf_prog *generic_xdp_prog;
> -
> - ASSERT_RTNL();
> -
> - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> - if (!generic_xdp_prog)
> - return 0;
> - return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> XDP_QUERY_PROG);
> -}
> -
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> -}
> -
> static int rtnl_xdp_report_one(struct sk_buff *skb, struct
> net_device *dev,
> u32 *prog_id, u8 *mode, u8 tgt_mode, u32
> attr,
> - u32 (*get_prog_id)(struct net_device
> *dev))
> + unsigned int tgt_flags)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, tgt_flags);
> if (!curr_id)
> return 0;
>
> @@ -1421,15 +1398,15 @@ static int rtnl_xdp_fill(struct sk_buff *skb,
> struct net_device *dev)
> prog_id = 0;
> mode = XDP_ATTACHED_NONE;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB,
> - IFLA_XDP_SKB_PROG_ID,
> rtnl_xdp_prog_skb);
> + IFLA_XDP_SKB_PROG_ID,
> XDP_FLAGS_SKB_MODE);
> if (err)
> goto err_cancel;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV,
> - IFLA_XDP_DRV_PROG_ID,
> rtnl_xdp_prog_drv);
> + IFLA_XDP_DRV_PROG_ID,
> XDP_FLAGS_DRV_MODE);
> if (err)
> goto err_cancel;
> err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW,
> - IFLA_XDP_HW_PROG_ID,
> rtnl_xdp_prog_hw);
> + IFLA_XDP_HW_PROG_ID,
> XDP_FLAGS_HW_MODE);
> if (err)
> goto err_cancel;
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-23 5:47 ` Saeed Mahameed
@ 2019-05-23 6:35 ` Björn Töpel
0 siblings, 0 replies; 15+ messages in thread
From: Björn Töpel @ 2019-05-23 6:35 UTC (permalink / raw)
To: Saeed Mahameed
Cc: toke, daniel, ast, netdev, magnus.karlsson, bjorn.topel,
jakub.kicinski, brouer, bpf
On Thu, 23 May 2019 at 07:47, Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Wed, 2019-05-22 at 14:53 +0200, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > 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.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > ---
> > 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;
> > +
>
>
> Maybe it will be nicer if you wrap all xdp related data in one struct,
> This is good as a weak enforcement so future xdp extensions will go to
> the same place and not spread all across the net_device struct.
>
> this is going to be handy when we start introducing XDP offloads.
>
> > /*
> > * 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)
> > {
> > - struct netdev_bpf xdp;
> > + struct bpf_prog *prog = NULL;
> >
> > - if (!bpf_op)
> > + if (hweight32(flags) != 1)
> > return 0;
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = cmd;
> > -
> > - /* Query must always succeed. */
> > - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
>
> since now this function can only query PROG or PROG_HW it is a good
> time to reflect that in the function name, maybe dev_xdp_query_prog is
> a better name ?
>
> > + 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;
> >
> > - 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,
> > struct netlink_ext_ack *extack, u32 flags,
> > struct bpf_prog *prog)
> > {
> > + bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
>
> I would assing bpf_op below :
> bpf_op = (target == XDP_FLAGS_SKB_MODE) ?
> generic_xdp_install :
> dev->netdev_ops->ndo_bpf;
>
> or just drop bpf_op and call dev->netdev_ops->ndo_bpf in the one place
> it bpf_op is being called.
>
> > struct netdev_bpf xdp;
> > + int err;
> >
> > memset(&xdp, 0, sizeof(xdp));
> > - if (flags & XDP_FLAGS_HW_MODE)
> > + if (target == XDP_FLAGS_HW_MODE)
> > 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;
>
> Are you sure no one is using flags
> >
> > - return bpf_op(dev, &xdp);
> > + err = (target == XDP_FLAGS_SKB_MODE) ?
> > + generic_xdp_install(dev, &xdp) :
> > + bpf_op(dev, &xdp);
> > + if (!err) {
> > + if (prog)
> > + dev->xdp_flags |= target;
> > + else
> > + dev->xdp_flags &= ~target;
> > +
> > + 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));
> > + }
> > + if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> > + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> > + NULL, dev->xdp_prog_flags,
> > NULL));
> > + }
> > + 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));
> > + }
>
> instead this long "if" dance, why don't you just use xdp->flags inside
> dev_xdp_install when prog is NULL ?
> I would even consider introducing an uninstall function, which doesn't
> receive flags, and will use xdp->flags directly..
>
> uninstall can be just a two liner function
>
> dev_xdp_uninstall() {
> xdp.prog = NULL;
> (dev->xdp_flags == XDP_FLAGS_SKB_MODE) ?
> generic_xdp_install(dev, &xdp) :
> dev->netdev_ops->ndo_bpf(dev, &xdp);
> dev->xdp_flags &= ~(XDP_FLAGS_SKB_MODE | XDP_FLAGS_HW_MODE |
> XDP_FLAGS_DRV_MODE);
> }
>
> > }
> >
> > /**
> > @@ -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;
> > 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;
>
> I don't think you really need this target parameter it only complicates
> things for no reason, all information you need are already in flags and
> dev->xdp_flags.
>
> >
> > - 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))) {
> > 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;
> > +
>
> since dev_xdp_install handles all cases now, you don't need this logic
> here, I would move this into dev_xdp_install, maybe introduce a helper
> dedicated function to choose the mode to be used.
>
> > + if ((target == XDP_FLAGS_SKB_MODE &&
> > + dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
> > + (target == XDP_FLAGS_DRV_MODE &&
> > + 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);
> > +
> > 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);
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index adcc045952c2..99ca58db297f 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1360,37 +1360,14 @@ static int rtnl_fill_link_ifmap(struct
> > sk_buff *skb, struct net_device *dev)
> > return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> > -{
> > - const struct bpf_prog *generic_xdp_prog;
> > -
> > - ASSERT_RTNL();
> > -
> > - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> > - if (!generic_xdp_prog)
> > - return 0;
> > - return generic_xdp_prog->aux->id;
> > -}
> > -
> > -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> > -{
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > XDP_QUERY_PROG);
> > -}
> > -
> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > -{
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > -}
> > -
> > static int rtnl_xdp_report_one(struct sk_buff *skb, struct
> > net_device *dev,
> > u32 *prog_id, u8 *mode, u8 tgt_mode, u32
> > attr,
> > - u32 (*get_prog_id)(struct net_device
> > *dev))
> > + unsigned int tgt_flags)
> > {
> > u32 curr_id;
> > int err;
> >
> > - curr_id = get_prog_id(dev);
> > + curr_id = dev_xdp_query(dev, tgt_flags);
> > if (!curr_id)
> > return 0;
> >
> > @@ -1421,15 +1398,15 @@ static int rtnl_xdp_fill(struct sk_buff *skb,
> > struct net_device *dev)
> > prog_id = 0;
> > mode = XDP_ATTACHED_NONE;
> > err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_SKB,
> > - IFLA_XDP_SKB_PROG_ID,
> > rtnl_xdp_prog_skb);
> > + IFLA_XDP_SKB_PROG_ID,
> > XDP_FLAGS_SKB_MODE);
> > if (err)
> > goto err_cancel;
> > err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_DRV,
> > - IFLA_XDP_DRV_PROG_ID,
> > rtnl_xdp_prog_drv);
> > + IFLA_XDP_DRV_PROG_ID,
> > XDP_FLAGS_DRV_MODE);
> > if (err)
> > goto err_cancel;
> > err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_HW,
> > - IFLA_XDP_HW_PROG_ID,
> > rtnl_xdp_prog_hw);
> > + IFLA_XDP_HW_PROG_ID,
> > XDP_FLAGS_HW_MODE);
> > if (err)
> > goto err_cancel;
> >
Saeed, thanks a lot for your comments! All good stuff. I'll address
them in the next version!
Thanks,
Björn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 21:04 ` Jakub Kicinski
2019-05-22 21:12 ` Björn Töpel
@ 2019-05-23 8:55 ` Toke Høiland-Jørgensen
1 sibling, 0 replies; 15+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-05-23 8:55 UTC (permalink / raw)
To: Jakub Kicinski, Björn Töpel
Cc: Alexei Starovoitov, Daniel Borkmann, Netdev,
Björn Töpel, Karlsson, Magnus, Jesper Dangaard Brouer,
bpf
Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
>> > > 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?
>> :-)
>
> Having things start to fail after they were successful/ignored
> is one of those ABI breakage types Linux and netdev usually takes
> pretty seriously, unfortunately. Especially when motivation is
> "it's more intuitive" :)
>
> If nobody chimes in please break out this behaviour change into
> a commit of its own.
Björn and I already had this discussion off-list. I think we ended up
concluding that since it's not changing kernel *behaviour*, but just
making an existing error explicit, it might be acceptable from an ABI
breakage PoV. And I'd generally prefer explicit errors over silent
failures.
But yeah, can totally see why it could also be considered a breaking
change...
-Toke
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-22 18:32 ` Jakub Kicinski
2019-05-22 20:54 ` Björn Töpel
@ 2019-05-28 17:06 ` Björn Töpel
2019-05-28 18:41 ` Jakub Kicinski
1 sibling, 1 reply; 15+ messages in thread
From: Björn Töpel @ 2019-05-28 17:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf
On Wed, 22 May 2019 at 20:32, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
[...]
>
> 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?
>
On a related note: I ran the test_offload.py test (thanks for pointing
that out!), and realized that my view of load flags was incorrect. To
double-check:
Given an XDP DRV capable netdev "eth0".
# ip link set dev eth0 xdp obj foo.o sec .text
# ip link set dev eth0 xdpdrv off
and
# ip link set dev eth0 xdpdrv obj foo.o sec .text
# ip link set dev eth0 xdp off
and
# ip link set dev eth0 xdpdrv obj foo.o sec .text
# ip link -force set dev eth0 xdp obj foo.o sec .text
and
# ip link set dev eth0 xdp obj foo.o sec .text
# ip link -force set dev eth0 xdpdrv obj foo.o sec .text
Should all fail. IOW, there's a distinction between explicit DRV and
auto-detected DRV? It's considered to be different flags.
Correct?
This was *not* my view. :-)
Thanks,
Björn
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-28 17:06 ` Björn Töpel
@ 2019-05-28 18:41 ` Jakub Kicinski
0 siblings, 0 replies; 15+ messages in thread
From: Jakub Kicinski @ 2019-05-28 18:41 UTC (permalink / raw)
To: Björn Töpel
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf
On Tue, 28 May 2019 19:06:21 +0200, Björn Töpel wrote:
> On Wed, 22 May 2019 at 20:32, Jakub Kicinski wrote:
> > 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?
> >
>
> On a related note: I ran the test_offload.py test (thanks for pointing
> that out!), and realized that my view of load flags was incorrect. To
> double-check:
>
> Given an XDP DRV capable netdev "eth0".
>
> # ip link set dev eth0 xdp obj foo.o sec .text
> # ip link set dev eth0 xdpdrv off
>
> and
>
> # ip link set dev eth0 xdpdrv obj foo.o sec .text
> # ip link set dev eth0 xdp off
>
> and
>
> # ip link set dev eth0 xdpdrv obj foo.o sec .text
> # ip link -force set dev eth0 xdp obj foo.o sec .text
>
> and
>
> # ip link set dev eth0 xdp obj foo.o sec .text
> # ip link -force set dev eth0 xdpdrv obj foo.o sec .text
>
> Should all fail. IOW, there's a distinction between explicit DRV and
> auto-detected DRV? It's considered to be different flags.
>
> Correct?
I think so. That's the way drivers which implement offloads work
(netdevsim and nfp).
However:
ip link set dev eth0 xdpdrv obj foo.o sec .text
ip link set dev eth0 xdpoffload off
ip link set dev eth0 xdpgeneric off
are fine. It's just the no flag case that's special, to avoid
confusion. If one always uses the flags there should be no errors.
> This was *not* my view. :-)
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-05-28 18:41 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-22 12:53 [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-22 13:02 ` Toke Høiland-Jørgensen
2019-05-22 18:32 ` Jakub Kicinski
2019-05-22 20:54 ` Björn Töpel
2019-05-22 21:04 ` Jakub Kicinski
2019-05-22 21:12 ` Björn Töpel
2019-05-23 8:55 ` Toke Høiland-Jørgensen
2019-05-28 17:06 ` Björn Töpel
2019-05-28 18:41 ` Jakub Kicinski
2019-05-23 5:47 ` Saeed Mahameed
2019-05-23 6:35 ` Björn Töpel
2019-05-22 12:53 ` [PATCH bpf-next 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
2019-05-22 17:41 ` [PATCH bpf-next 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Jakub Kicinski
2019-05-22 20:48 ` Björn Töpel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).