On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
<jakub.kicinski@xxxxxxxxxxxxx> 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!
