Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Anders Roxell]

On 25 August 2016 at 10:14, Maxim Uvarov  wrote:
> On 08/25/16 00:34, Anders Roxell wrote:
>
> +#include 

 > >why do we include an internal header file?
 > >
>>>
>>> >
>>> >because we need access to classifier bits, that is why this test is
>>> > under
>>> >linux-generic.
>>
>> OK, but do we really have to fiddle with internal classifier bits?
>>
>
> That is good question. For linux-generic it's bit's for other platform it
> might be something else.
> I would like to change input_flags_t (linux-generic internal) to something
> else which might be common
> for all platforms.

By definition if we want to extend something that will be useful for
all platforms
it should be in the public API.

> From other point of view if bit field will be changed in
> linux-generic we should not
> loose testing functionality. I'm not sure that all platforms support now
> pcap so that removing internal
> header might be not needed as this is internal platform test. When we will
> move to common then
> of course we need to remove internals. How about make it separate patch?

Yes if you include that first I'm all for it.

Anders

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Bill Fischofer]

On Thu, Aug 25, 2016 at 3:14 AM, Maxim Uvarov 
wrote:

> On 08/25/16 00:34, Anders Roxell wrote:
>
>> +#include 
>
 > >why do we include an internal header file?
 > >

>>> >
>>> >because we need access to classifier bits, that is why this test is
>>> under
>>> >linux-generic.
>>>
>> OK, but do we really have to fiddle with internal classifier bits?
>>
>>
> That is good question. For linux-generic it's bit's for other platform it
> might be something else.
> I would like to change input_flags_t (linux-generic internal) to something
> else which might be common
> for all platforms. From other point of view if bit field will be changed
> in linux-generic we should not
> loose testing functionality. I'm not sure that all platforms support now
> pcap so that removing internal
> header might be not needed as this is internal platform test. When we will
> move to common then
> of course we need to remove internals. How about make it separate patch?


As I recall, Matias reorganized those bits a couple of months back to get
some measurable performance gains, so I'd be cautious about making changes
just for 'tidiness' without measuring any impact


>
>
> Maxim.
>
>
>

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Maxim Uvarov]

On 08/25/16 00:34, Anders Roxell wrote:

+#include 

> >why do we include an internal header file?
> >

>
>because we need access to classifier bits, that is why this test is under
>linux-generic.

OK, but do we really have to fiddle with internal classifier bits?



That is good question. For linux-generic it's bit's for other platform 
it might be something else.
I would like to change input_flags_t (linux-generic internal) to 
something else which might be common
for all platforms. From other point of view if bit field will be changed 
in linux-generic we should not
loose testing functionality. I'm not sure that all platforms support now 
pcap so that removing internal
header might be not needed as this is internal platform test. When we 
will move to common then

of course we need to remove internals. How about make it separate patch?

Maxim.

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Anders Roxell]

