Björn Töpel <bjorn.topel@xxxxxxxxx> writes:
> 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!
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
