From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andriin@fb.com>, Martin KaFai Lau <kafai@fb.com>,
Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@chromium.org>,
Jesper Dangaard Brouer <hawk@kernel.org>,
"Michael S. Tsirkin" <mst@redhat.com>,
Romain Perier <romain.perier@gmail.com>,
Allen Pais <apais@linux.microsoft.com>,
Grygorii Strashko <grygorii.strashko@ti.com>,
Simon Horman <simon.horman@netronome.com>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Lorenzo Bianconi <lorenzo@kernel.org>,
Wei Yongjun <weiyongjun1@huawei.com>,
Jiri Benc <jbenc@redhat.com>,
oss-drivers@netronome.com, linux-omap@vger.kernel.org,
netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf 1/7] xdp: remove the xdp_attachment_flags_ok() callback
Date: Fri, 04 Dec 2020 18:12:18 +0100 [thread overview]
Message-ID: <87sg8l7ax9.fsf@toke.dk> (raw)
In-Reply-To: <20201204084847.04d9dc46@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Jakub Kicinski <kuba@kernel.org> writes:
> On Fri, 04 Dec 2020 10:38:06 +0100 Toke Høiland-Jørgensen wrote:
>> Jakub Kicinski <kuba@kernel.org> writes:
>> > On Thu, 03 Dec 2020 22:35:18 +0100 Toke Høiland-Jørgensen wrote:
>> >> Since we offloaded and non-offloaded programs can co-exist there doesn't
>> >> really seem to be any reason for the check anyway, and it's only used in
>> >> three drivers so let's just get rid of the callback entirely.
>> >
>> > I don't remember exactly now, but I think the concern was that using
>> > the unspecified mode is pretty ambiguous when interface has multiple
>> > programs attached.
>>
>> Right. I did scratch my head a bit for why the check was there in the
>> first place, but that makes sense, actually :)
>>
>> So how about we disallow unload without specifying a mode, but only if
>> more than one program is loaded?
>
> Are you including replacing as a form of unload? :)
Yeah, that's what I ended up with (in v2): Any time there are multiple
programs loaded, callers have to specify a mode flag to avoid ambiguity.
> IMHO the simpler the definition of the API / constraint the better.
> "You must specify the same flags" is pretty simple, as is copying the
> old behavior rather than trying to come up with new rules.
>
> But up to you, I don't mind either way, really..
Well that old behaviour was what led me to investigate this in the first
place: If you just look at a program that's loaded, see it's in driver
mode, and then try to unload it with flags set to XDP_MODE_DRV, it will
sometimes work and sometimes not depending on what flags that program
happened to have been loaded with. In libxdp this is exactly what we do
(look at the loaded program and set the corresponding mode flag), so I
ended up getting some really odd bug reports...
So I really don't want to keep the current behaviour; if what I propose
in v2 is OK with you I think we should just go with that :)
-Toke
next prev parent reply other threads:[~2020-12-04 17:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-03 21:35 [PATCH bpf 0/7] selftests/bpf: Restore test_offload.py to working order Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 1/7] xdp: remove the xdp_attachment_flags_ok() callback Toke Høiland-Jørgensen
2020-12-04 1:42 ` Jakub Kicinski
2020-12-04 9:38 ` Toke Høiland-Jørgensen
2020-12-04 16:48 ` Jakub Kicinski
2020-12-04 17:12 ` Toke Høiland-Jørgensen [this message]
2020-12-03 21:35 ` [PATCH bpf 2/7] selftests/bpf/test_offload.py: Remove check for program load flags match Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 3/7] netdevsim: Add debugfs toggle to reject BPF programs in verifier Toke Høiland-Jørgensen
2020-12-04 1:44 ` Jakub Kicinski
2020-12-04 9:39 ` Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 4/7] selftests/bpf/test_offload.py: only check verifier log on verification fails Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 5/7] selftests/bpf/test_offload.py: fix expected case of extack messages Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 6/7] selftests/bpf/test_offload.py: reset ethtool features after failed setting Toke Høiland-Jørgensen
2020-12-03 21:35 ` [PATCH bpf 7/7] selftests/bpf/test_offload.py: filter bpftool internal map when counting maps Toke Høiland-Jørgensen
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=87sg8l7ax9.fsf@toke.dk \
--to=toke@redhat.com \
--cc=andriin@fb.com \
--cc=apais@linux.microsoft.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=grygorii.strashko@ti.com \
--cc=gustavoars@kernel.org \
--cc=hawk@kernel.org \
--cc=jbenc@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@chromium.org \
--cc=kuba@kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=lorenzo@kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=oss-drivers@netronome.com \
--cc=romain.perier@gmail.com \
--cc=simon.horman@netronome.com \
--cc=songliubraving@fb.com \
--cc=weiyongjun1@huawei.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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