From: "Toke Høiland-Jørgensen" <toke@redhat.com> To: Jakub Kicinski <kuba@kernel.org> Cc: "Marek Majtyka" <alardam@gmail.com>, "Saeed Mahameed" <saeed@kernel.org>, "David Ahern" <dsahern@gmail.com>, "Maciej Fijalkowski" <maciej.fijalkowski@intel.com>, "John Fastabend" <john.fastabend@gmail.com>, "Jesper Dangaard Brouer" <jbrouer@redhat.com>, "Daniel Borkmann" <daniel@iogearbox.net>, "Maciej Fijalkowski" <maciejromanfijalkowski@gmail.com>, "Björn Töpel" <bjorn.topel@intel.com>, "Andrii Nakryiko" <andrii.nakryiko@gmail.com>, "Jonathan Lemon" <jonathan.lemon@gmail.com>, "Alexei Starovoitov" <ast@kernel.org>, "Network Development" <netdev@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>, hawk@kernel.org, bpf <bpf@vger.kernel.org>, intel-wired-lan <intel-wired-lan@lists.osuosl.org>, "Karlsson, Magnus" <magnus.karlsson@intel.com>, jeffrey.t.kirsher@intel.com Subject: Re: [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set Date: Wed, 10 Feb 2021 23:52:39 +0100 [thread overview] Message-ID: <87czx7r0w8.fsf@toke.dk> (raw) In-Reply-To: <20210210103135.38921f85@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 10 Feb 2021 11:53:53 +0100 Toke Høiland-Jørgensen wrote: >> >> I am a bit confused now. Did you mean validation tests of those XDP >> >> flags, which I am working on or some other validation tests? >> >> What should these tests verify? Can you please elaborate more on the >> >> topic, please - just a few sentences how are you see it? >> > >> > Conformance tests can be written for all features, whether they have >> > an explicit capability in the uAPI or not. But for those that do IMO >> > the tests should be required. >> > >> > Let me give you an example. This set adds a bit that says Intel NICs >> > can do XDP_TX and XDP_REDIRECT, yet we both know of the Tx queue >> > shenanigans. So can i40e do XDP_REDIRECT or can it not? >> > >> > If we have exhaustive conformance tests we can confidently answer that >> > question. And the answer may not be "yes" or "no", it may actually be >> > "we need more options because many implementations fall in between". >> > >> > I think readable (IOW not written in some insane DSL) tests can also >> > be useful for users who want to check which features their program / >> > deployment will require. >> >> While I do agree that that kind of conformance test would be great, I >> don't think it has to hold up this series (the perfect being the enemy >> of the good, and all that). We have a real problem today that userspace >> can't tell if a given driver implements, say, XDP_REDIRECT, and so >> people try to use it and spend days wondering which black hole their >> packets disappear into. And for things like container migration we need >> to be able to predict whether a given host supports a feature *before* >> we start the migration and try to use it. > > Unless you have a strong definition of what XDP_REDIRECT means the flag > itself is not worth much. We're not talking about normal ethtool feature > flags which are primarily stack-driven, XDP is implemented mostly by > the driver, each vendor can do their own thing. Maybe I've seen one > vendor incompatibility too many at my day job to hope for the best... I'm totally on board with documenting what a feature means. E.g., for XDP_REDIRECT, whether it's acceptable to fail the redirect in some situations even when it's active, or if there should always be a slow-path fallback. But I disagree that the flag is worthless without it. People are running into real issues with trying to run XDP_REDIRECT programs on a driver that doesn't support it at all, and it's incredibly confusing. The latest example popped up literally yesterday: https://lore.kernel.org/xdp-newbies/CAM-scZPPeu44FeCPGO=Qz=03CrhhfB1GdJ8FNEpPqP_G27c6mQ@mail.gmail.com/ >> I view the feature flags as a list of features *implemented* by the >> driver. Which should be pretty static in a given kernel, but may be >> different than the features currently *enabled* on a given system (due >> to, e.g., the TX queue stuff). > > Hm, maybe I'm not being clear enough. The way XDP_REDIRECT (your > example) is implemented across drivers differs in a meaningful ways. > Hence the need for conformance testing. We don't have a golden SW > standard to fall back on, like we do with HW offloads. I'm not disagreeing that we need to harmonise what "implementing a feature" means. Maybe I'm just not sure what you mean by "conformance testing"? What would that look like, specifically? A script in selftest that sets up a redirect between two interfaces that we tell people to run? Or what? How would you catch, say, that issue where if a machine has more CPUs than the NIC has TXQs things start falling apart? > Also IDK why those tests are considered such a huge ask. As I said most > vendors probably already have them, and so I'd guess do good distros. > So let's work together. I guess what I'm afraid of is that this will end up delaying or stalling a fix for a long-standing issue (which is what I consider this series as shown by the example above). Maybe you can alleviate that by expanding a bit on what you mean? -Toke
WARNING: multiple messages have this Message-ID (diff)
From: Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set Date: Wed, 10 Feb 2021 23:52:39 +0100 [thread overview] Message-ID: <87czx7r0w8.fsf@toke.dk> (raw) In-Reply-To: <20210210103135.38921f85@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 10 Feb 2021 11:53:53 +0100 Toke H?iland-J?rgensen wrote: >> >> I am a bit confused now. Did you mean validation tests of those XDP >> >> flags, which I am working on or some other validation tests? >> >> What should these tests verify? Can you please elaborate more on the >> >> topic, please - just a few sentences how are you see it? >> > >> > Conformance tests can be written for all features, whether they have >> > an explicit capability in the uAPI or not. But for those that do IMO >> > the tests should be required. >> > >> > Let me give you an example. This set adds a bit that says Intel NICs >> > can do XDP_TX and XDP_REDIRECT, yet we both know of the Tx queue >> > shenanigans. So can i40e do XDP_REDIRECT or can it not? >> > >> > If we have exhaustive conformance tests we can confidently answer that >> > question. And the answer may not be "yes" or "no", it may actually be >> > "we need more options because many implementations fall in between". >> > >> > I think readable (IOW not written in some insane DSL) tests can also >> > be useful for users who want to check which features their program / >> > deployment will require. >> >> While I do agree that that kind of conformance test would be great, I >> don't think it has to hold up this series (the perfect being the enemy >> of the good, and all that). We have a real problem today that userspace >> can't tell if a given driver implements, say, XDP_REDIRECT, and so >> people try to use it and spend days wondering which black hole their >> packets disappear into. And for things like container migration we need >> to be able to predict whether a given host supports a feature *before* >> we start the migration and try to use it. > > Unless you have a strong definition of what XDP_REDIRECT means the flag > itself is not worth much. We're not talking about normal ethtool feature > flags which are primarily stack-driven, XDP is implemented mostly by > the driver, each vendor can do their own thing. Maybe I've seen one > vendor incompatibility too many at my day job to hope for the best... I'm totally on board with documenting what a feature means. E.g., for XDP_REDIRECT, whether it's acceptable to fail the redirect in some situations even when it's active, or if there should always be a slow-path fallback. But I disagree that the flag is worthless without it. People are running into real issues with trying to run XDP_REDIRECT programs on a driver that doesn't support it at all, and it's incredibly confusing. The latest example popped up literally yesterday: https://lore.kernel.org/xdp-newbies/CAM-scZPPeu44FeCPGO=Qz=03CrhhfB1GdJ8FNEpPqP_G27c6mQ at mail.gmail.com/ >> I view the feature flags as a list of features *implemented* by the >> driver. Which should be pretty static in a given kernel, but may be >> different than the features currently *enabled* on a given system (due >> to, e.g., the TX queue stuff). > > Hm, maybe I'm not being clear enough. The way XDP_REDIRECT (your > example) is implemented across drivers differs in a meaningful ways. > Hence the need for conformance testing. We don't have a golden SW > standard to fall back on, like we do with HW offloads. I'm not disagreeing that we need to harmonise what "implementing a feature" means. Maybe I'm just not sure what you mean by "conformance testing"? What would that look like, specifically? A script in selftest that sets up a redirect between two interfaces that we tell people to run? Or what? How would you catch, say, that issue where if a machine has more CPUs than the NIC has TXQs things start falling apart? > Also IDK why those tests are considered such a huge ask. As I said most > vendors probably already have them, and so I'd guess do good distros. > So let's work together. I guess what I'm afraid of is that this will end up delaying or stalling a fix for a long-standing issue (which is what I consider this series as shown by the example above). Maybe you can alleviate that by expanding a bit on what you mean? -Toke
next prev parent reply other threads:[~2021-02-10 22:54 UTC|newest] Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-04 10:28 [PATCH v2 bpf 0/5] New netdev feature flags for XDP alardam 2020-12-04 10:28 ` [Intel-wired-lan] " alardam 2020-12-04 10:28 ` [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set alardam 2020-12-04 10:28 ` [Intel-wired-lan] " alardam 2020-12-04 12:18 ` Toke Høiland-Jørgensen 2020-12-04 12:18 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-04 12:46 ` Maciej Fijalkowski 2020-12-04 12:46 ` [Intel-wired-lan] " Maciej Fijalkowski 2020-12-04 15:21 ` Daniel Borkmann 2020-12-04 15:21 ` [Intel-wired-lan] " Daniel Borkmann 2020-12-04 17:20 ` Toke Høiland-Jørgensen 2020-12-04 17:20 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-04 22:19 ` Daniel Borkmann 2020-12-04 22:19 ` [Intel-wired-lan] " Daniel Borkmann 2020-12-07 11:54 ` Jesper Dangaard Brouer 2020-12-07 11:54 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-07 12:08 ` Toke Høiland-Jørgensen 2020-12-07 12:08 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-07 12:03 ` Toke Høiland-Jørgensen 2020-12-07 12:03 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-07 12:54 ` Jesper Dangaard Brouer 2020-12-07 12:54 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-07 20:52 ` John Fastabend 2020-12-07 20:52 ` [Intel-wired-lan] " John Fastabend 2020-12-07 22:38 ` Saeed Mahameed 2020-12-07 22:38 ` [Intel-wired-lan] " Saeed Mahameed 2020-12-07 23:07 ` Maciej Fijalkowski 2020-12-07 23:07 ` [Intel-wired-lan] " Maciej Fijalkowski 2020-12-09 6:03 ` John Fastabend 2020-12-09 6:03 ` [Intel-wired-lan] " John Fastabend 2020-12-09 9:54 ` Maciej Fijalkowski 2020-12-09 9:54 ` [Intel-wired-lan] " Maciej Fijalkowski 2020-12-09 11:52 ` Jesper Dangaard Brouer 2020-12-09 11:52 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-09 15:41 ` David Ahern 2020-12-09 15:41 ` [Intel-wired-lan] " David Ahern 2020-12-09 17:15 ` Saeed Mahameed 2020-12-09 17:15 ` [Intel-wired-lan] " Saeed Mahameed 2020-12-10 3:34 ` David Ahern 2020-12-10 3:34 ` [Intel-wired-lan] " David Ahern 2020-12-10 6:48 ` Saeed Mahameed 2020-12-10 6:48 ` [Intel-wired-lan] " Saeed Mahameed 2020-12-10 15:30 ` David Ahern 2020-12-10 15:30 ` [Intel-wired-lan] " David Ahern 2020-12-10 18:58 ` Saeed Mahameed 2020-12-10 18:58 ` [Intel-wired-lan] " Saeed Mahameed 2021-01-05 11:56 ` Marek Majtyka 2021-01-05 11:56 ` [Intel-wired-lan] " Marek Majtyka 2021-02-01 16:16 ` Toke Høiland-Jørgensen 2021-02-01 16:16 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2021-02-02 11:26 ` Marek Majtyka 2021-02-02 11:26 ` [Intel-wired-lan] " Marek Majtyka 2021-02-02 12:05 ` Toke Høiland-Jørgensen 2021-02-02 12:05 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2021-02-02 19:34 ` Jakub Kicinski 2021-02-02 19:34 ` [Intel-wired-lan] " Jakub Kicinski 2021-02-03 12:50 ` Marek Majtyka 2021-02-03 12:50 ` [Intel-wired-lan] " Marek Majtyka 2021-02-03 17:02 ` Jakub Kicinski 2021-02-03 17:02 ` [Intel-wired-lan] " Jakub Kicinski 2021-02-10 10:53 ` Toke Høiland-Jørgensen 2021-02-10 10:53 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2021-02-10 18:31 ` Jakub Kicinski 2021-02-10 18:31 ` [Intel-wired-lan] " Jakub Kicinski 2021-02-10 22:52 ` Toke Høiland-Jørgensen [this message] 2021-02-10 22:52 ` Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2021-02-12 1:26 ` Jakub Kicinski 2021-02-12 1:26 ` [Intel-wired-lan] " Jakub Kicinski 2021-02-12 2:05 ` Alexei Starovoitov 2021-02-12 2:05 ` [Intel-wired-lan] " Alexei Starovoitov 2021-02-12 7:02 ` Marek Majtyka 2021-02-12 7:02 ` [Intel-wired-lan] " Marek Majtyka 2021-02-16 14:30 ` Toke Høiland-Jørgensen 2021-02-16 14:30 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-09 15:44 ` David Ahern 2020-12-09 15:44 ` [Intel-wired-lan] " David Ahern 2020-12-10 13:32 ` Explaining XDP redirect bulk size design (Was: [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set) Jesper Dangaard Brouer 2020-12-10 13:32 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-10 14:14 ` Magnus Karlsson 2020-12-10 14:14 ` Magnus Karlsson 2020-12-10 17:30 ` Jesper Dangaard Brouer 2020-12-10 17:30 ` Jesper Dangaard Brouer 2020-12-10 19:20 ` Saeed Mahameed 2020-12-10 19:20 ` [Intel-wired-lan] " Saeed Mahameed 2020-12-08 1:01 ` [PATCH v2 bpf 1/5] net: ethtool: add xdp properties flag set David Ahern 2020-12-08 1:01 ` [Intel-wired-lan] " David Ahern 2020-12-08 8:28 ` Jesper Dangaard Brouer 2020-12-08 8:28 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-08 11:58 ` Toke Høiland-Jørgensen 2020-12-08 11:58 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-09 5:50 ` John Fastabend 2020-12-09 5:50 ` [Intel-wired-lan] " John Fastabend 2020-12-09 10:26 ` Toke Høiland-Jørgensen 2020-12-09 10:26 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-08 9:00 ` Jesper Dangaard Brouer 2020-12-08 9:00 ` [Intel-wired-lan] " Jesper Dangaard Brouer 2020-12-08 9:42 ` Daniel Borkmann 2020-12-08 9:42 ` [Intel-wired-lan] " Daniel Borkmann 2020-12-04 12:57 ` Maciej Fijalkowski 2020-12-04 12:57 ` [Intel-wired-lan] " Maciej Fijalkowski 2020-12-04 10:28 ` [PATCH v2 bpf 2/5] drivers/net: turn XDP properties on alardam 2020-12-04 10:28 ` [Intel-wired-lan] " alardam 2020-12-04 12:19 ` Toke Høiland-Jørgensen 2020-12-04 12:19 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-09 19:05 ` kernel test robot 2020-12-09 19:05 ` kernel test robot 2020-12-04 10:28 ` [PATCH v2 bpf 3/5] xsk: add usage of xdp properties flags alardam 2020-12-04 10:28 ` [Intel-wired-lan] " alardam 2020-12-04 10:29 ` [PATCH v2 bpf 4/5] xsk: add check for full support of XDP in bind alardam 2020-12-04 10:29 ` [Intel-wired-lan] " alardam 2020-12-04 10:29 ` [PATCH v2 bpf 5/5] ethtool: provide xdp info with XDP_PROPERTIES_GET alardam 2020-12-04 10:29 ` [Intel-wired-lan] " alardam 2020-12-04 17:20 ` [PATCH v2 bpf 0/5] New netdev feature flags for XDP Jakub Kicinski 2020-12-04 17:20 ` [Intel-wired-lan] " Jakub Kicinski 2020-12-04 17:26 ` Toke Høiland-Jørgensen 2020-12-04 17:26 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?= 2020-12-04 19:22 ` Jakub Kicinski 2020-12-04 19:22 ` [Intel-wired-lan] " Jakub Kicinski 2020-12-07 12:04 ` Toke Høiland-Jørgensen 2020-12-07 12:04 ` [Intel-wired-lan] " Toke =?unknown-8bit?q?H=C3=B8iland-J=C3=B8rgensen?=
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=87czx7r0w8.fsf@toke.dk \ --to=toke@redhat.com \ --cc=alardam@gmail.com \ --cc=andrii.nakryiko@gmail.com \ --cc=ast@kernel.org \ --cc=bjorn.topel@intel.com \ --cc=bpf@vger.kernel.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=dsahern@gmail.com \ --cc=hawk@kernel.org \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jbrouer@redhat.com \ --cc=jeffrey.t.kirsher@intel.com \ --cc=john.fastabend@gmail.com \ --cc=jonathan.lemon@gmail.com \ --cc=kuba@kernel.org \ --cc=maciej.fijalkowski@intel.com \ --cc=maciejromanfijalkowski@gmail.com \ --cc=magnus.karlsson@intel.com \ --cc=netdev@vger.kernel.org \ --cc=saeed@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.