Re: [dpdk-dev] [PATCH] test/service: add perf test for service on app lcore

[Phil Yang <Phil.Yang@arm.com>]

> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Tuesday, May 5, 2020 4:50 AM
> To: Harry van Haaren <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Phil Yang
> <Phil.Yang@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] test/service: add perf test for service on
> app lcore
> 
> 
> W dniu 01.05.2020 o 17:56, Harry van Haaren pisze:
> > Add a performance test to the service run on app lcore auto-
> > test. This test runs the service in a tight loop, and measures
> > cycles passed, printing the results. It provides a quick cycle
> > cost value, enabling measuring performance of the function to
> > run a service on an application lcore.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > I'm suggesting to merge this patch before the bugfix/C11 patch series,
> > (v2 currently here: https://protect2.fireeye.com/url?k=fda15556-
> a06d9cd2-fda0de19-0cc47aa8f5ba-
> 177ac65d20682aa8&q=1&u=http%3A%2F%2Fpatches.dpdk.org%2Fpatch%2F
> 69199%2F )
> > as this would enable users to benchmark the "before" and "after"
> > states of the bugfix/C11 patches easier.
> >
> > ---
> >   app/test/test_service_cores.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c
> > index a922c7ddc..469243314 100644
> > --- a/app/test/test_service_cores.c
> > +++ b/app/test/test_service_cores.c
> > @@ -789,8 +789,18 @@ service_app_lcore_poll_impl(const int mt_safe)
> >   				"MT Unsafe: App core1 didn't return -
> EBUSY");
> >   	}
> >
> > -	unregister_all();
> > +	/* Performance test: call in a loop, and measure tsc() */
> > +	const uint32_t perf_iters = (1 << 12);
> > +	uint64_t start = rte_rdtsc();
> > +	for (uint32_t i = 0; i < perf_iters; i++) {
> > +		int err = service_run_on_app_core_func(&id);
> > +		TEST_ASSERT_EQUAL(0, err, "perf test: returned run failure");
> > +	}
> > +	uint64_t end = rte_rdtsc();
> > +	printf("perf test for %s: %0.1f cycles per call\n", mt_safe ?
> > +		"MT Safe" : "MT Unsafe", (end - start)/(float)perf_iters);
> >
> > +	unregister_all();
> >   	return TEST_SUCCESS;
> >   }
> >
> 
> Hi Harry,
> 
> 
> I like the idea of adding this test. I checked it and it works fine.
> However have you considered adding it as a separate testcase or even
> better as "service_perf_autotest" command ?
> 
> With your changes the: service_app_lcore_mt_safe and
> service_app_lcore_mt_unsafe unit tests cases have multiple
> functionalities: they test simultaneous execution of service and they do
> performance checks.

+1 for this.

This patch will skip MT safe UT, but it will continue the MT safe performance test.   It's a defect. E.g:
-------
+ TestCase [12] : service_mt_safe_poll skipped
perf test for MT Safe: 40.2 cycles per call
 + TestCase [13] : service_app_lcore_mt_safe succeeded
perf test for MT Unsafe: 53.7 cycles per call
--------

If you want to put the performance test and functional test in the same test, I think it is better to add some indents before print the performance test output to align with the functional test output format.  Such as:
------
+ TestCase [13] : service_app_lcore_mt_safe succeeded
+		  perf test for MT Unsafe: 53.7 cycles per call
------


According to this performance case, the C11 version patches got 20% performance improvement on aarch64 and 8.5% on x86 for the MT unsafe case. In MT safe case, it got 10% performance improvement on aarch64 and 17% on x86. These are preliminary test results, only covered one testbed for each platform.

Thanks,
Phil