From: "Toke Høiland-Jørgensen" <toke@redhat•com>
To: "Björn Töpel" <bjorn.topel@gmail•com>,
"Jakub Kicinski" <jakub.kicinski@netronome•com>
Cc: "Alexei Starovoitov" <ast@kernel•org>,
"Daniel Borkmann" <daniel@iogearbox•net>,
Netdev <netdev@vger•kernel.org>,
"Björn Töpel" <bjorn.topel@intel•com>,
"Karlsson, Magnus" <magnus.karlsson@intel•com>,
"Jesper Dangaard Brouer" <brouer@redhat•com>,
bpf <bpf@vger•kernel.org>, "Saeed Mahameed" <saeedm@mellanox•com>
Subject: Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
Date: Mon, 03 Jun 2019 12:56:49 +0200 [thread overview]
Message-ID: <87muizgcni.fsf@toke.dk> (raw)
In-Reply-To: <CAJ+HfNh-ifUqJHL61e7nQysZRCrKhNeX_mZ6Vn2D786-XEfm3g@mail.gmail.com>
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
next prev parent reply other threads:[~2019-06-03 11:34 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87muizgcni.fsf@toke.dk \
--to=toke@redhat$(echo .)com \
--cc=ast@kernel$(echo .)org \
--cc=bjorn.topel@gmail$(echo .)com \
--cc=bjorn.topel@intel$(echo .)com \
--cc=bpf@vger$(echo .)kernel.org \
--cc=brouer@redhat$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=magnus.karlsson@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=saeedm@mellanox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox