Ivan, On 18 August 2015 at 21:15, Ivan Khoronzhuk <ivan.khoronz...@linaro.org> wrote:
> Bala, > > On 18.08.15 18:16, Bala Manoharan wrote: > >> Hi Ivan, >> >> On 18 August 2015 at 19:22, Ivan Khoronzhuk <ivan.khoronz...@linaro.org >> <mailto:ivan.khoronz...@linaro.org>> wrote: >> >> Hi, Bala >> >> Note: Your patch is based on API-NEXT and it obliged me to do some >> modifications >> with odp_pktio_param_t before testing. Also I'm still not sure about >> using >> odp_pmr_terms_cap(), but maybe it's OK to simply fail. >> > > I understand that you want to simly fail test, but on my opinion > that's slightly different result. It be good to print some msg like > PMR is not supported...I'm printing the msg like Unsupported term, but > it's not printed in the summary. It's just slightly different result and > it worries me a little. > > Okay. We can discuss this further in arch call. > >> Short pretesting review farther below >> >> >> >> On 12.08.15 11:53, Balasubramanian Manoharan wrote: >> >> Additional test suite is added to classification validation suite >> to test >> individual PMRs. This suite will test the defined PMRs by >> configuring >> pktio separately for every test case. >> >> Fixes: >> https://bugs.linaro.org/show_bug.cgi?id=1542 >> https://bugs.linaro.org/show_bug.cgi?id=1544 >> https://bugs.linaro.org/show_bug.cgi?id=1545 >> https://bugs.linaro.org/show_bug.cgi?id=1546 >> >> Signed-off-by: Balasubramanian Manoharan < >> bala.manoha...@linaro.org <mailto:bala.manoha...@linaro.org>> >> >> --- >> v2: Incorporates review comments from Ivan and Christophe >> >> helper/include/odp/helper/tcp.h | 4 + >> test/validation/classification/Makefile.am | 2 + >> test/validation/classification/classification.c | 5 + >> .../classification/odp_classification_common.c | 225 >> ++++++++ >> .../classification/odp_classification_test_pmr.c | 640 >> +++++++++++++++++++++ >> .../classification/odp_classification_tests.c | 152 +---- >> .../classification/odp_classification_testsuites.h | 10 +- >> 7 files changed, 906 insertions(+), 132 deletions(-) >> create mode 100644 >> test/validation/classification/odp_classification_common.c >> create mode 100644 >> test/validation/classification/odp_classification_test_pmr.c >> >> diff --git a/helper/include/odp/helper/tcp.h >> b/helper/include/odp/helper/tcp.h >> index defe422..b52784d 100644 >> --- a/helper/include/odp/helper/tcp.h >> +++ b/helper/include/odp/helper/tcp.h >> @@ -26,6 +26,10 @@ extern "C" { >> * @{ >> */ >> >> +/** TCP header length (Minimum Header length without options)*/ >> +/** If options field is added to TCP header then the correct >> header value >> +should be updated by the application */ >> +#define ODPH_TCPHDR_LEN 20 >> >> >> What about to name it ODPH_MIN_TCPHDR_LEN. >> >> >> As you have mentioned in your next mail this, I have followed existing >> ODP standard for naming convention. >> > > Don't forget to shorten the comment. > Okay. > > >> >> >> >> /** TCP header */ >> typedef struct ODP_PACKED { >> diff --git a/test/validation/classification/Makefile.am >> b/test/validation/classification/Makefile.am >> index ba468fa..050d5e6 100644 >> --- a/test/validation/classification/Makefile.am >> +++ b/test/validation/classification/Makefile.am >> @@ -3,6 +3,8 @@ include ../Makefile.inc >> noinst_LTLIBRARIES = libclassification.la < >> http://libclassification.la> >> >> libclassification_la_SOURCES = odp_classification_basic.c \ >> odp_classification_tests.c \ >> + odp_classification_test_pmr.c \ >> + odp_classification_common.c \ >> classification.c >> >> bin_PROGRAMS = classification_main$(EXEEXT) >> diff --git a/test/validation/classification/classification.c >> b/test/validation/classification/classification.c >> index 2582aaa..a88a301 100644 >> --- a/test/validation/classification/classification.c >> +++ b/test/validation/classification/classification.c >> @@ -18,6 +18,11 @@ static CU_SuiteInfo classification_suites[] = { >> .pInitFunc = classification_suite_init, >> .pCleanupFunc = >> classification_suite_term, >> }, >> + { .pName = "classification pmr tests", >> + .pTests = classification_suite_pmr, >> + .pInitFunc = >> classification_suite_pmr_init, >> + .pCleanupFunc = >> classification_suite_pmr_term, >> + }, >> >> >> I'm not sure if "classificatio pmr tests" should be after >> "classification tests" >> Maybe better to insert it before. Just in order of complexity. >> >> >> Yes. I will move this in the next patch. >> >> >> >> CU_SUITE_INFO_NULL, >> }; >> >> diff --git >> a/test/validation/classification/odp_classification_common.c >> b/test/validation/classification/odp_classification_common.c >> new file mode 100644 >> index 0000000..fe392c0 >> --- /dev/null >> +++ b/test/validation/classification/odp_classification_common.c >> @@ -0,0 +1,225 @@ >> +/* Copyright (c) 2015, Linaro Limited >> + * All rights reserved. >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +#include "odp_classification_testsuites.h" >> +#include <odp_cunit_common.h> >> +#include <odp/helper/eth.h> >> +#include <odp/helper/ip.h> >> +#include <odp/helper/udp.h> >> +#include <odp/helper/tcp.h> >> + >> +#define SHM_PKT_NUM_BUFS 32 >> +#define SHM_PKT_BUF_SIZE 1024 >> + >> +#define CLS_DEFAULT_SADDR "10.0.0.1/32 <http://10.0.0.1/32 >> >" >> +#define CLS_DEFAULT_DADDR "10.0.0.100/32 < >> http://10.0.0.100/32>" >> +#define CLS_DEFAULT_SPORT 1024 >> +#define CLS_DEFAULT_DPORT 2048 >> + >> +#define CLS_TEST_SPORT 4096 >> +#define CLS_TEST_DPORT 8192 >> +#define CLS_TEST_DADDR "10.0.0.5/32 <http://10.0.0.5/32 >> >" >> >> + >> +/* Test Packet values */ >> +#define DATA_MAGIC 0x01020304 >> +#define TEST_SEQ_INVALID ((uint32_t)~0) >> + >> +/** sequence number of IP packets */ >> +odp_atomic_u32_t seq; >> + >> +typedef struct cls_test_packet { >> + uint32be_t magic; >> + uint32be_t seq; >> +} cls_test_packet_t; >> + >> +int cls_pkt_set_seq(odp_packet_t pkt, bool flag_udp) >> +{ >> + static uint32_t seq; >> + cls_test_packet_t data; >> + uint32_t offset; >> + int status; >> + >> + data.magic = DATA_MAGIC; >> + data.seq = ++seq; >> + >> + offset = odp_packet_l4_offset(pkt); >> + CU_ASSERT_FATAL(offset != 0); >> + >> + if (flag_udp) >> + status = odp_packet_copydata_in(pkt, offset + >> ODPH_UDPHDR_LEN, >> + sizeof(data), >> &data); >> + else >> + status = odp_packet_copydata_in(pkt, offset + >> ODPH_TCPHDR_LEN, >> + sizeof(data), >> &data); >> + >> + return status; >> +} >> + >> +uint32_t cls_pkt_get_seq(odp_packet_t pkt) >> >> >> On my opinion it should be complementary to cls_pkt_set_seq(), >> with flag_udp or vise versa. >> >> >> It is a good point but actually I will have to read the UDP flag from the >> packet if I have to send the flag into the cls_pkt_get_seq(pkt) function >> but since we are sending the packet as input parameter to this function I >> think it is better to check the same in the function itself. >> > > I thing it shouldn't depend on external usage of the function. > Sorry, what decision? I thought your suggestion was to add "flag_udp" as part of input parameter to cls_pkt_get_seq(odp_packet_t pkt) function. IMO since we are sending the packet into this function it would be redundant to send the udp_flag information since this function can deduct the same from the packet. > > >> >> >> +{ >> + uint32_t offset; >> + cls_test_packet_t data; >> + odph_ipv4hdr_t *ip; >> + >> + ip = (odph_ipv4hdr_t *)odp_packet_l3_ptr(pkt, NULL); >> + offset = odp_packet_l4_offset(pkt); >> + >> + if (!offset && !ip) >> + return TEST_SEQ_INVALID; >> + >> + if (ip->proto == ODPH_IPPROTO_UDP) >> + odp_packet_copydata_out(pkt, offset + >> ODPH_UDPHDR_LEN, >> + sizeof(data), &data); >> + else >> + odp_packet_copydata_out(pkt, offset + >> ODPH_TCPHDR_LEN, >> + sizeof(data), &data); >> >> >> adding to previous comment. If you wish ,in the same way, as you >> checked IP >> proto field of IP header to identify UDP/TCP, you can count TCP >> header length >> instead of hardcode ODPH_TCPHDR_LEN. It's just proposition, to make >> the function >> independent from TCP header length and it's options. >> >> >> I had my initial implementation reading the TCP header length from tcp >> packet but i changed this since the received packets are created only by >> this validation suite. >> I am fine changing this to read the header length form tcp header field. >> > > Ok. > > >> >> >> + >> + if (data.magic == DATA_MAGIC) >> + return data.seq; >> + >> + return TEST_SEQ_INVALID; >> +} >> + >> +static inline >> +int parse_ipv4_string(const char *ipaddress, uint32_t *addr, >> uint32_t *mask) >> >> >> .... >> >> -- >> Regards, >> Ivan Khoronzhuk >> >> >> >> If you have additional comments I can wait for further comments before >> sending the next version. >> > > Yes, please wait a little. > > >> Regards, >> Bala >> > > -- > Regards, > Ivan Khoronzhuk > Regards, Bala
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
