On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
[off-list ref] wrote:

On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:

quoted

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?

quoted

+     u32                     xdp_flags;
+
 /*
  * Cache lines mostly used on transmit path
  */

quoted

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!