diff mbox series

[v4,3/3] test/devargs: add devargs test cases

Message ID 20211020111230.2441949-4-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series devargs: support path in global syntax | expand

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Xueming Li Oct. 20, 2021, 11:12 a.m. UTC
Initial version to test global devargs syntax.

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 app/test/meson.build    |   5 ++
 app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+)
 create mode 100644 app/test/test_devargs.c

Comments

Gaëtan Rivet Oct. 20, 2021, 11:55 a.m. UTC | #1
On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> Initial version to test global devargs syntax.
>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  app/test/meson.build    |   5 ++
>  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+)
>  create mode 100644 app/test/test_devargs.c
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index a16374b7a10..c4b0241010d 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
>      fast_tests += [['latencystats_autotest', true]]
>      fast_tests += [['pdump_autotest', true]]
>  endif
> +if dpdk_conf.has('RTE_NET_VIRTIO')
> +    test_deps += 'net_virtio'
> +    test_sources += 'test_devargs.c'
> +    fast_tests += [['devargs_autotest', true]]
> +endif

Hi Steven,

Thanks for adding new use-cases and expanding the expect.
The dep check for the test can be improved I think.

When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.

Additionally, even if net_virtio is not built, a subset of your test remains valid.

Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.

Instead of checking the dependencies in the meson file, you could detect support in preamble
of the test:

bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
bool eth_class_avail = [...]

Then during the test case, depending on the expect part of list[i],
you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
message could be printed to say that the test was skipped.
Xueming Li Oct. 20, 2021, 1:53 p.m. UTC | #2
On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> > Initial version to test global devargs syntax.
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  app/test/meson.build    |   5 ++
> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 189 insertions(+)
> >  create mode 100644 app/test/test_devargs.c
> > 
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index a16374b7a10..c4b0241010d 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> >      fast_tests += [['latencystats_autotest', true]]
> >      fast_tests += [['pdump_autotest', true]]
> >  endif
> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> > +    test_deps += 'net_virtio'
> > +    test_sources += 'test_devargs.c'
> > +    fast_tests += [['devargs_autotest', true]]
> > +endif
> 
> Hi Steven,
> 
> Thanks for adding new use-cases and expanding the expect.
> The dep check for the test can be improved I think.
> 
> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
> 
> Additionally, even if net_virtio is not built, a subset of your test remains valid.
> 
> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
> 
> Instead of checking the dependencies in the meson file, you could detect support in preamble
> of the test:
> 
> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> bool eth_class_avail = [...]
> 
> Then during the test case, depending on the expect part of list[i],
> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> message could be printed to say that the test was skipped.
> 

For vdev supported drivers, currently no API to find it. Macro worked
for me:

#ifdef RTE_NET_VIRTIO
		{ "net_virtio_user0,iface=test,path=/dev/vhost-
net,queues=1",
		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
		{
"net_virtio_user0,iface=test,path=/class/bus/,queues=1",
		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
#endif

PCI, vdev and eth is enabled by default, seems we don't need any flag.
But macros can be used as well to be safe, how do you think?
Gaëtan Rivet Oct. 20, 2021, 2:22 p.m. UTC | #3
On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
>> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
>> > Initial version to test global devargs syntax.
>> > 
>> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>> > ---
>> >  app/test/meson.build    |   5 ++
>> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 189 insertions(+)
>> >  create mode 100644 app/test/test_devargs.c
>> > 
>> > diff --git a/app/test/meson.build b/app/test/meson.build
>> > index a16374b7a10..c4b0241010d 100644
>> > --- a/app/test/meson.build
>> > +++ b/app/test/meson.build
>> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
>> >      fast_tests += [['latencystats_autotest', true]]
>> >      fast_tests += [['pdump_autotest', true]]
>> >  endif
>> > +if dpdk_conf.has('RTE_NET_VIRTIO')
>> > +    test_deps += 'net_virtio'
>> > +    test_sources += 'test_devargs.c'
>> > +    fast_tests += [['devargs_autotest', true]]
>> > +endif
>> 
>> Hi Steven,
>> 
>> Thanks for adding new use-cases and expanding the expect.
>> The dep check for the test can be improved I think.
>> 
>> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
>> 
>> Additionally, even if net_virtio is not built, a subset of your test remains valid.
>> 
>> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
>> 
>> Instead of checking the dependencies in the meson file, you could detect support in preamble
>> of the test:
>> 
>> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
>> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
>> bool eth_class_avail = [...]
>> 
>> Then during the test case, depending on the expect part of list[i],
>> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
>> message could be printed to say that the test was skipped.
>> 
>
> For vdev supported drivers, currently no API to find it. Macro worked
> for me:
>
> #ifdef RTE_NET_VIRTIO
> 		{ "net_virtio_user0,iface=test,path=/dev/vhost-
> net,queues=1",
> 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> 		{
> "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> #endif
>
> PCI, vdev and eth is enabled by default, seems we don't need any flag.
> But macros can be used as well to be safe, how do you think?

With the macro you will go back to the same issue the meson-based dep check has:
the macro will be set, but it's possible the relevant .so won't be loaded when
executing the test.

For Busses and classes, similarly is there a chance that even though they are built,
they won't be loaded at runtime and make the test result incorrect?

What do you think about switching from net_virtio to net_ring instead otherwise?
It seems a lighter, utility driver that might be present even in minimal builds.
It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.

The virtio-specific path-based parameters are irrelevant there, the actual
driver probe won't be executed anyway so any parameter looking like a path will
test properly.

Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.
Bruce Richardson Oct. 20, 2021, 2:34 p.m. UTC | #4
On Wed, Oct 20, 2021 at 04:22:54PM +0200, Gaëtan Rivet wrote:
> 
> 
> On Wed, Oct 20, 2021, at 15:53, Xueming(Steven) Li wrote:
> > On Wed, 2021-10-20 at 13:55 +0200, Gaëtan Rivet wrote:
> >> On Wed, Oct 20, 2021, at 13:12, Xueming Li wrote:
> >> > Initial version to test global devargs syntax.
> >> > 
> >> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> >> > ---
> >> >  app/test/meson.build    |   5 ++
> >> >  app/test/test_devargs.c | 184 ++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 189 insertions(+)
> >> >  create mode 100644 app/test/test_devargs.c
> >> > 
> >> > diff --git a/app/test/meson.build b/app/test/meson.build
> >> > index a16374b7a10..c4b0241010d 100644
> >> > --- a/app/test/meson.build
> >> > +++ b/app/test/meson.build
> >> > @@ -399,6 +399,11 @@ if dpdk_conf.has('RTE_NET_RING')
> >> >      fast_tests += [['latencystats_autotest', true]]
> >> >      fast_tests += [['pdump_autotest', true]]
> >> >  endif
> >> > +if dpdk_conf.has('RTE_NET_VIRTIO')
> >> > +    test_deps += 'net_virtio'
> >> > +    test_sources += 'test_devargs.c'
> >> > +    fast_tests += [['devargs_autotest', true]]
> >> > +endif
> >> 
> >> Hi Steven,
> >> 
> >> Thanks for adding new use-cases and expanding the expect.
> >> The dep check for the test can be improved I think.
> >> 
> >> When the build is shared, not all built drivers will be available, even if part of the meson config. It might generate false negative when running your test.
> >> 
> >> Additionally, even if net_virtio is not built, a subset of your test remains valid.
> >> 
> >> Finally, net_virtio might not be generic enough as a driver. A simpler one could be used, such as net_ring maybe.
> >> 
> >> Instead of checking the dependencies in the meson file, you could detect support in preamble
> >> of the test:
> >> 
> >> bool pci_bus_avail = (rte_bus_find_by_name("pci") != NULL);
> >> bool vdev_bus_avail = (rte_bus_find_by_name("vdev") != NULL);
> >> bool eth_class_avail = [...]
> >> 
> >> Then during the test case, depending on the expect part of list[i],
> >> you can skip the case if its deps were not found. In that case, am INFO or DEBUG level
> >> message could be printed to say that the test was skipped.
> >> 
> >
> > For vdev supported drivers, currently no API to find it. Macro worked
> > for me:
> >
> > #ifdef RTE_NET_VIRTIO
> > 		{ "net_virtio_user0,iface=test,path=/dev/vhost-
> > net,queues=1",
> > 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > 		{
> > "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
> > 		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
> > #endif
> >
> > PCI, vdev and eth is enabled by default, seems we don't need any flag.
> > But macros can be used as well to be safe, how do you think?
> 
> With the macro you will go back to the same issue the meson-based dep check has:
> the macro will be set, but it's possible the relevant .so won't be loaded when
> executing the test.
> 
> For Busses and classes, similarly is there a chance that even though they are built,
> they won't be loaded at runtime and make the test result incorrect?
> 
> What do you think about switching from net_virtio to net_ring instead otherwise?
> It seems a lighter, utility driver that might be present even in minimal builds.
> It is used in EAL tests, I think it makes more sense than net_virtio for unit tests.
> 
> The virtio-specific path-based parameters are irrelevant there, the actual
> driver probe won't be executed anyway so any parameter looking like a path will
> test properly.
> 
> Checkout app/test/test_eal_flags.c for examples of net_ring dummy devargs.
> 
I'd also recommend making use of the "TEST_SKIPPED" return value rather
than erroring out if a necessary dependency can't be found.
diff mbox series

Patch

diff --git a/app/test/meson.build b/app/test/meson.build
index a16374b7a10..c4b0241010d 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -399,6 +399,11 @@  if dpdk_conf.has('RTE_NET_RING')
     fast_tests += [['latencystats_autotest', true]]
     fast_tests += [['pdump_autotest', true]]
 endif
+if dpdk_conf.has('RTE_NET_VIRTIO')
+    test_deps += 'net_virtio'
+    test_sources += 'test_devargs.c'
+    fast_tests += [['devargs_autotest', true]]
+endif
 
 if dpdk_conf.has('RTE_LIB_POWER')
     test_deps += 'power'
diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
new file mode 100644
index 00000000000..13e95f052b0
--- /dev/null
+++ b/app/test/test_devargs.c
@@ -0,0 +1,184 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 NVIDIA Corporation & Affiliates
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_devargs.h>
+#include <rte_kvargs.h>
+#include <rte_bus.h>
+#include <rte_class.h>
+
+#include "test.h"
+
+/* Check layer arguments. */
+static int
+test_args(const char *devargs, const char *layer, const char *args, const int n)
+{
+	struct rte_kvargs *kvlist;
+
+	if (n == 0) {
+		if (args != NULL && strlen(args) > 0) {
+			printf("rte_devargs_parse(%s) %s args parsed (not expected)\n",
+			       devargs, layer);
+			return -1;
+		} else {
+			return 0;
+		}
+	}
+	if (args == NULL) {
+		printf("rte_devargs_parse(%s) %s args not parsed\n",
+		       devargs, layer);
+		return -1;
+	}
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_devargs_parse(%s) %s_str: %s not parsed\n",
+		       devargs, layer, args);
+		return -1;
+	}
+	if ((int)kvlist->count != n) {
+		printf("rte_devargs_parse(%s) %s_str: %s kv number %u, not %d\n",
+		       devargs, layer, args, kvlist->count, n);
+		return -1;
+	}
+	return 0;
+}
+
+/* Test several valid cases */
+static int
+test_valid_devargs(void)
+{
+	static const struct {
+		const char *devargs;
+		int bus_kv;
+		int class_kv;
+		int driver_kv;
+		const char *bus;
+		const char *name;
+		const char *class;
+	} list[] = {
+		/* Global devargs syntax: */
+		{ "bus=pci",
+		  1, 0, 0, "pci", NULL, NULL},
+		{ "class=eth",
+		  0, 1, 0, NULL, NULL, "eth" },
+		{ "bus=pci,addr=1:2.3/class=eth/driver=abc,k0=v0",
+		  2, 1, 2, "pci", "0000:01:02.3", "eth" },
+		{ "bus=vdev,name=/dev/file/name/class=eth",
+		  2, 1, 0, "vdev", "/dev/file/name", "eth" },
+		{ "bus=vdev,name=/class/bus/path/class=eth",
+		  2, 1, 0, "vdev", "/class/bus/path", "eth" },
+		{ "bus=vdev,name=///dblslsh/class=eth",
+		  2, 1, 0, "vdev", "///dblslsh", "eth" },
+		/* Legacy devargs syntax: */
+		{ "1:2.3", 0, 0, 0,
+		  "pci", "1:2.3", NULL },
+		{ "pci:1:2.3,k0=v0",
+		  0, 0, 1, "pci", "1:2.3", NULL },
+		{ "net_virtio_user0,iface=test,path=/dev/vhost-net,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+		{ "net_virtio_user0,iface=test,path=/class/bus/,queues=1",
+		  0, 0, 3, "vdev", "net_virtio_user0", NULL },
+	};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		memset(&da, 0, sizeof(da));
+		ret = rte_devargs_parse(&da, list[i].devargs);
+		if (ret < 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i].devargs, ret);
+			goto fail;
+		}
+		if ((list[i].bus_kv > 0 || list[i].bus != NULL) &&
+		    da.bus == NULL) {
+			printf("rte_devargs_parse(%s) bus not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "bus", da.bus_str,
+			      list[i].bus_kv) != 0)
+			goto fail;
+		if (list[i].bus != NULL &&
+		    strcmp(da.bus->name, list[i].bus) != 0) {
+			printf("rte_devargs_parse(%s) bus name (%s) not expected (%s)\n",
+			       list[i].devargs, da.bus->name, list[i].bus);
+			goto fail;
+		}
+		if ((list[i].class_kv > 0 || list[i].class != NULL) &&
+		    da.cls == NULL) {
+			printf("rte_devargs_parse(%s) class not parsed\n",
+			       list[i].devargs);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "class", da.cls_str,
+			      list[i].class_kv) != 0)
+			goto fail;
+		if (list[i].class != NULL &&
+		    strcmp(da.cls->name, list[i].class) != 0) {
+			printf("rte_devargs_parse(%s) class name (%s) not expected (%s)\n",
+			       list[i].devargs, da.cls->name, list[i].class);
+			goto fail;
+		}
+		if (test_args(list[i].devargs, "driver", da.drv_str,
+			      list[i].driver_kv) != 0)
+			goto fail;
+		if (list[i].name != NULL &&
+		    strcmp(da.name, list[i].name) != 0) {
+			printf("rte_devargs_parse(%s) device name (%s) not expected (%s)\n",
+			       list[i].devargs, da.name, list[i].name);
+			goto fail;
+		}
+		goto cleanup;
+fail:
+		fail = -1;
+cleanup:
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+/* Test several invalid cases */
+static int
+test_invalid_devargs(void)
+{
+	static const char * const list[] = {
+		"bus=wrong-bus",
+		"class=wrong-class"};
+	struct rte_devargs da;
+	uint32_t i;
+	int ret;
+	int fail = 0;
+
+	for (i = 0; i < RTE_DIM(list); i++) {
+		ret = rte_devargs_parse(&da, list[i]);
+		if (ret >= 0) {
+			printf("rte_devargs_parse(%s) returned %d (but should not)\n",
+			       list[i], ret);
+			fail = ret;
+		}
+		rte_devargs_reset(&da);
+	}
+	return fail;
+}
+
+static int
+test_devargs(void)
+{
+	printf("== test valid case ==\n");
+	if (test_valid_devargs() < 0)
+		return -1;
+	printf("== test invalid case ==\n");
+	if (test_invalid_devargs() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(devargs_autotest, test_devargs);