| Message ID | 20240930171753.2572922-10-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: ncdevmem: Add ncdevmem to ksft | expand |
On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > Use single last queue of the device and probe it dynamically. > Sorry I know there was a pending discussion in the last iteration that I didn't respond to. Been a rough week with me out sick a bit. For this, the issue I see is that by default only 1 queue binding will be tested, but I feel like test coverage for the multiple queues case by default is very nice because I actually ran into some issues making multi-queue binding work. Can we change this so that, by default, it binds to the last rxq_num/2 queues of the device? > Cc: Mina Almasry <almasrymina@google.com> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> > --- > tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c > index a1fa818c8229..900a661a61af 100644 > --- a/tools/testing/selftests/net/ncdevmem.c > +++ b/tools/testing/selftests/net/ncdevmem.c > @@ -48,8 +48,8 @@ static char *server_ip; > static char *client_ip; > static char *port; > static size_t do_validation; > -static int start_queue = 8; > -static int num_queues = 8; > +static int start_queue = -1; > +static int num_queues = 1; > static char *ifname; > static unsigned int ifindex; > static unsigned int dmabuf_id; > @@ -198,6 +198,33 @@ void validate_buffer(void *line, size_t size) > fprintf(stdout, "Validated buffer\n"); > } > > +static int rxq_num(int ifindex) > +{ > + struct ethtool_channels_get_req *req; > + struct ethtool_channels_get_rsp *rsp; > + struct ynl_error yerr; > + struct ynl_sock *ys; > + int num = -1; > + > + ys = ynl_sock_create(&ynl_ethtool_family, &yerr); > + if (!ys) { > + fprintf(stderr, "YNL: %s\n", yerr.msg); > + return -1; > + } > + > + req = ethtool_channels_get_req_alloc(); > + ethtool_channels_get_req_set_header_dev_index(req, ifindex); > + rsp = ethtool_channels_get(ys, req); > + if (rsp) > + num = rsp->rx_count + rsp->combined_count; > + ethtool_channels_get_req_free(req); > + ethtool_channels_get_rsp_free(rsp); > + > + ynl_sock_destroy(ys); > + > + return num; > +} > + > #define run_command(cmd, ...) \ > ({ \ > char command[256]; \ > @@ -672,6 +699,15 @@ int main(int argc, char *argv[]) > > ifindex = if_nametoindex(ifname); > > + if (start_queue < 0) { > + start_queue = rxq_num(ifindex) - 1; I think the only changes needed are: start_queue = rxq_num(ifindex) / 2; num_queues = rxq_num(ifindex) - start_queue; (I may have an off-by-1 error somewhere with this math).
On 10/03, Mina Almasry wrote: > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > Use single last queue of the device and probe it dynamically. > > > > Sorry I know there was a pending discussion in the last iteration that > I didn't respond to. Been a rough week with me out sick a bit. > > For this, the issue I see is that by default only 1 queue binding will > be tested, but I feel like test coverage for the multiple queues case > by default is very nice because I actually ran into some issues making > multi-queue binding work. > > Can we change this so that, by default, it binds to the last rxq_num/2 > queues of the device? I'm probably missing something, but why do you think exercising this from the probe/selftest mode is not enough? It might be confusing for the readers to understand why we bind to half of the queues and flow steer into them when in reality there is only single tcp flow. IOW, can we keep these two modes: 1. server / client - use single queue 2. selftest / probe - use more than 1 queue by default (and I'll remove the checks that enforce the number of queues for this mode to let the users override)
On Thu, Oct 3, 2024 at 10:02 AM Stanislav Fomichev <stfomichev@gmail.com> wrote: > > On 10/03, Mina Almasry wrote: > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > Use single last queue of the device and probe it dynamically. > > > > > > > Sorry I know there was a pending discussion in the last iteration that > > I didn't respond to. Been a rough week with me out sick a bit. > > > > For this, the issue I see is that by default only 1 queue binding will > > be tested, but I feel like test coverage for the multiple queues case > > by default is very nice because I actually ran into some issues making > > multi-queue binding work. > > > > Can we change this so that, by default, it binds to the last rxq_num/2 > > queues of the device? > > I'm probably missing something, but why do you think exercising this from > the probe/selftest mode is not enough? It might be confusing for the readers > to understand why we bind to half of the queues and flow steer into them > when in reality there is only single tcp flow. > > IOW, can we keep these two modes: > 1. server / client - use single queue > 2. selftest / probe - use more than 1 queue by default (and I'll remove the > checks that enforce the number of queues for this mode to let the > users override) Ah, I see. Thanks for the explanation. My paranoia here is that we don't notice multi-queue binding regressions because the tests are often run in data path mode and we don't use or notice failures in the probe mode. I will concede my paranoia is just that and this is not very likely to happen, but also if it is confusing to bind multi-queues and then just use one, then we could remedy that with a comment and keep the accidental test coverage. It also makes the test simpler to always bind the same # of queues rather than special case data and control path tests. But your 2 mode approach sounds fine as well. But to implement that you need more than to remove the checks that enforce the number of queues, right? In probe mode num_queues should be rxq_num/2, and in server mode num_queues should be 1, yes?
On 10/03, Mina Almasry wrote: > On Thu, Oct 3, 2024 at 10:02 AM Stanislav Fomichev <stfomichev@gmail.com> wrote: > > > > On 10/03, Mina Almasry wrote: > > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > > > Use single last queue of the device and probe it dynamically. > > > > > > > > > > Sorry I know there was a pending discussion in the last iteration that > > > I didn't respond to. Been a rough week with me out sick a bit. > > > > > > For this, the issue I see is that by default only 1 queue binding will > > > be tested, but I feel like test coverage for the multiple queues case > > > by default is very nice because I actually ran into some issues making > > > multi-queue binding work. > > > > > > Can we change this so that, by default, it binds to the last rxq_num/2 > > > queues of the device? > > > > I'm probably missing something, but why do you think exercising this from > > the probe/selftest mode is not enough? It might be confusing for the readers > > to understand why we bind to half of the queues and flow steer into them > > when in reality there is only single tcp flow. > > > > IOW, can we keep these two modes: > > 1. server / client - use single queue > > 2. selftest / probe - use more than 1 queue by default (and I'll remove the > > checks that enforce the number of queues for this mode to let the > > users override) > > Ah, I see. Thanks for the explanation. > > My paranoia here is that we don't notice multi-queue binding > regressions because the tests are often run in data path mode and we > don't use or notice failures in the probe mode. > > I will concede my paranoia is just that and this is not very likely to > happen, but also if it is confusing to bind multi-queues and then just > use one, then we could remedy that with a comment and keep the > accidental test coverage. It also makes the test simpler to always > bind the same # of queues rather than special case data and control > path tests. > > But your 2 mode approach sounds fine as well. But to implement that > you need more than to remove the checks that enforce the number of > queues, right? In probe mode num_queues should be rxq_num/2, and in > server mode num_queues should be 1, yes? Yes, I'll follow your suggestion with `start_queues = num_queues / 2` for the selftest part. Tentatively (default to 1/2 queues, if want to override - provide both -t an -q): index 90aacfb3433f..3a456c058241 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -64,7 +64,7 @@ static char *client_ip; static char *port; static size_t do_validation; static int start_queue = -1; -static int num_queues = 1; +static int num_queues = -1; static char *ifname; static unsigned int ifindex; static unsigned int dmabuf_id; @@ -706,19 +706,31 @@ int main(int argc, char *argv[]) } } - if (!server_ip) - error(1, 0, "Missing -s argument\n"); - - if (!port) - error(1, 0, "Missing -p argument\n"); - if (!ifname) error(1, 0, "Missing -f argument\n"); ifindex = if_nametoindex(ifname); - if (start_queue < 0) { - start_queue = rxq_num(ifindex) - 1; + if (!server_ip && !client_ip) { + if (start_queue < 0 && num_queues < 0) { + num_queues = rxq_num(ifindex); + if (num_queues < 0) + error(1, 0, "couldn't detect number of queues\n"); + /* make sure can bind to multiple queues */ + start_queues = num_queues / 2; + num_queues /= 2; + } + + if (start_queue < 0 || num_queues < 0) + error(1, 0, "Both -t and -q are requred\n"); + + run_devmem_tests(); + return 0; + } + + if (start_queue < 0 && num_queues < 0) { + num_queues = 1; + start_queue = rxq_num(ifindex) - num_queues;
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c index a1fa818c8229..900a661a61af 100644 --- a/tools/testing/selftests/net/ncdevmem.c +++ b/tools/testing/selftests/net/ncdevmem.c @@ -48,8 +48,8 @@ static char *server_ip; static char *client_ip; static char *port; static size_t do_validation; -static int start_queue = 8; -static int num_queues = 8; +static int start_queue = -1; +static int num_queues = 1; static char *ifname; static unsigned int ifindex; static unsigned int dmabuf_id; @@ -198,6 +198,33 @@ void validate_buffer(void *line, size_t size) fprintf(stdout, "Validated buffer\n"); } +static int rxq_num(int ifindex) +{ + struct ethtool_channels_get_req *req; + struct ethtool_channels_get_rsp *rsp; + struct ynl_error yerr; + struct ynl_sock *ys; + int num = -1; + + ys = ynl_sock_create(&ynl_ethtool_family, &yerr); + if (!ys) { + fprintf(stderr, "YNL: %s\n", yerr.msg); + return -1; + } + + req = ethtool_channels_get_req_alloc(); + ethtool_channels_get_req_set_header_dev_index(req, ifindex); + rsp = ethtool_channels_get(ys, req); + if (rsp) + num = rsp->rx_count + rsp->combined_count; + ethtool_channels_get_req_free(req); + ethtool_channels_get_rsp_free(rsp); + + ynl_sock_destroy(ys); + + return num; +} + #define run_command(cmd, ...) \ ({ \ char command[256]; \ @@ -672,6 +699,15 @@ int main(int argc, char *argv[]) ifindex = if_nametoindex(ifname); + if (start_queue < 0) { + start_queue = rxq_num(ifindex) - 1; + + if (start_queue < 0) + error(1, 0, "couldn't detect number of queues\n"); + + fprintf(stderr, "using queues %d..%d\n", start_queue, start_queue + num_queues); + } + for (; optind < argc; optind++) fprintf(stderr, "extra arguments: %s\n", argv[optind]);
Use single last queue of the device and probe it dynamically. Cc: Mina Almasry <almasrymina@google.com> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)