* [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code
@ 2019-05-31 9:42 Björn Töpel
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Björn Töpel @ 2019-05-31 9:42 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, bjorn.topel, brouer, bpf,
jakub.kicinski, saeedm
Here's yet another attempt tomove 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.
The patch passes test_offload.py from selftests. Thanks to Jakub for
pointing this out.
I, hopefully, addressed all comments from Jakub and Saeed, except one;
I did not move the XDP struct net_device into a struct of its own.
Thanks,
Björn
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 | 23 ++-
include/net/xdp.h | 6 +-
net/core/dev.c | 143 +++++++++++-------
net/core/rtnetlink.c | 53 ++++---
net/core/xdp.c | 20 ++-
20 files changed, 137 insertions(+), 231 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
@ 2019-05-31 9:42 ` Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
` (3 more replies)
2019-05-31 9:42 ` [PATCH bpf-next v2 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
2019-05-31 9:45 ` [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2 siblings, 4 replies; 23+ messages in thread
From: Björn Töpel @ 2019-05-31 9:42 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, brouer, bpf,
jakub.kicinski, saeedm
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 two new members: xdp_prog_hw and
xdp_flags. The former is the offloaded XDP program, if any, and the
latter tracks the flags that the supplied when attaching the XDP
program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
The xdp_prog member, previously only used for SKB_MODE, is shared with
DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
mutually exclusive. To differentiate between the two modes, a new
internal flag is introduced as well.
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. Now that xdp_prog member is shared from a query
program perspective, 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.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
include/linux/netdevice.h | 15 +++--
include/net/xdp.h | 4 ++
net/core/dev.c | 138 ++++++++++++++++++++++++--------------
net/core/rtnetlink.c | 53 +++++++--------
net/core/xdp.c | 17 +++--
5 files changed, 139 insertions(+), 88 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 44b47e9df94a..f3a875a52c6c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1940,6 +1940,9 @@ struct net_device {
#endif
struct hlist_node index_hlist;
+ struct bpf_prog *xdp_prog_hw;
+ u32 xdp_flags;
+
/*
* Cache lines mostly used on transmit path
*/
@@ -2043,11 +2046,14 @@ struct net_device {
};
#define to_net_dev(d) container_of(d, struct net_device, dev)
+/* Reusing the XDP flags space for kernel internal flag */
+#define XDP_FLAGS_KERN_GENERIC (1U << 4)
+static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
+
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_KERN_GENERIC;
}
#define NETDEV_ALIGN 32
@@ -3684,8 +3690,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 mode);
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/include/net/xdp.h b/include/net/xdp.h
index 0f25b3675c5c..3691280c8fc1 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -51,6 +51,7 @@ struct xdp_mem_info {
};
struct page_pool;
+struct netlink_ext_ack;
struct zero_copy_allocator {
void (*free)(struct zero_copy_allocator *zca, unsigned long handle);
@@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
void xdp_attachment_setup(struct xdp_attachment_info *info,
struct netdev_bpf *bpf);
+bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
+ struct netlink_ext_ack *extack);
+
#endif /* __LINUX_NET_XDP_H__ */
diff --git a/net/core/dev.c b/net/core/dev.c
index b6b8505cfb3e..1a9da508149a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8005,21 +8005,43 @@ 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)
+static u32 dev_xdp_query_generic(struct net_device *dev)
{
- struct netdev_bpf xdp;
+ struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
- if (!bpf_op)
- return 0;
+ return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
+ prog->aux->id : 0;
+}
- memset(&xdp, 0, sizeof(xdp));
- xdp.command = cmd;
+static u32 dev_xdp_query_drv(struct net_device *dev)
+{
+ struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
+
+ return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
+ prog->aux->id : 0;
+}
+
+static u32 dev_xdp_current_mode(struct net_device *dev)
+{
+ struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
- /* Query must always succeed. */
- WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
+ if (prog)
+ return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
+ XDP_FLAGS_DRV_MODE;
+ return 0;
+}
- return xdp.prog_id;
+u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
+{
+ switch (mode) {
+ case XDP_FLAGS_DRV_MODE:
+ return dev_xdp_query_drv(dev);
+ case XDP_FLAGS_SKB_MODE:
+ return dev_xdp_query_generic(dev);
+ case XDP_FLAGS_HW_MODE:
+ return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
+ }
+ return 0;
}
static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
@@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
struct bpf_prog *prog)
{
struct netdev_bpf xdp;
+ int err;
memset(&xdp, 0, sizeof(xdp));
- if (flags & XDP_FLAGS_HW_MODE)
+ if (flags & XDP_FLAGS_HW_MODE) {
xdp.command = XDP_SETUP_PROG_HW;
- else
+ xdp.flags = XDP_FLAGS_HW_MODE;
+ } else {
xdp.command = XDP_SETUP_PROG;
+ xdp.flags = flags;
+ }
xdp.extack = extack;
- xdp.flags = flags;
xdp.prog = prog;
- return bpf_op(dev, &xdp);
+ err = bpf_op(dev, &xdp);
+ if (err)
+ return err;
+
+ if (flags & XDP_FLAGS_HW_MODE) {
+ dev->xdp_prog_hw = prog;
+ return 0;
+ }
+
+ rcu_assign_pointer(dev->xdp_prog, prog);
+ dev->xdp_flags = prog ? flags : 0;
+ return 0;
}
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));
+ struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
+ bpf_op_t bpf_op;
- /* 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,
+ if (prog) {
+ bpf_op = dev_xdp_query_generic(dev) ?
+ generic_xdp_install : dev->netdev_ops->ndo_bpf;
+ WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
NULL));
+ }
+ if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
+ WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
+ NULL, XDP_FLAGS_HW_MODE, NULL));
}
/**
@@ -8080,41 +8104,49 @@ 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;
struct bpf_prog *prog = NULL;
- bpf_op_t bpf_op, bpf_chk;
+ u32 mode, curr_mode;
+ bpf_op_t bpf_op;
bool offload;
int err;
ASSERT_RTNL();
offload = flags & XDP_FLAGS_HW_MODE;
- query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
+ mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
- bpf_op = bpf_chk = ops->ndo_bpf;
+ bpf_op = dev->netdev_ops->ndo_bpf;
if (!bpf_op && (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 (fd >= 0) {
- if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
+ if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
+ mode = XDP_FLAGS_SKB_MODE;
+
+ curr_mode = dev_xdp_current_mode(dev);
+
+ if (!offload && curr_mode && (mode ^ curr_mode) &
+ (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
+ if (fd >= 0) {
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)) {
+ return 0;
+ }
+
+ if (!offload && dev_xdp_query(dev, mode) &&
+ !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
+ return -EBUSY;
+
+ if (fd >= 0) {
+ if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
+ dev_xdp_query(dev, mode)) {
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, !!bpf_op);
if (IS_ERR(prog))
return PTR_ERR(prog);
@@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
}
}
+ if (mode == XDP_FLAGS_SKB_MODE) {
+ bpf_op = generic_xdp_install;
+ flags |= XDP_FLAGS_KERN_GENERIC;
+ }
err = dev_xdp_install(dev, bpf_op, 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..5e396fd01d8b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1360,42 +1360,44 @@ 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)
+static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
{
- 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);
+ switch (tgt_mode) {
+ case XDP_ATTACHED_DRV:
+ return XDP_FLAGS_DRV_MODE;
+ case XDP_ATTACHED_SKB:
+ return XDP_FLAGS_SKB_MODE;
+ case XDP_ATTACHED_HW:
+ return XDP_FLAGS_HW_MODE;
+ }
+ return 0;
}
-static u32 rtnl_xdp_prog_hw(struct net_device *dev)
+static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
{
- return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
- XDP_QUERY_PROG_HW);
+ switch (tgt_mode) {
+ case XDP_ATTACHED_DRV:
+ return IFLA_XDP_DRV_PROG_ID;
+ case XDP_ATTACHED_SKB:
+ return IFLA_XDP_SKB_PROG_ID;
+ case XDP_ATTACHED_HW:
+ return IFLA_XDP_HW_PROG_ID;
+ }
+ return 0;
}
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))
+ u32 *prog_id, u8 *mode, u8 tgt_mode)
{
u32 curr_id;
int err;
- curr_id = get_prog_id(dev);
+ curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
if (!curr_id)
return 0;
*prog_id = curr_id;
- err = nla_put_u32(skb, attr, curr_id);
+ err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
if (err)
return err;
@@ -1420,16 +1422,13 @@ 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);
+ err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
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);
+ err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
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);
+ err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
if (err)
goto err_cancel;
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 4b2b194f4f1f..af92c04a58d8 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -388,16 +388,23 @@ int xdp_attachment_query(struct xdp_attachment_info *info,
}
EXPORT_SYMBOL_GPL(xdp_attachment_query);
-bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
- struct netdev_bpf *bpf)
+bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
+ struct netlink_ext_ack *extack)
{
- if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
- NL_SET_ERR_MSG(bpf->extack,
- "program loaded with different flags");
+ if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
+ NL_SET_ERR_MSG(extack, "program loaded with different flags");
return false;
}
return true;
}
+
+bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
+ struct netdev_bpf *bpf)
+{
+ if (info->prog)
+ return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
+ return true;
+}
EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
void xdp_attachment_setup(struct xdp_attachment_info *info,
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH bpf-next v2 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW}
2019-05-31 9:42 [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
@ 2019-05-31 9:42 ` Björn Töpel
2019-05-31 9:45 ` [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-05-31 9:42 UTC (permalink / raw)
To: toke, ast, daniel, netdev
Cc: Björn Töpel, magnus.karlsson, brouer, bpf,
jakub.kicinski, saeedm
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 52110e54e621..27b93e8b844d 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1097,26 +1097,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 0d4115c9e20b..2517bd5c74b4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2513,28 +2513,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 f3a875a52c6c..83236af25590 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 3691280c8fc1..abcd86a31c0f 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -160,8 +160,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 1a9da508149a..4e7c81a76400 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 af92c04a58d8..295b4d4fd259 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_prog_flags_ok(u32 old_flags, u32 new_flags,
struct netlink_ext_ack *extack)
{
--
2.20.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code
2019-05-31 9:42 [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 9:42 ` [PATCH bpf-next v2 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
@ 2019-05-31 9:45 ` Björn Töpel
2 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-05-31 9:45 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev
Cc: Karlsson, Magnus, Björn Töpel, Jesper Dangaard Brouer,
bpf, Jakub Kicinski, Saeed Mahameed
On Fri, 31 May 2019 at 11:42, Björn Töpel <bjorn.topel@gmail.com> wrote:
>
[...]
>
> I, hopefully, addressed all comments from Jakub and Saeed, except one;
> I did not move the XDP struct net_device into a struct of its own.
>
Uhm, the last sentence was weird.
What I meant was: I did not move the newly introduced XDP members
(flags/hw prog) into a struct of its own.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
@ 2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 19:42 ` Jakub Kicinski
2019-06-01 19:57 ` Jakub Kicinski
2019-06-01 9:50 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Saeed Mahameed @ 2019-05-31 19:18 UTC (permalink / raw)
To: toke, daniel, ast, netdev, bjorn.topel
Cc: magnus.karlsson, bjorn.topel, jakub.kicinski, brouer, bpf
On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw and
> xdp_flags. The former is the offloaded XDP program, if any, and the
> latter tracks the flags that the supplied when attaching the XDP
> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>
> The xdp_prog member, previously only used for SKB_MODE, is shared
> with
> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> mutually exclusive. To differentiate between the two modes, a new
> internal flag is introduced as well.
Just thinking out loud, why can't we allow any combination of
HW/DRV/SKB modes? they are totally different attach points in a totally
different checkpoints in a frame life cycle.
Down the road i think we will utilize this fact and start introducing
SKB helpers for SKB mode and driver helpers for DRV mode..
>
> 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. Now that xdp_prog member is shared from a query
> program perspective, 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.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/netdevice.h | 15 +++--
> include/net/xdp.h | 4 ++
> net/core/dev.c | 138 ++++++++++++++++++++++++----------
> ----
> net/core/rtnetlink.c | 53 +++++++--------
> net/core/xdp.c | 17 +++--
> 5 files changed, 139 insertions(+), 88 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> @@ -2043,11 +2046,14 @@ struct net_device {
> };
> #define to_net_dev(d) container_of(d, struct net_device, dev)
>
> +/* Reusing the XDP flags space for kernel internal flag */
> +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> +
> 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_KERN_GENERIC;
> }
>
> #define NETDEV_ALIGN 32
> @@ -3684,8 +3690,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 mode);
> 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/include/net/xdp.h b/include/net/xdp.h
> index 0f25b3675c5c..3691280c8fc1 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -51,6 +51,7 @@ struct xdp_mem_info {
> };
>
> struct page_pool;
> +struct netlink_ext_ack;
>
> struct zero_copy_allocator {
> void (*free)(struct zero_copy_allocator *zca, unsigned long
> handle);
> @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> xdp_attachment_info *info,
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> struct netdev_bpf *bpf);
>
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack);
> +
> #endif /* __LINUX_NET_XDP_H__ */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b6b8505cfb3e..1a9da508149a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8005,21 +8005,43 @@ 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)
> +static u32 dev_xdp_query_generic(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - if (!bpf_op)
> - return 0;
> + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> + prog->aux->id : 0;
> +}
>
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = cmd;
> +static u32 dev_xdp_query_drv(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> +
> + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> + prog->aux->id : 0;
> +}
> +
> +static u32 dev_xdp_current_mode(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - /* Query must always succeed. */
> - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> + if (prog)
> + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE
> :
> + XDP_FLAGS_DRV_MODE;
> + return 0;
> +}
>
> - return xdp.prog_id;
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> +{
> + switch (mode) {
> + case XDP_FLAGS_DRV_MODE:
> + return dev_xdp_query_drv(dev);
> + case XDP_FLAGS_SKB_MODE:
> + return dev_xdp_query_generic(dev);
> + case XDP_FLAGS_HW_MODE:
> + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id :
> 0;
> + }
> + return 0;
> }
>
> static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> *dev, bpf_op_t bpf_op,
> struct bpf_prog *prog)
> {
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (flags & XDP_FLAGS_HW_MODE) {
> xdp.command = XDP_SETUP_PROG_HW;
> - else
> + xdp.flags = XDP_FLAGS_HW_MODE;
> + } else {
> xdp.command = XDP_SETUP_PROG;
> + xdp.flags = flags;
> + }
> xdp.extack = extack;
> - xdp.flags = flags;
> xdp.prog = prog;
>
> - return bpf_op(dev, &xdp);
> + err = bpf_op(dev, &xdp);
> + if (err)
> + return err;
> +
> + if (flags & XDP_FLAGS_HW_MODE) {
> + dev->xdp_prog_hw = prog;
> + return 0;
> + }
> +
> + rcu_assign_pointer(dev->xdp_prog, prog);
> + dev->xdp_flags = prog ? flags : 0;
> + return 0;
> }
>
> 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));
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> + bpf_op_t bpf_op;
>
> - /* 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,
> + if (prog) {
> + bpf_op = dev_xdp_query_generic(dev) ?
> + generic_xdp_install : dev->netdev_ops-
> >ndo_bpf;
> + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev-
> >xdp_flags,
> NULL));
> + }
> + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> + NULL, XDP_FLAGS_HW_MODE,
> NULL));
> }
>
> /**
> @@ -8080,41 +8104,49 @@ 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;
> struct bpf_prog *prog = NULL;
> - bpf_op_t bpf_op, bpf_chk;
> + u32 mode, curr_mode;
> + bpf_op_t bpf_op;
> bool offload;
> int err;
>
> ASSERT_RTNL();
>
> offload = flags & XDP_FLAGS_HW_MODE;
> - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
>
> - bpf_op = bpf_chk = ops->ndo_bpf;
> + bpf_op = dev->netdev_ops->ndo_bpf;
> if (!bpf_op && (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 (fd >= 0) {
> - if (!offload && __dev_xdp_query(dev, bpf_chk,
> XDP_QUERY_PROG)) {
> + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> + mode = XDP_FLAGS_SKB_MODE;
> +
> + curr_mode = dev_xdp_current_mode(dev);
> +
> + if (!offload && curr_mode && (mode ^ curr_mode) &
> + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
if i am reading this correctly this is equivalent to :
if (!offload && (curre_mode != mode))
offlad is false then curr_mode and mode must be DRV or GENERIC ..
better if you keep bitwise operations for actual bitmasks, mode and
curr_mode are not bitmask, they can hold one value each .. according to
your logic..
> + if (fd >= 0) {
> 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)) {
> + return 0;
> + }
> +
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
> +
> + if (fd >= 0) {
> + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> + dev_xdp_query(dev, mode)) {
> 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,
> !!bpf_op);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
>
> @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> struct netlink_ext_ack *extack,
> }
> }
>
> + if (mode == XDP_FLAGS_SKB_MODE) {
> + bpf_op = generic_xdp_install;
> + flags |= XDP_FLAGS_KERN_GENERIC;
> + }
> err = dev_xdp_install(dev, bpf_op, 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..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ 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)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - 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);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> 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))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode),
> curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 4b2b194f4f1f..af92c04a58d8 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> xdp_attachment_info *info,
> }
> EXPORT_SYMBOL_GPL(xdp_attachment_query);
>
> -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> - struct netdev_bpf *bpf)
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack)
> {
> - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES)
> {
> - NL_SET_ERR_MSG(bpf->extack,
> - "program loaded with different flags");
> + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> + NL_SET_ERR_MSG(extack, "program loaded with different
> flags");
> return false;
> }
> return true;
> }
> +
> +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> + struct netdev_bpf *bpf)
> +{
> + if (info->prog)
> + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf-
> >extack);
> + return true;
> +}
> EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
>
> void xdp_attachment_setup(struct xdp_attachment_info *info,
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
@ 2019-06-01 9:50 ` kbuild test robot
2019-06-01 18:12 ` Jonathan Lemon
2019-06-01 20:02 ` Jakub Kicinski
3 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2019-06-01 9:50 UTC (permalink / raw)
To: Björn Töpel
Cc: kbuild-all, toke, ast, daniel, netdev, Björn Töpel,
magnus.karlsson, brouer, bpf, jakub.kicinski, saeedm
[-- Attachment #1: Type: text/plain, Size: 24325 bytes --]
Hi "Björn,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Bj-rn-T-pel/net-xdp-refactor-XDP_QUERY_PROG-_HW-to-netdev/20190601-053952
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
reproduce: make htmldocs
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Function parameter or member 'range' not described in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'mm' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'start' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'end' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:823: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2812: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:375: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: no structured comments found
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:128: warning: Incorrect use of kernel-doc format: Documentation Makefile include scripts source @atomic_obj
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: no structured comments found
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
drivers/gpu/drm/scheduler/sched_main.c:419: warning: Function parameter or member 'full_recovery' not described in 'drm_sched_start'
drivers/gpu/drm/i915/i915_vma.h:50: warning: cannot understand function prototype: 'struct i915_vma '
drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
drivers/gpu/drm/i915/intel_guc_fwif.h:536: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
drivers/gpu/drm/i915/i915_reg.h:156: warning: bad line:
include/linux/interconnect.h:1: warning: no structured comments found
include/linux/skbuff.h:897: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
include/net/sock.h:520: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
include/linux/netdevice.h:2047: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
>> include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_prog_hw' not described in 'net_device'
>> include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_flags' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
Documentation/admin-guide/mm/numaperf.rst:24: ERROR: Unexpected indentation.
Documentation/admin-guide/mm/numaperf.rst:24: WARNING: Inline substitution_reference start-string without end-string.
Documentation/admin-guide/mm/numaperf.rst:25: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/bpf/btf.rst:154: ERROR: Unexpected indentation.
Documentation/bpf/btf.rst:163: ERROR: Unexpected indentation.
lib/list_sort.c:128: WARNING: Definition list ends without a blank line; unexpected unindent.
lib/list_sort.c:161: ERROR: Unexpected indentation.
lib/list_sort.c:162: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/xarray.h:232: ERROR: Unexpected indentation.
kernel/time/hrtimer.c:1120: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:349: WARNING: Inline literal start-string without end-string.
include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
Documentation/driver-api/gpio/driver.rst:419: ERROR: Unknown target name: "devm".
include/linux/i2c.h:510: WARNING: Inline strong start-string without end-string.
drivers/ata/libata-core.c:5944: ERROR: Unknown target name: "hw".
drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1959: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/regulator/driver.h:289: ERROR: Unknown target name: "regulator_regmap_x_voltage".
Documentation/driver-api/soundwire/locking.rst:50: ERROR: Inconsistent literal block quoting.
Documentation/driver-api/soundwire/locking.rst:51: WARNING: Line block ends without a blank line.
Documentation/driver-api/soundwire/locking.rst:55: WARNING: Inline substitution_reference start-string without end-string.
Documentation/driver-api/soundwire/locking.rst:56: WARNING: Line block ends without a blank line.
include/linux/spi/spi.h:380: ERROR: Unexpected indentation.
fs/posix_acl.c:636: WARNING: Inline emphasis start-string without end-string.
fs/debugfs/inode.c:385: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:464: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:496: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:583: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:394: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:400: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:439: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:445: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:484: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:490: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:530: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:536: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:578: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:584: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:845: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:851: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:898: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:904: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1096: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1102: WARNING: Inline literal start-string without end-string.
Documentation/firmware-guide/acpi/enumeration.rst:430: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:432: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:434: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:435: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:436: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:439: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:441: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:445: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:447: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:448: WARNING: Block quote ends without a blank line; unexpected unindent.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2024: WARNING: Inline emphasis start-string without end-string.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2026: WARNING: Inline emphasis start-string without end-string.
Documentation/networking/af_xdp.rst:319: WARNING: Literal block expected; none found.
Documentation/networking/af_xdp.rst:326: WARNING: Literal block expected; none found.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:43: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:63: ERROR: Unexpected indentation.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/netdevice.h:3492: WARNING: Inline emphasis start-string without end-string.
include/linux/netdevice.h:3492: WARNING: Inline emphasis start-string without end-string.
net/core/dev.c:4999: ERROR: Unknown target name: "page_is".
Documentation/powerpc/isa-versions.rst:15: ERROR: Malformed table.
Column span alignment problem in table line 10.
vim +2047 include/linux/netdevice.h
^1da177e4 Linus Torvalds 2005-04-16 2014
43cb76d91 Greg Kroah-Hartman 2002-04-09 2015 struct device dev;
0c509a6c9 Eric W. Biederman 2009-10-29 2016 const struct attribute_group *sysfs_groups[4];
a953be53c Michael Dalton 2014-01-16 2017 const struct attribute_group *sysfs_rx_queue_group;
38f7b870d Patrick McHardy 2007-06-13 2018
38f7b870d Patrick McHardy 2007-06-13 2019 const struct rtnl_link_ops *rtnl_link_ops;
f25f4e448 Peter P Waskiewicz Jr 2007-07-06 2020
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2021 /* for setting kernel sock attribute on TCP connection setup */
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2022 #define GSO_MAX_SIZE 65536
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2023 unsigned int gso_max_size;
30b678d84 Ben Hutchings 2012-07-30 2024 #define GSO_MAX_SEGS 65535
30b678d84 Ben Hutchings 2012-07-30 2025 u16 gso_max_segs;
743b03a83 Eric Dumazet 2016-04-09 2026
7a6b6f515 Jeff Kirsher 2008-11-25 2027 #ifdef CONFIG_DCB
329535432 Stephen Hemminger 2009-10-05 2028 const struct dcbnl_rtnl_ops *dcbnl_ops;
2f90b8657 Alexander Duyck 2008-11-20 2029 #endif
ffcfe25bb Alexander Duyck 2018-07-09 2030 s16 num_tc;
4f57c087d John Fastabend 2011-01-17 2031 struct netdev_tc_txq tc_to_txq[TC_MAX_QUEUE];
4f57c087d John Fastabend 2011-01-17 2032 u8 prio_tc_map[TC_BITMASK + 1];
2f90b8657 Alexander Duyck 2008-11-20 2033
d11ead756 Ben Hutchings 2011-11-25 2034 #if IS_ENABLED(CONFIG_FCOE)
4d288d576 Yi Zou 2009-02-27 2035 unsigned int fcoe_ddp_xid;
4d288d576 Yi Zou 2009-02-27 2036 #endif
86f8515f9 Daniel Borkmann 2013-12-29 2037 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
5bc1421e3 Neil Horman 2011-11-22 2038 struct netprio_map __rcu *priomap;
5bc1421e3 Neil Horman 2011-11-22 2039 #endif
c1f19b51d Richard Cochran 2010-07-17 2040 struct phy_device *phydev;
e679c9c1d Russell King 2018-03-28 2041 struct sfp_bus *sfp_bus;
23d3b8bfb Eric Dumazet 2012-09-05 2042 struct lock_class_key *qdisc_tx_busylock;
f9eb8aea2 Eric Dumazet 2016-06-06 2043 struct lock_class_key *qdisc_running_key;
d746d707a Anuradha Karuppiah 2015-07-14 2044 bool proto_down;
619411432 Heiner Kallweit 2018-09-24 2045 unsigned wol_enabled:1;
^1da177e4 Linus Torvalds 2005-04-16 2046 };
43cb76d91 Greg Kroah-Hartman 2002-04-09 @2047 #define to_net_dev(d) container_of(d, struct net_device, dev)
^1da177e4 Linus Torvalds 2005-04-16 2048
:::::: The code at line 2047 was first introduced by commit
:::::: 43cb76d91ee85f579a69d42bc8efc08bac560278 Network: convert network devices to use struct device instead of class_device
:::::: TO: Greg Kroah-Hartman <gregkh@suse.de>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7240 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 9:50 ` kbuild test robot
@ 2019-06-01 18:12 ` Jonathan Lemon
2019-06-03 8:39 ` Björn Töpel
2019-06-01 20:02 ` Jakub Kicinski
3 siblings, 1 reply; 23+ messages in thread
From: Jonathan Lemon @ 2019-06-01 18:12 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, Björn Töpel,
magnus.karlsson, brouer, bpf, jakub.kicinski, saeedm
On 31 May 2019, at 2:42, 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 two new members: xdp_prog_hw and
> xdp_flags. The former is the offloaded XDP program, if any, and the
> latter tracks the flags that the supplied when attaching the XDP
> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>
> The xdp_prog member, previously only used for SKB_MODE, is shared with
> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> mutually exclusive. To differentiate between the two modes, a new
> internal flag is introduced as well.
I'm not entirely clear why this new flag is needed - GENERIC seems to
be an alias for SKB_MODE, so why just use SKB_MODE directly?
If the user does not explicitly specify a type (skb|drv|hw), then the
command should choose the correct type and then behave as if this type
was specified.
The logic in dev_change_xdp_fd() is too complicated. It disallows
setting (drv|skb), but allows (hw|skb), which I'm not sure is
intentional.
It should be clearer as to which combinations are allowed.
--
Jonathan
>
> 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. Now that xdp_prog member is shared from a query
> program perspective, 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.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/netdevice.h | 15 +++--
> include/net/xdp.h | 4 ++
> net/core/dev.c | 138
> ++++++++++++++++++++++++--------------
> net/core/rtnetlink.c | 53 +++++++--------
> net/core/xdp.c | 17 +++--
> 5 files changed, 139 insertions(+), 88 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> @@ -2043,11 +2046,14 @@ struct net_device {
> };
> #define to_net_dev(d) container_of(d, struct net_device, dev)
>
> +/* Reusing the XDP flags space for kernel internal flag */
> +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> +
> 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_KERN_GENERIC;
> }
>
> #define NETDEV_ALIGN 32
> @@ -3684,8 +3690,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 mode);
> 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/include/net/xdp.h b/include/net/xdp.h
> index 0f25b3675c5c..3691280c8fc1 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -51,6 +51,7 @@ struct xdp_mem_info {
> };
>
> struct page_pool;
> +struct netlink_ext_ack;
>
> struct zero_copy_allocator {
> void (*free)(struct zero_copy_allocator *zca, unsigned long handle);
> @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> xdp_attachment_info *info,
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> struct netdev_bpf *bpf);
>
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack);
> +
> #endif /* __LINUX_NET_XDP_H__ */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b6b8505cfb3e..1a9da508149a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8005,21 +8005,43 @@ 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)
> +static u32 dev_xdp_query_generic(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - if (!bpf_op)
> - return 0;
> + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> + prog->aux->id : 0;
> +}
>
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = cmd;
> +static u32 dev_xdp_query_drv(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> +
> + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> + prog->aux->id : 0;
> +}
> +
> +static u32 dev_xdp_current_mode(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - /* Query must always succeed. */
> - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> + if (prog)
> + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
> + XDP_FLAGS_DRV_MODE;
return xdp->flags & XDP_FLAGS_MODE;
> + return 0;
> +}
>
> - return xdp.prog_id;
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> +{
> + switch (mode) {
> + case XDP_FLAGS_DRV_MODE:
> + return dev_xdp_query_drv(dev);
> + case XDP_FLAGS_SKB_MODE:
> + return dev_xdp_query_generic(dev);
> + case XDP_FLAGS_HW_MODE:
> + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
> + }
> + return 0;
> }
>
> static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> *dev, bpf_op_t bpf_op,
> struct bpf_prog *prog)
> {
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (flags & XDP_FLAGS_HW_MODE) {
> xdp.command = XDP_SETUP_PROG_HW;
> - else
> + xdp.flags = XDP_FLAGS_HW_MODE;
> + } else {
> xdp.command = XDP_SETUP_PROG;
> + xdp.flags = flags;
> + }
> xdp.extack = extack;
> - xdp.flags = flags;
> xdp.prog = prog;
>
> - return bpf_op(dev, &xdp);
> + err = bpf_op(dev, &xdp);
> + if (err)
> + return err;
> +
> + if (flags & XDP_FLAGS_HW_MODE) {
> + dev->xdp_prog_hw = prog;
> + return 0;
> + }
> +
> + rcu_assign_pointer(dev->xdp_prog, prog);
> + dev->xdp_flags = prog ? flags : 0;
> + return 0;
> }
>
> 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));
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> + bpf_op_t bpf_op;
>
> - /* 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,
> + if (prog) {
> + bpf_op = dev_xdp_query_generic(dev) ?
> + generic_xdp_install : dev->netdev_ops->ndo_bpf;
> + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
> NULL));
> + }
> + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> + NULL, XDP_FLAGS_HW_MODE, NULL));
> }
>
> /**
> @@ -8080,41 +8104,49 @@ 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;
> struct bpf_prog *prog = NULL;
> - bpf_op_t bpf_op, bpf_chk;
> + u32 mode, curr_mode;
> + bpf_op_t bpf_op;
> bool offload;
> int err;
>
> ASSERT_RTNL();
>
> offload = flags & XDP_FLAGS_HW_MODE;
> - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
>
> - bpf_op = bpf_chk = ops->ndo_bpf;
> + bpf_op = dev->netdev_ops->ndo_bpf;
> if (!bpf_op && (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 (fd >= 0) {
> - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> + mode = XDP_FLAGS_SKB_MODE;
> +
> + curr_mode = dev_xdp_current_mode(dev);
> +
> + if (!offload && curr_mode && (mode ^ curr_mode) &
> + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> + if (fd >= 0) {
> 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)) {
> + return 0;
> + }
> +
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
> +
> + if (fd >= 0) {
> + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> + dev_xdp_query(dev, mode)) {
> 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, !!bpf_op);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
>
> @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> struct netlink_ext_ack *extack,
> }
> }
>
> + if (mode == XDP_FLAGS_SKB_MODE) {
> + bpf_op = generic_xdp_install;
> + flags |= XDP_FLAGS_KERN_GENERIC;
> + }
> err = dev_xdp_install(dev, bpf_op, 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..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ 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)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - 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);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> 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))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 4b2b194f4f1f..af92c04a58d8 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> xdp_attachment_info *info,
> }
> EXPORT_SYMBOL_GPL(xdp_attachment_query);
>
> -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> - struct netdev_bpf *bpf)
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack)
> {
> - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
> - NL_SET_ERR_MSG(bpf->extack,
> - "program loaded with different flags");
> + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> + NL_SET_ERR_MSG(extack, "program loaded with different flags");
> return false;
> }
> return true;
> }
> +
> +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> + struct netdev_bpf *bpf)
> +{
> + if (info->prog)
> + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
> + return true;
> +}
> EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
>
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> --
> 2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 19:18 ` Saeed Mahameed
@ 2019-06-01 19:42 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
2019-06-01 19:57 ` Jakub Kicinski
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 19:42 UTC (permalink / raw)
To: Saeed Mahameed
Cc: toke, daniel, ast, netdev, bjorn.topel, magnus.karlsson,
bjorn.topel, brouer, bpf
On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw and
> > xdp_flags. The former is the offloaded XDP program, if any, and the
> > latter tracks the flags that the supplied when attaching the XDP
> > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >
> > The xdp_prog member, previously only used for SKB_MODE, is shared
> > with
> > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > mutually exclusive. To differentiate between the two modes, a new
> > internal flag is introduced as well.
>
> Just thinking out loud, why can't we allow any combination of
> HW/DRV/SKB modes? they are totally different attach points in a totally
> different checkpoints in a frame life cycle.
FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> Down the road i think we will utilize this fact and start introducing
> SKB helpers for SKB mode and driver helpers for DRV mode..
Any reason why we would want the extra complexity? There is cls_bpf
if someone wants skb features after all..
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 19:42 ` Jakub Kicinski
@ 2019-06-01 19:57 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 19:57 UTC (permalink / raw)
To: Saeed Mahameed
Cc: toke, daniel, ast, netdev, bjorn.topel, magnus.karlsson,
bjorn.topel, brouer, bpf
On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > + mode = XDP_FLAGS_SKB_MODE;
> > +
> > + curr_mode = dev_xdp_current_mode(dev);
> > +
> > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
>
> if i am reading this correctly this is equivalent to :
>
> if (!offload && (curre_mode != mode))
> offlad is false then curr_mode and mode must be DRV or GENERIC ..
Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
about the diff.
> better if you keep bitwise operations for actual bitmasks, mode and
> curr_mode are not bitmask, they can hold one value each .. according to
> your logic..
Well, they hold one bit each, whether one bit is a bitmap perhaps is
disputable? :)
I think the logic is fine.
What happened to my request to move the change in behaviour for
disabling to a separate patch, tho, Bjorn? :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
` (2 preceding siblings ...)
2019-06-01 18:12 ` Jonathan Lemon
@ 2019-06-01 20:02 ` Jakub Kicinski
2019-06-03 9:07 ` Björn Töpel
3 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 20:02 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, Björn Töpel,
magnus.karlsson, brouer, bpf, saeedm
On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
IDK if we should pay the cost of this pointer for every netdev on the
system just for the single production driver out there that implements
HW offload :( I'm on the fence about this..
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ 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)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - 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);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> 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))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
> 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);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
So you remove all the attr and flag params just to add a conversion
helpers to get them based on mode? Why? Seems like unnecessary churn,
and questionable change :S
Otherwise looks good to me!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 18:12 ` Jonathan Lemon
@ 2019-06-03 8:39 ` Björn Töpel
2019-06-03 14:58 ` Jonathan Lemon
2019-06-03 23:11 ` Daniel Borkmann
0 siblings, 2 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 8:39 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf, Jakub Kicinski, Saeed Mahameed
On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
>
> On 31 May 2019, at 2:42, 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 two new members: xdp_prog_hw and
> > xdp_flags. The former is the offloaded XDP program, if any, and the
> > latter tracks the flags that the supplied when attaching the XDP
> > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >
> > The xdp_prog member, previously only used for SKB_MODE, is shared with
> > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > mutually exclusive. To differentiate between the two modes, a new
> > internal flag is introduced as well.
>
> I'm not entirely clear why this new flag is needed - GENERIC seems to
> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>
> If the user does not explicitly specify a type (skb|drv|hw), then the
> command should choose the correct type and then behave as if this type
> was specified.
>
Yes, this is kind of hairy.
SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
SKB, DRV, HW, SKB+HW DRV+HW.
What complicates things further, is that SKB and DRV can be implicitly
(auto/no flags) or explicitly enabled (flags).
If a user doesn't pass any flags, the "best supported mode" should be
selected. If this "auto mode" is used, it should be seen as a third
mode. E.g.
ip link set dev eth0 xdp on -- OK
ip link set dev eth0 xdp off -- OK
ip link set dev eth0 xdp on -- OK # generic auto selected
ip link set dev eth0 xdpgeneric off -- NOK, bad flags
ip link set dev eth0 xdp on -- OK # drv auto selected
ip link set dev eth0 xdpdrv off -- NOK, bad flags
...and so on. The idea is that a user should use the same set of flags always.
The internal "GENERIC" flag is only to determine if the xdp_prog
represents a DRV version or SKB version. Maybe it would be clearer
just to add an additional xdp_prog_drv to the net_device, instead?
> The logic in dev_change_xdp_fd() is too complicated. It disallows
> setting (drv|skb), but allows (hw|skb), which I'm not sure is
> intentional.
>
> It should be clearer as to which combinations are allowed.
Fair point. I'll try to clean it up further.
Björn
> --
> Jonathan
>
>
>
> >
> > 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. Now that xdp_prog member is shared from a query
> > program perspective, 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.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > ---
> > include/linux/netdevice.h | 15 +++--
> > include/net/xdp.h | 4 ++
> > net/core/dev.c | 138
> > ++++++++++++++++++++++++--------------
> > net/core/rtnetlink.c | 53 +++++++--------
> > net/core/xdp.c | 17 +++--
> > 5 files changed, 139 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..f3a875a52c6c 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1940,6 +1940,9 @@ struct net_device {
> > #endif
> > struct hlist_node index_hlist;
> >
> > + struct bpf_prog *xdp_prog_hw;
> > + u32 xdp_flags;
> > +
> > /*
> > * Cache lines mostly used on transmit path
> > */
> > @@ -2043,11 +2046,14 @@ struct net_device {
> > };
> > #define to_net_dev(d) container_of(d, struct net_device, dev)
> >
> > +/* Reusing the XDP flags space for kernel internal flag */
> > +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> > +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> > +
> > 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_KERN_GENERIC;
> > }
> >
> > #define NETDEV_ALIGN 32
> > @@ -3684,8 +3690,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 mode);
> > 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/include/net/xdp.h b/include/net/xdp.h
> > index 0f25b3675c5c..3691280c8fc1 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -51,6 +51,7 @@ struct xdp_mem_info {
> > };
> >
> > struct page_pool;
> > +struct netlink_ext_ack;
> >
> > struct zero_copy_allocator {
> > void (*free)(struct zero_copy_allocator *zca, unsigned long handle);
> > @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> > xdp_attachment_info *info,
> > void xdp_attachment_setup(struct xdp_attachment_info *info,
> > struct netdev_bpf *bpf);
> >
> > +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> > + struct netlink_ext_ack *extack);
> > +
> > #endif /* __LINUX_NET_XDP_H__ */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index b6b8505cfb3e..1a9da508149a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8005,21 +8005,43 @@ 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)
> > +static u32 dev_xdp_query_generic(struct net_device *dev)
> > {
> > - struct netdev_bpf xdp;
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> >
> > - if (!bpf_op)
> > - return 0;
> > + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> > + prog->aux->id : 0;
> > +}
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = cmd;
> > +static u32 dev_xdp_query_drv(struct net_device *dev)
> > +{
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> > +
> > + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> > + prog->aux->id : 0;
> > +}
> > +
> > +static u32 dev_xdp_current_mode(struct net_device *dev)
> > +{
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> >
> > - /* Query must always succeed. */
> > - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> > + if (prog)
> > + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
> > + XDP_FLAGS_DRV_MODE;
>
> return xdp->flags & XDP_FLAGS_MODE;
>
>
> > + return 0;
> > +}
> >
> > - return xdp.prog_id;
> > +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> > +{
> > + switch (mode) {
> > + case XDP_FLAGS_DRV_MODE:
> > + return dev_xdp_query_drv(dev);
> > + case XDP_FLAGS_SKB_MODE:
> > + return dev_xdp_query_generic(dev);
> > + case XDP_FLAGS_HW_MODE:
> > + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
> > + }
> > + return 0;
> > }
> >
> > static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> > *dev, bpf_op_t bpf_op,
> > struct bpf_prog *prog)
> > {
> > struct netdev_bpf xdp;
> > + int err;
> >
> > memset(&xdp, 0, sizeof(xdp));
> > - if (flags & XDP_FLAGS_HW_MODE)
> > + if (flags & XDP_FLAGS_HW_MODE) {
> > xdp.command = XDP_SETUP_PROG_HW;
> > - else
> > + xdp.flags = XDP_FLAGS_HW_MODE;
> > + } else {
> > xdp.command = XDP_SETUP_PROG;
> > + xdp.flags = flags;
> > + }
> > xdp.extack = extack;
> > - xdp.flags = flags;
> > xdp.prog = prog;
> >
> > - return bpf_op(dev, &xdp);
> > + err = bpf_op(dev, &xdp);
> > + if (err)
> > + return err;
> > +
> > + if (flags & XDP_FLAGS_HW_MODE) {
> > + dev->xdp_prog_hw = prog;
> > + return 0;
> > + }
> > +
> > + rcu_assign_pointer(dev->xdp_prog, prog);
> > + dev->xdp_flags = prog ? flags : 0;
> > + return 0;
> > }
> >
> > 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));
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> > + bpf_op_t bpf_op;
> >
> > - /* 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,
> > + if (prog) {
> > + bpf_op = dev_xdp_query_generic(dev) ?
> > + generic_xdp_install : dev->netdev_ops->ndo_bpf;
> > + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
> > NULL));
> > + }
> > + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> > + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> > + NULL, XDP_FLAGS_HW_MODE, NULL));
> > }
> >
> > /**
> > @@ -8080,41 +8104,49 @@ 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;
> > struct bpf_prog *prog = NULL;
> > - bpf_op_t bpf_op, bpf_chk;
> > + u32 mode, curr_mode;
> > + bpf_op_t bpf_op;
> > bool offload;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > offload = flags & XDP_FLAGS_HW_MODE;
> > - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> > + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
> >
> > - bpf_op = bpf_chk = ops->ndo_bpf;
> > + bpf_op = dev->netdev_ops->ndo_bpf;
> > if (!bpf_op && (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 (fd >= 0) {
> > - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > + mode = XDP_FLAGS_SKB_MODE;
> > +
> > + curr_mode = dev_xdp_current_mode(dev);
> > +
> > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > + if (fd >= 0) {
> > 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)) {
> > + return 0;
> > + }
> > +
> > + if (!offload && dev_xdp_query(dev, mode) &&
> > + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> > + return -EBUSY;
> > +
> > + if (fd >= 0) {
> > + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> > + dev_xdp_query(dev, mode)) {
> > 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, !!bpf_op);
> > if (IS_ERR(prog))
> > return PTR_ERR(prog);
> >
> > @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> > struct netlink_ext_ack *extack,
> > }
> > }
> >
> > + if (mode == XDP_FLAGS_SKB_MODE) {
> > + bpf_op = generic_xdp_install;
> > + flags |= XDP_FLAGS_KERN_GENERIC;
> > + }
> > err = dev_xdp_install(dev, bpf_op, 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..5e396fd01d8b 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1360,42 +1360,44 @@ 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)
> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> > {
> > - 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);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return XDP_FLAGS_DRV_MODE;
> > + case XDP_ATTACHED_SKB:
> > + return XDP_FLAGS_SKB_MODE;
> > + case XDP_ATTACHED_HW:
> > + return XDP_FLAGS_HW_MODE;
> > + }
> > + return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return IFLA_XDP_DRV_PROG_ID;
> > + case XDP_ATTACHED_SKB:
> > + return IFLA_XDP_SKB_PROG_ID;
> > + case XDP_ATTACHED_HW:
> > + return IFLA_XDP_HW_PROG_ID;
> > + }
> > + return 0;
> > }
> >
> > 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))
> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
> > {
> > u32 curr_id;
> > int err;
> >
> > - curr_id = get_prog_id(dev);
> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> > if (!curr_id)
> > return 0;
> >
> > *prog_id = curr_id;
> > - err = nla_put_u32(skb, attr, curr_id);
> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> > if (err)
> > return err;
> >
> > @@ -1420,16 +1422,13 @@ 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_SKB);
> > 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_DRV);
> > 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_HW);
> > if (err)
> > goto err_cancel;
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 4b2b194f4f1f..af92c04a58d8 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> > xdp_attachment_info *info,
> > }
> > EXPORT_SYMBOL_GPL(xdp_attachment_query);
> >
> > -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> > - struct netdev_bpf *bpf)
> > +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> > + struct netlink_ext_ack *extack)
> > {
> > - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
> > - NL_SET_ERR_MSG(bpf->extack,
> > - "program loaded with different flags");
> > + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> > + NL_SET_ERR_MSG(extack, "program loaded with different flags");
> > return false;
> > }
> > return true;
> > }
> > +
> > +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> > + struct netdev_bpf *bpf)
> > +{
> > + if (info->prog)
> > + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
> > + return true;
> > +}
> > EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
> >
> > void xdp_attachment_setup(struct xdp_attachment_info *info,
> > --
> > 2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 19:42 ` Jakub Kicinski
@ 2019-06-03 9:04 ` Björn Töpel
2019-06-03 21:20 ` Saeed Mahameed
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:04 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke, daniel, ast, netdev, magnus.karlsson,
bjorn.topel, brouer, bpf
On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw and
> > > xdp_flags. The former is the offloaded XDP program, if any, and the
> > > latter tracks the flags that the supplied when attaching the XDP
> > > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> > >
> > > The xdp_prog member, previously only used for SKB_MODE, is shared
> > > with
> > > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > > mutually exclusive. To differentiate between the two modes, a new
> > > internal flag is introduced as well.
> >
> > Just thinking out loud, why can't we allow any combination of
> > HW/DRV/SKB modes? they are totally different attach points in a totally
> > different checkpoints in a frame life cycle.
>
> FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
>
I've always seen the SKB-mode as something that will eventually be removed.
Clickable link:
https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-P
> > Down the road i think we will utilize this fact and start introducing
> > SKB helpers for SKB mode and driver helpers for DRV mode..
>
> Any reason why we would want the extra complexity? There is cls_bpf
> if someone wants skb features after all..
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 19:57 ` Jakub Kicinski
@ 2019-06-03 9:04 ` Björn Töpel
2019-06-03 17:03 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:04 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke, daniel, ast, netdev, magnus.karlsson,
bjorn.topel, brouer, bpf
On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > + mode = XDP_FLAGS_SKB_MODE;
> > > +
> > > + curr_mode = dev_xdp_current_mode(dev);
> > > +
> > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> >
> > if i am reading this correctly this is equivalent to :
> >
> > if (!offload && (curre_mode != mode))
> > offlad is false then curr_mode and mode must be DRV or GENERIC ..
>
> Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> about the diff.
>
> > better if you keep bitwise operations for actual bitmasks, mode and
> > curr_mode are not bitmask, they can hold one value each .. according to
> > your logic..
>
> Well, they hold one bit each, whether one bit is a bitmap perhaps is
> disputable? :)
>
> I think the logic is fine.
>
Hmm, but changing to:
if (!offload && curr_mode && mode != curr_mode)
is equal, and to Saeed's point, clearer. I'll go that route in a v3.
> What happened to my request to move the change in behaviour for
> disabling to a separate patch, tho, Bjorn? :)
Actually, I left that out completely. This patch doesn't change the
behavior. After I realized how the flags *should* be used, I don't
think my v1 change makes sense anymore. My v1 patch was to give an
error if you tried to disable, say generic if drv was enabled via
"auto detect/no flags". But this is catched by looking at the flags.
What I did, however, was moving the flags check into change_fd so that
the driver doesn't have to do the check. E.g. the Intel drivers didn't
do correct checking of flags.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 20:02 ` Jakub Kicinski
@ 2019-06-03 9:07 ` Björn Töpel
2019-06-03 10:56 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:07 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, Saeed Mahameed
On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..f3a875a52c6c 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1940,6 +1940,9 @@ struct net_device {
> > #endif
> > struct hlist_node index_hlist;
> >
> > + struct bpf_prog *xdp_prog_hw;
>
> IDK if we should pay the cost of this pointer for every netdev on the
> system just for the single production driver out there that implements
> HW offload :( I'm on the fence about this..
>
Hmm. Adding a config option? Keep the QUERY_PROG_HW?
> > + u32 xdp_flags;
> > +
> > /*
> > * Cache lines mostly used on transmit path
> > */
>
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index adcc045952c2..5e396fd01d8b 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1360,42 +1360,44 @@ 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)
> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> > {
> > - 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);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return XDP_FLAGS_DRV_MODE;
> > + case XDP_ATTACHED_SKB:
> > + return XDP_FLAGS_SKB_MODE;
> > + case XDP_ATTACHED_HW:
> > + return XDP_FLAGS_HW_MODE;
> > + }
> > + return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return IFLA_XDP_DRV_PROG_ID;
> > + case XDP_ATTACHED_SKB:
> > + return IFLA_XDP_SKB_PROG_ID;
> > + case XDP_ATTACHED_HW:
> > + return IFLA_XDP_HW_PROG_ID;
> > + }
> > + return 0;
> > }
> >
> > 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))
> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
> > {
> > u32 curr_id;
> > int err;
> >
> > - curr_id = get_prog_id(dev);
> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> > if (!curr_id)
> > return 0;
> >
> > *prog_id = curr_id;
> > - err = nla_put_u32(skb, attr, curr_id);
> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> > if (err)
> > return err;
> >
> > @@ -1420,16 +1422,13 @@ 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
> > 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
> > 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);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
> > if (err)
> > goto err_cancel;
> >
>
> So you remove all the attr and flag params just to add a conversion
> helpers to get them based on mode? Why? Seems like unnecessary churn,
> and questionable change :S
>
Fair enough. I'll address this!
> Otherwise looks good to me!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:07 ` Björn Töpel
@ 2019-06-03 10:56 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 23+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-03 10:56 UTC (permalink / raw)
To: Björn Töpel, Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, Netdev,
Björn Töpel, Karlsson, Magnus, Jesper Dangaard Brouer,
bpf, Saeed Mahameed
Björn Töpel <bjorn.topel@gmail.com> writes:
> On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>>
>> On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
>> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> > index 44b47e9df94a..f3a875a52c6c 100644
>> > --- a/include/linux/netdevice.h
>> > +++ b/include/linux/netdevice.h
>> > @@ -1940,6 +1940,9 @@ struct net_device {
>> > #endif
>> > struct hlist_node index_hlist;
>> >
>> > + struct bpf_prog *xdp_prog_hw;
>>
>> IDK if we should pay the cost of this pointer for every netdev on the
>> system just for the single production driver out there that implements
>> HW offload :( I'm on the fence about this..
>>
>
> Hmm. Adding a config option? Keep the QUERY_PROG_HW?
>
>> > + u32 xdp_flags;
>> > +
>> > /*
>> > * Cache lines mostly used on transmit path
>> > */
>>
>> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> > index adcc045952c2..5e396fd01d8b 100644
>> > --- a/net/core/rtnetlink.c
>> > +++ b/net/core/rtnetlink.c
>> > @@ -1360,42 +1360,44 @@ 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)
>> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
>> > {
>> > - 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);
>> > + switch (tgt_mode) {
>> > + case XDP_ATTACHED_DRV:
>> > + return XDP_FLAGS_DRV_MODE;
>> > + case XDP_ATTACHED_SKB:
>> > + return XDP_FLAGS_SKB_MODE;
>> > + case XDP_ATTACHED_HW:
>> > + return XDP_FLAGS_HW_MODE;
>> > + }
>> > + return 0;
>> > }
>> >
>> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
>> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
>> > {
>> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
>> > - XDP_QUERY_PROG_HW);
>> > + switch (tgt_mode) {
>> > + case XDP_ATTACHED_DRV:
>> > + return IFLA_XDP_DRV_PROG_ID;
>> > + case XDP_ATTACHED_SKB:
>> > + return IFLA_XDP_SKB_PROG_ID;
>> > + case XDP_ATTACHED_HW:
>> > + return IFLA_XDP_HW_PROG_ID;
>> > + }
>> > + return 0;
>> > }
>> >
>> > 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))
>> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
>> > {
>> > u32 curr_id;
>> > int err;
>> >
>> > - curr_id = get_prog_id(dev);
>> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
>> > if (!curr_id)
>> > return 0;
>> >
>> > *prog_id = curr_id;
>> > - err = nla_put_u32(skb, attr, curr_id);
>> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
>> > if (err)
>> > return err;
>> >
>> > @@ -1420,16 +1422,13 @@ 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);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
>> > 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);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
>> > 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);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
>> > if (err)
>> > goto err_cancel;
>> >
>>
>> So you remove all the attr and flag params just to add a conversion
>> helpers to get them based on mode? Why? Seems like unnecessary churn,
>> and questionable change :S
>>
>
> Fair enough. I'll address this!
I think this was actually my idea, wasn't it? :)
My thought being that if you just do the minimal change here, we'll end
up with three empty wrapper functions, which we might as well just fold
into the caller...
-Toke
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 8:39 ` Björn Töpel
@ 2019-06-03 14:58 ` Jonathan Lemon
2019-06-03 23:11 ` Daniel Borkmann
1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Lemon @ 2019-06-03 14:58 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, Jakub Kicinski, Saeed Mahameed
On 3 Jun 2019, at 1:39, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com>
> wrote:
>>
>> On 31 May 2019, at 2:42, 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 two new members: xdp_prog_hw and
>>> xdp_flags. The former is the offloaded XDP program, if any, and the
>>> latter tracks the flags that the supplied when attaching the XDP
>>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>>>
>>> The xdp_prog member, previously only used for SKB_MODE, is shared
>>> with
>>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
>>> mutually exclusive. To differentiate between the two modes, a new
>>> internal flag is introduced as well.
>>
>> I'm not entirely clear why this new flag is needed - GENERIC seems to
>> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>>
>> If the user does not explicitly specify a type (skb|drv|hw), then the
>> command should choose the correct type and then behave as if this
>> type
>> was specified.
>>
>
> Yes, this is kind of hairy.
>
> SKB and DRV are mutually exclusive, but HW is not. IOW, valid options
> are:
> SKB, DRV, HW, SKB+HW DRV+HW.
Fair enough, that was the understanding that I had from the code,
although I'm not sure about the usage of SKB+HW mode.
>
> What complicates things further, is that SKB and DRV can be implicitly
> (auto/no flags) or explicitly enabled (flags).
>
> If a user doesn't pass any flags, the "best supported mode" should be
> selected. If this "auto mode" is used, it should be seen as a third
> mode. E.g.
>
> ip link set dev eth0 xdp on -- OK
> ip link set dev eth0 xdp off -- OK
>
> ip link set dev eth0 xdp on -- OK # generic auto selected
> ip link set dev eth0 xdpgeneric off -- NOK, bad flags
>
> ip link set dev eth0 xdp on -- OK # drv auto selected
> ip link set dev eth0 xdpdrv off -- NOK, bad flags
>
> ...and so on. The idea is that a user should use the same set of flags
> always.
I'm not sure about this. The "xdp" mode shouldn't be treated as a
separate mode, it should be "best supported mode", as indicated above.
From my view, it should select the appropriate mode, and then proceed
as if the user had specified that mode, rather than being treated as an
independent mode.
ip link set dev eth0 xdp on - OK
ip link set dev eth0 xdp off - OK
ip link set dev eth0 xdp on - OK, selected dev
ip link set dev eth0 xdpgeneric off - NOK, not running
ip link set dev eth0 xdpdrv off - OK
> The internal "GENERIC" flag is only to determine if the xdp_prog
> represents a DRV version or SKB version. Maybe it would be clearer
> just to add an additional xdp_prog_drv to the net_device, instead?
I'd go the other way, and remove GENERIC, leaving only SKB, DRV, and HW.
The appropriate mode flag (SKB|DRV) is enough to indicate the type of
xdp_prog.
--
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:04 ` Björn Töpel
@ 2019-06-03 17:03 ` Jakub Kicinski
2019-06-04 5:16 ` Björn Töpel
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-03 17:03 UTC (permalink / raw)
To: Björn Töpel
Cc: Saeed Mahameed, toke, daniel, ast, netdev, magnus.karlsson,
bjorn.topel, brouer, bpf
On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > > + mode = XDP_FLAGS_SKB_MODE;
> > > > +
> > > > + curr_mode = dev_xdp_current_mode(dev);
> > > > +
> > > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > >
> > > if i am reading this correctly this is equivalent to :
> > >
> > > if (!offload && (curre_mode != mode))
> > > offlad is false then curr_mode and mode must be DRV or GENERIC ..
> >
> > Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> > about the diff.
> >
> > > better if you keep bitwise operations for actual bitmasks, mode and
> > > curr_mode are not bitmask, they can hold one value each .. according to
> > > your logic..
> >
> > Well, they hold one bit each, whether one bit is a bitmap perhaps is
> > disputable? :)
> >
> > I think the logic is fine.
> >
>
> Hmm, but changing to:
>
> if (!offload && curr_mode && mode != curr_mode)
>
> is equal, and to Saeed's point, clearer. I'll go that route in a v3.
Sorry, you're right, the flags get mangled before they get here, so
yeah, this condition should work. Confusingly.
> > What happened to my request to move the change in behaviour for
> > disabling to a separate patch, tho, Bjorn? :)
>
> Actually, I left that out completely. This patch doesn't change the
> behavior. After I realized how the flags *should* be used, I don't
> think my v1 change makes sense anymore. My v1 patch was to give an
> error if you tried to disable, say generic if drv was enabled via
> "auto detect/no flags". But this is catched by looking at the flags.
>
> What I did, however, was moving the flags check into change_fd so that
> the driver doesn't have to do the check. E.g. the Intel drivers didn't
> do correct checking of flags.
Ugh. Could you please rewrite the conditions to make the fd >= check
consistently the outside if? Also could you add extack to this:
+ if (!offload && dev_xdp_query(dev, mode) &&
+ !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
+ return -EBUSY;
It's unclear what it's doing.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:04 ` Björn Töpel
@ 2019-06-03 21:20 ` Saeed Mahameed
2019-06-04 5:18 ` Björn Töpel
2019-06-04 10:17 ` Jesper Dangaard Brouer
0 siblings, 2 replies; 23+ messages in thread
From: Saeed Mahameed @ 2019-06-03 21:20 UTC (permalink / raw)
To: jakub.kicinski, bjorn.topel
Cc: toke, magnus.karlsson, daniel, brouer, ast, netdev, bpf, bjorn.topel
On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw
> > > > and
> > > > xdp_flags. The former is the offloaded XDP program, if any, and
> > > > the
> > > > latter tracks the flags that the supplied when attaching the
> > > > XDP
> > > > program. The flags only apply to SKB_MODE or DRV_MODE, not
> > > > HW_MODE.
> > > >
> > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > shared
> > > > with
> > > > DRV_MODE. This is OK, due to the fact that SKB_MODE and
> > > > DRV_MODE are
> > > > mutually exclusive. To differentiate between the two modes, a
> > > > new
> > > > internal flag is introduced as well.
> > >
> > > Just thinking out loud, why can't we allow any combination of
> > > HW/DRV/SKB modes? they are totally different attach points in a
> > > totally
> > > different checkpoints in a frame life cycle.
> >
> > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> >
>
> I've always seen the SKB-mode as something that will eventually be
> removed.
>
I don't think so, we are too deep into SKB-mode.
> Clickable link:
> https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-
> P
>
So we are all hanging on Jesper's refactoring ideas that are not
getting any priority for now :).
> > > Down the road i think we will utilize this fact and start
> > > introducing
> > > SKB helpers for SKB mode and driver helpers for DRV mode..
> >
> > Any reason why we would want the extra complexity? There is
> > cls_bpf
> > if someone wants skb features after all..
Donno, SKB mode is earlier in the stack maybe ..
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 8:39 ` Björn Töpel
2019-06-03 14:58 ` Jonathan Lemon
@ 2019-06-03 23:11 ` Daniel Borkmann
2019-06-04 5:37 ` Björn Töpel
1 sibling, 1 reply; 23+ messages in thread
From: Daniel Borkmann @ 2019-06-03 23:11 UTC (permalink / raw)
To: Björn Töpel, Jonathan Lemon
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov, Netdev,
Björn Töpel, Karlsson, Magnus, Jesper Dangaard Brouer,
bpf, Jakub Kicinski, Saeed Mahameed
On 06/03/2019 10:39 AM, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
>> On 31 May 2019, at 2:42, 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 two new members: xdp_prog_hw and
>>> xdp_flags. The former is the offloaded XDP program, if any, and the
>>> latter tracks the flags that the supplied when attaching the XDP
>>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>>>
>>> The xdp_prog member, previously only used for SKB_MODE, is shared with
>>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
>>> mutually exclusive. To differentiate between the two modes, a new
>>> internal flag is introduced as well.
>>
>> I'm not entirely clear why this new flag is needed - GENERIC seems to
>> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>>
>> If the user does not explicitly specify a type (skb|drv|hw), then the
>> command should choose the correct type and then behave as if this type
>> was specified.
>
> Yes, this is kind of hairy.
>
> SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
> SKB, DRV, HW, SKB+HW DRV+HW.
Correct, HW is a bit special here in that it helps offloading parts of
the DRV XDP program to NIC, but also do RSS steering in BPF etc, hence
this combo is intentionally allowed (see also git log).
> What complicates things further, is that SKB and DRV can be implicitly
> (auto/no flags) or explicitly enabled (flags).
Mainly out of historic context: originally the fallback to SKB mode was
implicit if the ndo_bpf was missing. But there are use cases where we
want to fail if the driver does not support native XDP to avoid surprises.
> If a user doesn't pass any flags, the "best supported mode" should be
> selected. If this "auto mode" is used, it should be seen as a third
> mode. E.g.
>
> ip link set dev eth0 xdp on -- OK
> ip link set dev eth0 xdp off -- OK
>
> ip link set dev eth0 xdp on -- OK # generic auto selected
> ip link set dev eth0 xdpgeneric off -- NOK, bad flags
This would work if the auto selection would have selected XDP generic.
> ip link set dev eth0 xdp on -- OK # drv auto selected
> ip link set dev eth0 xdpdrv off -- NOK, bad flags
This would work if the auto selection chose native XDP previously. Are
you saying it's not the case?
Also, what is the use case in mixing these commands? It should be xdp
on+off, xdpdrv on+off, and so on. Are you saying you would prefer a
xdp{,any} off that uninstalls everything? Isn't this mostly a user space
issue to whatever orchestrates XDP?
> ...and so on. The idea is that a user should use the same set of flags always.
>
> The internal "GENERIC" flag is only to determine if the xdp_prog
> represents a DRV version or SKB version. Maybe it would be clearer
> just to add an additional xdp_prog_drv to the net_device, instead?
>
>> The logic in dev_change_xdp_fd() is too complicated. It disallows
>> setting (drv|skb), but allows (hw|skb), which I'm not sure is
>> intentional.
>>
>> It should be clearer as to which combinations are allowed.
>
> Fair point. I'll try to clean it up further.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 17:03 ` Jakub Kicinski
@ 2019-06-04 5:16 ` Björn Töpel
0 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke, daniel, ast, netdev, magnus.karlsson,
bjorn.topel, brouer, bpf
On Mon, 3 Jun 2019 at 19:03, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > > > + mode = XDP_FLAGS_SKB_MODE;
> > > > > +
> > > > > + curr_mode = dev_xdp_current_mode(dev);
> > > > > +
> > > > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > > >
> > > > if i am reading this correctly this is equivalent to :
> > > >
> > > > if (!offload && (curre_mode != mode))
> > > > offlad is false then curr_mode and mode must be DRV or GENERIC ..
> > >
> > > Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> > > about the diff.
> > >
> > > > better if you keep bitwise operations for actual bitmasks, mode and
> > > > curr_mode are not bitmask, they can hold one value each .. according to
> > > > your logic..
> > >
> > > Well, they hold one bit each, whether one bit is a bitmap perhaps is
> > > disputable? :)
> > >
> > > I think the logic is fine.
> > >
> >
> > Hmm, but changing to:
> >
> > if (!offload && curr_mode && mode != curr_mode)
> >
> > is equal, and to Saeed's point, clearer. I'll go that route in a v3.
>
> Sorry, you're right, the flags get mangled before they get here, so
> yeah, this condition should work. Confusingly.
>
> > > What happened to my request to move the change in behaviour for
> > > disabling to a separate patch, tho, Bjorn? :)
> >
> > Actually, I left that out completely. This patch doesn't change the
> > behavior. After I realized how the flags *should* be used, I don't
> > think my v1 change makes sense anymore. My v1 patch was to give an
> > error if you tried to disable, say generic if drv was enabled via
> > "auto detect/no flags". But this is catched by looking at the flags.
> >
> > What I did, however, was moving the flags check into change_fd so that
> > the driver doesn't have to do the check. E.g. the Intel drivers didn't
> > do correct checking of flags.
>
> Ugh. Could you please rewrite the conditions to make the fd >= check
> consistently the outside if? Also could you add extack to this:
>
The reason I moved the if-statement (checking if we're mixing
drv/skb), is because I'd like to catch the no-op (e.g. xdpdrv active
and calling xdpgeneric off) early (the return 0, under the if (fd >=
check).
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
>
> It's unclear what it's doing.
This checks whether the flags have changed, pulling out the logic from
the drivers. xdp_prog_flags_ok adds to extack, resuing the flags_ok
function. The xdp_attachment_flags_ok OTOH is not necessary anymore,
and should be further cleaned up. I'll address this and make the this
clause more clear.
Björn
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 21:20 ` Saeed Mahameed
@ 2019-06-04 5:18 ` Björn Töpel
2019-06-04 10:17 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:18 UTC (permalink / raw)
To: Saeed Mahameed
Cc: jakub.kicinski, toke, magnus.karlsson, daniel, brouer, ast,
netdev, bpf, bjorn.topel
On Mon, 3 Jun 2019 at 23:20, Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw
> > > > > and
> > > > > xdp_flags. The former is the offloaded XDP program, if any, and
> > > > > the
> > > > > latter tracks the flags that the supplied when attaching the
> > > > > XDP
> > > > > program. The flags only apply to SKB_MODE or DRV_MODE, not
> > > > > HW_MODE.
> > > > >
> > > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > > shared
> > > > > with
> > > > > DRV_MODE. This is OK, due to the fact that SKB_MODE and
> > > > > DRV_MODE are
> > > > > mutually exclusive. To differentiate between the two modes, a
> > > > > new
> > > > > internal flag is introduced as well.
> > > >
> > > > Just thinking out loud, why can't we allow any combination of
> > > > HW/DRV/SKB modes? they are totally different attach points in a
> > > > totally
> > > > different checkpoints in a frame life cycle.
> > >
> > > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> > >
> >
> > I've always seen the SKB-mode as something that will eventually be
> > removed.
> >
>
> I don't think so, we are too deep into SKB-mode.
>
You might be right. :-(
Are you envisioning sk_buff specfic XDP_SKB functions, that only apply
for XDP_SKB? Ick.
> > Clickable link:
> > https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-
> > P
> >
>
> So we are all hanging on Jesper's refactoring ideas that are not
> getting any priority for now :).
>
>
> > > > Down the road i think we will utilize this fact and start
> > > > introducing
> > > > SKB helpers for SKB mode and driver helpers for DRV mode..
> > >
> > > Any reason why we would want the extra complexity? There is
> > > cls_bpf
> > > if someone wants skb features after all..
>
> Donno, SKB mode is earlier in the stack maybe ..
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 23:11 ` Daniel Borkmann
@ 2019-06-04 5:37 ` Björn Töpel
0 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:37 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jonathan Lemon, Toke Høiland-Jørgensen,
Alexei Starovoitov, Netdev, Björn Töpel, Karlsson,
Magnus, Jesper Dangaard Brouer, bpf, Jakub Kicinski,
Saeed Mahameed
On Tue, 4 Jun 2019 at 01:11, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/03/2019 10:39 AM, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
> >> On 31 May 2019, at 2:42, 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 two new members: xdp_prog_hw and
> >>> xdp_flags. The former is the offloaded XDP program, if any, and the
> >>> latter tracks the flags that the supplied when attaching the XDP
> >>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >>>
> >>> The xdp_prog member, previously only used for SKB_MODE, is shared with
> >>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> >>> mutually exclusive. To differentiate between the two modes, a new
> >>> internal flag is introduced as well.
> >>
> >> I'm not entirely clear why this new flag is needed - GENERIC seems to
> >> be an alias for SKB_MODE, so why just use SKB_MODE directly?
> >>
> >> If the user does not explicitly specify a type (skb|drv|hw), then the
> >> command should choose the correct type and then behave as if this type
> >> was specified.
> >
> > Yes, this is kind of hairy.
> >
> > SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
> > SKB, DRV, HW, SKB+HW DRV+HW.
>
> Correct, HW is a bit special here in that it helps offloading parts of
> the DRV XDP program to NIC, but also do RSS steering in BPF etc, hence
> this combo is intentionally allowed (see also git log).
>
> > What complicates things further, is that SKB and DRV can be implicitly
> > (auto/no flags) or explicitly enabled (flags).
>
> Mainly out of historic context: originally the fallback to SKB mode was
> implicit if the ndo_bpf was missing. But there are use cases where we
> want to fail if the driver does not support native XDP to avoid surprises.
>
> > If a user doesn't pass any flags, the "best supported mode" should be
> > selected. If this "auto mode" is used, it should be seen as a third
> > mode. E.g.
> >
> > ip link set dev eth0 xdp on -- OK
> > ip link set dev eth0 xdp off -- OK
> >
> > ip link set dev eth0 xdp on -- OK # generic auto selected
> > ip link set dev eth0 xdpgeneric off -- NOK, bad flags
>
> This would work if the auto selection would have selected XDP generic.
>
> > ip link set dev eth0 xdp on -- OK # drv auto selected
> > ip link set dev eth0 xdpdrv off -- NOK, bad flags
>
> This would work if the auto selection chose native XDP previously. Are
> you saying it's not the case?
>
Yes, that is *not* the case for some drivers. With the Intel drivers
we didn't check the flags at all at XDP attachment (check out the
usage of xdp_attachment_flags_ok), but e.g. nfp and netdevsim does.
Grep for 'program loaded with different flags' in the test_offload.py
selftest. I like this approach, and my patch does this flag check in
dev_change_xdp_fd.
> Also, what is the use case in mixing these commands? It should be xdp
> on+off, xdpdrv on+off, and so on. Are you saying you would prefer a
> xdp{,any} off that uninstalls everything? Isn't this mostly a user space
> issue to whatever orchestrates XDP?
>
No, I'm not suggesting a change. There is no use-case mixing them.
What the flags ok checks do is returning an error (like nfp and
netdevsim does) if a user tries to mix, say, "xdp" and explicit
xdpdrv/xdpgeneric". This patch moves this check to the generic
function dev_change_xdp_fd.
There seems to be a confusion about how this is supposed to be used.
It was for me, e.g. I though using "enable with xdp and disable with
xdpdrv" was OK. This was the reason why I added an error on "disable
with xdpgeneric off, if xdpdrv is active" in my first revision of the
series. I removed this in v2, after Jakub pointed out the
test_offload.py test, which is a great showcase/test of what should be
allowed and what shouldn't in terms of flags.
TL;DR: Let's stick to what test_offload.py asserts, for all XDP.
> > ...and so on. The idea is that a user should use the same set of flags always.
> >
> > The internal "GENERIC" flag is only to determine if the xdp_prog
> > represents a DRV version or SKB version. Maybe it would be clearer
> > just to add an additional xdp_prog_drv to the net_device, instead?
> >
> >> The logic in dev_change_xdp_fd() is too complicated. It disallows
> >> setting (drv|skb), but allows (hw|skb), which I'm not sure is
> >> intentional.
> >>
> >> It should be clearer as to which combinations are allowed.
> >
> > Fair point. I'll try to clean it up further.
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 21:20 ` Saeed Mahameed
2019-06-04 5:18 ` Björn Töpel
@ 2019-06-04 10:17 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 23+ messages in thread
From: Jesper Dangaard Brouer @ 2019-06-04 10:17 UTC (permalink / raw)
To: Saeed Mahameed
Cc: jakub.kicinski, bjorn.topel, toke, magnus.karlsson, daniel, ast,
netdev, bpf, bjorn.topel, brouer, John Fastabend
On Mon, 3 Jun 2019 21:20:30 +0000
Saeed Mahameed <saeedm@mellanox.com> wrote:
> On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > On Fri, 2019-05-31 at 11:42 +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 two new members: xdp_prog_hw
> > > > > and xdp_flags. The former is the offloaded XDP program, if
> > > > > any, and the latter tracks the flags that the supplied when
> > > > > attaching the XDP program. The flags only apply to SKB_MODE
> > > > > or DRV_MODE, not HW_MODE.
> > > > >
> > > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > > shared with DRV_MODE. This is OK, due to the fact that
> > > > > SKB_MODE and DRV_MODE are mutually exclusive. To
> > > > > differentiate between the two modes, a new internal flag is
> > > > > introduced as well.
> > > >
> > > > Just thinking out loud, why can't we allow any combination of
> > > > HW/DRV/SKB modes? they are totally different attach points in a
> > > > totally different checkpoints in a frame life cycle.
The DRV_MODE and SKB_MODE is tricky to run concurrently. The XDP-redirect
scheme (designed by John) use a BPF helper (bpf_xdp_redirect_map) that
set global per-CPU state (this_cpu_ptr(&bpf_redirect_info)).
The tricky part (which I warned about, and we already have some fixes
for) is that the XDP/BPF-prog can call bpf_redirect_map, which update
the per-CPU state, but it can still choose not to return XDP_REDIRECT,
which then miss an invocation of xdp_do_redirect().
Later, your SKB_MODE XDP/BPF-prog can return XDP_REDIRECT without
calling the helper, and then use/reach to the per-CPU info set by the
DRV_MODE program, which is NOT something I want us to "support".
> > > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> >
> > I've always seen the SKB-mode as something that will eventually be
> > removed.
>
> I don't think so, we are too deep into SKB-mode.
I wish we could remove it. After we got native XDP support in veth,
then its original purpose is gone, which were making it easier for
developers to get something working on their laptop, without having to
deploy it to the server with physical hardware all the time.
> > Clickable link:
> > https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/
> >
>
> So we are all hanging on Jesper's refactoring ideas that are not
> getting any priority for now :).
Well, that is not true. This patchset _is_ the refactor idea that
Bjørn is taking over and working on. Specifically [2] from above link.
[2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org#refactor-idea-xdpbpf_prog-into-netdev_rx_queuenet_device
> > > > Down the road i think we will utilize this fact and start
> > > > introducing SKB helpers for SKB mode and driver helpers for DRV
> > > > mode..
> > >
> > > Any reason why we would want the extra complexity? There is
> > > cls_bpf if someone wants skb features after all..
>
> Donno, SKB mode is earlier in the stack maybe ..
From a BPF perspective you cannot introduce SKB helpers for SKB mode.
An XDP-prog have bpf prog type XDP (BPF_PROG_TYPE_XDP), and the program
itself is identical regardless of attaching for DRV_MODE or SKB_MODE.
You cannot detect this at attach time, due to tail-calls (which have
painted us into a corner).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-06-04 10:17 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-31 9:42 [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code Björn Töpel
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 19:42 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
2019-06-03 21:20 ` Saeed Mahameed
2019-06-04 5:18 ` Björn Töpel
2019-06-04 10:17 ` Jesper Dangaard Brouer
2019-06-01 19:57 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
2019-06-03 17:03 ` Jakub Kicinski
2019-06-04 5:16 ` Björn Töpel
2019-06-01 9:50 ` kbuild test robot
2019-06-01 18:12 ` Jonathan Lemon
2019-06-03 8:39 ` Björn Töpel
2019-06-03 14:58 ` Jonathan Lemon
2019-06-03 23:11 ` Daniel Borkmann
2019-06-04 5:37 ` Björn Töpel
2019-06-01 20:02 ` Jakub Kicinski
2019-06-03 9:07 ` Björn Töpel
2019-06-03 10:56 ` Toke Høiland-Jørgensen
2019-05-31 9:42 ` [PATCH bpf-next v2 2/2] net: xdp: remove XDP_QUERY_PROG{,_HW} Björn Töpel
2019-05-31 9:45 ` [PATCH bpf-next v2 0/2] net: xdp: refactor the XDP_QUERY_PROG and XDP_QUERY_PROG_HW code 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).