Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
From: Jakub Kicinski <hidden>
Date: 2019-06-01 20:02:28
Also in:
bpf
On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
quoted hunk ↗ jump to hunk
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 */
quoted hunk ↗ jump to hunk
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!