test/service: add perf test for service on app lcore

  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: http://patches.dpdk.org/patch/69199/ )
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(-)
  

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%2F69199%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.


Best regards

Lukasz
  

> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Monday, May 4, 2020 9:50 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: honnappa.nagarahalli@arm.com; phil.yang@arm.com
> Subject: Re: [dpdk-dev] [PATCH] test/service: add perf test for service on app
> lcore

<snip code>

> > +	unregister_all();
> >   	return TEST_SUCCESS;
> >   }
> >
> 
> Hi Harry,

Hey Lukasz,

> I like the idea of adding this test. I checked it and it works fine.

Thanks for testing - please send a "Tested-by: .. <email>" and we can track
your input into the git logs :)

> However have you considered adding it as a separate testcase or even
> better as "service_perf_autotest" command ?

Actually I started implementing a new function with a very similar name,
but realized that the configuration/mapping would be duplicated, only to
have a different named test... so opted for this smaller diff solution.

> 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.

Correct - if you feel strongly that this isn't right I can refactor to a v2,
however I expect there would be more duplicated code to manage, and
hence I decided the above approach was simpler and easier.

> Best regards
> Lukasz

Regards, -Harry
  

W dniu 05.05.2020 o 12:21, Van Haaren, Harry pisze:
>> -----Original Message-----
>> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> Sent: Monday, May 4, 2020 9:50 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
>> Cc: honnappa.nagarahalli@arm.com; phil.yang@arm.com
>> Subject: Re: [dpdk-dev] [PATCH] test/service: add perf test for service on app
>> lcore
> <snip code>
>
>>> +	unregister_all();
>>>    	return TEST_SUCCESS;
>>>    }
>>>
>> Hi Harry,
> Hey Lukasz,
>
>> I like the idea of adding this test. I checked it and it works fine.
> Thanks for testing - please send a "Tested-by: .. <email>" and we can track
> your input into the git logs :)

No problem, I just wanted to get you opinion first, but here you go:

Tested-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

>
>> However have you considered adding it as a separate testcase or even
>> better as "service_perf_autotest" command ?
> Actually I started implementing a new function with a very similar name,
> but realized that the configuration/mapping would be duplicated, only to
> have a different named test... so opted for this smaller diff solution.
>
>> 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.
> Correct - if you feel strongly that this isn't right I can refactor to a v2,
> however I expect there would be more duplicated code to manage, and
> hence I decided the above approach was simpler and easier.
It's up to you and some more dpdk-experienced guys. Isn't there a way 
not to duplicate the code but wrap it in a function?
You don't really need that much, because you run the performance test 
after the former version of test is completed. You don't need 2 lcores 
also as you measure direct start of service function, which also is 
almost empty as it immediately returns because the loop breaking 
conditions are already set.
>
>> Best regards
>> Lukasz
> Regards, -Harry
  

> -----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
  

On Fri, May 1, 2020 at 5:56 PM Harry van Haaren
<harry.van.haaren@intel.com> wrote:
>
> 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: http://patches.dpdk.org/patch/69199/ )
> 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++) {

- How long does this test take now?
We tend to put performance tests in dedicated tests to avoid issues in Travis.
I suppose this is quick, but still want a confirmation.


- Centos7/RHEL7 gcc is not happy with this.

http://mails.dpdk.org/archives/test-report/2020-May/129993.html

../app/test/test_service_cores.c: In function ‘service_app_lcore_poll_impl’:
../app/test/test_service_cores.c:795:2: error: ‘for’ loop initial
declarations are only allowed in C99 mode
  for (uint32_t i = 0; i < perf_iters; i++) {
  ^
../app/test/test_service_cores.c:795:2: note: use option -std=c99 or
-std=gnu99 to compile your code


> +               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;
>  }
>

- Can you look at Phil comments too?
  

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, May 6, 2020 4:44 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev <dev@dpdk.org>; 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
<snip>
> > +       /* 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++) {
> 
> - How long does this test take now?
> We tend to put performance tests in dedicated tests to avoid issues in Travis.
> I suppose this is quick, but still want a confirmation.

Can confirm this is quick, blink of an eye quick, maybe ~250k cycles.


> - Centos7/RHEL7 gcc is not happy with this.
> 
> http://mails.dpdk.org/archives/test-report/2020-May/129993.html
> 
> ../app/test/test_service_cores.c: In function ‘service_app_lcore_poll_impl’:
> ../app/test/test_service_cores.c:795:2: error: ‘for’ loop initial
> declarations are only allowed in C99 mode
>   for (uint32_t i = 0; i < perf_iters; i++) {
>   ^
> ../app/test/test_service_cores.c:795:2: note: use option -std=c99 or
> -std=gnu99 to compile your code

Will fix in v2.


> > +               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;
> >  }
> >
> 
> - Can you look at Phil comments too?

Yes - will update to use padding at start of line in output.

V2 on the way.