On 2016-08-09 15:46, Maxim Uvarov wrote:
> On 08/06/16 22:30, Anders Roxell wrote:
> >--- a/test/linux-generic/m4/configure.m4
> >>+++ b/test/linux-generic/m4/configure.m4
> >>@@ -1,5 +1,6 @@
> >>  AC_CONFIG_FILES([test/linux-generic/Makefile
> >> test/linux-generic/validation/api/shmem/Makefile
> >> test/linux-generic/validation/api/pktio/Makefile
> >>+test/linux-generic/cls/Makefile
> here is validation group and all non validation tests are in alphabetic
> order bellow.

Basically, they are in some order. I see what you are trying to achieve.
Then we need some more patches to fix it.

Cheers,
Anders

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Anders Roxell]

On 2016-08-08 09:59, Maxim Uvarov wrote:
> Hello Anders,
> 
> thanks for review, there are 2 comments bellow. Will send v2.
> 
> Maxim.
> 
> On 08/06/16 22:30, Anders Roxell wrote:
> >On 2016-08-02 19:08, Maxim Uvarov wrote:
> >>add pcap play back test which takes 2 arguments: 1 - pcap file,
> >>2 - packet mask to match. Intend is to test odp with different
> >>input traffic to check internal implementation functions. In
> >>current case it's test for vlan tag instertion for packet mmap:
> >>pkt_mmap_vlan_insert().
> >>
> >>Signed-off-by: Maxim Uvarov 
> >>---
> >>  v2: up the patch, dirrect execution looks like works.
> >>
> >>  test/linux-generic/.gitignore   |   1 +
> >>  test/linux-generic/Makefile.am  |  10 +-
> >>  test/linux-generic/cls/.gitignore   |   3 +
> >>  test/linux-generic/cls/Makefile.am  |  13 ++
> >>  test/linux-generic/cls/cls.c| 349 
> >> 
> >>  test/linux-generic/cls/cls_main.c   |  12 ++
> >>  test/linux-generic/cls/cls_suites.h |   1 +
> >>  test/linux-generic/cls/vlan.pcap| Bin 0 -> 9728 bytes
> >>  test/linux-generic/cls/vlan_run.sh  |  49 +
> >>  test/linux-generic/m4/configure.m4  |   1 +
> >>  10 files changed, 435 insertions(+), 4 deletions(-)
> >>  create mode 100644 test/linux-generic/cls/.gitignore
> >>  create mode 100644 test/linux-generic/cls/Makefile.am
> >>  create mode 100644 test/linux-generic/cls/cls.c
> >>  create mode 100644 test/linux-generic/cls/cls_main.c
> >>  create mode 100644 test/linux-generic/cls/cls_suites.h
> >>  create mode 100644 test/linux-generic/cls/vlan.pcap
> >>  create mode 100755 test/linux-generic/cls/vlan_run.sh
> >>
> >>diff --git a/test/linux-generic/.gitignore b/test/linux-generic/.gitignore
> >>index 5dabf91..f65c7c1 100644
> >>--- a/test/linux-generic/.gitignore
> >>+++ b/test/linux-generic/.gitignore
> >>@@ -1,3 +1,4 @@
> >>  *.log
> >>  *.trs
> >>  tests-validation.env
> >>+test_out.pcap
> >>diff --git a/test/linux-generic/Makefile.am b/test/linux-generic/Makefile.am
> >>index f5cc52d..9acbab0 100644
> >>--- a/test/linux-generic/Makefile.am
> >>+++ b/test/linux-generic/Makefile.am
> >>@@ -1,5 +1,8 @@
> >>  include $(top_srcdir)/test/Makefile.inc
> >>  TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/common_plat/validation
> >>+TEST_EXTENSIONS = .sh
> >>+
> >>+dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
> >>  ALL_API_VALIDATION_DIR = ${top_builddir}/test/common_plat/validation/api
> >>@@ -37,11 +40,14 @@ TESTS = validation/api/pktio/pktio_run.sh \
> >>  SUBDIRS += validation/api/pktio\
> >>   validation/api/shmem\
> >>+  cls\
> >>   pktio_ipc\
> >>   ring
> >>  if HAVE_PCAP
> >>  TESTS += validation/api/pktio/pktio_run_pcap.sh
> >>+TESTS += cls/vlan_run.sh
> >>+dist_check_SCRIPTS += cls/vlan_run.sh cls/vlan.pcap
> >>  endif
> >>  if netmap_support
> >>  TESTS += validation/api/pktio/pktio_run_netmap.sh
> >>@@ -61,10 +67,6 @@ SUBDIRS += validation/api/pktio
> >>  endif
> >>  endif
> >>-TEST_EXTENSIONS = .sh
> >>-
> >>-dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
> >>-
> >>  test_SCRIPTS = $(dist_check_SCRIPTS)
> >>  tests-validation.env:
> >>diff --git a/test/linux-generic/cls/.gitignore 
> >>b/test/linux-generic/cls/.gitignore
> >>new file mode 100644
> >>index 000..9447625
> >>--- /dev/null
> >>+++ b/test/linux-generic/cls/.gitignore
> >>@@ -0,0 +1,3 @@
> >>+cls_main
> >>+*.log
> >>+*.trs
> >log and trs shouldn't be needed here, since its in
> >test/linux-generic/.gitignore.
> >
> >This isn't done in a common way anywhere... Maybe we need to look into
> >it?
> >
> >>diff --git a/test/linux-generic/cls/Makefile.am 
> >>b/test/linux-generic/cls/Makefile.am
> >>new file mode 100644
> >>index 000..43fb0bc
> >>--- /dev/null
> >>+++ b/test/linux-generic/cls/Makefile.am
> >>@@ -0,0 +1,13 @@
> >>+include ../Makefile.inc
> >>+
> >>+noinst_LTLIBRARIES = libtestcls.la
> >Why do we need to create a library?
> 
> we do it everywhere, I just follow existence Makefiles. As Christophe
> explained me it's because all tests
> will be combined to one library and later be executed.

OK.

> 
> >>+libtestcls_la_SOURCES = cls.c
> >>+libtestcls_la_CFLAGS = $(AM_CFLAGS) $(INCCUNIT_COMMON) $(INCODP)
> >>+
> >>+test_PROGRAMS = cls_main$(EXEEXT)
> >>+dist_cls_main_SOURCES = cls_main.c
> >>+
> >>+cls_main_LDFLAGS = $(AM_LDFLAGS)
> >>+cls_main_LDADD = libtestcls.la $(LIBCUNIT_COMMON) $(LIBODP)
> >>+
> >>+noinst_HEADERS = cls_suites.h
> >>diff --git a/test/linux-generic/cls/cls.c b/test/linux-generic/cls/cls.c
> >>new file mode 100644
> >>index 000..d527ec8
> >>--- /dev/null
> >>+++ b/test/linux-generic/cls/cls.c
> >>@@ -0,0 +1,349 @@
> >>+/* Copyright (c) 2016, Linaro Limited
> >>+ * All rights reserved.
> >>+ *
> >>+ * SPDX-License-Identifier: BSD-3-Clause
> >>+ */
> >>+
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+#include 
> >>+
> >>+#include 
> >>+
> >>+#include 
> >>+#include 
> 

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Maxim Uvarov]

Hello Anders,

do you agree with current order? My answer was bellow.

Thank you,
Maxim.


On 08/09/16 15:46, Maxim Uvarov wrote:

On 08/06/16 22:30, Anders Roxell wrote:

--- a/test/linux-generic/m4/configure.m4
>+++ b/test/linux-generic/m4/configure.m4
>@@ -1,5 +1,6 @@
>  AC_CONFIG_FILES([test/linux-generic/Makefile
>   test/linux-generic/validation/api/shmem/Makefile
>   test/linux-generic/validation/api/pktio/Makefile
>+ test/linux-generic/cls/Makefile
here is validation group and all non validation tests are in 
alphabetic order bellow.


Maxim.

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Maxim Uvarov]

On 08/06/16 22:30, Anders Roxell wrote:

--- a/test/linux-generic/m4/configure.m4
>+++ b/test/linux-generic/m4/configure.m4
>@@ -1,5 +1,6 @@
>  AC_CONFIG_FILES([test/linux-generic/Makefile
> test/linux-generic/validation/api/shmem/Makefile
> test/linux-generic/validation/api/pktio/Makefile
>+test/linux-generic/cls/Makefile
here is validation group and all non validation tests are in alphabetic 
order bellow.


Maxim.

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Maxim Uvarov]

Hello Anders,

thanks for review, there are 2 comments bellow. Will send v2.

Maxim.

On 08/06/16 22:30, Anders Roxell wrote:

On 2016-08-02 19:08, Maxim Uvarov wrote:

add pcap play back test which takes 2 arguments: 1 - pcap file,
2 - packet mask to match. Intend is to test odp with different
input traffic to check internal implementation functions. In
current case it's test for vlan tag instertion for packet mmap:
pkt_mmap_vlan_insert().

Signed-off-by: Maxim Uvarov 
---
  v2: up the patch, dirrect execution looks like works.

  test/linux-generic/.gitignore   |   1 +
  test/linux-generic/Makefile.am  |  10 +-
  test/linux-generic/cls/.gitignore   |   3 +
  test/linux-generic/cls/Makefile.am  |  13 ++
  test/linux-generic/cls/cls.c| 349 
  test/linux-generic/cls/cls_main.c   |  12 ++
  test/linux-generic/cls/cls_suites.h |   1 +
  test/linux-generic/cls/vlan.pcap| Bin 0 -> 9728 bytes
  test/linux-generic/cls/vlan_run.sh  |  49 +
  test/linux-generic/m4/configure.m4  |   1 +
  10 files changed, 435 insertions(+), 4 deletions(-)
  create mode 100644 test/linux-generic/cls/.gitignore
  create mode 100644 test/linux-generic/cls/Makefile.am
  create mode 100644 test/linux-generic/cls/cls.c
  create mode 100644 test/linux-generic/cls/cls_main.c
  create mode 100644 test/linux-generic/cls/cls_suites.h
  create mode 100644 test/linux-generic/cls/vlan.pcap
  create mode 100755 test/linux-generic/cls/vlan_run.sh

diff --git a/test/linux-generic/.gitignore b/test/linux-generic/.gitignore
index 5dabf91..f65c7c1 100644
--- a/test/linux-generic/.gitignore
+++ b/test/linux-generic/.gitignore
@@ -1,3 +1,4 @@
  *.log
  *.trs
  tests-validation.env
+test_out.pcap
diff --git a/test/linux-generic/Makefile.am b/test/linux-generic/Makefile.am
index f5cc52d..9acbab0 100644
--- a/test/linux-generic/Makefile.am
+++ b/test/linux-generic/Makefile.am
@@ -1,5 +1,8 @@
  include $(top_srcdir)/test/Makefile.inc
  TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/common_plat/validation
+TEST_EXTENSIONS = .sh
+
+dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
  
  ALL_API_VALIDATION_DIR = ${top_builddir}/test/common_plat/validation/api
  
@@ -37,11 +40,14 @@ TESTS = validation/api/pktio/pktio_run.sh \
  
  SUBDIRS += validation/api/pktio\

   validation/api/shmem\
+  cls\
   pktio_ipc\
   ring
  
  if HAVE_PCAP

  TESTS += validation/api/pktio/pktio_run_pcap.sh
+TESTS += cls/vlan_run.sh
+dist_check_SCRIPTS += cls/vlan_run.sh cls/vlan.pcap
  endif
  if netmap_support
  TESTS += validation/api/pktio/pktio_run_netmap.sh
@@ -61,10 +67,6 @@ SUBDIRS += validation/api/pktio
  endif
  endif
  
-TEST_EXTENSIONS = .sh

-
-dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
-
  test_SCRIPTS = $(dist_check_SCRIPTS)
  
  tests-validation.env:

diff --git a/test/linux-generic/cls/.gitignore 
b/test/linux-generic/cls/.gitignore
new file mode 100644
index 000..9447625
--- /dev/null
+++ b/test/linux-generic/cls/.gitignore
@@ -0,0 +1,3 @@
+cls_main
+*.log
+*.trs

log and trs shouldn't be needed here, since its in
test/linux-generic/.gitignore.

This isn't done in a common way anywhere... Maybe we need to look into
it?


diff --git a/test/linux-generic/cls/Makefile.am 
b/test/linux-generic/cls/Makefile.am
new file mode 100644
index 000..43fb0bc
--- /dev/null
+++ b/test/linux-generic/cls/Makefile.am
@@ -0,0 +1,13 @@
+include ../Makefile.inc
+
+noinst_LTLIBRARIES = libtestcls.la

Why do we need to create a library?


we do it everywhere, I just follow existence Makefiles. As Christophe 
explained me it's because all tests

will be combined to one library and later be executed.


+libtestcls_la_SOURCES = cls.c
+libtestcls_la_CFLAGS = $(AM_CFLAGS) $(INCCUNIT_COMMON) $(INCODP)
+
+test_PROGRAMS = cls_main$(EXEEXT)
+dist_cls_main_SOURCES = cls_main.c
+
+cls_main_LDFLAGS = $(AM_LDFLAGS)
+cls_main_LDADD = libtestcls.la $(LIBCUNIT_COMMON) $(LIBODP)
+
+noinst_HEADERS = cls_suites.h
diff --git a/test/linux-generic/cls/cls.c b/test/linux-generic/cls/cls.c
new file mode 100644
index 000..d527ec8
--- /dev/null
+++ b/test/linux-generic/cls/cls.c
@@ -0,0 +1,349 @@
+/* Copyright (c) 2016, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 

why do we include an internal header file?



because we need access to classifier bits, that is why this test is 
under linux-generic.



+
+#include "cls_suites.h"
+
+/** Get rid of path in filename - only for unix-type paths using '/' */
+#define NO_PATH(file_name) (strrchr((file_name), '/') ? \
+   strrchr((file_name), '/') + 1 : (file_name))

Maybe its enough basename?
http://linux.die.net/man/3/basename


yes, but again this define in almost each our test file. Probably 

Re: [lng-odp] [PATCHv2] test: linux-gen: add pcap playback test

[Bill Fischofer]

On Tue, Aug 2, 2016 at 11:08 AM, Maxim Uvarov 
wrote:

> add pcap play back test which takes 2 arguments: 1 - pcap file,
> 2 - packet mask to match. Intend is to test odp with different
> input traffic to check internal implementation functions. In
> current case it's test for vlan tag instertion for packet mmap:
> pkt_mmap_vlan_insert().
>
> Signed-off-by: Maxim Uvarov 
>

Reviewed-and-tested-by: Bill Fischofer 


> ---
>  v2: up the patch, dirrect execution looks like works.
>
>  test/linux-generic/.gitignore   |   1 +
>  test/linux-generic/Makefile.am  |  10 +-
>  test/linux-generic/cls/.gitignore   |   3 +
>  test/linux-generic/cls/Makefile.am  |  13 ++
>  test/linux-generic/cls/cls.c| 349 ++
> ++
>  test/linux-generic/cls/cls_main.c   |  12 ++
>  test/linux-generic/cls/cls_suites.h |   1 +
>  test/linux-generic/cls/vlan.pcap| Bin 0 -> 9728 bytes
>  test/linux-generic/cls/vlan_run.sh  |  49 +
>  test/linux-generic/m4/configure.m4  |   1 +
>  10 files changed, 435 insertions(+), 4 deletions(-)
>  create mode 100644 test/linux-generic/cls/.gitignore
>  create mode 100644 test/linux-generic/cls/Makefile.am
>  create mode 100644 test/linux-generic/cls/cls.c
>  create mode 100644 test/linux-generic/cls/cls_main.c
>  create mode 100644 test/linux-generic/cls/cls_suites.h
>  create mode 100644 test/linux-generic/cls/vlan.pcap
>  create mode 100755 test/linux-generic/cls/vlan_run.sh
>
> diff --git a/test/linux-generic/.gitignore b/test/linux-generic/.gitignore
> index 5dabf91..f65c7c1 100644
> --- a/test/linux-generic/.gitignore
> +++ b/test/linux-generic/.gitignore
> @@ -1,3 +1,4 @@
>  *.log
>  *.trs
>  tests-validation.env
> +test_out.pcap
> diff --git a/test/linux-generic/Makefile.am b/test/linux-generic/Makefile.
> am
> index f5cc52d..9acbab0 100644
> --- a/test/linux-generic/Makefile.am
> +++ b/test/linux-generic/Makefile.am
> @@ -1,5 +1,8 @@
>  include $(top_srcdir)/test/Makefile.inc
>  TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/common_plat/validation
> +TEST_EXTENSIONS = .sh
> +
> +dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
>
>  ALL_API_VALIDATION_DIR = ${top_builddir}/test/common_plat/validation/api
>
> @@ -37,11 +40,14 @@ TESTS = validation/api/pktio/pktio_run.sh \
>
>  SUBDIRS += validation/api/pktio\
>validation/api/shmem\
> +  cls\
>pktio_ipc\
>ring
>
>  if HAVE_PCAP
>  TESTS += validation/api/pktio/pktio_run_pcap.sh
> +TESTS += cls/vlan_run.sh
> +dist_check_SCRIPTS += cls/vlan_run.sh cls/vlan.pcap
>  endif
>  if netmap_support
>  TESTS += validation/api/pktio/pktio_run_netmap.sh
> @@ -61,10 +67,6 @@ SUBDIRS += validation/api/pktio
>  endif
>  endif
>
> -TEST_EXTENSIONS = .sh
> -
> -dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
> -
>  test_SCRIPTS = $(dist_check_SCRIPTS)
>
>  tests-validation.env:
> diff --git a/test/linux-generic/cls/.gitignore b/test/linux-generic/cls/.
> gitignore
> new file mode 100644
> index 000..9447625
> --- /dev/null
> +++ b/test/linux-generic/cls/.gitignore
> @@ -0,0 +1,3 @@
> +cls_main
> +*.log
> +*.trs
> diff --git a/test/linux-generic/cls/Makefile.am b/test/linux-generic/cls/
> Makefile.am
> new file mode 100644
> index 000..43fb0bc
> --- /dev/null
> +++ b/test/linux-generic/cls/Makefile.am
> @@ -0,0 +1,13 @@
> +include ../Makefile.inc
> +
> +noinst_LTLIBRARIES = libtestcls.la
> +libtestcls_la_SOURCES = cls.c
> +libtestcls_la_CFLAGS = $(AM_CFLAGS) $(INCCUNIT_COMMON) $(INCODP)
> +
> +test_PROGRAMS = cls_main$(EXEEXT)
> +dist_cls_main_SOURCES = cls_main.c
> +
> +cls_main_LDFLAGS = $(AM_LDFLAGS)
> +cls_main_LDADD = libtestcls.la $(LIBCUNIT_COMMON) $(LIBODP)
> +
> +noinst_HEADERS = cls_suites.h
> diff --git a/test/linux-generic/cls/cls.c b/test/linux-generic/cls/cls.c
> new file mode 100644
> index 000..d527ec8
> --- /dev/null
> +++ b/test/linux-generic/cls/cls.c
> @@ -0,0 +1,349 @@
> +/* Copyright (c) 2016, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "cls_suites.h"
> +
> +/** Get rid of path in filename - only for unix-type paths using '/' */
> +#define NO_PATH(file_name) (strrchr((file_name), '/') ? \
> +   strrchr((file_name), '/') + 1 : (file_name))
> +
> +/**
> + * Print usage information
> + */
> +static void usage(char *progname)
> +{
> +   printf("\n"
> +  "This is test application to verify that linux-generic
> classifier\n"
> +  "correctly classifies packets on input. Main intend is add
> more code\n"
> +  "coverage for internal functions playing different traffic
> recorded to\n"
> +  "pcap files."
> +  "\n"