Re: [lng-odp] Mailing test again

[Zoltan Kiss]

It is still:

Why is this message in Spam? It has a from address in kalray.eu but has 
failed kalray.eu's required tests for authentication.  Learn more


The learn more link points here:

https://support.google.com/mail/answer/1366858?hl=en-GB=5

Here are your headers:

Delivered-To: zoltan.k...@linaro.org
Received: by 10.36.73.98 with SMTP id z95csp819200ita;
Thu, 28 Jul 2016 07:09:11 -0700 (PDT)
X-Received: by 10.200.48.42 with SMTP id f39mr57525396qte.81.1469714951052;
Thu, 28 Jul 2016 07:09:11 -0700 (PDT)
Return-Path: 
Received: from lists.linaro.org (lists.linaro.org. [54.225.227.206])
by mx.google.com with ESMTP id 
h194si5385901qka.234.2016.07.28.07.09.10;

Thu, 28 Jul 2016 07:09:11 -0700 (PDT)
Received-SPF: pass (google.com: domain of 
lng-odp-boun...@lists.linaro.org designates 54.225.227.206 as permitted 
sender) client-ip=54.225.227.206;

Authentication-Results: mx.google.com;
   spf=pass (google.com: domain of lng-odp-boun...@lists.linaro.org 
designates 54.225.227.206 as permitted sender) 
smtp.mailfrom=lng-odp-boun...@lists.linaro.org;

   dmarc=fail (p=QUARANTINE dis=QUARANTINE) header.from=kalray.eu
Received: by lists.linaro.org (Postfix, from userid 109)
id 7220A684D4; Thu, 28 Jul 2016 14:09:10 + (UTC)
X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on ip-10-142-244-252
X-Spam-Level:
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=disabled
version=3.4.0
Received: from [127.0.0.1] (localhost [127.0.0.1])
by lists.linaro.org (Postfix) with ESMTP id 65359684E4;
Thu, 28 Jul 2016 14:09:07 + (UTC)
X-Original-To: lng-odp@lists.linaro.org
Delivered-To: lng-odp@lists.linaro.org
Received: by lists.linaro.org (Postfix, from userid 109)
 id 0C0E4684E4; Thu, 28 Jul 2016 14:09:06 + (UTC)
Received: from zimbra1.kalray.eu (zimbra1.kalray.eu [92.103.151.219])
 by lists.linaro.org (Postfix) with ESMTPS id 30A95684A5
 for ; Thu, 28 Jul 2016 14:09:04 + (UTC)
Received: from localhost (localhost [127.0.0.1])
 by zimbra1.kalray.eu (Postfix) with ESMTP id 196D7280A22;
 Thu, 28 Jul 2016 16:09:03 +0200 (CEST)
Received: from zimbra1.kalray.eu ([127.0.0.1])
 by localhost (zimbra1.kalray.eu [127.0.0.1]) (amavisd-new, port 10032)
 with ESMTP id GRx88yLP_clv; Thu, 28 Jul 2016 16:09:02 +0200 (CEST)
Received: from localhost (localhost [127.0.0.1])
 by zimbra1.kalray.eu (Postfix) with ESMTP id CC040280A06;
 Thu, 28 Jul 2016 16:09:02 +0200 (CEST)
DKIM-Filter: OpenDKIM Filter v2.9.2 zimbra1.kalray.eu CC040280A06
X-Virus-Scanned: amavisd-new at kalray.eu
Received: from zimbra1.kalray.eu ([127.0.0.1])
 by localhost (zimbra1.kalray.eu [127.0.0.1]) (amavisd-new, port 10026)
 with ESMTP id kdrVwn9cca-7; Thu, 28 Jul 2016 16:09:02 +0200 (CEST)
Received: from [10.0.2.109] (LFbn-1-4650-116.w92-171.abo.wanadoo.fr
 [92.171.135.116])
 by zimbra1.kalray.eu (Postfix) with ESMTPSA id 6F1552807D0;
 Thu, 28 Jul 2016 16:09:02 +0200 (CEST)
To: LNG ODP Mailman List 
From: Nicolas Morey-Chaisemartin 
Message-ID: <3d88cea4-0b52-f439-4aad-db3701562...@kalray.eu>
Date: Thu, 28 Jul 2016 16:09:01 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101
 Thunderbird/47.0
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Cc: Luca Sokoll , ciaran.mo...@linaro.org
Subject: [lng-odp] Mailing test again
X-BeenThere: lng-odp@lists.linaro.org
X-Mailman-Version: 2.1.16
Precedence: list
List-Id: "The OpenDataPlane \(ODP\) List" 
List-Unsubscribe: ,
 
List-Archive: 
List-Post: 
List-Help: 
List-Subscribe: ,
 
Errors-To: lng-odp-boun...@lists.linaro.org
Sender: "lng-odp" 

On 28/07/16 15:09, Nicolas Morey-Chaisemartin wrote:

 Another mailing list test fo find oud why kalray's email end up in spam for 
gmail users.

Nicolas

Re: [lng-odp] [PATCH] configure.ac: fix have_pcap AM_CONDITIONAL

[Zoltan Kiss]

Ping

On 18/07/16 15:40, Zoltan Kiss wrote:

This causes "./configure: line 20048: test: =: unary operator expected" error
messages, while the generated code was like this:

 if test $pktio_dpdk_support = yes ; then
  PKTIO_DPDK_TRUE=
  PKTIO_DPDK_FALSE='#'
else
...

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/configure.ac b/configure.ac
index c0eb207..97310c4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -163,7 +163,7 @@ AC_SUBST([testdir])
 AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ])
 AM_CONDITIONAL([PKTIO_IPC], [test x$pktio_ipc_support = xyes])
 AM_CONDITIONAL([PKTIO_DPDK], [test x$pktio_dpdk_support = xyes ])
-AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes])
+AM_CONDITIONAL([HAVE_PCAP], [test x$have_pcap = xyes])
 AM_CONDITIONAL([SDK_INSTALL_PATH_], [test "x${SDK_INSTALL_PATH_}" = "x1"])
 AM_CONDITIONAL([test_installdir], [test "$testdir" != ""])
 AM_CONDITIONAL([cunit_support], [test x$cunit_support = xyes ])

Re: [lng-odp] [PATCH] linux-dpdk/m4/configure.m4: move m4_include above ac_check_*

[Zoltan Kiss]

Applied to ODP-DPDK (with subject line formatted a bit)

On 21/07/16 15:42, Anders Roxell wrote:

Signed-off-by: Anders Roxell 
---
 platform/linux-dpdk/m4/configure.m4 | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/platform/linux-dpdk/m4/configure.m4 
b/platform/linux-dpdk/m4/configure.m4
index 1d04fd0..f370afb 100644
--- a/platform/linux-dpdk/m4/configure.m4
+++ b/platform/linux-dpdk/m4/configure.m4
@@ -28,6 +28,12 @@ AC_LINK_IFELSE(
 echo "Use newer version. For gcc > 4.7.0"
 exit -1)

+# linux-generic PCAP support is not relevant as the code doesn't use
+# linux-generic pktio at all. And DPDK has its own PCAP support anyway
+AM_CONDITIONAL([HAVE_PCAP], [false])
+m4_include([platform/linux-dpdk/m4/odp_pthread.m4])
+m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
+
 #
 # Check that SDK_INSTALL_PATH provided to right dpdk version
 #
@@ -67,12 +73,6 @@ AC_CHECK_HEADERS([rte_config.h], [],
 LDFLAGS=$OLD_LDFLAGS
 CPPFLAGS=$OLD_CPPFLAGS

-# linux-generic PCAP support is not relevant as the code doesn't use
-# linux-generic pktio at all. And DPDK has its own PCAP support anyway
-AM_CONDITIONAL([HAVE_PCAP], [false])
-m4_include([platform/linux-dpdk/m4/odp_pthread.m4])
-m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
-
 AC_CONFIG_FILES([platform/linux-dpdk/Makefile
 platform/linux-dpdk/test/Makefile
 platform/linux-dpdk/test/pktio/Makefile

Re: [lng-odp] [PATCH] linux-dpdk/m4/configure.m4: move m4_include above ac_check_*

[Zoltan Kiss]

Reviewed-by: Zoltan Kiss <zoltan.k...@linaro.org>

On 21/07/16 15:42, Anders Roxell wrote:

Signed-off-by: Anders Roxell <anders.rox...@linaro.org>
---
 platform/linux-dpdk/m4/configure.m4 | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/platform/linux-dpdk/m4/configure.m4 
b/platform/linux-dpdk/m4/configure.m4
index 1d04fd0..f370afb 100644
--- a/platform/linux-dpdk/m4/configure.m4
+++ b/platform/linux-dpdk/m4/configure.m4
@@ -28,6 +28,12 @@ AC_LINK_IFELSE(
 echo "Use newer version. For gcc > 4.7.0"
 exit -1)

+# linux-generic PCAP support is not relevant as the code doesn't use
+# linux-generic pktio at all. And DPDK has its own PCAP support anyway
+AM_CONDITIONAL([HAVE_PCAP], [false])
+m4_include([platform/linux-dpdk/m4/odp_pthread.m4])
+m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
+
 #
 # Check that SDK_INSTALL_PATH provided to right dpdk version
 #
@@ -67,12 +73,6 @@ AC_CHECK_HEADERS([rte_config.h], [],
 LDFLAGS=$OLD_LDFLAGS
 CPPFLAGS=$OLD_CPPFLAGS

-# linux-generic PCAP support is not relevant as the code doesn't use
-# linux-generic pktio at all. And DPDK has its own PCAP support anyway
-AM_CONDITIONAL([HAVE_PCAP], [false])
-m4_include([platform/linux-dpdk/m4/odp_pthread.m4])
-m4_include([platform/linux-dpdk/m4/odp_openssl.m4])
-
 AC_CONFIG_FILES([platform/linux-dpdk/Makefile
 platform/linux-dpdk/test/Makefile
 platform/linux-dpdk/test/pktio/Makefile

[lng-odp] [PATCH] configure.ac: fix have_pcap AM_CONDITIONAL

[Zoltan Kiss]

This causes "./configure: line 20048: test: =: unary operator expected" error
messages, while the generated code was like this:

 if test $pktio_dpdk_support = yes ; then
  PKTIO_DPDK_TRUE=
  PKTIO_DPDK_FALSE='#'
else
...

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/configure.ac b/configure.ac
index c0eb207..97310c4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -163,7 +163,7 @@ AC_SUBST([testdir])
 AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ])
 AM_CONDITIONAL([PKTIO_IPC], [test x$pktio_ipc_support = xyes])
 AM_CONDITIONAL([PKTIO_DPDK], [test x$pktio_dpdk_support = xyes ])
-AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes])
+AM_CONDITIONAL([HAVE_PCAP], [test x$have_pcap = xyes])
 AM_CONDITIONAL([SDK_INSTALL_PATH_], [test "x${SDK_INSTALL_PATH_}" = "x1"])
 AM_CONDITIONAL([test_installdir], [test "$testdir" != ""])
 AM_CONDITIONAL([cunit_support], [test x$cunit_support = xyes ])

Re: [lng-odp] Test mail again

[Zoltan Kiss]

It ended up in the spam folder again, but I forget to check why before 
moving it from them, and gmail forgot that. You could try sending an 
email to me to my address privately.


On 08/07/16 06:59, Nicolas Morey-Chaisemartin wrote:

Me again and again. Sorry for the spam.

As for the previous ones (that you probably didn't see anyway):
If people using GMail, can see this (or find this in their junk folder), could 
you reply to this mail.
In the case where it was marked as Junk, could you also forward Gmail infos 
about why the mail was marked like this.

Regards

Nicolas

Re: [lng-odp] Yet Another Test Mail

[Zoltan Kiss]

Hi,

Yes, both your mails landed in my spam folder. Gmail said:

Why is this message in Spam? It has a from address in kalray.eu but has 
failed kalray.eu's required tests for authentication.  Learn more


Then there is a link:

https://support.google.com/mail/answer/1366858?hl=en-GB=5

Zoli

On 01/07/16 20:01, Nicolas Morey-Chaisemartin wrote:

Me again. Sorry for the spam.

As for the previous one (that yuo probably didn't see anyway):
If people using GMail, can see this (or find this in their junk folder), could 
you reply to this mail.
In the case where it was marked as Junk, could you also forward Gmail infos 
about why the mail was marked like this.

Regards

Nicolas

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [lng-odp-ovs] Error while installing ODP-v1.8

[Zoltan Kiss]

I've just posted the 1.10 patches for OVS, you'll need to apply them too.

On 29/06/16 17:06, Bill Fischofer wrote:

There were issues with unknown architectures giving this error in ODP
v1.8. This has been corrected in ODP v1.10.  Can you try with that release?

On Wed, Jun 29, 2016 at 11:05 AM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

Hi,

ODP_CACHE_LINE_SIZE is defined in platform/linux-generic/arch/[your
arch]/odp/api/cpu_arch.h, or in
platform/linux-generic/include/odp/api/align.h, I guess there is
some screwup there. You should ask about this on the lng-odp list,
I've added it to this mail.

Regards,

Zoltan


On 21/06/16 09:20, nilesh kakade wrote:

Hi,
As you said I tried to install ODP v1.8 but still its giving
following
error at time of make command


Making all in platform/linux-generic
make[1]: Entering directory `/home/mit12/odp/platform/linux-generic'
CC   odp_buffer.lo
In file included from ./include/odp_pool_internal.h:23:0,
   from odp_buffer.c:8:
./include/odp_align_internal.h:63:23: error: 'ODP_CACHE_LINE_SIZE'
undeclared here (not in a function)
ODP_ALIGN_ROUNDUP(x, ODP_CACHE_LINE_SIZE)
 ^
./include/odp_align_internal.h:49:4: note: in definition of macro
'ODP_ALIGN_ROUNDUP'
((align) * (((x) + align - 1) / (align)))
  ^
./include/odp_buffer_internal.h:150:14: note: in expansion of macro
'ODP_CACHE_LINE_SIZE_ROUNDUP'
uint8_t
pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))];
^
In file included from odp_buffer.c:8:0:
./include/odp_pool_internal.h:97:2: error: requested alignment
is not an
integer constant
odp_ticketlock_tlock ODP_ALIGNED_CACHE;
^
./include/odp_pool_internal.h:142:2: error: requested alignment
is not
an integer constant
local_cache_t local_cache[ODP_THREAD_COUNT_MAX]
ODP_ALIGNED_CACHE;
^
In file included from odp_buffer.c:10:0:
./include/odp_buffer_inlines.h: In function 'odp_buf_to_hdr':
./include/odp_buffer_inlines.h:57:1: error: control reaches end of
non-void function [-Werror=return-type]
   }
   ^
cc1: all warnings being treated as errors
make[1]: *** [odp_buffer.lo] Error 1
make[1]: Leaving directory `/home/mit12/odp/platform/linux-generic'
make: *** [all-recursive] Error 1
mit12@mit12-All-Series:~/odp$

please see the error and kindly help me further.

Thanks & Regards.

___
lng-odp-ovs mailing list
lng-odp-...@lists.linaro.org <mailto:lng-odp-...@lists.linaro.org>
https://lists.linaro.org/mailman/listinfo/lng-odp-ovs



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] Error while installing ODP-v1.8

[Zoltan Kiss]

Hi,

ODP_CACHE_LINE_SIZE is defined in platform/linux-generic/arch/[your 
arch]/odp/api/cpu_arch.h, or in 
platform/linux-generic/include/odp/api/align.h, I guess there is some 
screwup there. You should ask about this on the lng-odp list, I've added 
it to this mail.


Regards,

Zoltan

On 21/06/16 09:20, nilesh kakade wrote:

Hi,
As you said I tried to install ODP v1.8 but still its giving following
error at time of make command


Making all in platform/linux-generic
make[1]: Entering directory `/home/mit12/odp/platform/linux-generic'
   CC   odp_buffer.lo
In file included from ./include/odp_pool_internal.h:23:0,
  from odp_buffer.c:8:
./include/odp_align_internal.h:63:23: error: 'ODP_CACHE_LINE_SIZE'
undeclared here (not in a function)
   ODP_ALIGN_ROUNDUP(x, ODP_CACHE_LINE_SIZE)
^
./include/odp_align_internal.h:49:4: note: in definition of macro
'ODP_ALIGN_ROUNDUP'
   ((align) * (((x) + align - 1) / (align)))
 ^
./include/odp_buffer_internal.h:150:14: note: in expansion of macro
'ODP_CACHE_LINE_SIZE_ROUNDUP'
   uint8_t pad[ODP_CACHE_LINE_SIZE_ROUNDUP(sizeof(odp_buffer_hdr_t))];
   ^
In file included from odp_buffer.c:8:0:
./include/odp_pool_internal.h:97:2: error: requested alignment is not an
integer constant
   odp_ticketlock_tlock ODP_ALIGNED_CACHE;
   ^
./include/odp_pool_internal.h:142:2: error: requested alignment is not
an integer constant
   local_cache_t local_cache[ODP_THREAD_COUNT_MAX] ODP_ALIGNED_CACHE;
   ^
In file included from odp_buffer.c:10:0:
./include/odp_buffer_inlines.h: In function 'odp_buf_to_hdr':
./include/odp_buffer_inlines.h:57:1: error: control reaches end of
non-void function [-Werror=return-type]
  }
  ^
cc1: all warnings being treated as errors
make[1]: *** [odp_buffer.lo] Error 1
make[1]: Leaving directory `/home/mit12/odp/platform/linux-generic'
make: *** [all-recursive] Error 1
mit12@mit12-All-Series:~/odp$

please see the error and kindly help me further.

Thanks & Regards.

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] linux-generic: timer: correct definition of ODP_TIMEOUT_INVALID

[Zoltan Kiss]

Hi,

ODP-DPDK uses 0 or NULL for all _INVALID constants, because that's what 
rte_mbuf's using for that purpose. How could this fix work there? I 
mean, simply just applying this patch doesn't work, because then this 
value will be in conflict with ODP_EVENT_INVALID et all.


Zoli

On 10/06/16 01:09, Bill Fischofer wrote:

Resolve bug https://bugs.linaro.org/show_bug.cgi?id=2316 by changing the
typedef of ODP_TIMEOUT_INVALID to be 0x to be consistent with other
buffer types. This enables pool 0 to be used as a timeout pool.

Signed-off-by: Bill Fischofer 
---
  platform/linux-generic/include/odp/api/plat/timer_types.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/platform/linux-generic/include/odp/api/plat/timer_types.h 
b/platform/linux-generic/include/odp/api/plat/timer_types.h
index 93ea162..68d6f6f 100644
--- a/platform/linux-generic/include/odp/api/plat/timer_types.h
+++ b/platform/linux-generic/include/odp/api/plat/timer_types.h
@@ -36,7 +36,7 @@ typedef ODP_HANDLE_T(odp_timer_t);

  typedef ODP_HANDLE_T(odp_timeout_t);

-#define ODP_TIMEOUT_INVALID  _odp_cast_scalar(odp_timeout_t, 0)
+#define ODP_TIMEOUT_INVALID  _odp_cast_scalar(odp_timeout_t, 0x)

  /**
   * @}


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] linux-generic: classification: use proper accessor to set packet length

[Zoltan Kiss]

And repurpose packet_set_len() for this, as it is no longer used.
This was introduced by the following commit, and break compatibility with
ODP-DPDK:

bd18047a "linux-gen: pktio: don't allocate new packets in classifier"

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_packet_internal.h | 4 ++--
 platform/linux-generic/odp_classification.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d5ace12..a84a6f8 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -278,9 +278,9 @@ static inline uint32_t packet_len(odp_packet_hdr_t *pkt_hdr)
return pkt_hdr->frame_len;
 }
 
-static inline void packet_set_len(odp_packet_t pkt, uint32_t len)
+static inline void packet_set_len(odp_packet_hdr_t *pkt_hdr, uint32_t len)
 {
-   odp_packet_hdr(pkt)->frame_len = len;
+   pkt_hdr->frame_len = len;
 }
 
 static inline int packet_parse_l2_not_done(odp_packet_hdr_t *pkt_hdr)
diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 7520bdc..d2cc081 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -815,7 +815,7 @@ int cls_classify_packet(pktio_entry_t *entry, const uint8_t 
*base, uint16_t len,
cos_t *cos;
 
packet_parse_reset(pkt_hdr);
-   pkt_hdr->frame_len = len;
+   packet_set_len(pkt_hdr, len);
 
_odp_parse_common(pkt_hdr, base);
cos = cls_select_cos(entry, base, pkt_hdr);
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] autotools: define test extensions to skip on valgrind test

[Zoltan Kiss]

Hi,

Now that I tried to port this to ODP-DPDK, the pktio tests started to 
fail. It turned out Valgrind overwrites the LOG_COMPILER, which is not 
used in odp-linux, but odp-dpdk has that. TEST_EXTENSIONS makes the 
testcase run without the wrapper, even if Valgrind is not used.
Moreover, do we need this at all? I mean, this script runs the pktio 
tests, which are therefore not tested with Valgrind.


Zoli

On 17/05/16 15:57, Maxim Uvarov wrote:

valgrind should not check bash wrappers. Accoding to doc:
https://www.gnu.org/software/gnulib/manual/html_node/Running-self_002dtests-under-valgrind.html
TEST_EXTENSIONS has to be set.
https://bugs.linaro.org/show_bug.cgi?id=2230

Signed-off-by: Maxim Uvarov 
---
  platform/linux-generic/test/Makefile.am| 14 --
  platform/linux-generic/test/pktio/Makefile.am  | 10 +-
  .../linux-generic/test/pktio/{pktio_run => pktio_run.sh}   |  0
  .../test/pktio/{pktio_run_dpdk => pktio_run_dpdk.sh}   |  0
  .../test/pktio/{pktio_run_netmap => pktio_run_netmap.sh}   |  0
  .../test/pktio/{pktio_run_pcap => pktio_run_pcap.sh}   |  0
  .../test/pktio/{pktio_run_tap => pktio_run_tap.sh} |  0
  platform/linux-generic/test/pktio_ipc/Makefile.am  |  2 +-
  .../test/pktio_ipc/{pktio_ipc_run => pktio_ipc_run.sh} |  0
  test/performance/Makefile.am   |  6 --
  test/performance/{odp_l2fwd_run => odp_l2fwd_run.sh}   |  0
  .../{odp_scheduling_run => odp_scheduling_run.sh}  |  0
  12 files changed, 18 insertions(+), 14 deletions(-)
  rename platform/linux-generic/test/pktio/{pktio_run => pktio_run.sh} (100%)
  rename platform/linux-generic/test/pktio/{pktio_run_dpdk => 
pktio_run_dpdk.sh} (100%)
  rename platform/linux-generic/test/pktio/{pktio_run_netmap => 
pktio_run_netmap.sh} (100%)
  rename platform/linux-generic/test/pktio/{pktio_run_pcap => 
pktio_run_pcap.sh} (100%)
  rename platform/linux-generic/test/pktio/{pktio_run_tap => pktio_run_tap.sh} 
(100%)
  rename platform/linux-generic/test/pktio_ipc/{pktio_ipc_run => 
pktio_ipc_run.sh} (100%)
  rename test/performance/{odp_l2fwd_run => odp_l2fwd_run.sh} (100%)
  rename test/performance/{odp_scheduling_run => odp_scheduling_run.sh} (100%)

diff --git a/platform/linux-generic/test/Makefile.am 
b/platform/linux-generic/test/Makefile.am
index 05998e3..f74185d 100644
--- a/platform/linux-generic/test/Makefile.am
+++ b/platform/linux-generic/test/Makefile.am
@@ -6,8 +6,8 @@ ODP_MODULES = pktio \
  shmem

  if test_vald
-TESTS = pktio/pktio_run \
-   pktio/pktio_run_tap \
+TESTS = pktio/pktio_run.sh \
+   pktio/pktio_run_tap.sh \
ring/ringtest$(EXEEXT) \
shmem/shmem_linux \
${top_builddir}/test/validation/atomic/atomic_main$(EXEEXT) \
@@ -38,20 +38,22 @@ TESTS = pktio/pktio_run \
  SUBDIRS = $(ODP_MODULES)

  if HAVE_PCAP
-TESTS += pktio/pktio_run_pcap
+TESTS += pktio/pktio_run_pcap.sh
  endif
  if PKTIO_IPC
-TESTS += pktio_ipc/pktio_ipc_run
+TESTS += pktio_ipc/pktio_ipc_run.sh
  SUBDIRS += pktio_ipc
  endif
  if netmap_support
-TESTS += pktio/pktio_run_netmap
+TESTS += pktio/pktio_run_netmap.sh
  endif
  if PKTIO_DPDK
-TESTS += pktio/pktio_run_dpdk
+TESTS += pktio/pktio_run_dpdk.sh
  endif
  endif

+TEST_EXTENSIONS = .sh
+
  dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)

  test_SCRIPTS = $(dist_check_SCRIPTS)
diff --git a/platform/linux-generic/test/pktio/Makefile.am 
b/platform/linux-generic/test/pktio/Makefile.am
index 3dcc1ee..4a14343 100644
--- a/platform/linux-generic/test/pktio/Makefile.am
+++ b/platform/linux-generic/test/pktio/Makefile.am
@@ -1,15 +1,15 @@
  dist_check_SCRIPTS = pktio_env \
-pktio_run \
-pktio_run_tap
+pktio_run.sh \
+pktio_run_tap.sh

  if HAVE_PCAP
-dist_check_SCRIPTS += pktio_run_pcap
+dist_check_SCRIPTS += pktio_run_pcap.sh
  endif
  if netmap_support
-dist_check_SCRIPTS += pktio_run_netmap
+dist_check_SCRIPTS += pktio_run_netmap.sh
  endif
  if PKTIO_DPDK
-dist_check_SCRIPTS += pktio_run_dpdk
+dist_check_SCRIPTS += pktio_run_dpdk.sh
  endif

  test_SCRIPTS = $(dist_check_SCRIPTS)
diff --git a/platform/linux-generic/test/pktio/pktio_run 
b/platform/linux-generic/test/pktio/pktio_run.sh
similarity index 100%
rename from platform/linux-generic/test/pktio/pktio_run
rename to platform/linux-generic/test/pktio/pktio_run.sh
diff --git a/platform/linux-generic/test/pktio/pktio_run_dpdk 
b/platform/linux-generic/test/pktio/pktio_run_dpdk.sh
similarity index 100%
rename from platform/linux-generic/test/pktio/pktio_run_dpdk
rename to platform/linux-generic/test/pktio/pktio_run_dpdk.sh
diff --git a/platform/linux-generic/test/pktio/pktio_run_netmap 
b/platform/linux-generic/test/pktio/pktio_run_netmap.sh
similarity index 100%
rename from platform/linux-generic/test/pktio/pktio_run_netmap
rename to 

Re: [lng-odp] [PATCHv2 1/2] linux-generic: packet: copy user area as part of odp_packet_copy()

[Zoltan Kiss]

I think that last hunk for test/validation/packet/packet.c should go to 
the second patch, otherwise, for the series:


Reviewed-by: Zoltan Kiss <zoltan.k...@linaro.org>

On 09/06/16 00:11, Bill Fischofer wrote:

Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2310 by copying the
user area as part of odp_packet_copy(). The copy fails if the user area
size of the destination pool is not large enough to hold the source packet
user area.

Reported-by: Zoltan Kiss <zoltan.k...@linaro.org>
Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org>
---
v2: Zoltan review comments.  Include additional metadata like ts, color, that
 were added more recently.

  .../linux-generic/include/odp_packet_internal.h|  5 +-
  platform/linux-generic/odp_packet.c| 23 --
  test/validation/packet/packet.c| 53 +-
  3 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d5ace12..ab54227 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -192,6 +192,9 @@ static inline void 
copy_packet_parser_metadata(odp_packet_hdr_t *src_hdr,
dst_hdr->l4_len = src_hdr->l4_len;

dst_hdr->dst_queue  = src_hdr->dst_queue;
+   dst_hdr->flow_hash  = src_hdr->flow_hash;
+   dst_hdr->timestamp  = src_hdr->timestamp;
+   dst_hdr->op_result  = src_hdr->op_result;
  }

  static inline void *packet_map(odp_packet_hdr_t *pkt_hdr,
@@ -294,7 +297,7 @@ static inline int 
packet_parse_not_complete(odp_packet_hdr_t *pkt_hdr)
  }

  /* Forward declarations */
-void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt);
+int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt);

  odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse);

diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index 2868736..40549a8 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -756,22 +756,11 @@ odp_packet_t odp_packet_copy(odp_packet_t pkt, odp_pool_t 
pool)
  {
odp_packet_hdr_t *srchdr = odp_packet_hdr(pkt);
uint32_t pktlen = srchdr->frame_len;
-   uint32_t meta_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr);
odp_packet_t newpkt = odp_packet_alloc(pool, pktlen);

if (newpkt != ODP_PACKET_INVALID) {
-   odp_packet_hdr_t *newhdr = odp_packet_hdr(newpkt);
-   uint8_t *newstart, *srcstart;
-
-   /* Must copy metadata first, followed by packet data */
-   newstart = (uint8_t *)newhdr + meta_offset;
-   srcstart = (uint8_t *)srchdr + meta_offset;
-
-   memcpy(newstart, srcstart,
-  sizeof(odp_packet_hdr_t) - meta_offset);
-
-   if (odp_packet_copy_from_pkt(newpkt, 0, pkt, 0,
-pktlen) != 0) {
+   if (_odp_packet_copy_md_to_packet(pkt, newpkt) ||
+   odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, pktlen)) {
odp_packet_free(newpkt);
newpkt = ODP_PACKET_INVALID;
}
@@ -966,7 +955,7 @@ int odp_packet_is_valid(odp_packet_t pkt)
   *
   */

-void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt)
+int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt)
  {
odp_packet_hdr_t *srchdr = odp_packet_hdr(srcpkt);
odp_packet_hdr_t *dsthdr = odp_packet_hdr(dstpkt);
@@ -986,6 +975,12 @@ void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, 
odp_packet_t dstpkt)
odp_atomic_load_u32(
>buf_hdr.ref_count));
copy_packet_parser_metadata(srchdr, dsthdr);
+
+   /* Metadata copied, but return indication of whether the packet
+* user area was truncated in the process. Note this can only
+* happen when copying between different pools.
+*/
+   return dsthdr->buf_hdr.uarea_size < srchdr->buf_hdr.uarea_size;
  }

  /**
diff --git a/test/validation/packet/packet.c b/test/validation/packet/packet.c
index 6770339..f71b658 100644
--- a/test/validation/packet/packet.c
+++ b/test/validation/packet/packet.c
@@ -863,32 +863,41 @@ free_packet:
odp_packet_free(pkt);
  }

-#define COMPARE_INFLAG(p1, p2, flag) \
+#define COMPARE_HAS_INFLAG(p1, p2, flag) \
CU_ASSERT(odp_packet_has_##flag(p1) == odp_packet_has_##flag(p2))

+#define COMPARE_INFLAG(p1, p2, flag) \
+   CU_ASSERT(odp_packet_##flag(p1) == odp_packet_##flag(p2))
+
  static void _packet_compare_inflags(odp_packet_t pkt1, odp_packet_t pkt2)
  {
-   COMPARE_INFLAG(pkt1, pkt2, l2);
-   COMPARE_INFLAG(pkt1, pkt2, l3);

Re: [lng-odp] [API-NEXT PATCHv1 0/4] implement mechanism to allow inline and no inline use of ODP API

[Zoltan Kiss]

Hi,

ODP-DPDK already has a simpler solution for this problem, the first 
patch was posted here as well:


https://git.linaro.org/lng/odp-dpdk.git/commitdiff/968237b592a8162041a4edf0091575fd1390d647

It can get simpler as it doesn't need a separate config option, but it 
automatically turns on when --enable-shared=yes (which is the default). 
When you have shared libraries, you definitely need this, as whoever 
links to it doesn't know about your inlines. But if you only compile 
static libraries, you'll need its headers anyway, so you can have inline 
functions as well.


Regards,

Zoltan

On 06/06/16 09:44, huanggaoyang wrote:

ODP needs to be portable across similar architectures with differing ODP 
implementations.
Inline functions will inhibit the compatibility.

In this patch, I add some Macros to determain those inline functions are 
exposed in .h files as before
or put together in a .c file as normal functions without "static inline" 
property.

configure by "--enable-compatibility-mode=yes" will enable the compatibility 
mode.

configure without it:
   ODP_BINARY_COMPATIBLE == 0
   in .h files:
  ODP_ENABLE_INLINE == 1
  ODP_STATIC is static
  ODP_INLINE is inline
   in odp_compatible.c & helper/compatible.c
  codes are disabled since ODP_BINARY_COMPATIBLE == 0
All the codes and APIs are just as before

configure with --enable-compatibility-mode=yes:
   ODP_BINARY_COMPATIBLE == 1
   in .h files:
  ODP_ENABLE_INLINE == 0
  all the former inline functions are disabled.
   in odp_compatible.c & helper/compatible.c
  ODP_STATIC is nothing
  ODP_INLINE is nothing
  ODP_ENABLE_INLINE == 1
  codes are enabled, and the inline functions in .h files are enabled without 
"static inline"

"CFLAGS of helper should inherit from ODP main target" is already proposed by a 
patch of mine last week.
I also include it here because the compile of helper needs it

I find this line of code:
#define ODP_STATIC static
will cause a check-patch warning: "storage class should be at the beginning of the 
declaration".
Is it a false positive? Would someone can help me resolve it?

huanggaoyang (4):
   linux-generic:configure:add configure option
 --enable-compatibility-mode
   linux-generic:compatibility:implement mechanism to allow inline and no
 inline use of ODP API
   helper: CFLAGS of helper should inherit from ODP main target
   helper: implement mechanism to allow inline and no inline use of ODP
 Helper API

  configure.ac   |  13 ++
  helper/Makefile.am |   5 +-
  helper/compatible.c|  24 
  helper/hashtable.c |   7 +-
  helper/include/odp/helper/chksum.h |  71 +-
  helper/include/odp/helper/ip.h |  11 +-
  helper/include/odp/helper/table.h  |   1 -
  helper/lineartable.c   |   8 +-
  helper/odph_hashtable.h|   1 -
  helper/odph_lineartable.h  |   9 +-
  helper/test/table.c|   2 -
  platform/linux-generic/Makefile.am |   1 +
  platform/linux-generic/include/odp/api/atomic.h| 155 -
  platform/linux-generic/include/odp/api/byteorder.h |  27 ++--
  .../linux-generic/include/odp/api/compatible.h |  62 +
  .../include/odp/api/plat/buffer_types.h|   6 +-
  .../include/odp/api/plat/classification_types.h|   9 +-
  .../include/odp/api/plat/crypto_types.h|  10 +-
  .../include/odp/api/plat/event_types.h |   6 +-
  .../include/odp/api/plat/packet_io_types.h |   6 +-
  .../include/odp/api/plat/packet_types.h|   8 +-
  .../include/odp/api/plat/pool_types.h  |   6 +-
  .../include/odp/api/plat/queue_types.h |   6 +-
  .../include/odp/api/plat/shared_memory_types.h |   6 +-
  .../include/odp/api/plat/traffic_mngr_types.h  |   1 -
  platform/linux-generic/include/odp/api/std_clib.h  |   9 +-
  platform/linux-generic/include/odp/api/sync.h  |   7 +-
  platform/linux-generic/odp_compatible.c|  46 ++
  28 files changed, 361 insertions(+), 162 deletions(-)
  create mode 100644 helper/compatible.c
  create mode 100644 platform/linux-generic/include/odp/api/compatible.h
  create mode 100644 platform/linux-generic/odp_compatible.c


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 2/2] validation: packet: ensure user area is copied correctly

[Zoltan Kiss]

Reviewed-by: Zoltan Kiss <zoltan.k...@linaro.org>

On 07/06/16 15:53, Bill Fischofer wrote:

As part of resolution of Bug https://bugs.linaro.org/show_bug.cgi?id=2310
make sure that odp_packet_copy() handles user area copies correctly. The
copy should fail if the target pool's user area size is not large enough
to contain the source user area.

Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org>
---
  test/validation/packet/packet.c | 69 +++--
  1 file changed, 67 insertions(+), 2 deletions(-)

diff --git a/test/validation/packet/packet.c b/test/validation/packet/packet.c
index 6770339..095f760 100644
--- a/test/validation/packet/packet.c
+++ b/test/validation/packet/packet.c
@@ -14,7 +14,7 @@
  /* Reserve some tailroom for tests */
  #define PACKET_TAILROOM_RESERVE  4

-static odp_pool_t packet_pool;
+static odp_pool_t packet_pool, packet_pool_no_uarea, packet_pool_double_uarea;
  static uint32_t packet_len;

  static uint32_t segmented_packet_len;
@@ -65,6 +65,24 @@ int packet_suite_init(void)
if (packet_pool == ODP_POOL_INVALID)
return -1;

+   params.pkt.uarea_size = 0;
+   packet_pool_no_uarea = odp_pool_create("packet_pool_no_uarea",
+  );
+   if (packet_pool_no_uarea == ODP_POOL_INVALID) {
+   odp_pool_destroy(packet_pool);
+   return -1;
+   }
+
+   params.pkt.uarea_size = 2 * sizeof(struct udata_struct);
+   packet_pool_double_uarea = odp_pool_create("packet_pool_double_uarea",
+  );
+
+   if (packet_pool_double_uarea == ODP_POOL_INVALID) {
+   odp_pool_destroy(packet_pool_no_uarea);
+   odp_pool_destroy(packet_pool);
+   return -1;
+   }
+
test_packet = odp_packet_alloc(packet_pool, packet_len);

for (i = 0; i < packet_len; i++) {
@@ -113,8 +131,12 @@ int packet_suite_term(void)
  {
odp_packet_free(test_packet);
odp_packet_free(segmented_test_packet);
-   if (odp_pool_destroy(packet_pool) != 0)
+
+   if (odp_pool_destroy(packet_pool_double_uarea) != 0 ||
+   odp_pool_destroy(packet_pool_no_uarea) != 0 ||
+   odp_pool_destroy(packet_pool) != 0)
return -1;
+
return 0;
  }

@@ -913,6 +935,20 @@ static void _packet_compare_data(odp_packet_t pkt1, 
odp_packet_t pkt2)
}
  }

+static void _packet_compare_udata(odp_packet_t pkt1, odp_packet_t pkt2)
+{
+   uint32_t usize1 = odp_packet_user_area_size(pkt1);
+   uint32_t usize2 = odp_packet_user_area_size(pkt2);
+
+   void *uaddr1 = odp_packet_user_area(pkt1);
+   void *uaddr2 = odp_packet_user_area(pkt2);
+
+   uint32_t cmplen = usize1 <= usize2 ? usize1 : usize2;
+
+   if (cmplen)
+   CU_ASSERT(!memcmp(uaddr1, uaddr2, cmplen));
+}
+
  static void _packet_compare_offset(odp_packet_t pkt1, uint32_t off1,
   odp_packet_t pkt2, uint32_t off2,
   uint32_t len)
@@ -948,6 +984,11 @@ void packet_test_copy(void)
uint32_t i, plen, seg_len, src_offset, dst_offset;
void *pkt_data;

+   pkt = odp_packet_copy(test_packet, packet_pool_no_uarea);
+   CU_ASSERT(pkt == ODP_PACKET_INVALID);
+   if (pkt != ODP_PACKET_INVALID)
+   odp_packet_free(pkt);
+
pkt = odp_packet_copy(test_packet, odp_packet_pool(test_packet));
CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID);
_packet_compare_data(pkt, test_packet);
@@ -962,6 +1003,30 @@ void packet_test_copy(void)

_packet_compare_inflags(pkt, pkt_copy);
_packet_compare_data(pkt, pkt_copy);
+   CU_ASSERT(odp_packet_user_area_size(pkt) ==
+ odp_packet_user_area_size(test_packet));
+   _packet_compare_udata(pkt, pkt_copy);
+   odp_packet_free(pkt_copy);
+   odp_packet_free(pkt);
+
+   pkt = odp_packet_copy(test_packet, packet_pool_double_uarea);
+   CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID);
+   _packet_compare_data(pkt, test_packet);
+   pool = odp_packet_pool(pkt);
+   CU_ASSERT_FATAL(pool != ODP_POOL_INVALID);
+   pkt_copy = odp_packet_copy(pkt, pool);
+   CU_ASSERT_FATAL(pkt_copy != ODP_PACKET_INVALID);
+
+   CU_ASSERT(pkt != pkt_copy);
+   CU_ASSERT(odp_packet_data(pkt) != odp_packet_data(pkt_copy));
+   CU_ASSERT(odp_packet_len(pkt) == odp_packet_len(pkt_copy));
+
+   _packet_compare_inflags(pkt, pkt_copy);
+   _packet_compare_data(pkt, pkt_copy);
+   CU_ASSERT(odp_packet_user_area_size(pkt) ==
+ 2 * odp_packet_user_area_size(test_packet));
+   _packet_compare_udata(pkt, pkt_copy);
+   _packet_compare_udata(pkt, test_packet);
odp_packet_free(pkt_copy);

/* Now test copy_part */


___
lng-odp mailing list
lng-odp@lis

Re: [lng-odp] [PATCH 1/2] linux-generic: packet: copy user area as part of odp_packet_copy()

[Zoltan Kiss]

Hi,

On 07/06/16 15:53, Bill Fischofer wrote:

Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2310 by copying the
user area as part of odp_packet_copy(). The copy fails if the user area
size of the destination pool is not large enough to hold the source packet
user area.

Signed-off-by: Bill Fischofer 
---
  platform/linux-generic/include/odp_packet_internal.h |  2 +-
  platform/linux-generic/odp_packet.c  | 18 --
  2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d5ace12..5c74d97 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -294,7 +294,7 @@ static inline int 
packet_parse_not_complete(odp_packet_hdr_t *pkt_hdr)
  }

  /* Forward declarations */
-void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt);
+int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt);

  odp_packet_t packet_alloc(odp_pool_t pool_hdl, uint32_t len, int parse);

diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index 2868736..f92c257 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -756,22 +756,11 @@ odp_packet_t odp_packet_copy(odp_packet_t pkt, odp_pool_t 
pool)
  {
odp_packet_hdr_t *srchdr = odp_packet_hdr(pkt);
uint32_t pktlen = srchdr->frame_len;
-   uint32_t meta_offset = ODP_FIELD_SIZEOF(odp_packet_hdr_t, buf_hdr);
odp_packet_t newpkt = odp_packet_alloc(pool, pktlen);

if (newpkt != ODP_PACKET_INVALID) {
-   odp_packet_hdr_t *newhdr = odp_packet_hdr(newpkt);
-   uint8_t *newstart, *srcstart;
-
-   /* Must copy metadata first, followed by packet data */
-   newstart = (uint8_t *)newhdr + meta_offset;
-   srcstart = (uint8_t *)srchdr + meta_offset;
-
-   memcpy(newstart, srcstart,
-  sizeof(odp_packet_hdr_t) - meta_offset);
-
-   if (odp_packet_copy_from_pkt(newpkt, 0, pkt, 0,
-pktlen) != 0) {
+   if (_odp_packet_copy_md_to_packet(pkt, newpkt) ||
+   odp_packet_copy_from_pkt(newpkt, 0, pkt, 0, pktlen)) {
odp_packet_free(newpkt);
newpkt = ODP_PACKET_INVALID;
}
@@ -966,7 +955,7 @@ int odp_packet_is_valid(odp_packet_t pkt)
   *
   */

-void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt)
+int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, odp_packet_t dstpkt)


There are other callers of this function, we should either check the 
return value there or make a comment at least about why it's not 
possible for an error to happen.



  {
odp_packet_hdr_t *srchdr = odp_packet_hdr(srcpkt);
odp_packet_hdr_t *dsthdr = odp_packet_hdr(dstpkt);
@@ -986,6 +975,7 @@ void _odp_packet_copy_md_to_packet(odp_packet_t srcpkt, 
odp_packet_t dstpkt)
odp_atomic_load_u32(
>buf_hdr.ref_count));
copy_packet_parser_metadata(srchdr, dsthdr);
+   return dsthdr->buf_hdr.uarea_size < srchdr->buf_hdr.uarea_size;


I think we should do this check before we do any copy


  }

  /**


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] packet copy questions

[Zoltan Kiss]

And what about this first question:

Should _odp_packet_copy_md_to_packet() copy timestamp and op_result?

On 07/06/16 13:03, Bill Fischofer wrote:

Sounds like this should be considered a bug. I'll open one to track it.

On Tue, Jun 7, 2016 at 2:18 AM, Savolainen, Petri (Nokia - FI/Espoo)
<petri.savolai...@nokia.com <mailto:petri.savolai...@nokia.com>> wrote:

/**
  * Full copy of a packet
  *
  * Create a new copy of the packet. The new packet is exact copy of
the source
  * packet (incl. data and metadata). The pool must have been
created with
  * ODP_POOL_PACKET type.
  *

All metadata should be copied into the new packet. User controls
user_area sizes in dst and src pools and should take care that dst
pool has always at least as much user area as the src pool (when
packets will be copied between those pools). This is no different
from the actual packet data, everything will be copied as long as
the dst pool can store as large packets as the src pool.

The function just fails if copy cannot be performed (all packet data
or metadata cannot be stored into dst pool).

-Petri



From: Bill Fischofer [mailto:bill.fischo...@linaro.org
<mailto:bill.fischo...@linaro.org>]
Sent: Monday, June 06, 2016 10:53 PM
    To: Zoltan Kiss <zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>
Cc: lng-odp <lng-odp@lists.linaro.org
<mailto:lng-odp@lists.linaro.org>>; Savolainen, Petri (Nokia -
FI/Espoo) <petri.savolai...@nokia.com
<mailto:petri.savolai...@nokia.com>>
Subject: Re: packet copy questions

The user area is explicitly reserved for use by the application. As
a result any copying or other manipulation of it is the
application's responsibility. The exception to this is ODP APIs that
implicitly make a new copy of a source packet (e.g., in response to
odp_packet_add_data() reallocating the packet) since the
reallocation is transparent to the application.

For the specific case of odp_packet_copy(), the reason why there is
no implied copy of the user area is that odp_packet_copy() allows
copies to be made in another pool and each pool may independently
specify the size of any user area associated with packets allocated
from it. So it's up to the application to decide what is appropriate
    in this case.

On Mon, Jun 6, 2016 at 2:35 PM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:
Hi,

During the ODP-DPDK 1.10 upgrade I found these two things:

- Should _odp_packet_copy_md_to_packet() copy timestamp and op_result?
- odp_packet_copy() doesn't copy the user area. I think it would be
better if it just calls _odp_packet_copy_md_to_packet()

Regards,

Zoltan



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] packet copy questions

[Zoltan Kiss]

Hi,

During the ODP-DPDK 1.10 upgrade I found these two things:

- Should _odp_packet_copy_md_to_packet() copy timestamp and op_result?
- odp_packet_copy() doesn't copy the user area. I think it would be 
better if it just calls _odp_packet_copy_md_to_packet()


Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [lng-odp-dpdk] [RFC PATCH] linux-dpdk: Enable inlines if dynamic library is not used

[Zoltan Kiss]

Hi,

On 30/05/16 02:46, Bill Fischofer wrote:


diff --git a/configure.ac  b/configure.ac

index 99772a1..199a6e2 100644
--- a/configure.ac 
+++ b/configure.ac 
@@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test
x$test_helper = xyes ])
 AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
 AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
 AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
+if test x$enable_shared != xyes;
+then
+INLINES="INLINES"
+else
+INLINES="__NOTHING"
+fi
+AC_SUBST(INLINES)

 ##
 # Setup doxygen documentation
diff --git a/platform/linux-dpdk/Makefile.am
b/platform/linux-dpdk/Makefile.am
index eadaf63..49ad877 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
  $(srcdir)/include/odp/api/hash.h \
  $(srcdir)/include/odp/api/hints.h \
  $(srcdir)/include/odp/api/init.h \
+ $(srcdir)/include/odp/api/inlines.h \
  $(srcdir)/include/odp/api/packet_flags.h \
  $(srcdir)/include/odp/api/packet.h \
+ $(srcdir)/include/odp/api/packet_inlines.h \
  $(srcdir)/include/odp/api/packet_io.h \
  $(srcdir)/include/odp/api/pool.h \
  $(srcdir)/include/odp/api/queue.h \
diff --git a/platform/linux-dpdk/include/odp/api/inlines.h.in

b/platform/linux-dpdk/include/odp/api/inlines.h.in

new file mode 100644
index 000..1e8bac8
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/api/inlines.h.in

@@ -0,0 +1,27 @@
+/* Copyright (c) 2016, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP packet inline functions
+ */



I just noticed, this comment is also wrong.


+
+#ifndef ODP_PLAT_INLINES_H_
+#define ODP_PLAT_INLINES_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define @INLINES@


Why the @ symbols here? Also, as an ODP configuration option, 
shouldn't this be something like ODP_INLINES to avoid potential name 
space issues?


AC_SUBST(INLINES) should replace @INLINES@ to the value determined in 
the configure script. That's why platform/linux-dpdk/m4/configure.m4 
includes this header file as well.
Yes, we can prepend it with "ODP_", but I would add an underscore to the 
beginning, because this supposed to be an internal configuration 
variable, applications are not supposed to use it directly. But they 
include the generated inlines.h, so when they get to packet_inlines.h, 
they'll know whether they should inline the functions or not.


Regards,

Zoli
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [RFC PATCH] linux-dpdk: Enable inlines if dynamic library is not used

[Zoltan Kiss]

In order to have proper packaging and dynamic linking we need a fixed ABI.
For that, each function has to be in the library file. But that hurts
performance a bit, as small accessor functions couldn't be inlined,
despite it's a viable option with static linking.
This patch lets the platform automatically decide what should be done.
./configure generates inlines.h based on whether --enable-shared=yes was
added or not. If yes, it enables the INLINES macro.
The accessors are moved to a packet_inlines.h file, by default it is
included from odp_packet.c, and they appear as fully fledged functions.
If INLINES defined, the accessors appear as static inline functions in the
header, and the application can directly inline them.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 .gitignore |  1 +
 configure.ac   |  7 ++
 platform/linux-dpdk/Makefile.am|  2 +
 platform/linux-dpdk/include/odp/api/inlines.h.in   | 27 +++
 platform/linux-dpdk/include/odp/api/packet.h   | 60 +--
 .../linux-dpdk/include/odp/api/packet_inlines.h| 87 ++
 platform/linux-dpdk/m4/configure.m4|  3 +-
 platform/linux-dpdk/odp_packet.c   |  3 +
 8 files changed, 133 insertions(+), 57 deletions(-)
 create mode 100644 platform/linux-dpdk/include/odp/api/inlines.h.in
 create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h

diff --git a/.gitignore b/.gitignore
index 2b1da19..4040e2b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,5 +40,6 @@ ltmain.sh
 m4/*.m4
 missing
 pkgconfig/libodp*.pc
+platform/linux-dpdk/include/odp/api/inlines.h
 tags
 test-driver
diff --git a/configure.ac b/configure.ac
index 99772a1..199a6e2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -141,6 +141,13 @@ AM_CONDITIONAL([test_helper], [test x$test_helper = xyes ])
 AM_CONDITIONAL([HAVE_DOXYGEN], [test "x${DOXYGEN}" = "xdoxygen"])
 AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ])
 AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"])
+if test x$enable_shared != xyes;
+then
+INLINES="INLINES"
+else
+INLINES="__NOTHING"
+fi
+AC_SUBST(INLINES)
 
 ##
 # Setup doxygen documentation
diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index eadaf63..49ad877 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -47,8 +47,10 @@ odpapiinclude_HEADERS = \
  $(srcdir)/include/odp/api/hash.h \
  $(srcdir)/include/odp/api/hints.h \
  $(srcdir)/include/odp/api/init.h \
+ $(srcdir)/include/odp/api/inlines.h \
  $(srcdir)/include/odp/api/packet_flags.h \
  $(srcdir)/include/odp/api/packet.h \
+ $(srcdir)/include/odp/api/packet_inlines.h \
  $(srcdir)/include/odp/api/packet_io.h \
  $(srcdir)/include/odp/api/pool.h \
  $(srcdir)/include/odp/api/queue.h \
diff --git a/platform/linux-dpdk/include/odp/api/inlines.h.in 
b/platform/linux-dpdk/include/odp/api/inlines.h.in
new file mode 100644
index 000..1e8bac8
--- /dev/null
+++ b/platform/linux-dpdk/include/odp/api/inlines.h.in
@@ -0,0 +1,27 @@
+/* Copyright (c) 2016, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP packet inline functions
+ */
+
+#ifndef ODP_PLAT_INLINES_H_
+#define ODP_PLAT_INLINES_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#define @INLINES@
+
+#ifdef __cplusplus
+}
+#endif
+
+
+#endif /* ODP_PLAT_INLINES_H_ */
diff --git a/platform/linux-dpdk/include/odp/api/packet.h 
b/platform/linux-dpdk/include/odp/api/packet.h
index 7c0db32..0fb31e3 100644
--- a/platform/linux-dpdk/include/odp/api/packet.h
+++ b/platform/linux-dpdk/include/odp/api/packet.h
@@ -27,62 +27,10 @@ extern "C" {
 /** @ingroup odp_packet
  *  @{
  */
-
-extern const unsigned int buf_addr_offset;
-extern const unsigned int data_off_offset;
-extern const unsigned int pkt_len_offset;
-extern const unsigned int seg_len_offset;
-extern const unsigned int udata_len_offset;
-extern const unsigned int udata_offset;
-extern const unsigned int rss_offset;
-extern const unsigned int ol_flags_offset;
-extern const uint64_t rss_flag;
-
-/*
- * NOTE: These functions are inlined because they are on a performance hot 
path.
- * As we can't force the application to directly include DPDK headers we have 
to
- * export these fields through constants calculated compile time in
- * odp_packet.c, where we can see the DPDK definitions.
- *
- */
-static inline uint32_t odp_packet_len(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
-}
-
-stati

[lng-odp] opendataplane.org "Downloads" page changes

[Zoltan Kiss]

Hi,

The odp-dpdk part of http://www.opendataplane.org/downloads/ needs updating:
- The patchworks links should be deleted, as nobody uses it, given that 
I'm the only maintainer and the only regular commiter.
- The "latest stable release link" is also very old, and doesn't fit for 
odp-dpdk: I only tag the LAST commit which supports a particular ODP 
API/DPDK version pair. I would definitely not call that "stable". This 
mode of operation has problems, but since nobody is interested to figure 
out a better way [1], I kept it. So I want to remove this link as well
- somewhere around the top left corner box, where the odp-linux and 
odp-dpdk links are, we should add the following text for clarification:


"odp-linux can interface with DPDK poll mode drivers, but it retains 
everything else as generic, including the buffer management 
implementation. Therefore it has to copy everything during receive and 
transmit between its own buffers and DPDK ones.
odp-dpdk is derived from odp-linux, but it is entirely DPDK focused, and 
tries to connect as much DPDK API's to ODP as possible. It uses DPDK 
buffer management underneath, so it doesn't need the aforementioned copy"



[1] https://lists.linaro.org/pipermail/lng-odp-dpdk/2015-October/001158.html


Zoli
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 1/2] linux-generic: includes: typo correction

[Zoltan Kiss]

Reviewed-by: Zoltan Kiss <zoltan.k...@linaro.org>

On 12/05/16 17:29, Bill Fischofer wrote:

Correct typo in spelling of _LARGEFILE64_SOURCE. This also removes an
extraneous non-ascii character from the source file.

Suggested-by: Zoltan Kiss <zoltan.k...@linaro.org>
Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org>
---
  platform/linux-generic/include/odp_posix_extensions.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/platform/linux-generic/include/odp_posix_extensions.h 
b/platform/linux-generic/include/odp_posix_extensions.h
index 4b6e335..2c468b4 100644
--- a/platform/linux-generic/include/odp_posix_extensions.h
+++ b/platform/linux-generic/include/odp_posix_extensions.h
@@ -17,7 +17,7 @@
   * Enable POSIX and GNU extensions
   *
   * This macro defines:
- *   o  _BSD_SOURCE, _SVID_SOURCE, _ATFILE_SOURCE, _LARGE‐FILE64_SOURCE,
+ *   o  _BSD_SOURCE, _SVID_SOURCE, _ATFILE_SOURCE, _LARGEFILE64_SOURCE,
   *  _ISOC99_SOURCE, _XOPEN_SOURCE_EXTENDED, _POSIX_SOURCE
   *   o  _POSIX_C_SOURCE with the value:
   ** 200809L since  glibc v2.10 (== POSIX.1-2008 base specification)


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] OpenDataPlane is now on Wikipedia

[Zoltan Kiss]

Hi,

The link to Wind River is broken, it should point here:

https://en.wikipedia.org/wiki/Wind_River_Systems

On 12/05/16 13:27, Raj Murali wrote:

Dear All,

I am happy to inform you all that, we are now on Wikipedia - at last.
https://en.wikipedia.org/wiki/Open_Data_Plane_(ODP)

Thanks to all for sharing links and contributing content used in the page.

Raj Murali
Director - Linaro Networking Group
Skype: rajmurali_s  │ M: +91 98450 70135

Linaro.org ***│ *Open source software for ARM
SoCs




___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] Non-ASCII characters in the repo

[Zoltan Kiss]

Hi,

I've came accross a lot in the doc folder, I don't know if we want to do 
anything about them, but here is an easy way to find them:

git grep --color='auto' -P -n "[\x80-\xFF]"

There is only one place outside docs:

diff --git a/platform/linux-generic/include/odp_posix_extensions.h 
b/platform/linux-generic/include/odp_posix_extensions.h
index 4b6e335..426c412 100644
--- a/platform/linux-generic/include/odp_posix_extensions.h
+++ b/platform/linux-generic/include/odp_posix_extensions.h
@@ -17,7 +17,7 @@
  * Enable POSIX and GNU extensions
  *
  * This macro defines:
- *   o  _BSD_SOURCE, _SVID_SOURCE, _ATFILE_SOURCE, _LARGE‐FILE64_SOURCE,
+ *   o  _BSD_SOURCE, _SVID_SOURCE, _ATFILE_SOURCE, _LARGE-FILE64_SOURCE,
  *  _ISOC99_SOURCE, _XOPEN_SOURCE_EXTENDED, _POSIX_SOURCE
  *   o  _POSIX_C_SOURCE with the value:
  ** 200809L since  glibc v2.10 (== POSIX.1-2008 base specification)



Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] run checkpatch automatically

[Zoltan Kiss]

Hi,

As Mike raised my attention, I'm sloppy in that, so I decided to integrate it 
with my git wrapper.
It is a shell script in my home directory which precedes git so I can enforce 
stuff not configurable for git.
git hooks can do similar stuff, but they have to be created for each 
repository, while this works on all of them.

$ cat ~/bin/git
#!/bin/sh

command=$1
shift

case $command in
*clean)
# don't clean out Eclipse settings
/usr/bin/git $command -e .cproject -e .project -e .settings "$@"
;;
*commit)
# if there is checkpatch, don't allow commit when it fails
if [ -e scripts/checkpatch.pl ]; then
/usr/bin/git diff --cached | scripts/checkpatch.pl --no-signoff 
-q -
if [ $? -ne 0 ]; then
exit
fi
fi
/usr/bin/git $command "$@"
if [ $? -ne 0 ]; then
exit
fi
# run checkpatch again to look at the commit message
if [ -e scripts/checkpatch.pl ]; then
/usr/bin/git format-patch -1 --stdout | scripts/checkpatch.pl 
--no-signoff -q -
fi
;;
*)
/usr/bin/git $command "$@"
;;
esac
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2 3/3] linux-generic: pktio: don't return packet after failed enqueue

[Zoltan Kiss]

If queue_enq() fails, there is a serious internal error, the packet
should be released instead of passing it back to pktin_poll() for
enqueueing on an another one.
Also return the error if there is any, not just the queue_enq() ones.
If CoS drops the packet, doesn't report it as an error

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

v2:
- rebase
- don't report discards as failure
- add comment

 .../linux-generic/include/odp_packet_io_internal.h |  3 +-
 platform/linux-generic/pktio/dpdk.c| 10 +++---
 platform/linux-generic/pktio/netmap.c  |  8 ++---
 platform/linux-generic/pktio/pktio_common.c| 36 +-
 platform/linux-generic/pktio/socket.c  |  9 +++---
 platform/linux-generic/pktio/socket_mmap.c |  7 ++---
 6 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_io_internal.h 
b/platform/linux-generic/include/odp_packet_io_internal.h
index de6dab6..7dd578b 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -211,8 +211,7 @@ typedef struct pktio_if_ops {
 } pktio_if_ops_t;
 
 int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
-   uint16_t buf_len, odp_time_t *ts,
-   odp_packet_t *pkt_ret);
+   uint16_t buf_len, odp_time_t *ts);
 
 extern void *pktio_entry_ptr[];
 
diff --git a/platform/linux-generic/pktio/dpdk.c 
b/platform/linux-generic/pktio/dpdk.c
index 5315297..57c7d6e 100644
--- a/platform/linux-generic/pktio/dpdk.c
+++ b/platform/linux-generic/pktio/dpdk.c
@@ -712,10 +712,12 @@ static inline int mbuf_to_pkt(pktio_entry_t *pktio_entry,
pkt_len = rte_pktmbuf_pkt_len(mbuf);
 
if (pktio_cls_enabled(pktio_entry)) {
-   if (_odp_packet_cls_enq(pktio_entry,
-   (const uint8_t *)buf, pkt_len,
-   ts, _table[nb_pkts]))
-   nb_pkts++;
+   int ret;
+   ret = _odp_packet_cls_enq(pktio_entry,
+ (const uint8_t *)buf,
+ pkt_len, ts);
+   if (ret && ret != -ENOENT)
+   nb_pkts = ret;
} else {
pkt = packet_alloc(pktio_entry->s.pkt_dpdk.pool,
   pkt_len, 1);
diff --git a/platform/linux-generic/pktio/netmap.c 
b/platform/linux-generic/pktio/netmap.c
index 6065922..3e6a4f0 100644
--- a/platform/linux-generic/pktio/netmap.c
+++ b/platform/linux-generic/pktio/netmap.c
@@ -608,10 +608,10 @@ static inline int netmap_pkt_to_odp(pktio_entry_t 
*pktio_entry,
 
if (pktio_cls_enabled(pktio_entry)) {
ret = _odp_packet_cls_enq(pktio_entry, (const uint8_t *)buf,
- len, ts, pkt_out);
-   if (ret)
-   return 0;
-   return -1;
+ len, ts);
+   if (ret && ret != -ENOENT)
+   return ret;
+   return 0;
} else {
odp_packet_hdr_t *pkt_hdr;
 
diff --git a/platform/linux-generic/pktio/pktio_common.c 
b/platform/linux-generic/pktio/pktio_common.c
index 942ea8c..8a01477 100644
--- a/platform/linux-generic/pktio/pktio_common.c
+++ b/platform/linux-generic/pktio/pktio_common.c
@@ -7,10 +7,27 @@
 
 #include 
 #include 
+#include 
 
+/**
+ * Classify packet, copy it in a odp_packet_t and enqueue to the right queue
+ *
+ * pktio_entry pktio where it arrived
+ * basepacket data
+ * buf_len packet length
+ *
+ * Return values:
+ * 0 on success, packet is consumed
+ * -ENOENT CoS dropped the packet
+ * -EFAULT Bug
+ * -EINVAL Config error
+ * -ENOMEM Target CoS pool exhausted
+ *
+ * Note: *base is not released, only pkt if there is an error
+ *
+ */
 int _odp_packet_cls_enq(pktio_entry_t *pktio_entry,
-   const uint8_t *base, uint16_t buf_len,
-   odp_time_t *ts, odp_packet_t *pkt_ret)
+   const uint8_t *base, uint16_t buf_len, odp_time_t *ts)
 {
cos_t *cos;
odp_packet_t pkt;
@@ -25,14 +42,17 @@ int _odp_packet_cls_enq(pktio_entry_t *pktio_entry,
cos = pktio_select_cos(pktio_entry, base, _pkt_hdr);
 
/* if No CoS found then drop the packet */
-   if (cos == NULL || cos->s.queue == NULL || cos->s.pool == NULL)
-   return 0;
+   if (cos == NULL)
+   return -EINVAL;
+
+   if (cos->s.queue == NULL || cos->s.pool == NULL)
+   return -EFAULT;
 
pool = cos->s.pool->s.pool_hdl;
 
pkt = odp_packet_alloc(p

[lng-odp] [PATCH v2 2/3] linux-generic: pktio: classification error handling fixes for loop

[Zoltan Kiss]

Several minor changes:

- remove 'int j' as it no longer has function
- add 'int failed' to count errors, and return it (or 0 if no failure)
- in_errors counts the packet numbers, not the octets
- in_discards the same, count it with 'discarded'
- in_ucast_pkts should not count these packets
- as a side effect, separate this code path from normal handling completely

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

v2:
- rebase
- fixs discards as well

 platform/linux-generic/pktio/loop.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index f7ccbd9..f4883c1 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -51,7 +51,7 @@ static int loopback_close(pktio_entry_t *pktio_entry)
 static int loopback_recv(pktio_entry_t *pktio_entry, odp_packet_t pkts[],
 unsigned len)
 {
-   int nbr, i, j;
+   int nbr, i;
odp_buffer_hdr_t *hdr_tbl[QUEUE_MULTI_MAX];
queue_entry_t *qentry;
odp_packet_hdr_t *pkt_hdr;
@@ -72,7 +72,8 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
}
 
if (pktio_cls_enabled(pktio_entry)) {
-   for (i = 0, j = 0; i < nbr; i++) {
+   int failed = 0, discarded = 0;
+   for (i = 0; i < nbr; i++) {
int ret;
pkt = _odp_packet_from_buffer(odp_hdr_to_buf
  (hdr_tbl[i]));
@@ -87,18 +88,19 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
odp_packet_len(pkt);
break;
case -ENOENT:
-   pktio_entry->s.stats.in_discards +=
-   odp_packet_len(pkt);
+   discarded++;
break;
case -EFAULT:
-   pktio_entry->s.stats.in_errors +=
-   odp_packet_len(pkt);
+   failed++;
break;
default:
ret = queue_enq(qentry, hdr_tbl[i], 0);
}
}
-   nbr = j;
+   pktio_entry->s.stats.in_errors += failed;
+   pktio_entry->s.stats.in_discards += discarded;
+   pktio_entry->s.stats.in_ucast_pkts += nbr - failed - discarded;
+   return -failed;
} else {
for (i = 0; i < nbr; ++i) {
pkts[i] = _odp_packet_from_buffer(odp_hdr_to_buf
@@ -110,11 +112,9 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkts[i]);
}
+   pktio_entry->s.stats.in_ucast_pkts += nbr;
+   return nbr;
}
-
-   pktio_entry->s.stats.in_ucast_pkts += nbr;
-
-   return nbr;
 }
 
 static int loopback_send(pktio_entry_t *pktio_entry,
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2 1/3] linux-generic: classification: release the packet as soon as an error happens

[Zoltan Kiss]

Move release to _odp_packet_classifier(), because caller has no way to
know if new_pkt were allocated. In that case there would be a double free
on pkt, and new_pkt would be leaked.
The only exception is when cos == NULL or odp_packet_copy() fails, in that case
the packet is reenqued.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

v2:
- add comment for this function
- use errno.h values
- let caller handle EINVAL and ENOMEM
- loopback reenques them
- fix in stats that CoS drop means actually discard

 platform/linux-generic/odp_classification.c | 41 ++---
 platform/linux-generic/pktio/loop.c | 16 ---
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 25e1c2c..a3fe545 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #define LOCK(a)  odp_spinlock_lock(a)
@@ -746,6 +747,20 @@ int pktio_classifier_init(pktio_entry_t *entry)
return 0;
 }
 
+/**
+ * Classify packet and enqueue to the right queue
+ *
+ * entry   pktio where it arrived
+ * pkt packet handle
+ *
+ * Return values:
+ * 0 on success, packet is consumed
+ * -ENOENT CoS dropped the packet
+ * -EFAULT Bug, packet is released
+ * -EINVAL Config error, packet is NOT released
+ * -ENOMEM Target CoS pool exhausted, packet is NOT released
+ */
+
 int _odp_packet_classifier(pktio_entry_t *entry, odp_packet_t pkt)
 {
queue_entry_t *queue;
@@ -754,8 +769,10 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
odp_packet_t new_pkt;
uint8_t *pkt_addr;
 
-   if (entry == NULL)
-   return -1;
+   if (entry == NULL) {
+   odp_packet_free(pkt);
+   return -EFAULT;
+   }
 
pkt_hdr = odp_packet_hdr(pkt);
pkt_addr = odp_packet_data(pkt);
@@ -763,18 +780,17 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
/* Matching PMR and selecting the CoS for the packet*/
cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
if (cos == NULL)
-   return -1;
+   return -EINVAL;
 
-   if (cos->s.pool == NULL)
-   return -1;
-
-   if (cos->s.queue == NULL)
-   return -1;
+   if (cos->s.queue == NULL || cos->s.pool == NULL) {
+   odp_packet_free(pkt);
+   return -ENOENT;
+   }
 
if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
new_pkt = odp_packet_copy(pkt, cos->s.pool->s.pool_hdl);
if (new_pkt == ODP_PACKET_INVALID)
-   return -1;
+   return -ENOMEM;
odp_packet_free(pkt);
} else {
new_pkt = pkt;
@@ -782,7 +798,12 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
 
/* Enqueuing the Packet based on the CoS */
queue = cos->s.queue;
-   return queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+   if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0)) {
+   odp_packet_free(new_pkt);
+   return -EFAULT;
+   } else {
+   return 0;
+   }
 }
 
 cos_t *pktio_select_cos(pktio_entry_t *entry, const uint8_t *pkt_addr,
diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 5819e66..f7ccbd9 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -73,19 +73,29 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
 
if (pktio_cls_enabled(pktio_entry)) {
for (i = 0, j = 0; i < nbr; i++) {
+   int ret;
pkt = _odp_packet_from_buffer(odp_hdr_to_buf
  (hdr_tbl[i]));
pkt_hdr = odp_packet_hdr(pkt);
packet_parse_reset(pkt_hdr);
packet_parse_l2(pkt_hdr);
-   if (!_odp_packet_classifier(pktio_entry, pkt)) {
+   ret = _odp_packet_classifier(pktio_entry, pkt);
+   switch (ret) {
+   case 0:
packet_set_ts(pkt_hdr, ts);
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkt);
-   } else {
+   break;
+   case -ENOENT:
+   pktio_entry->s.stats.in_discards +=
+   odp_packet_len(pkt);
+   break;
+   case -EFAULT:

[lng-odp] [PATCH v2 0/3] Fix pktio classification error handling

[Zoltan Kiss]

Fixing up things in error handling, and harmonizing different codebases.

Zoltan Kiss (3):
  linux-generic: classification: release the packet as soon as an error
happens
  linux-generic: pktio: classification error handling fixes for loop
  linux-generic: pktio: don't return packet after failed enqueue

 .../linux-generic/include/odp_packet_io_internal.h |  3 +-
 platform/linux-generic/odp_classification.c| 41 --
 platform/linux-generic/pktio/dpdk.c| 10 +++---
 platform/linux-generic/pktio/loop.c| 34 +++---
 platform/linux-generic/pktio/netmap.c  |  8 ++---
 platform/linux-generic/pktio/pktio_common.c| 36 ++-
 platform/linux-generic/pktio/socket.c  |  9 +++--
 platform/linux-generic/pktio/socket_mmap.c |  7 ++--
 8 files changed, 99 insertions(+), 49 deletions(-)

-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 1/3] linux-generic: classification: release the packet as soon as an error happens

[Zoltan Kiss]

On 02/05/16 09:56, Bala Manoharan wrote:


On 29 April 2016 at 18:28, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:



On 28/04/16 18:08, Bala Manoharan wrote:


On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>>
wrote:



 On 28/04/16 10:29, Bala Manoharan wrote:


     On 27 April 2016 at 21:43, Zoltan Kiss
<zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>
 <mailto:zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>
 <mailto:zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org> <mailto:zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>>>
 wrote:

  Move release to _odp_packet_classifier(), because
caller
 has no way to
  know if new_pkt were allocated. In that case there
would be
 a double
  free
  on pkt, and new_pkt would be leaked.


 I am little skeptical about this classifier module
freeing up
 the packet
 since the error in CoS can only happen during a wrong
 configuration by
 the application

 I'm not sure this is wrong config:
 - cos == NULL shouldn't happen, my understanding is that
not setting
 a default queue is impossible (although I haven't found it
spelled
 out in the header files explicitly), so this is an internal
error


Whenever application creates a pktio with enable_classifier
field set in
odp_pktio_params_t then it should call the function
   odp_pktio_default_cos_set() to set the default CoS with the
pktio. The
scenario of cos == NULL will only occur if a pktio is created with
classification enabled and default CoS not set and hence this is a
configuration error.


So you say in this case these packets should end up on
entry->s.in_queue[index[idx]].queue in pktin_poll()? Why? Or do you
have something else in mind?
I would say it's better if different pktio's handle this the same way.


I am saying we should not generalise the handling for all the different
pktio, we need to move this handling to the pktio level so that if in
future some pktio wants to change the behaviour it will be possible.


I don't think our users would be happy if different pktios would handle 
the same error in different ways. Do you have an example in mind where 
it is necessary?



The
configuration error is a serious issue and applications will be
interested to raise alarms for this scenario.


They will be, we'll return an error, which will trigger a log message in 
pktin_poll(). This won't change.

Do you want a separate error message for the cos == NULL scenario?






 - (cos->s.queue == NULL || cos->s.pool == NULL) means the
packet
 should be dropped. Although I don't know how drop_policy should
 affect this, it's not clear from the description of
odp_cls_cos_create()

 - (entry == NULL) is impossible at the moment, but as you
said, in
 the future other callers might make this mistake, so better
to be
 prepared.
 - odp_packet_copy() fails means we can't put this packet
where it
 supposed to arrive, I think it would be very confusing for the
 application to receive it then
 - queue_enq() failure is again a sign of serious issues


pool or queue error could also be transient the pool could get empty


This is not an overload issue, as odp_cls_cos_create() says:

"@note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for
queue and pool associated with a class of service and when any one
of these values are configured as INVALID then the packets assigned
to the CoS gets dropped."


The packets are dropped only when pool handle is invalid,


The above quoted text contradicts that, do you plan to change it with a 
patch to API-NEXT?



but in the
case of odp_packet_copy() or queue_enq() error the pool or queue is not
invalid it is because the pool does not have sufficient buffers.


If you look into queue_enq(), it returns non-zero if the queue status is 
screwed up, it has nothing to do with pool exhaustion.


Packet

pools are allocated and deallocated during packet handling and if the
rate of incoming packets is higher for a small amount of time the packet
pool might get empty and odp_packet_copy() API will fail and in this
scenario the pktio can decide e

Re: [lng-odp] [PATCH 1/3] linux-generic: classification: release the packet as soon as an error happens

[Zoltan Kiss]

On 28/04/16 18:08, Bala Manoharan wrote:


On 28 April 2016 at 21:50, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:



On 28/04/16 10:29, Bala Manoharan wrote:


On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>>
wrote:

 Move release to _odp_packet_classifier(), because caller
has no way to
 know if new_pkt were allocated. In that case there would be
a double
 free
 on pkt, and new_pkt would be leaked.


I am little skeptical about this classifier module freeing up
the packet
since the error in CoS can only happen during a wrong
configuration by
the application

I'm not sure this is wrong config:
- cos == NULL shouldn't happen, my understanding is that not setting
a default queue is impossible (although I haven't found it spelled
out in the header files explicitly), so this is an internal error


Whenever application creates a pktio with enable_classifier field set in
odp_pktio_params_t then it should call the function
  odp_pktio_default_cos_set() to set the default CoS with the pktio. The
scenario of cos == NULL will only occur if a pktio is created with
classification enabled and default CoS not set and hence this is a
configuration error.


So you say in this case these packets should end up on 
entry->s.in_queue[index[idx]].queue in pktin_poll()? Why? Or do you have 
something else in mind?

I would say it's better if different pktio's handle this the same way.



- (cos->s.queue == NULL || cos->s.pool == NULL) means the packet
should be dropped. Although I don't know how drop_policy should
affect this, it's not clear from the description of odp_cls_cos_create()

- (entry == NULL) is impossible at the moment, but as you said, in
the future other callers might make this mistake, so better to be
prepared.
- odp_packet_copy() fails means we can't put this packet where it
supposed to arrive, I think it would be very confusing for the
application to receive it then
- queue_enq() failure is again a sign of serious issues


pool or queue error could also be transient the pool could get empty


This is not an overload issue, as odp_cls_cos_create() says:

"@note ODP_QUEUE_INVALID and ODP_POOL_INVALID are valid values for queue 
and pool associated with a class of service and when any one of these 
values are configured as INVALID then the packets assigned to the CoS 
gets dropped."



after sometime depending upon the load in the network. Imagine a
scenario where the application sends a decrypted packet into the
loopback device for classification and would not want the packet to be
dropped immediately on transient pool exhaustion but would prefer
delayed sending of the packet.


Or do you mean when odp_packet_copy() fails? (This second part suggest 
that) Still, how would that happen? Currently this call graph looks like 
this:


schedule()
pktin_poll() [currently it can place returned packets on 
entry->s.in_queue[index[idx]].queue]

odp_pktin_recv()
[pktio's receive function]
[_odp_packet_classifier() or _odp_packet_cls_enq(), depending on your pktio]

At which level should we handle this and how?

(And I think we still should drop the packets in the other error cases 
I've mentioned, in an unified way for all pktios)






and hence it would be better if the error is percolated
to the application

The error itself is returned through odp_pktin_recv() when you apply
the next two patch, but as we discussed if classification is enabled
it shouldn't be called directly. pktin_poll() will notice that
though, and print a message, but it won't enqueue when queue_enq()
fails, see the 3rd patch.

so that he can take some intelligent action ( either
reconfigure classification and send the packet again or free the
packet ).


This function is only used on the receive side, I'm not sure what
you mean by "send the packet again".


The application might want some specific packets not be dropped under
any scenario even when there is a rate limiting and in case of loopback
mode the classifier module is attached directly to the output as if
there is a physical loopback and hence in that scenario the loopback
device might want to send the packet again after sometime when the pool
or queue gets empty rather than dropping it immediately.


So you say the loopback receive function should get back the packets 
when odp_packet_copy() fails, and send it again to loopback?
That's a hard question whether its a good idea or not. You can count on 
the fact that the packets are never lost there, but you are risking 
recovery time during an overload scenario (or m

Re: [lng-odp] [PATCH 1/3] linux-generic: classification: release the packet as soon as an error happens

[Zoltan Kiss]

On 28/04/16 10:29, Bala Manoharan wrote:


On 27 April 2016 at 21:43, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

Move release to _odp_packet_classifier(), because caller has no way to
know if new_pkt were allocated. In that case there would be a double
free
on pkt, and new_pkt would be leaked.


I am little skeptical about this classifier module freeing up the packet
since the error in CoS can only happen during a wrong configuration by
the application

I'm not sure this is wrong config:
- cos == NULL shouldn't happen, my understanding is that not setting a 
default queue is impossible (although I haven't found it spelled out in 
the header files explicitly), so this is an internal error
- (cos->s.queue == NULL || cos->s.pool == NULL) means the packet should 
be dropped. Although I don't know how drop_policy should affect this, 
it's not clear from the description of odp_cls_cos_create()
- (entry == NULL) is impossible at the moment, but as you said, in the 
future other callers might make this mistake, so better to be prepared.
- odp_packet_copy() fails means we can't put this packet where it 
supposed to arrive, I think it would be very confusing for the 
application to receive it then

- queue_enq() failure is again a sign of serious issues


and hence it would be better if the error is percolated
to the application
The error itself is returned through odp_pktin_recv() when you apply the 
next two patch, but as we discussed if classification is enabled it 
shouldn't be called directly. pktin_poll() will notice that though, and 
print a message, but it won't enqueue when queue_enq() fails, see the 
3rd patch.



so that he can take some intelligent action ( either
reconfigure classification and send the packet again or free the packet ).


This function is only used on the receive side, I'm not sure what you 
mean by "send the packet again".





Regarding the issue of freeing up the packet when a new packet is
created by the classification module, we can better change the signature
of _odp_packet_classifier() API to receive odp_packet_t as a pointer and
update the pointer with new packet.

The main reason I am against freeing up the packet inside classification
module is that this function is currently called only by loopback device
but in future development it could be called from other sources also and
hence it is better if freeing of the packet during error is done by the
caller rather than classification module.


I think we should implement this when we get there, and see it fit 
better. Currently it's broken anyway.
Plus, why would it be better to release by the caller, rather than on 
the spot? As I explained above, these error cases seems to be major 
internal issues, none of them seem like the caller can do anything.




Regards,
Bala


    Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>
---
  platform/linux-generic/odp_classification.c | 21 -
  platform/linux-generic/pktio/loop.c |  1 -
  2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c
b/platform/linux-generic/odp_classification.c
index 3a18a78..a1466fd 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -734,22 +734,20 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)
 odp_packet_t new_pkt;
 uint8_t *pkt_addr;

-   if (entry == NULL)
+   if (entry == NULL) {
+   odp_packet_free(pkt);
 return -1;
+   }

 pkt_hdr = odp_packet_hdr(pkt);
 pkt_addr = odp_packet_data(pkt);

 /* Matching PMR and selecting the CoS for the packet*/
 cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
-   if (cos == NULL)
-   return -1;
-
-   if (cos->s.pool == NULL)
-   return -1;
-
-   if (cos->s.queue == NULL)
+   if (cos == NULL || cos->s.queue == NULL || cos->s.pool ==
NULL) {
+   odp_packet_free(pkt);
 return -1;
+   }

 if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
 new_pkt = odp_packet_copy(pkt,
cos->s.pool->s.pool_hdl);
@@ -762,7 +760,12 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)

 /* Enqueuing the Packet based on the CoS */
 queue = cos->s.queue;
-   return queue_enq(queue,
odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+   if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt),
0)) {
+   odp_packet_free(new_pkt);
+   return -1;
+   } else {
+   return 0;
+   }

[lng-odp] [PATCH 3/3] linux-generic: pktio: don't return packet after failed enqueue

[Zoltan Kiss]

If queue_enq() fails, there is a serious internal error, the packet
should be released instead of passing it back to pktin_poll() for
enqueueing on an another one.
Also return the error if there is any, not just the queue_enq() ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_packet_io_internal.h |  2 +-
 platform/linux-generic/pktio/pktio_common.c | 13 ++---
 platform/linux-generic/pktio/socket.c   |  5 ++---
 platform/linux-generic/pktio/socket_mmap.c  |  4 ++--
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_io_internal.h 
b/platform/linux-generic/include/odp_packet_io_internal.h
index cca5c39..19b0551 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -169,7 +169,7 @@ typedef struct pktio_if_ops {
 } pktio_if_ops_t;
 
 int _odp_packet_cls_enq(pktio_entry_t *pktio_entry, const uint8_t *base,
-   uint16_t buf_len, odp_packet_t *pkt_ret);
+   uint16_t buf_len);
 
 extern void *pktio_entry_ptr[];
 
diff --git a/platform/linux-generic/pktio/pktio_common.c 
b/platform/linux-generic/pktio/pktio_common.c
index c568da3..394ee63 100644
--- a/platform/linux-generic/pktio/pktio_common.c
+++ b/platform/linux-generic/pktio/pktio_common.c
@@ -9,8 +9,7 @@
 #include 
 
 int _odp_packet_cls_enq(pktio_entry_t *pktio_entry,
-   const uint8_t *base, uint16_t buf_len,
-   odp_packet_t *pkt_ret)
+   const uint8_t *base, uint16_t buf_len)
 {
cos_t *cos;
odp_packet_t pkt;
@@ -25,28 +24,28 @@ int _odp_packet_cls_enq(pktio_entry_t *pktio_entry,
 
/* if No CoS found then drop the packet */
if (cos == NULL || cos->s.queue == NULL || cos->s.pool == NULL)
-   return 0;
+   return -1;
 
pool = cos->s.pool->s.pool_hdl;
 
pkt = odp_packet_alloc(pool, buf_len);
if (odp_unlikely(pkt == ODP_PACKET_INVALID))
-   return 0;
+   return -1;
 
copy_packet_parser_metadata(_hdr, odp_packet_hdr(pkt));
odp_packet_hdr(pkt)->input = pktio_entry->s.handle;
 
if (odp_packet_copydata_in(pkt, 0, buf_len, base) != 0) {
odp_packet_free(pkt);
-   return 0;
+   return -1;
}
 
/* Parse and set packet header data */
odp_packet_pull_tail(pkt, odp_packet_len(pkt) - buf_len);
ret = queue_enq(cos->s.queue, odp_buf_to_hdr((odp_buffer_t)pkt), 0);
if (ret < 0) {
-   *pkt_ret = pkt;
-   return 1;
+   odp_packet_free(pkt);
+   return -1;
}
 
return 0;
diff --git a/platform/linux-generic/pktio/socket.c 
b/platform/linux-generic/pktio/socket.c
index edc36a1..6563565 100644
--- a/platform/linux-generic/pktio/socket.c
+++ b/platform/linux-generic/pktio/socket.c
@@ -649,10 +649,9 @@ static int sock_mmsg_recv(pktio_entry_t *pktio_entry,
eth_hdr->h_source)))
continue;
 
-   ret = _odp_packet_cls_enq(pktio_entry, base,
- pkt_len, _table[nb_rx]);
+   ret = _odp_packet_cls_enq(pktio_entry, base, pkt_len);
if (ret)
-   nb_rx++;
+   nb_rx = -1;
}
} else {
struct iovec iovecs[ODP_PACKET_SOCKET_MAX_BURST_RX]
diff --git a/platform/linux-generic/pktio/socket_mmap.c 
b/platform/linux-generic/pktio/socket_mmap.c
index caca708..d7ca865 100644
--- a/platform/linux-generic/pktio/socket_mmap.c
+++ b/platform/linux-generic/pktio/socket_mmap.c
@@ -149,9 +149,9 @@ static inline unsigned pkt_mmap_v2_rx(pktio_entry_t 
*pktio_entry,
 
if (pktio_cls_enabled(pktio_entry)) {
ret = _odp_packet_cls_enq(pktio_entry, pkt_buf,
- pkt_len, _table[nb_rx]);
+ pkt_len);
if (ret)
-   nb_rx++;
+   nb_rx = -1;
} else {
odp_packet_hdr_t *hdr;
 
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 1/3] linux-generic: classification: release the packet as soon as an error happens

[Zoltan Kiss]

Move release to _odp_packet_classifier(), because caller has no way to
know if new_pkt were allocated. In that case there would be a double free
on pkt, and new_pkt would be leaked.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/odp_classification.c | 21 -
 platform/linux-generic/pktio/loop.c |  1 -
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 3a18a78..a1466fd 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -734,22 +734,20 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
odp_packet_t new_pkt;
uint8_t *pkt_addr;
 
-   if (entry == NULL)
+   if (entry == NULL) {
+   odp_packet_free(pkt);
return -1;
+   }
 
pkt_hdr = odp_packet_hdr(pkt);
pkt_addr = odp_packet_data(pkt);
 
/* Matching PMR and selecting the CoS for the packet*/
cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
-   if (cos == NULL)
-   return -1;
-
-   if (cos->s.pool == NULL)
-   return -1;
-
-   if (cos->s.queue == NULL)
+   if (cos == NULL || cos->s.queue == NULL || cos->s.pool == NULL) {
+   odp_packet_free(pkt);
return -1;
+   }
 
if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
new_pkt = odp_packet_copy(pkt, cos->s.pool->s.pool_hdl);
@@ -762,7 +760,12 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
 
/* Enqueuing the Packet based on the CoS */
queue = cos->s.queue;
-   return queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+   if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0)) {
+   odp_packet_free(new_pkt);
+   return -1;
+   } else {
+   return 0;
+   }
 }
 
 cos_t *pktio_select_cos(pktio_entry_t *entry, const uint8_t *pkt_addr,
diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index f6a8c1d..676e98b 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -76,7 +76,6 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
} else {
pktio_entry->s.stats.in_errors +=
odp_packet_len(pkt);
-   odp_packet_free(pkt);
}
}
nbr = j;
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 2/3] linux-generic: pktio: classification error handling fixes for loop

[Zoltan Kiss]

Several minor changes:

- remove 'int j' as it no longer has function
- add 'int failed' to count errors, and return it (or 0 if no failure)
- in_errors counts the packet numbers, not the octets
- in_ucast_pkts should not count these failed packets
- as a side effect, separate this code path from normal handling completely

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/pktio/loop.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 676e98b..60bd4b4 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -51,7 +51,7 @@ static int loopback_close(pktio_entry_t *pktio_entry)
 static int loopback_recv(pktio_entry_t *pktio_entry, odp_packet_t pkts[],
 unsigned len)
 {
-   int nbr, i, j;
+   int nbr, i;
odp_buffer_hdr_t *hdr_tbl[QUEUE_MULTI_MAX];
queue_entry_t *qentry;
odp_packet_hdr_t *pkt_hdr;
@@ -64,7 +64,8 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
nbr = queue_deq_multi(qentry, hdr_tbl, len);
 
if (pktio_cls_enabled(pktio_entry)) {
-   for (i = 0, j = 0; i < nbr; i++) {
+   int failed = 0;
+   for (i = 0; i < nbr; i++) {
pkt = _odp_packet_from_buffer(odp_hdr_to_buf
  (hdr_tbl[i]));
pkt_hdr = odp_packet_hdr(pkt);
@@ -74,11 +75,12 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkt);
} else {
-   pktio_entry->s.stats.in_errors +=
-   odp_packet_len(pkt);
+   failed--;
}
}
-   nbr = j;
+   pktio_entry->s.stats.in_errors += failed;
+   pktio_entry->s.stats.in_ucast_pkts += nbr - failed;
+   return failed;
} else {
for (i = 0; i < nbr; ++i) {
pkts[i] = _odp_packet_from_buffer(odp_hdr_to_buf
@@ -89,11 +91,9 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkts[i]);
}
+   pktio_entry->s.stats.in_ucast_pkts += nbr;
+   return nbr;
}
-
-   pktio_entry->s.stats.in_ucast_pkts += nbr;
-
-   return nbr;
 }
 
 static int loopback_send(pktio_entry_t *pktio_entry, odp_packet_t pkt_tbl[],
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 0/3] Fix pktio classification error handling

[Zoltan Kiss]

Hopefully we covered every case now.

Zoltan Kiss (3):
  linux-generic: classification: release the packet as soon as an error
happens
  linux-generic: pktio: classification error handling fixes for loop
  linux-generic: pktio: don't return packet after failed enqueue

 .../linux-generic/include/odp_packet_io_internal.h  |  2 +-
 platform/linux-generic/odp_classification.c | 21 -
 platform/linux-generic/pktio/loop.c | 19 +--
 platform/linux-generic/pktio/pktio_common.c | 13 ++---
 platform/linux-generic/pktio/socket.c   |  5 ++---
 platform/linux-generic/pktio/socket_mmap.c  |  4 ++--
 6 files changed, 32 insertions(+), 32 deletions(-)

-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] classification questions

[Zoltan Kiss]

Bala, any opinions?

On 25/04/16 18:51, Zoltan Kiss wrote:


Hi,

This is what we talked about at the arch call, I summarized it up what
I've found, so hopefully it'll be easier for you to comment:

Socket, socket_mmap and dpdk pktio uses _odp_packet_cls_enq(), it can
have 3 different returns:
- pktio_select_cos(), odp_packet_alloc() or odp_packet_copydata_in()
problem: returns 0, the caller then leaks the memory passed through
*base, I think we should fix that. Plus, I think we should pass back
some of these errors through odp_pktin_recv(). At least the alloc and
copydata, not sure about pktio_select_cos(). What do you think?
- queue_enq() succeeds, and then it returns 0, the caller don't have to
care. That's OK
- queue_enq() fails, caller passes it back to pktin_poll(), which puts
it on the pktio_entry->s.in_queue[...].queue. Is this what we want? Or
should we rather drop the packet? What is the rationale for either
decision?

On the other hand, _odp_packet_classifier() (which is used by loopback,
and I want to use it for ODP-DPDK) works a bit differently:
- pktio_select_cos(), odp_packet_alloc() or odp_packet_copydata_in()
problem AND queue_enq() failures: returns -1, the caller releases the
buffer and update error stats. pktin_poll() never sees the packets
failed in queue_enq(). This is the same question like above, we should
have some consistency. Plus again, we might want to escalate the alloc
and copydata failures.
- queue_enq() succeeds, caller doesn't care anymore.

Regards,

Zoltan



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [RFC PATCH] linux-dpdk: add --enable-inlines configure option

[Zoltan Kiss]

Another option we just discussed with Mike: how about we use the 
--enable-dynamic configure option instead of this? If people want 
dynamic libraries, they want ABI compatibility, if not, they don't. I 
think the two thing are quite directly related to each other.
In that case the question is how do you use that setting in your source 
files ...


On 22/04/16 19:11, Zoltan Kiss wrote:

In order to have a fixed ABI, we need each function to be in the library
file. But that hurts performance a bit, as small accessor functions
couldn't be inlined. To have the ability to use both, this patch
creates a new configure option, which enables the use of inline
functions for users who want better performance.
The accessors are moved to a packet_inlines.h file, by default it is
included from odp_packet.c, and they appear as fully fledged functions.
If --enable-inlines=yes is added to ./configure, they won't be compiled
into the .so file, but the application has to add "#define INLINES" before
including odp.h

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  configure.ac   |  9 +++
  platform/linux-dpdk/Makefile.am|  1 +
  platform/linux-dpdk/include/odp/api/packet.h   | 59 +--
  .../linux-dpdk/include/odp/api/packet_inlines.h| 86 ++
  platform/linux-dpdk/odp_packet.c   |  3 +
  5 files changed, 102 insertions(+), 56 deletions(-)
  create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h

diff --git a/configure.ac b/configure.ac
index 99772a1..6b8fb76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,6 +173,15 @@ AC_ARG_ENABLE([debug],
  ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG"

  ##
+# Enable/disable static inlines
+##
+AC_ARG_ENABLE([inlines],
+[  --enable-inlines  allow inlines],
+[if test "x$enableval" = "xyes"; then
+ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
+fi])
+
+##
  # Save and set temporary compilation flags
  ##
  OLD_LDFLAGS=$LDFLAGS
diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index eadaf63..b8cba23 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -49,6 +49,7 @@ odpapiinclude_HEADERS = \
  $(srcdir)/include/odp/api/init.h \
  $(srcdir)/include/odp/api/packet_flags.h \
  $(srcdir)/include/odp/api/packet.h \
+ $(srcdir)/include/odp/api/packet_inlines.h \
  $(srcdir)/include/odp/api/packet_io.h \
  $(srcdir)/include/odp/api/pool.h \
  $(srcdir)/include/odp/api/queue.h \
diff --git a/platform/linux-dpdk/include/odp/api/packet.h 
b/platform/linux-dpdk/include/odp/api/packet.h
index 7c0db32..e2ba78a 100644
--- a/platform/linux-dpdk/include/odp/api/packet.h
+++ b/platform/linux-dpdk/include/odp/api/packet.h
@@ -27,62 +27,9 @@ extern "C" {
  /** @ingroup odp_packet
   *  @{
   */
-
-extern const unsigned int buf_addr_offset;
-extern const unsigned int data_off_offset;
-extern const unsigned int pkt_len_offset;
-extern const unsigned int seg_len_offset;
-extern const unsigned int udata_len_offset;
-extern const unsigned int udata_offset;
-extern const unsigned int rss_offset;
-extern const unsigned int ol_flags_offset;
-extern const uint64_t rss_flag;
-
-/*
- * NOTE: These functions are inlined because they are on a performance hot 
path.
- * As we can't force the application to directly include DPDK headers we have 
to
- * export these fields through constants calculated compile time in
- * odp_packet.c, where we can see the DPDK definitions.
- *
- */
-static inline uint32_t odp_packet_len(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
-}
-
-static inline uint32_t odp_packet_seg_len(odp_packet_t pkt)
-{
-   return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
-}
-
-static inline void *odp_packet_user_area(odp_packet_t pkt)
-{
-   return (void *)((char *)pkt + udata_offset);
-}
-
-static inline uint32_t odp_packet_user_area_size(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
-}
-
-static inline void *odp_packet_data(odp_packet_t pkt)
-{
-   char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
-   uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + 
data_off_offset);
-   return (void *)(*buf_addr + data_off);
-}
-
-static inline uint32_t odp_packet_flow_hash(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + rss_offset);
-}
-
-static inline void odp_packet_flow_hash_set(od

[lng-odp] classification questions

[Zoltan Kiss]

Hi,

This is what we talked about at the arch call, I summarized it up what
I've found, so hopefully it'll be easier for you to comment:

Socket, socket_mmap and dpdk pktio uses _odp_packet_cls_enq(), it can
have 3 different returns:
- pktio_select_cos(), odp_packet_alloc() or odp_packet_copydata_in()
problem: returns 0, the caller then leaks the memory passed through
*base, I think we should fix that. Plus, I think we should pass back
some of these errors through odp_pktin_recv(). At least the alloc and
copydata, not sure about pktio_select_cos(). What do you think?
- queue_enq() succeeds, and then it returns 0, the caller don't have to
care. That's OK
- queue_enq() fails, caller passes it back to pktin_poll(), which puts
it on the pktio_entry->s.in_queue[...].queue. Is this what we want? Or
should we rather drop the packet? What is the rationale for either decision?

On the other hand, _odp_packet_classifier() (which is used by loopback,
and I want to use it for ODP-DPDK) works a bit differently:
- pktio_select_cos(), odp_packet_alloc() or odp_packet_copydata_in()
problem AND queue_enq() failures: returns -1, the caller releases the
buffer and update error stats. pktin_poll() never sees the packets
failed in queue_enq(). This is the same question like above, we should
have some consistency. Plus again, we might want to escalate the alloc
and copydata failures.
- queue_enq() succeeds, caller doesn't care anymore.

Regards,

Zoltan


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [RFC PATCH] linux-dpdk: add --enable-inlines configure option

[Zoltan Kiss]

Yes, we can have a more general name, but I tend towards using something 
along the lines of --enable-abi-breakage


On 25/04/16 12:59, Bill Fischofer wrote:

--enable-inlines seems reasonable but is perhaps only one component in
what's needed to enable abi mode operation.  So do we need/want an
umbrella --enable-abi option that turns on all sub-options (whatever
they may be) to achieve ABI mode?

On Mon, Apr 25, 2016 at 6:39 AM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

Originally I planned to have something like "-DINLINES=0/1", but
that would force the application to define this macro in any case.
Creating a config.h would be a way to overcome this maybe.




    On 22/04/16 19:11, Zoltan Kiss wrote:



##
+# Enable/disable static inlines

+##
+AC_ARG_ENABLE([inlines],
+[  --enable-inlines  allow inlines],
+[if test "x$enableval" = "xyes"; then
+ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
+fi])
+



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [RFC PATCH] linux-dpdk: add --enable-inlines configure option

[Zoltan Kiss]

Originally I planned to have something like "-DINLINES=0/1", but that 
would force the application to define this macro in any case.

Creating a config.h would be a way to overcome this maybe.



On 22/04/16 19:11, Zoltan Kiss wrote:

  ##
+# Enable/disable static inlines
+##
+AC_ARG_ENABLE([inlines],
+[  --enable-inlines  allow inlines],
+[if test "x$enableval" = "xyes"; then
+ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
+fi])
+

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH v2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

On 22/04/16 17:22, Bala Manoharan wrote:



On 22 April 2016 at 20:28, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

Hi Bala,

While looking at this, I realized I don't understand something: we
pass the received packets to classification, and it enqueues them to
queues. But we also return the number of received packets, along
with the handles to those packets, which requires a quite different
behavior from the caller of odp_pktin_recv(): it can't actually use
those handles, as those packets are on a queue already. Am I missing
something? Or should we mention this in the function description?


The function only returns the handles of those packets which were not
en-queued by classification, basically it checks the return value of
_odp_packet_classifier() and if it is an error then the packets are
returned from loop_recv() function and are handled as if there was no
classification enabled for these packets.

If it makes sense I can change the logic to delete the packets if there
is an error in classification and we can maybe print a error message.


I think both cases makes sense. Petri, do you have an opinion?

Also, it would be good to document somewhere, that you can trigger 
classification by calling odp_pktin_recv(), and therefore it's normal to 
receive 0 packets in that case.




Regards,
Bala


Regards,

Zoltan


On 13/04/16 15:20, Zoltan Kiss wrote:

diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..f6a8c1d 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,10 +70,13 @@ static int loopback_recv(pktio_entry_t
*pktio_entry, odp_packet_t pkts[],
 pkt_hdr = odp_packet_hdr(pkt);
 packet_parse_reset(pkt_hdr);
 packet_parse_l2(pkt_hdr);
-   if (0 >
_odp_packet_classifier(pktio_entry, pkt)) {
-   pkts[j++] = pkt;
+   if (!_odp_packet_classifier(pktio_entry,
pkt)) {
 pktio_entry->s.stats.in_octets +=
-   odp_packet_len(pkts[i]);
+   odp_packet_len(pkt);
+   } else {
+   pktio_entry->s.stats.in_errors +=
+   odp_packet_len(pkt);
+   odp_packet_free(pkt);
 }
 }
 nbr = j;



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [RFC PATCH] linux-dpdk: add --enable-inlines configure option

[Zoltan Kiss]

In order to have a fixed ABI, we need each function to be in the library
file. But that hurts performance a bit, as small accessor functions
couldn't be inlined. To have the ability to use both, this patch
creates a new configure option, which enables the use of inline
functions for users who want better performance.
The accessors are moved to a packet_inlines.h file, by default it is
included from odp_packet.c, and they appear as fully fledged functions.
If --enable-inlines=yes is added to ./configure, they won't be compiled
into the .so file, but the application has to add "#define INLINES" before
including odp.h

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 configure.ac   |  9 +++
 platform/linux-dpdk/Makefile.am|  1 +
 platform/linux-dpdk/include/odp/api/packet.h   | 59 +--
 .../linux-dpdk/include/odp/api/packet_inlines.h| 86 ++
 platform/linux-dpdk/odp_packet.c   |  3 +
 5 files changed, 102 insertions(+), 56 deletions(-)
 create mode 100644 platform/linux-dpdk/include/odp/api/packet_inlines.h

diff --git a/configure.ac b/configure.ac
index 99772a1..6b8fb76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,6 +173,15 @@ AC_ARG_ENABLE([debug],
 ODP_CFLAGS="$ODP_CFLAGS -DODP_DEBUG=$ODP_DEBUG"
 
 ##
+# Enable/disable static inlines
+##
+AC_ARG_ENABLE([inlines],
+[  --enable-inlines  allow inlines],
+[if test "x$enableval" = "xyes"; then
+ODP_CFLAGS="$ODP_CFLAGS -DINLINES"
+fi])
+
+##
 # Save and set temporary compilation flags
 ##
 OLD_LDFLAGS=$LDFLAGS
diff --git a/platform/linux-dpdk/Makefile.am b/platform/linux-dpdk/Makefile.am
index eadaf63..b8cba23 100644
--- a/platform/linux-dpdk/Makefile.am
+++ b/platform/linux-dpdk/Makefile.am
@@ -49,6 +49,7 @@ odpapiinclude_HEADERS = \
  $(srcdir)/include/odp/api/init.h \
  $(srcdir)/include/odp/api/packet_flags.h \
  $(srcdir)/include/odp/api/packet.h \
+ $(srcdir)/include/odp/api/packet_inlines.h \
  $(srcdir)/include/odp/api/packet_io.h \
  $(srcdir)/include/odp/api/pool.h \
  $(srcdir)/include/odp/api/queue.h \
diff --git a/platform/linux-dpdk/include/odp/api/packet.h 
b/platform/linux-dpdk/include/odp/api/packet.h
index 7c0db32..e2ba78a 100644
--- a/platform/linux-dpdk/include/odp/api/packet.h
+++ b/platform/linux-dpdk/include/odp/api/packet.h
@@ -27,62 +27,9 @@ extern "C" {
 /** @ingroup odp_packet
  *  @{
  */
-
-extern const unsigned int buf_addr_offset;
-extern const unsigned int data_off_offset;
-extern const unsigned int pkt_len_offset;
-extern const unsigned int seg_len_offset;
-extern const unsigned int udata_len_offset;
-extern const unsigned int udata_offset;
-extern const unsigned int rss_offset;
-extern const unsigned int ol_flags_offset;
-extern const uint64_t rss_flag;
-
-/*
- * NOTE: These functions are inlined because they are on a performance hot 
path.
- * As we can't force the application to directly include DPDK headers we have 
to
- * export these fields through constants calculated compile time in
- * odp_packet.c, where we can see the DPDK definitions.
- *
- */
-static inline uint32_t odp_packet_len(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + pkt_len_offset);
-}
-
-static inline uint32_t odp_packet_seg_len(odp_packet_t pkt)
-{
-   return *(uint16_t *)(void *)((char *)pkt + seg_len_offset);
-}
-
-static inline void *odp_packet_user_area(odp_packet_t pkt)
-{
-   return (void *)((char *)pkt + udata_offset);
-}
-
-static inline uint32_t odp_packet_user_area_size(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + udata_len_offset);
-}
-
-static inline void *odp_packet_data(odp_packet_t pkt)
-{
-   char** buf_addr = (char **)(void *)((char *)pkt + buf_addr_offset);
-   uint16_t data_off = *(uint16_t *)(void *)((char *)pkt + 
data_off_offset);
-   return (void *)(*buf_addr + data_off);
-}
-
-static inline uint32_t odp_packet_flow_hash(odp_packet_t pkt)
-{
-   return *(uint32_t *)(void *)((char *)pkt + rss_offset);
-}
-
-static inline void odp_packet_flow_hash_set(odp_packet_t pkt, uint32_t 
flow_hash)
-{
-   *(uint32_t *)(void *)((char *)pkt + rss_offset) = flow_hash;
-   *(uint64_t *)(void *)((char *)pkt + ol_flags_offset) |= rss_flag;
-}
-
+#ifdef INLINES
+#include 
+#endif
 /**
  * @}
  */
diff --git a/platform/linux-dpdk/include/odp/api/packet_inlines.h 
b/platform/linux-dpdk/include/odp/api/packet_inlines.h
new file mode 100644
index 00

Re: [lng-odp] [PATCH v2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

Hi Bala,

While looking at this, I realized I don't understand something: we pass 
the received packets to classification, and it enqueues them to queues. 
But we also return the number of received packets, along with the 
handles to those packets, which requires a quite different behavior from 
the caller of odp_pktin_recv(): it can't actually use those handles, as 
those packets are on a queue already. Am I missing something? Or should 
we mention this in the function description?


Regards,

Zoltan

On 13/04/16 15:20, Zoltan Kiss wrote:

diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..f6a8c1d 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,10 +70,13 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pkt_hdr = odp_packet_hdr(pkt);
packet_parse_reset(pkt_hdr);
packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry, pkt)) {
-   pkts[j++] = pkt;
+   if (!_odp_packet_classifier(pktio_entry, pkt)) {
pktio_entry->s.stats.in_octets +=
-   odp_packet_len(pkts[i]);
+   odp_packet_len(pkt);
+   } else {
+   pktio_entry->s.stats.in_errors +=
+   odp_packet_len(pkt);
+   odp_packet_free(pkt);
}
}
nbr = j;

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2] validation: pktio: remove checks from stats test

[Zoltan Kiss]

This test sets up two interfaces and connect them to each other, so in
theory these numbers should be the same. However when you use a pktio
which doesn't have full control of the interface, it could happen that
other players, e.g. various services of the operating system start to
send traffic out on the newly created interfaces. It won't be visible
for ODP when going out, but coming in it will increase the counters.
This breaks the test on ODP-DPDK, unnecessarily. On ODP-Linux it does not,
because it checks the system level statistics, not the ODP level ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

v2:
- fix commit log typo
- remove pkt number check as well, and edit the title

diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index d52a520..78cf44a 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -1258,8 +1258,6 @@ void pktio_test_statistics_counters(void)
CU_ASSERT(ret == 0);
CU_ASSERT((stats[1].in_ucast_pkts == 0) ||
  (stats[1].in_ucast_pkts >= (uint64_t)pkts));
-   CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
-   CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
CU_ASSERT((stats[0].out_octets == 0) ||
  (stats[0].out_octets >=
  (PKT_LEN_NORMAL * (uint64_t)pkts)));
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] validation: pktio: remove octet check from stats test

[Zoltan Kiss]

On 15/04/16 14:45, Maxim Uvarov wrote:

On 04/05/16 19:16, Zoltan Kiss wrote:

This test sets up two interface and connect them to each other, so in
theory these two numbers should be the same. However when you use a pktio
which doesn't have full control of the interface, it could happen that
other players, e.g. various services of the operating system start to
send traffic out on the newly created interfaces. It won't be visible
for ODP when going out, but coming in it will increase the counters.
This breaks the test on ODP-DPDK, unnecessarily. On ODP-Linux it does
not,
because it checks the system level statistics, not the ODP level ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/test/validation/pktio/pktio.c
b/test/validation/pktio/pktio.c
index cb403a6..73b702c 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -1256,7 +1256,6 @@ void pktio_test_statistics_counters(void)
  CU_ASSERT((stats[1].in_ucast_pkts == 0) ||
(stats[1].in_ucast_pkts >= (uint64_t)pkts));

that is strange that if:

  CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);

passes and this:

-CU_ASSERT(stats[0].out_octets == stats[1].in_octets);

is not.

number of packet should be linked to number of bytes. Might be skip this
test in pktio_check_statistics_counters()
if we see some traffic after start?


It's actually because ODP-DPDK sets ucast_pkts counters to 0, because it 
doesn't have a separate counter for unicast packets. I'll resend




Maxim.

  CU_ASSERT((stats[0].out_octets == 0) ||
(stats[0].out_octets >=
(PKT_LEN_NORMAL * (uint64_t)pkts)));



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [API-NEXT PATCH] api: queue: remove const qualifier of passed down event array

[Zoltan Kiss]

As the description says, normally this function consumes the events, which
means there is no point to expect the array to stay intact. Other functions
like odp_pktout_send() doesn't do that as well.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

diff --git a/include/odp/api/spec/queue.h b/include/odp/api/spec/queue.h
index 7626ca7..c420fde 100644
--- a/include/odp/api/spec/queue.h
+++ b/include/odp/api/spec/queue.h
@@ -237,7 +237,7 @@ int odp_queue_enq(odp_queue_t queue, odp_event_t ev);
  * @return Number of events actually enqueued (0 ... num)
  * @retval <0 on failure
  */
-int odp_queue_enq_multi(odp_queue_t queue, const odp_event_t events[], int 
num);
+int odp_queue_enq_multi(odp_queue_t queue, odp_event_t events[], int num);
 
 /**
  * Queue dequeue
diff --git a/platform/linux-generic/odp_queue.c 
b/platform/linux-generic/odp_queue.c
index 342ffa2..29faa29 100644
--- a/platform/linux-generic/odp_queue.c
+++ b/platform/linux-generic/odp_queue.c
@@ -647,7 +647,7 @@ int queue_enq_multi(queue_entry_t *queue, odp_buffer_hdr_t 
*buf_hdr[],
return num; /* All events enqueued */
 }
 
-int odp_queue_enq_multi(odp_queue_t handle, const odp_event_t ev[], int num)
+int odp_queue_enq_multi(odp_queue_t handle, odp_event_t ev[], int num)
 {
odp_buffer_hdr_t *buf_hdr[QUEUE_MULTI_MAX];
queue_entry_t *queue;
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH v2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

Maxim, what's the planned time to apply this?

On 15/04/16 06:31, Bala Manoharan wrote:

Reviewed-by: Balasubramanian Manoharan <bala.manoha...@linaro.org
<mailto:bala.manoha...@linaro.org>>

On 13 April 2016 at 19:50, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

In case of error the 'pkt' should be released by the calling function.
Currently loopback gives it back to the receiver and report it as
success
in the stats.

    Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>
---

v2: handle release in caller instead, and adjust stats.

  platform/linux-generic/odp_classification.c | 10 +++---
  platform/linux-generic/pktio/loop.c |  9 ++---
  2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c
b/platform/linux-generic/odp_classification.c
index 8522023..3a18a78 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)
 if (cos == NULL)
 return -1;

-   if (cos->s.pool == NULL) {
-   odp_packet_free(pkt);
+   if (cos->s.pool == NULL)
 return -1;
-   }

-   if (cos->s.queue == NULL) {
-   odp_packet_free(pkt);
+   if (cos->s.queue == NULL)
 return -1;
-   }

 if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
 new_pkt = odp_packet_copy(pkt,
cos->s.pool->s.pool_hdl);
-   odp_packet_free(pkt);
 if (new_pkt == ODP_PACKET_INVALID)
 return -1;
+   odp_packet_free(pkt);
 } else {
 new_pkt = pkt;
 }
diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..f6a8c1d 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,10 +70,13 @@ static int loopback_recv(pktio_entry_t
*pktio_entry, odp_packet_t pkts[],
 pkt_hdr = odp_packet_hdr(pkt);
 packet_parse_reset(pkt_hdr);
 packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry,
pkt)) {
-   pkts[j++] = pkt;
+   if (!_odp_packet_classifier(pktio_entry, pkt)) {
 pktio_entry->s.stats.in_octets +=
-   odp_packet_len(pkts[i]);
+   odp_packet_len(pkt);
+   } else {
+   pktio_entry->s.stats.in_errors +=
+   odp_packet_len(pkt);
+   odp_packet_free(pkt);
 }
 }
 nbr = j;
--
1.9.1



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] odp_cpumask_default_worker() and availability of the returned CPU's

[Zoltan Kiss]

Hi,

A quick question: my understanding is that odp_cpumask_default_worker() 
returns the available CPU's, but it doesn't guarantee that when you 
start your thread's/processes they will be still available, right? It 
should be the applications responsibility to not start more threads than 
it wants on the same CPU.
Also, I understood that it is not a strict requirement to have one 
thread per CPU, just a best practice, and it's legal to start more of 
them on the same CPU (just highly not advised)


Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] odp_queue_enq_multi() question

[Zoltan Kiss]

On 18/04/16 15:47, Bill Fischofer wrote:



On Mon, Apr 18, 2016 at 9:25 AM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

Hi,

The second parameter of this function is an array pointer for the
events we want to send:

int odp_queue_enq_multi(odp_queue_t queue, const odp_event_t
events[], int num);

I wonder if that const qualifier makes sense there. As the
description says, normally the events are consumed by this function,
so protecting the handle passed seems unnecessary. After the
function returned the application shouldn't touch it.
odp_pktio_send[_queue] doesn't have this const either.
odp-linux circumvents that by building a new array with the buffer
header pointers, but other implementations might want to use this
array right away.
How about removing this const qualifier?


Submit a patch?


I will, I wanted to ask around first. Petri, any objections?





Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
https://lists.linaro.org/mailman/listinfo/lng-odp



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] odp_queue_enq_multi() question

[Zoltan Kiss]

Hi,

The second parameter of this function is an array pointer for the events 
we want to send:


int odp_queue_enq_multi(odp_queue_t queue, const odp_event_t events[], 
int num);


I wonder if that const qualifier makes sense there. As the description 
says, normally the events are consumed by this function, so protecting 
the handle passed seems unnecessary. After the function returned the 
application shouldn't touch it.

odp_pktio_send[_queue] doesn't have this const either.
odp-linux circumvents that by building a new array with the buffer 
header pointers, but other implementations might want to use this array 
right away.

How about removing this const qualifier?

Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [lng-odp-ovs] error while configuring ODP-OVS for v1.6 and v1.8 ./configure --with-odp, cannot find ODP headers

[Zoltan Kiss]

Thanks, that makes sense. I'm using the check-odp scripts, but for some 
reason they don't install into the specified target dir.
Nilesh, you should run make install on ODP, and then pass the install 
path (or not if you install into standard dirs) to ODP.


Regards,

Zoltan

On 18/04/16 14:18, Maxim Uvarov wrote:

On 04/18/16 15:50, Zoltan Kiss wrote:

Hi,

I've tried this out as well, and it fails for me too. I've used the
following configure command:

./configure--with-odp=[installdir] --with-odp-platform=linux-generic
LDFLAGS="$(LDFLAGS) -L$(REPOS)/[installdir]/lib/ -lpcap -lm

The problem is that libodp.a is in /lib/.libs, not in a directory upper.

Anders, Maxim, do you know why is that on the v1.8.0.0 tag?



it's libtool does. Because you did not do make install it's under .libs

Maxim.


Regards,

Zoltan


On 17/04/16 06:41, nilesh kakade wrote:



Hi All,

I want to install ODP-OVS, for that I have tried ODP version v1.8.0.0.
When I am trying to configure ODP-OVS with configuration option
./configure --with-odp it is throwing following error which is
highlighted by bold.

checking whether gcc -std=gnu99 accepts -Wno-unused... yes
checking whether gcc -std=gnu99 accepts -Wno-unused-parameter... yes
checking target hint for cgcc... x86_64
checking whether make has GNU make $(if) extension... yes
checking for DPDK... no
checking for ODP Debug... yes
checking for ODP Platform... yes
checking for ODP...*configure: error: cannot find ODP headers


*
Because of above error I tried ODP version v1.6.0.0, but same error is
appeared.
*
configure: error: cannot find ODP headers.

*
./configure --with-odp, is it the correct option to use? or is there any
other configuration option.
*

*


___
lng-odp-ovs mailing list
lng-odp-...@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp-ovs




___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [lng-odp-ovs] error while configuring ODP-OVS for v1.6 and v1.8 ./configure --with-odp, cannot find ODP headers

[Zoltan Kiss]

Hi,

I've tried this out as well, and it fails for me too. I've used the 
following configure command:


./configure--with-odp=[installdir] --with-odp-platform=linux-generic 
LDFLAGS="$(LDFLAGS) -L$(REPOS)/[installdir]/lib/ -lpcap -lm


The problem is that libodp.a is in /lib/.libs, not in a directory upper.

Anders, Maxim, do you know why is that on the v1.8.0.0 tag?

Regards,

Zoltan


On 17/04/16 06:41, nilesh kakade wrote:



Hi All,

I want to install ODP-OVS, for that I have tried ODP version v1.8.0.0.
When I am trying to configure ODP-OVS with configuration option
./configure --with-odp it is throwing following error which is
highlighted by bold.

checking whether gcc -std=gnu99 accepts -Wno-unused... yes
checking whether gcc -std=gnu99 accepts -Wno-unused-parameter... yes
checking target hint for cgcc... x86_64
checking whether make has GNU make $(if) extension... yes
checking for DPDK... no
checking for ODP Debug... yes
checking for ODP Platform... yes
checking for ODP...*configure: error: cannot find ODP headers


*
Because of above error I tried ODP version v1.6.0.0, but same error is
appeared.
*
configure: error: cannot find ODP headers.

*
./configure --with-odp, is it the correct option to use? or is there any
other configuration option.
*

*


___
lng-odp-ovs mailing list
lng-odp-...@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp-ovs


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] validation: pktio: remove octet check from stats test

[Zoltan Kiss]

Ping

On 06/04/16 11:43, Zoltan Kiss wrote:



On 05/04/16 17:16, Zoltan Kiss wrote:

This test sets up two interface and connect them to each other, so in


s/interface/interfaces/

Maxim, could you fix that when commit? (assuming the patch is OK)

And an another note: Maxim told me when I brought this up first to
disable stuff, and I haven't replied then. The problem is you can't
possibly disable all OS services which tries to hook into the interface
up notification, because it is a moving target.


theory these two numbers should be the same. However when you use a pktio
which doesn't have full control of the interface, it could happen that
other players, e.g. various services of the operating system start to
send traffic out on the newly created interfaces. It won't be visible
for ODP when going out, but coming in it will increase the counters.
This breaks the test on ODP-DPDK, unnecessarily. On ODP-Linux it does
not,
because it checks the system level statistics, not the ODP level ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/test/validation/pktio/pktio.c
b/test/validation/pktio/pktio.c
index cb403a6..73b702c 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -1256,7 +1256,6 @@ void pktio_test_statistics_counters(void)
  CU_ASSERT((stats[1].in_ucast_pkts == 0) ||
(stats[1].in_ucast_pkts >= (uint64_t)pkts));
  CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
-CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
  CU_ASSERT((stats[0].out_octets == 0) ||
(stats[0].out_octets >=
(PKT_LEN_NORMAL * (uint64_t)pkts)));


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 1/2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

Hi,

I've sent a new patch, I just wanted to mention that I couldn't find 
this behavior described in user-guide-cls.adoc


Zoli

On 13/04/16 13:40, Bala Manoharan wrote:



The classification module drops a
packet only on specific cases either on error CoS or when the packet
pool is full.


Could you point me to the documentation where this behavior is
described?


The scenario is described in classification users-guide documentation.
Pls let me know if we need to add additional information in the document.

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

In case of error the 'pkt' should be released by the calling function.
Currently loopback gives it back to the receiver and report it as success
in the stats.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---

v2: handle release in caller instead, and adjust stats.

 platform/linux-generic/odp_classification.c | 10 +++---
 platform/linux-generic/pktio/loop.c |  9 ++---
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 8522023..3a18a78 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -745,21 +745,17 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
if (cos == NULL)
return -1;
 
-   if (cos->s.pool == NULL) {
-   odp_packet_free(pkt);
+   if (cos->s.pool == NULL)
return -1;
-   }
 
-   if (cos->s.queue == NULL) {
-   odp_packet_free(pkt);
+   if (cos->s.queue == NULL)
return -1;
-   }
 
if (odp_packet_pool(pkt) != cos->s.pool->s.pool_hdl) {
new_pkt = odp_packet_copy(pkt, cos->s.pool->s.pool_hdl);
-   odp_packet_free(pkt);
if (new_pkt == ODP_PACKET_INVALID)
return -1;
+   odp_packet_free(pkt);
} else {
new_pkt = pkt;
}
diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..f6a8c1d 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,10 +70,13 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pkt_hdr = odp_packet_hdr(pkt);
packet_parse_reset(pkt_hdr);
packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry, pkt)) {
-   pkts[j++] = pkt;
+   if (!_odp_packet_classifier(pktio_entry, pkt)) {
pktio_entry->s.stats.in_octets +=
-   odp_packet_len(pkts[i]);
+   odp_packet_len(pkt);
+   } else {
+   pktio_entry->s.stats.in_errors +=
+   odp_packet_len(pkt);
+   odp_packet_free(pkt);
}
}
nbr = j;
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 1/2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

On 13/04/16 13:40, Bala Manoharan wrote:



Regards,
Bala

On 12 April 2016 at 17:34, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:



On 11/04/16 05:01, Bala Manoharan wrote:

Hi,

Comments inline...

Regards,
Bala

On 8 April 2016 at 00:23, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>>
wrote:

 The callers are only interested whether there is a CoS or
not. It is not
 an error if there isn't, so it has a positive return value. Any
 other case
 needs to release the packet, which is not handled after calling
 queue_enq().

     Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>
 <mailto:zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>>
 ---
   platform/linux-generic/odp_classification.c | 9 +++--
   platform/linux-generic/pktio/loop.c | 2 +-
   2 files changed, 8 insertions(+), 3 deletions(-)

 diff --git a/platform/linux-generic/odp_classification.c
 b/platform/linux-generic/odp_classification.c
 index f5e4673..4f2974b 100644
 --- a/platform/linux-generic/odp_classification.c
 +++ b/platform/linux-generic/odp_classification.c
 @@ -743,7 +743,7 @@ int
_odp_packet_classifier(pktio_entry_t *entry,
 odp_packet_t pkt)
  /* Matching PMR and selecting the CoS for the packet*/
  cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
  if (cos == NULL)
 -   return -1;
 +   return 1;


The scenario of NULL Cos will occur only whether there is an invalid
configuration done by the application. If no other CoS matches the
packet then default CoS will be applied, this NULL will occur
only if
there is no defaultCoS configured and hence it is better to
return the
error to the calling function so that a proper action could be
taken.


Ok, then we should free the packet here as well.


IMO, we should free the packet at the loopback device function. so that
whenever the _odp_packet_classifier() function returns an error value
then the packet has to be freed by the calling function.


If you just look at the next few lines of this code, you'll see that 
this function releases the buffer in two error scenario.







  if (cos->s.pool == NULL) {
  odp_packet_free(pkt);
 @@ -766,7 +766,12 @@ int _odp_packet_classifier(pktio_entry_t
 *entry, odp_packet_t pkt)

  /* Enqueuing the Packet based on the CoS */
  queue = cos->s.queue;
 -   return queue_enq(queue,
 odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
 +   if (queue_enq(queue,
odp_buf_to_hdr((odp_buffer_t)new_pkt),
 0)) {
 +   odp_packet_free(new_pkt);
 +   return -1;
 +   } else {
 +   return 0;
 +   }


IMO packets should be freed only by the receiving module rather
than by
the classifier in this specific error.


The only user, the loopback devices doesn't do so, it only adjusts
the statistics. That is where the memory leaks btw.


Okay. Then we have to modify the loopback device function to free the
packet when there is an error in _odp_packet_classifier() function.



The classification module drops a
packet only on specific cases either on error CoS or when the packet
pool is full.


Could you point me to the documentation where this behavior is
described?


The scenario is described in classification users-guide documentation.
Pls let me know if we need to add additional information in the document.



The idea of returning an error value is that the caller
function knows that the packet is not enqueued and it has to
either free
the packet or check the configuration.


Currently we return -1, and the packet buffer is either released or
not. The only caller doesn't care anyway, and I don't think it could
do much more anyway, other than printing a big error message and
probably abort.


Current behaviour is that when there is an error in classification, we
return -1and it means that the packet has not be enqueued into
classification queue and the buffer has not been freed.


There are two error scenarios when that's not true, see above.







You had mentioned that this p

Re: [lng-odp] [PATCH 1/2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

The original behavior of loopback_recv() is incorrect then, because it 
gives back the failed packets to the caller in pkts[], some of the might 
had been already released.


On 11/04/16 05:01, Bala Manoharan wrote:

  int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt)
diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..5ed4ca9 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,7 +70,7 @@ static int loopback_recv(pktio_entry_t *pktio_entry,
odp_packet_t pkts[],
 pkt_hdr = odp_packet_hdr(pkt);
 packet_parse_reset(pkt_hdr);
 packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry, pkt)) {
+   if (_odp_packet_classifier(pktio_entry, pkt) == 1) {
 pkts[j++] = pkt;
 pktio_entry->s.stats.in_octets +=
 odp_packet_len(pkts[i]);
--

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 1/2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

On 11/04/16 05:01, Bala Manoharan wrote:

Hi,

Comments inline...

Regards,
Bala

On 8 April 2016 at 00:23, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

The callers are only interested whether there is a CoS or not. It is not
an error if there isn't, so it has a positive return value. Any
other case
needs to release the packet, which is not handled after calling
queue_enq().

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>
---
  platform/linux-generic/odp_classification.c | 9 +++--
  platform/linux-generic/pktio/loop.c | 2 +-
  2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c
b/platform/linux-generic/odp_classification.c
index f5e4673..4f2974b 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -743,7 +743,7 @@ int _odp_packet_classifier(pktio_entry_t *entry,
odp_packet_t pkt)
 /* Matching PMR and selecting the CoS for the packet*/
 cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
 if (cos == NULL)
-   return -1;
+   return 1;


The scenario of NULL Cos will occur only whether there is an invalid
configuration done by the application. If no other CoS matches the
packet then default CoS will be applied, this NULL will occur only if
there is no defaultCoS configured and hence it is better to return the
error to the calling function so that a proper action could be taken.


Ok, then we should free the packet here as well.





 if (cos->s.pool == NULL) {
 odp_packet_free(pkt);
@@ -766,7 +766,12 @@ int _odp_packet_classifier(pktio_entry_t
*entry, odp_packet_t pkt)

 /* Enqueuing the Packet based on the CoS */
 queue = cos->s.queue;
-   return queue_enq(queue,
odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+   if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt),
0)) {
+   odp_packet_free(new_pkt);
+   return -1;
+   } else {
+   return 0;
+   }


IMO packets should be freed only by the receiving module rather than by
the classifier in this specific error.


The only user, the loopback devices doesn't do so, it only adjusts the 
statistics. That is where the memory leaks btw.



The classification module drops a
packet only on specific cases either on error CoS or when the packet
pool is full.


Could you point me to the documentation where this behavior is described?


The idea of returning an error value is that the caller
function knows that the packet is not enqueued and it has to either free
the packet or check the configuration.


Currently we return -1, and the packet buffer is either released or not. 
The only caller doesn't care anyway, and I don't think it could do much 
more anyway, other than printing a big error message and probably abort.





You had mentioned that this patch is required to prevent a memory leak,
can you please elaborate on the scenario when a leak is observed?




  }

  int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt)
diff --git a/platform/linux-generic/pktio/loop.c
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..5ed4ca9 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,7 +70,7 @@ static int loopback_recv(pktio_entry_t
*pktio_entry, odp_packet_t pkts[],
 pkt_hdr = odp_packet_hdr(pkt);
 packet_parse_reset(pkt_hdr);
 packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry,
pkt)) {
+   if (_odp_packet_classifier(pktio_entry, pkt)
== 1) {
 pkts[j++] = pkt;
 pktio_entry->s.stats.in_octets +=
 odp_packet_len(pkts[i]);
--
1.9.1



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 2/2] linux-generic: classification: remove unused code

[Zoltan Kiss]

This function is not referred.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_classification_internal.h |  9 ++---
 platform/linux-generic/odp_classification.c  | 10 --
 2 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/platform/linux-generic/include/odp_classification_internal.h 
b/platform/linux-generic/include/odp_classification_internal.h
index 86b40fc..3a88462 100644
--- a/platform/linux-generic/include/odp_classification_internal.h
+++ b/platform/linux-generic/include/odp_classification_internal.h
@@ -54,6 +54,8 @@ based on the l3_preference value of the pktio
 cos_t *match_qos_cos(pktio_entry_t *entry, const uint8_t *pkt_addr,
 odp_packet_hdr_t *hdr);
 /**
+@internal
+
 Packet Classifier
 
 Start function for Packet Classifier
@@ -61,13 +63,6 @@ This function calls Classifier module internal functions for 
a given packet and
 enqueues the packet to specific Queue based on PMR and CoS selected.
 The packet is allocated from the pool associated with the CoS
 **/
-int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt);
-
-/**
-@internal
-
-Same as packet classifier uses linux-generic internal pktio struct
-**/
 int _odp_packet_classifier(pktio_entry_t *entry, odp_packet_t pkt);
 
 /**
diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 4f2974b..2ad190b 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -774,16 +774,6 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
}
 }
 
-int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt)
-{
-   pktio_entry_t *entry;
-
-   entry = get_pktio_entry(pktio);
-   if (entry == NULL)
-   return -1;
-   return _odp_packet_classifier(entry, pkt);
-}
-
 cos_t *pktio_select_cos(pktio_entry_t *entry, const uint8_t *pkt_addr,
odp_packet_hdr_t *pkt_hdr)
 {
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 1/2] linux-generic: classification: fix error checking _odp_packet_classifier()

[Zoltan Kiss]

The callers are only interested whether there is a CoS or not. It is not
an error if there isn't, so it has a positive return value. Any other case
needs to release the packet, which is not handled after calling
queue_enq().

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/odp_classification.c | 9 +++--
 platform/linux-generic/pktio/loop.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index f5e4673..4f2974b 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -743,7 +743,7 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
/* Matching PMR and selecting the CoS for the packet*/
cos = pktio_select_cos(entry, pkt_addr, pkt_hdr);
if (cos == NULL)
-   return -1;
+   return 1;
 
if (cos->s.pool == NULL) {
odp_packet_free(pkt);
@@ -766,7 +766,12 @@ int _odp_packet_classifier(pktio_entry_t *entry, 
odp_packet_t pkt)
 
/* Enqueuing the Packet based on the CoS */
queue = cos->s.queue;
-   return queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0);
+   if (queue_enq(queue, odp_buf_to_hdr((odp_buffer_t)new_pkt), 0)) {
+   odp_packet_free(new_pkt);
+   return -1;
+   } else {
+   return 0;
+   }
 }
 
 int packet_classifier(odp_pktio_t pktio, odp_packet_t pkt)
diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 0ea6d0e..5ed4ca9 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -70,7 +70,7 @@ static int loopback_recv(pktio_entry_t *pktio_entry, 
odp_packet_t pkts[],
pkt_hdr = odp_packet_hdr(pkt);
packet_parse_reset(pkt_hdr);
packet_parse_l2(pkt_hdr);
-   if (0 > _odp_packet_classifier(pktio_entry, pkt)) {
+   if (_odp_packet_classifier(pktio_entry, pkt) == 1) {
pkts[j++] = pkt;
pktio_entry->s.stats.in_octets +=
odp_packet_len(pkts[i]);
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 0/2] Minor classification fixes

[Zoltan Kiss]

The first one prevents memory leak/double-free situations though.

Zoltan Kiss (2):
  linux-generic: classification: fix error checking
_odp_packet_classifier()
  linux-generic: classification: remove unused code

 .../linux-generic/include/odp_classification_internal.h |  9 ++---
 platform/linux-generic/odp_classification.c | 17 ++---
 platform/linux-generic/pktio/loop.c |  2 +-
 3 files changed, 9 insertions(+), 19 deletions(-)

-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] linux-generic: packet_io: fix state handling

[Zoltan Kiss]

Ping, I think it only waits for merge

On 03/04/16 20:41, Bill Fischofer wrote:



On Fri, Apr 1, 2016 at 11:44 AM, Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>> wrote:

The current two state doesn't tell odp_pktio_close() whether the
device has ever
been started, which is important to decide whether it should flush
anything or
not.
The new OPENED state only happen after odp_pktio_open(), after a
start -> stop
transition it goes to a STOPPED state. Both valid for configuration,
so instead
of "== STATE_STOPPED", check "!= STATE_STARTED".

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org
<mailto:zoltan.k...@linaro.org>>


Reviewed-and-tested-by: Bill Fischofer <bill.fischo...@linaro.org
<mailto:bill.fischo...@linaro.org>>

---
diff --git a/platform/linux-generic/include/odp_packet_io_internal.h
b/platform/linux-generic/include/odp_packet_io_internal.h
index 7636768..cca5c39 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -89,8 +89,10 @@ struct pktio_entry {
 pkt_tap_t pkt_tap;  /**< using TAP for
IO */
 };
 enum {
-   STATE_START = 0,
-   STATE_STOP
+   STATE_OPENED = 0,   /**< After open() */
+   STATE_STARTED,  /**< After start() */
+   STATE_STOPPED   /**< Same as OPENED, but
only happens
+   after STARTED */
 } state;
 classifier_t cls;   /**< classifier linked with
this pktio*/
 odp_pktio_stats_t stats;/**< statistic counters for
pktio */
diff --git a/platform/linux-generic/odp_packet_io.c
b/platform/linux-generic/odp_packet_io.c
index 9192be2..f927c15 100644
--- a/platform/linux-generic/odp_packet_io.c
+++ b/platform/linux-generic/odp_packet_io.c
@@ -228,7 +228,7 @@ static odp_pktio_t setup_pktio_entry(const char
*name, odp_pool_t pool,
 } else {
 snprintf(pktio_entry->s.name <http://s.name>,
  sizeof(pktio_entry->s.name
<http://s.name>), "%s", name);
-   pktio_entry->s.state = STATE_STOP;
+   pktio_entry->s.state = STATE_OPENED;
 }

 unlock_entry(pktio_entry);
@@ -343,7 +343,8 @@ int odp_pktio_close(odp_pktio_t id)
 if (entry == NULL)
 return -1;

-   flush_in_queues(entry);
+   if (entry->s.state == STATE_STOPPED)
+   flush_in_queues(entry);

 lock_entry(entry);

@@ -374,14 +375,14 @@ int odp_pktio_start(odp_pktio_t id)
 return -1;

 lock_entry(entry);
-   if (entry->s.state == STATE_START) {
+   if (entry->s.state == STATE_STARTED) {
 unlock_entry(entry);
 return -1;
 }
 if (entry->s.ops->start)
 res = entry->s.ops->start(entry);
 if (!res)
-   entry->s.state = STATE_START;
+   entry->s.state = STATE_STARTED;

 unlock_entry(entry);

@@ -409,13 +410,13 @@ static int _pktio_stop(pktio_entry_t *entry)
  {
 int res = 0;

-   if (entry->s.state == STATE_STOP)
+   if (entry->s.state != STATE_STARTED)
 return -1;

 if (entry->s.ops->stop)
 res = entry->s.ops->stop(entry);
 if (!res)
-   entry->s.state = STATE_STOP;
+   entry->s.state = STATE_STOPPED;

 return res;
  }
@@ -502,7 +503,7 @@ static int _odp_pktio_send(odp_pktio_t id,
odp_packet_t pkt_table[], int len)
 return -1;

 odp_ticketlock_lock(_entry->s.txl);
-   if (pktio_entry->s.state == STATE_STOP ||
+   if (pktio_entry->s.state != STATE_STARTED ||
 pktio_entry->s.param.out_mode ==
ODP_PKTOUT_MODE_DISABLED) {
 odp_ticketlock_unlock(_entry->s.txl);
 __odp_errno = EPERM;
@@ -650,7 +651,7 @@ int pktin_poll(pktio_entry_t *entry, int
num_queue, int index[])
 if (odp_unlikely(entry->s.num_in_queue == 0))
 return -1;

-   if (entry->s.state == STATE_STOP)
+   if (entry->s.state != STATE_STARTED)
 return 0;

 for (idx = 0; idx < num_queue; idx++) {
@@ -725,7 +726,7 @@ int odp_pktio_promisc_mode_set(odp_pktio_t id,
odp_bool_t enable)

Re: [lng-odp] [PATCH] validation: pktio: remove octet check from stats test

[Zoltan Kiss]

On 05/04/16 17:16, Zoltan Kiss wrote:

This test sets up two interface and connect them to each other, so in


s/interface/interfaces/

Maxim, could you fix that when commit? (assuming the patch is OK)

And an another note: Maxim told me when I brought this up first to 
disable stuff, and I haven't replied then. The problem is you can't 
possibly disable all OS services which tries to hook into the interface 
up notification, because it is a moving target.



theory these two numbers should be the same. However when you use a pktio
which doesn't have full control of the interface, it could happen that
other players, e.g. various services of the operating system start to
send traffic out on the newly created interfaces. It won't be visible
for ODP when going out, but coming in it will increase the counters.
This breaks the test on ODP-DPDK, unnecessarily. On ODP-Linux it does not,
because it checks the system level statistics, not the ODP level ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index cb403a6..73b702c 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -1256,7 +1256,6 @@ void pktio_test_statistics_counters(void)
CU_ASSERT((stats[1].in_ucast_pkts == 0) ||
  (stats[1].in_ucast_pkts >= (uint64_t)pkts));
CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
-   CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
CU_ASSERT((stats[0].out_octets == 0) ||
  (stats[0].out_octets >=
  (PKT_LEN_NORMAL * (uint64_t)pkts)));


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] validation: pktio: remove octet check from stats test

[Zoltan Kiss]

This test sets up two interface and connect them to each other, so in
theory these two numbers should be the same. However when you use a pktio
which doesn't have full control of the interface, it could happen that
other players, e.g. various services of the operating system start to
send traffic out on the newly created interfaces. It won't be visible
for ODP when going out, but coming in it will increase the counters.
This breaks the test on ODP-DPDK, unnecessarily. On ODP-Linux it does not,
because it checks the system level statistics, not the ODP level ones.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index cb403a6..73b702c 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -1256,7 +1256,6 @@ void pktio_test_statistics_counters(void)
CU_ASSERT((stats[1].in_ucast_pkts == 0) ||
  (stats[1].in_ucast_pkts >= (uint64_t)pkts));
CU_ASSERT(stats[0].out_ucast_pkts == stats[1].in_ucast_pkts);
-   CU_ASSERT(stats[0].out_octets == stats[1].in_octets);
CU_ASSERT((stats[0].out_octets == 0) ||
  (stats[0].out_octets >=
  (PKT_LEN_NORMAL * (uint64_t)pkts)));
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] flush_in_queues() on devices never started

[Zoltan Kiss]

I've sent a patch to fix that. I've also found that this error handling 
could be necessary. start() has finished, so I think we should call 
stop(). And the state should be either STOPPED or OPENED, I'm not sure 
in that.



@@ -395,6 +396,11 @@ int odp_pktio_start(odp_pktio_t id)

if (entry->s.in_queue[i].queue == 
ODP_QUEUE_INVALID) {

ODP_ERR("No input queue\n");
+   res = entry->s.ops->stop(entry);
+   if (!res)
+   entry->s.state = STATE_STOPPED;
+   else
+   ODP_ERR("Failed to stop 
interface\n");

return -1;
}



On 01/04/16 08:13, Savolainen, Petri (Nokia - FI/Espoo) wrote:

Hi,

The issue is in odp-linux implementation of odp_pktio_close(). It should try to flush only 
if the interface was ever started (there would be something to flush). We'd need to extend 
pktio interface states so that close call can tell the difference of open() -> close() 
(state == STATE_OPENED) vs. open() -> start() -> stop() -> close() (STATE == 
STOPPED).

int odp_pktio_close(odp_pktio_t id)
{
pktio_entry_t *entry;
int res;

entry = get_pktio_entry(id);
if (entry == NULL)
return -1;

Add this if clause here:
// Flush if state STOPPED, don’t flush if only OPENED
// Error check if state is STARTED (interface is active).
if (entry->s.state == STATE_STOPPED)
flush_in_queues(entry);


-Petri



-Original Message-
From: EXT Zoltan Kiss [mailto:zoltan.k...@linaro.org]
Sent: Thursday, March 31, 2016 9:00 PM
To: Elo, Matias (Nokia - FI/Espoo) <matias@nokia.com>; Savolainen,
Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Cc: lng-odp <lng-odp@lists.linaro.org>
Subject: Re: flush_in_queues() on devices never started

I forgot to Cc the list

On 31/03/16 17:03, Zoltan Kiss wrote:

Hi,

I ran into a problem when pktio_test_pktin_queue_config_direct() plays
with queue config then call close. That calls flush_in_queues(), which
tries to receive on a device which was never started, therefore run into
a segfault when trying to access the queues never created.
I've used the pkt_dpdk->started variable to avoid that in
recv_pkt_dpdk_queue(), but I wonder if that's the proper way. I guess
odp-linux would have the same issue on DPDK pktio. Any better idea?
I would also prefer avoiding branching on a hotpath like this. And we
should check if we need a similar check on send side.

diff --git a/platform/linux-dpdk/odp_packet_dpdk.c
b/platform/linux-dpdk/odp_packet_dpdk.c
index 7ddbac6..fdc461e 100644
--- a/platform/linux-dpdk/odp_packet_dpdk.c
+++ b/platform/linux-dpdk/odp_packet_dpdk.c
@@ -352,6 +352,9 @@ static int recv_pkt_dpdk_queue(pktio_entry_t
*pktio_entry, int index,
  pkt_dpdk_t * const pkt_dpdk = _entry->s.pkt_dpdk;
  uint8_t min = pkt_dpdk->min_rx_burst;

+   if (!pkt_dpdk->started)
+   return 0;
+
  if (odp_unlikely(min > len)) {
  ODP_DBG("PMD requires >%d buffers burst. "
  "Current %d, dropped %d\n", min, len, min -

len);



Regards,

Zoltan

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] linux-generic: packet_io: fix state handling

[Zoltan Kiss]

The current two state doesn't tell odp_pktio_close() whether the device has ever
been started, which is important to decide whether it should flush anything or
not.
The new OPENED state only happen after odp_pktio_open(), after a start -> stop
transition it goes to a STOPPED state. Both valid for configuration, so instead
of "== STATE_STOPPED", check "!= STATE_STARTED".

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
diff --git a/platform/linux-generic/include/odp_packet_io_internal.h 
b/platform/linux-generic/include/odp_packet_io_internal.h
index 7636768..cca5c39 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -89,8 +89,10 @@ struct pktio_entry {
pkt_tap_t pkt_tap;  /**< using TAP for IO */
};
enum {
-   STATE_START = 0,
-   STATE_STOP
+   STATE_OPENED = 0,   /**< After open() */
+   STATE_STARTED,  /**< After start() */
+   STATE_STOPPED   /**< Same as OPENED, but only happens
+   after STARTED */
} state;
classifier_t cls;   /**< classifier linked with this pktio*/
odp_pktio_stats_t stats;/**< statistic counters for pktio */
diff --git a/platform/linux-generic/odp_packet_io.c 
b/platform/linux-generic/odp_packet_io.c
index 9192be2..f927c15 100644
--- a/platform/linux-generic/odp_packet_io.c
+++ b/platform/linux-generic/odp_packet_io.c
@@ -228,7 +228,7 @@ static odp_pktio_t setup_pktio_entry(const char *name, 
odp_pool_t pool,
} else {
snprintf(pktio_entry->s.name,
 sizeof(pktio_entry->s.name), "%s", name);
-   pktio_entry->s.state = STATE_STOP;
+   pktio_entry->s.state = STATE_OPENED;
}
 
unlock_entry(pktio_entry);
@@ -343,7 +343,8 @@ int odp_pktio_close(odp_pktio_t id)
if (entry == NULL)
return -1;
 
-   flush_in_queues(entry);
+   if (entry->s.state == STATE_STOPPED)
+   flush_in_queues(entry);
 
lock_entry(entry);
 
@@ -374,14 +375,14 @@ int odp_pktio_start(odp_pktio_t id)
return -1;
 
lock_entry(entry);
-   if (entry->s.state == STATE_START) {
+   if (entry->s.state == STATE_STARTED) {
unlock_entry(entry);
return -1;
}
if (entry->s.ops->start)
res = entry->s.ops->start(entry);
if (!res)
-   entry->s.state = STATE_START;
+   entry->s.state = STATE_STARTED;
 
unlock_entry(entry);
 
@@ -409,13 +410,13 @@ static int _pktio_stop(pktio_entry_t *entry)
 {
int res = 0;
 
-   if (entry->s.state == STATE_STOP)
+   if (entry->s.state != STATE_STARTED)
return -1;
 
if (entry->s.ops->stop)
res = entry->s.ops->stop(entry);
if (!res)
-   entry->s.state = STATE_STOP;
+   entry->s.state = STATE_STOPPED;
 
return res;
 }
@@ -502,7 +503,7 @@ static int _odp_pktio_send(odp_pktio_t id, odp_packet_t 
pkt_table[], int len)
return -1;
 
odp_ticketlock_lock(_entry->s.txl);
-   if (pktio_entry->s.state == STATE_STOP ||
+   if (pktio_entry->s.state != STATE_STARTED ||
pktio_entry->s.param.out_mode == ODP_PKTOUT_MODE_DISABLED) {
odp_ticketlock_unlock(_entry->s.txl);
__odp_errno = EPERM;
@@ -650,7 +651,7 @@ int pktin_poll(pktio_entry_t *entry, int num_queue, int 
index[])
if (odp_unlikely(entry->s.num_in_queue == 0))
return -1;
 
-   if (entry->s.state == STATE_STOP)
+   if (entry->s.state != STATE_STARTED)
return 0;
 
for (idx = 0; idx < num_queue; idx++) {
@@ -725,7 +726,7 @@ int odp_pktio_promisc_mode_set(odp_pktio_t id, odp_bool_t 
enable)
ODP_DBG("already freed pktio\n");
return -1;
}
-   if (entry->s.state != STATE_STOP) {
+   if (entry->s.state == STATE_STARTED) {
unlock_entry(entry);
return -1;
}
@@ -891,8 +892,10 @@ void odp_pktio_print(odp_pktio_t id)
"  type %s\n", entry->s.ops->name);
len += snprintf([len], n - len,
"  state%s\n",
-   entry->s.state ==  STATE_START ? "start" :
-  (entry->s.state ==  STATE_STOP ? "stop" : "unknown"));
+   entry->s.state ==  STATE_STARTED ? "start" :
+  (entry->s.state ==  STATE_STOPPED ? "stop" :
+

Re: [lng-odp] flush_in_queues() on devices never started

[Zoltan Kiss]

I forgot to Cc the list

On 31/03/16 17:03, Zoltan Kiss wrote:

Hi,

I ran into a problem when pktio_test_pktin_queue_config_direct() plays
with queue config then call close. That calls flush_in_queues(), which
tries to receive on a device which was never started, therefore run into
a segfault when trying to access the queues never created.
I've used the pkt_dpdk->started variable to avoid that in
recv_pkt_dpdk_queue(), but I wonder if that's the proper way. I guess
odp-linux would have the same issue on DPDK pktio. Any better idea?
I would also prefer avoiding branching on a hotpath like this. And we
should check if we need a similar check on send side.

diff --git a/platform/linux-dpdk/odp_packet_dpdk.c
b/platform/linux-dpdk/odp_packet_dpdk.c
index 7ddbac6..fdc461e 100644
--- a/platform/linux-dpdk/odp_packet_dpdk.c
+++ b/platform/linux-dpdk/odp_packet_dpdk.c
@@ -352,6 +352,9 @@ static int recv_pkt_dpdk_queue(pktio_entry_t
*pktio_entry, int index,
 pkt_dpdk_t * const pkt_dpdk = _entry->s.pkt_dpdk;
 uint8_t min = pkt_dpdk->min_rx_burst;

+   if (!pkt_dpdk->started)
+   return 0;
+
 if (odp_unlikely(min > len)) {
 ODP_DBG("PMD requires >%d buffers burst. "
 "Current %d, dropped %d\n", min, len, min - len);


Regards,

Zoltan

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] validation: timer: don't access non-existing timers

[Zoltan Kiss]

Ping

On 18/03/16 17:25, Zoltan Kiss wrote:

Since e5c85d3f "validation: timer: handle early exhaustion of pool" the
workers can handle if object caches retain packets, but with enough
threads it can happen that a late starting thread won't be able to
allocate any. This for loop should take that into account and not
trying to access tt[0].ev.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  test/validation/timer/timer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/validation/timer/timer.c b/test/validation/timer/timer.c
index 5854b32..7b76dbf 100644
--- a/test/validation/timer/timer.c
+++ b/test/validation/timer/timer.c
@@ -338,7 +338,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED)
uint32_t ms;
uint64_t prev_tick = odp_timer_current_tick(tp);

-   for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) {
+   for (ms = 0; ms < 7 * RANGE_MS / 10 && allocated > 0; ms++) {
odp_event_t ev;
while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) {
/* Subtract one from prev_tick to allow for timeouts


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH v2 0/2] packet_flags changes

[Zoltan Kiss]

Ping, this patch is a prereq for a fix in ODP-DPDK

On 18/03/16 17:20, Zoltan Kiss wrote:

These have to be applied in order, otherwise the code is not dependent on each
other.

v2: remove "odp_" from the internal function names

Zoltan Kiss (2):
   linux-generic: packet_flags: use accessors to modify eth and l2 flag
   linux-generic: packet: don't look for L2 header if there isn't any

  .../linux-generic/include/odp_classification_inlines.h|  2 +-
  platform/linux-generic/include/odp_packet_internal.h  | 15 +++
  platform/linux-generic/odp_classification.c   |  4 ++--
  platform/linux-generic/odp_packet.c   |  5 +
  4 files changed, 23 insertions(+), 3 deletions(-)


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2 0/2] packet_flags changes

[Zoltan Kiss]

These have to be applied in order, otherwise the code is not dependent on each
other.

v2: remove "odp_" from the internal function names

Zoltan Kiss (2):
  linux-generic: packet_flags: use accessors to modify eth and l2 flag
  linux-generic: packet: don't look for L2 header if there isn't any

 .../linux-generic/include/odp_classification_inlines.h|  2 +-
 platform/linux-generic/include/odp_packet_internal.h  | 15 +++
 platform/linux-generic/odp_classification.c   |  4 ++--
 platform/linux-generic/odp_packet.c   |  5 +
 4 files changed, 23 insertions(+), 3 deletions(-)

-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2 2/2] linux-generic: packet: don't look for L2 header if there isn't any

[Zoltan Kiss]

The L2 offset functions should consider the L2 flag: return negative
answer if there isn't any, and implicitly set it when offset is set.
E.g. user created packets don't have L2 headers immediately.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_packet_internal.h | 5 +
 platform/linux-generic/odp_packet.c  | 5 +
 2 files changed, 10 insertions(+)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index 7974a20..92b770f 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -272,6 +272,11 @@ static inline int packet_hdr_has_l2(odp_packet_hdr_t 
*pkt_hdr)
return pkt_hdr->input_flags.l2;
 }
 
+static inline void packet_hdr_has_l2_set(odp_packet_hdr_t *pkt_hdr, int val)
+{
+   pkt_hdr->input_flags.l2 = val;
+}
+
 static inline int packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
 {
return pkt_hdr->input_flags.eth;
diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index aac42b6..2c44316 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -353,12 +353,16 @@ uint32_t odp_packet_user_area_size(odp_packet_t pkt)
 void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len)
 {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!packet_hdr_has_l2(pkt_hdr))
+   return NULL;
return packet_map(pkt_hdr, pkt_hdr->l2_offset, len);
 }
 
 uint32_t odp_packet_l2_offset(odp_packet_t pkt)
 {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!packet_hdr_has_l2(pkt_hdr))
+   return ODP_PACKET_OFFSET_INVALID;
return pkt_hdr->l2_offset;
 }
 
@@ -369,6 +373,7 @@ int odp_packet_l2_offset_set(odp_packet_t pkt, uint32_t 
offset)
if (offset >= pkt_hdr->frame_len)
return -1;
 
+   packet_hdr_has_l2_set(pkt_hdr, 1);
pkt_hdr->l2_offset = offset;
return 0;
 }
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 2/2] linux-generic: packet: don't look for L2 header if there isn't any

[Zoltan Kiss]

My bad, I read those comments made for the other patch but I haven't 
applied them on this one.


On 18/03/16 12:07, Maxim Uvarov wrote:

On 03/18/16 14:39, Zoltan Kiss wrote:

Ping, added Bill



ping for what?  There are comments to rename functions.

Maxim.


On 11/03/16 07:03, Zoltan Kiss wrote:

Ping

On 03/03/16 03:05, Zoltan Kiss wrote:

The L2 offset functions should consider the L2 flag: return negative
answer if there isn't any, and implicitly set it when offset is set.
E.g. user created packets don't have L2 headers immediately.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  platform/linux-generic/include/odp_packet_internal.h | 5 +
  platform/linux-generic/odp_packet.c  | 5 +
  2 files changed, 10 insertions(+)

diff --git a/platform/linux-generic/include/odp_packet_internal.h
b/platform/linux-generic/include/odp_packet_internal.h
index d9fe544..1dc875e 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -263,6 +263,11 @@ static inline int
odp_packet_hdr_has_l2(odp_packet_hdr_t *pkt_hdr)
  return pkt_hdr->input_flags.l2;
  }
+static inline void odp_packet_hdr_has_l2_set(odp_packet_hdr_t
*pkt_hdr, int val)
+{
+pkt_hdr->input_flags.l2 = val;
+}
+
  static inline int odp_packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
  {
  return pkt_hdr->input_flags.eth;
diff --git a/platform/linux-generic/odp_packet.c
b/platform/linux-generic/odp_packet.c
index db85b5e..94d7f85 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -353,12 +353,16 @@ uint32_t odp_packet_user_area_size(odp_packet_t
pkt)
  void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len)
  {
  odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+if (!odp_packet_hdr_has_l2(pkt_hdr))
+return NULL;
  return packet_map(pkt_hdr, pkt_hdr->l2_offset, len);
  }
  uint32_t odp_packet_l2_offset(odp_packet_t pkt)
  {
  odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+if (!odp_packet_hdr_has_l2(pkt_hdr))
+return ODP_PACKET_OFFSET_INVALID;
  return pkt_hdr->l2_offset;
  }
@@ -369,6 +373,7 @@ int odp_packet_l2_offset_set(odp_packet_t pkt,
uint32_t offset)
  if (offset >= pkt_hdr->frame_len)
  return -1;
+odp_packet_hdr_has_l2_set(pkt_hdr, 1);
  pkt_hdr->l2_offset = offset;
  return 0;
  }





___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH v2 1/2] linux-generic: packet_flags: use accessors to modify eth and l2 flag

[Zoltan Kiss]

This makes it possible for other implementations like ODP-DPDK to reuse
classification code while using a different packet API.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_classification_inlines.h |  2 +-
 platform/linux-generic/include/odp_packet_internal.h| 10 ++
 platform/linux-generic/odp_classification.c |  4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/include/odp_classification_inlines.h 
b/platform/linux-generic/include/odp_classification_inlines.h
index 8bd010d..bac5b48 100644
--- a/platform/linux-generic/include/odp_classification_inlines.h
+++ b/platform/linux-generic/include/odp_classification_inlines.h
@@ -162,7 +162,7 @@ static inline int verify_pmr_dmac(const uint8_t *pkt_addr,
uint64_t dmac_be = 0;
const odph_ethhdr_t *eth;
 
-   if (!pkt_hdr->input_flags.eth)
+   if (!packet_hdr_has_eth(pkt_hdr))
return 0;
 
eth = (const odph_ethhdr_t *)(pkt_addr + pkt_hdr->l2_offset);
diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index b632ece..7974a20 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -267,6 +267,16 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt);
 /* Convert a buffer handle to a packet handle */
 odp_packet_t _odp_packet_from_buffer(odp_buffer_t buf);
 
+static inline int packet_hdr_has_l2(odp_packet_hdr_t *pkt_hdr)
+{
+   return pkt_hdr->input_flags.l2;
+}
+
+static inline int packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
+{
+   return pkt_hdr->input_flags.eth;
+}
+
 int _odp_parse_common(odp_packet_hdr_t *pkt_hdr, const uint8_t *parseptr);
 
 int _odp_cls_parse(odp_packet_hdr_t *pkt_hdr, const uint8_t *parseptr);
diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index 6c9f7c1..f5e4673 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -843,8 +843,8 @@ cos_t *match_qos_l2_cos(pmr_l2_cos_t *l2_cos, const uint8_t 
*pkt_addr,
const odph_vlanhdr_t *vlan;
uint16_t qos;
 
-   if (hdr->input_flags.l2 && hdr->input_flags.vlan &&
-   hdr->input_flags.eth) {
+   if (packet_hdr_has_l2(hdr) && hdr->input_flags.vlan &&
+   packet_hdr_has_eth(hdr)) {
eth = (const odph_ethhdr_t *)(pkt_addr + hdr->l2_offset);
vlan = (const odph_vlanhdr_t *)(>type);
qos = odp_be_to_cpu_16(vlan->tci);
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH] validation: timer: don't access non-existing timers

[Zoltan Kiss]

Note, this is just a resend with a different set of CC recipients

On 18/03/16 17:25, Zoltan Kiss wrote:

Since e5c85d3f "validation: timer: handle early exhaustion of pool" the
workers can handle if object caches retain packets, but with enough
threads it can happen that a late starting thread won't be able to
allocate any. This for loop should take that into account and not
trying to access tt[0].ev.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  test/validation/timer/timer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/validation/timer/timer.c b/test/validation/timer/timer.c
index 5854b32..7b76dbf 100644
--- a/test/validation/timer/timer.c
+++ b/test/validation/timer/timer.c
@@ -338,7 +338,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED)
uint32_t ms;
uint64_t prev_tick = odp_timer_current_tick(tp);

-   for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) {
+   for (ms = 0; ms < 7 * RANGE_MS / 10 && allocated > 0; ms++) {
odp_event_t ev;
while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) {
/* Subtract one from prev_tick to allow for timeouts


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 2/2] linux-generic: packet: don't look for L2 header if there isn't any

[Zoltan Kiss]

Ping, added Bill

On 11/03/16 07:03, Zoltan Kiss wrote:

Ping

On 03/03/16 03:05, Zoltan Kiss wrote:

The L2 offset functions should consider the L2 flag: return negative
answer if there isn't any, and implicitly set it when offset is set.
E.g. user created packets don't have L2 headers immediately.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  platform/linux-generic/include/odp_packet_internal.h | 5 +
  platform/linux-generic/odp_packet.c  | 5 +
  2 files changed, 10 insertions(+)

diff --git a/platform/linux-generic/include/odp_packet_internal.h
b/platform/linux-generic/include/odp_packet_internal.h
index d9fe544..1dc875e 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -263,6 +263,11 @@ static inline int
odp_packet_hdr_has_l2(odp_packet_hdr_t *pkt_hdr)
  return pkt_hdr->input_flags.l2;
  }
+static inline void odp_packet_hdr_has_l2_set(odp_packet_hdr_t
*pkt_hdr, int val)
+{
+pkt_hdr->input_flags.l2 = val;
+}
+
  static inline int odp_packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
  {
  return pkt_hdr->input_flags.eth;
diff --git a/platform/linux-generic/odp_packet.c
b/platform/linux-generic/odp_packet.c
index db85b5e..94d7f85 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -353,12 +353,16 @@ uint32_t odp_packet_user_area_size(odp_packet_t
pkt)
  void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len)
  {
  odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+if (!odp_packet_hdr_has_l2(pkt_hdr))
+return NULL;
  return packet_map(pkt_hdr, pkt_hdr->l2_offset, len);
  }
  uint32_t odp_packet_l2_offset(odp_packet_t pkt)
  {
  odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+if (!odp_packet_hdr_has_l2(pkt_hdr))
+return ODP_PACKET_OFFSET_INVALID;
  return pkt_hdr->l2_offset;
  }
@@ -369,6 +373,7 @@ int odp_packet_l2_offset_set(odp_packet_t pkt,
uint32_t offset)
  if (offset >= pkt_hdr->frame_len)
  return -1;
+odp_packet_hdr_has_l2_set(pkt_hdr, 1);
  pkt_hdr->l2_offset = offset;
  return 0;
  }



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] validation: pktio: add more debug log to pktio_pkt_seq()

[Zoltan Kiss]

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 test/validation/pktio/pktio.c | 26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index 9443cd2..cb403a6 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -176,29 +176,45 @@ static uint32_t pktio_pkt_seq(odp_packet_t pkt)
pkt_head_t head;
pkt_tail_t tail;
 
-   if (pkt == ODP_PACKET_INVALID)
+   if (pkt == ODP_PACKET_INVALID) {
+   fprintf(stderr, "error: pkt invalid\n");
return TEST_SEQ_INVALID;
+   }
 
off = odp_packet_l4_offset(pkt);
-   if (off ==  ODP_PACKET_OFFSET_INVALID)
+   if (off ==  ODP_PACKET_OFFSET_INVALID) {
+   fprintf(stderr, "error: offset invalid\n");
return TEST_SEQ_INVALID;
+   }
 
off += ODPH_UDPHDR_LEN;
-   if (odp_packet_copydata_out(pkt, off, sizeof(head), ) != 0)
+   if (odp_packet_copydata_out(pkt, off, sizeof(head), ) != 0) {
+   fprintf(stderr, "error: header copy failed\n");
return TEST_SEQ_INVALID;
+   }
 
-   if (head.magic != TEST_SEQ_MAGIC)
+   if (head.magic != TEST_SEQ_MAGIC) {
+   fprintf(stderr, "error: header magic invalid %u\n", head.magic);
return TEST_SEQ_INVALID;
+   }
 
if (odp_packet_len(pkt) == packet_len) {
off = packet_len - sizeof(tail);
-   if (odp_packet_copydata_out(pkt, off, sizeof(tail), ) != 0)
+   if (odp_packet_copydata_out(pkt, off, sizeof(tail), ) != 
0) {
+   fprintf(stderr, "error: header copy failed\n");
return TEST_SEQ_INVALID;
+   }
 
if (tail.magic == TEST_SEQ_MAGIC) {
seq = head.seq;
CU_ASSERT(seq != TEST_SEQ_INVALID);
+   } else {
+   fprintf(stderr, "error: tail magic invalid %u\n",
+   tail.magic);
}
+   } else {
+   fprintf(stderr, "error: packet length invalid: %u (%u)\n",
+   odp_packet_len(pkt), packet_len);
}
 
return seq;
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [PATCH 2/2] linux-generic: packet: don't look for L2 header if there isn't any

[Zoltan Kiss]

Ping

On 03/03/16 03:05, Zoltan Kiss wrote:

The L2 offset functions should consider the L2 flag: return negative
answer if there isn't any, and implicitly set it when offset is set.
E.g. user created packets don't have L2 headers immediately.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
  platform/linux-generic/include/odp_packet_internal.h | 5 +
  platform/linux-generic/odp_packet.c  | 5 +
  2 files changed, 10 insertions(+)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d9fe544..1dc875e 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -263,6 +263,11 @@ static inline int odp_packet_hdr_has_l2(odp_packet_hdr_t 
*pkt_hdr)
return pkt_hdr->input_flags.l2;
  }
  
+static inline void odp_packet_hdr_has_l2_set(odp_packet_hdr_t *pkt_hdr, int val)

+{
+   pkt_hdr->input_flags.l2 = val;
+}
+
  static inline int odp_packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
  {
return pkt_hdr->input_flags.eth;
diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index db85b5e..94d7f85 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -353,12 +353,16 @@ uint32_t odp_packet_user_area_size(odp_packet_t pkt)
  void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len)
  {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!odp_packet_hdr_has_l2(pkt_hdr))
+   return NULL;
return packet_map(pkt_hdr, pkt_hdr->l2_offset, len);
  }
  
  uint32_t odp_packet_l2_offset(odp_packet_t pkt)

  {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!odp_packet_hdr_has_l2(pkt_hdr))
+   return ODP_PACKET_OFFSET_INVALID;
return pkt_hdr->l2_offset;
  }
  
@@ -369,6 +373,7 @@ int odp_packet_l2_offset_set(odp_packet_t pkt, uint32_t offset)

if (offset >= pkt_hdr->frame_len)
return -1;
  
+	odp_packet_hdr_has_l2_set(pkt_hdr, 1);

pkt_hdr->l2_offset = offset;
return 0;
  }


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] validation: timer: don't access non-existing timers

[Zoltan Kiss]

Since e5c85d3f "validation: timer: handle early exhaustion of pool" the
workers can handle if object caches retain packets, but with enough
threads it can happen that a late starting thread won't be able to
allocate any. This for loop should take that into account and not
trying to access tt[0].ev.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 test/validation/timer/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/validation/timer/timer.c b/test/validation/timer/timer.c
index 004670a..85a5061 100644
--- a/test/validation/timer/timer.c
+++ b/test/validation/timer/timer.c
@@ -338,7 +338,7 @@ static void *worker_entrypoint(void *arg TEST_UNUSED)
uint32_t ms;
uint64_t prev_tick = odp_timer_current_tick(tp);
 
-   for (ms = 0; ms < 7 * RANGE_MS / 10; ms++) {
+   for (ms = 0; ms < 7 * RANGE_MS / 10 && allocated > 0; ms++) {
odp_event_t ev;
while ((ev = odp_queue_deq(queue)) != ODP_EVENT_INVALID) {
/* Subtract one from prev_tick to allow for timeouts
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH] validation: pktio: add more debug log to pktio_pkt_seq()

[Zoltan Kiss]

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 test/validation/pktio/pktio.c | 26 +-
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/test/validation/pktio/pktio.c b/test/validation/pktio/pktio.c
index a5f25da..084e5cd 100644
--- a/test/validation/pktio/pktio.c
+++ b/test/validation/pktio/pktio.c
@@ -172,29 +172,45 @@ static uint32_t pktio_pkt_seq(odp_packet_t pkt)
pkt_head_t head;
pkt_tail_t tail;
 
-   if (pkt == ODP_PACKET_INVALID)
+   if (pkt == ODP_PACKET_INVALID) {
+   fprintf(stderr, "error: pkt invalid\n");
return TEST_SEQ_INVALID;
+   }
 
off = odp_packet_l4_offset(pkt);
-   if (off ==  ODP_PACKET_OFFSET_INVALID)
+   if (off ==  ODP_PACKET_OFFSET_INVALID) {
+   fprintf(stderr, "error: offset invalid\n");
return TEST_SEQ_INVALID;
+   }
 
off += ODPH_UDPHDR_LEN;
-   if (odp_packet_copydata_out(pkt, off, sizeof(head), ) != 0)
+   if (odp_packet_copydata_out(pkt, off, sizeof(head), ) != 0) {
+   fprintf(stderr, "error: header copy failed\n");
return TEST_SEQ_INVALID;
+   }
 
-   if (head.magic != TEST_SEQ_MAGIC)
+   if (head.magic != TEST_SEQ_MAGIC) {
+   fprintf(stderr, "error: header magic invalid %u\n", head.magic);
return TEST_SEQ_INVALID;
+   }
 
if (odp_packet_len(pkt) == packet_len) {
off = packet_len - sizeof(tail);
-   if (odp_packet_copydata_out(pkt, off, sizeof(tail), ) != 0)
+   if (odp_packet_copydata_out(pkt, off, sizeof(tail), ) != 
0) {
+   fprintf(stderr, "error: header copy failed\n");
return TEST_SEQ_INVALID;
+   }
 
if (tail.magic == TEST_SEQ_MAGIC) {
seq = head.seq;
CU_ASSERT(seq != TEST_SEQ_INVALID);
+   } else {
+   fprintf(stderr, "error: tail magic invalid %u\n",
+   tail.magic);
}
+   } else {
+   fprintf(stderr, "error: packet length invalid: %u (%u)\n",
+   odp_packet_len(pkt), packet_len);
}
 
return seq;
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 2/2] linux-generic: packet: don't look for L2 header if there isn't any

[Zoltan Kiss]

The L2 offset functions should consider the L2 flag: return negative
answer if there isn't any, and implicitly set it when offset is set.
E.g. user created packets don't have L2 headers immediately.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_packet_internal.h | 5 +
 platform/linux-generic/odp_packet.c  | 5 +
 2 files changed, 10 insertions(+)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d9fe544..1dc875e 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -263,6 +263,11 @@ static inline int odp_packet_hdr_has_l2(odp_packet_hdr_t 
*pkt_hdr)
return pkt_hdr->input_flags.l2;
 }
 
+static inline void odp_packet_hdr_has_l2_set(odp_packet_hdr_t *pkt_hdr, int 
val)
+{
+   pkt_hdr->input_flags.l2 = val;
+}
+
 static inline int odp_packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
 {
return pkt_hdr->input_flags.eth;
diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index db85b5e..94d7f85 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -353,12 +353,16 @@ uint32_t odp_packet_user_area_size(odp_packet_t pkt)
 void *odp_packet_l2_ptr(odp_packet_t pkt, uint32_t *len)
 {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!odp_packet_hdr_has_l2(pkt_hdr))
+   return NULL;
return packet_map(pkt_hdr, pkt_hdr->l2_offset, len);
 }
 
 uint32_t odp_packet_l2_offset(odp_packet_t pkt)
 {
odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   if (!odp_packet_hdr_has_l2(pkt_hdr))
+   return ODP_PACKET_OFFSET_INVALID;
return pkt_hdr->l2_offset;
 }
 
@@ -369,6 +373,7 @@ int odp_packet_l2_offset_set(odp_packet_t pkt, uint32_t 
offset)
if (offset >= pkt_hdr->frame_len)
return -1;
 
+   odp_packet_hdr_has_l2_set(pkt_hdr, 1);
pkt_hdr->l2_offset = offset;
return 0;
 }
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 0/2] packet_flags changes

[Zoltan Kiss]

These have to be applied in order, otherwise the code is not dependent on each
other.

Zoltan Kiss (2)
  linux-generic: packet_flags: use accessors to modify eth and l2 flag
  linux-generic: packet: don't look for L2 header if there isn't any
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] [PATCH 1/2] linux-generic: packet_flags: use accessors to modify eth and l2 flag

[Zoltan Kiss]

This makes it possible for other implementations like ODP-DPDK to reuse
classification code while using a different packet API.

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>
---
 platform/linux-generic/include/odp_classification_inlines.h |  2 +-
 platform/linux-generic/include/odp_packet_internal.h| 10 ++
 platform/linux-generic/odp_classification.c |  4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/platform/linux-generic/include/odp_classification_inlines.h 
b/platform/linux-generic/include/odp_classification_inlines.h
index 96cf77e..2318349 100644
--- a/platform/linux-generic/include/odp_classification_inlines.h
+++ b/platform/linux-generic/include/odp_classification_inlines.h
@@ -162,7 +162,7 @@ static inline int verify_pmr_dmac(const uint8_t *pkt_addr,
uint64_t dmac_be = 0;
const odph_ethhdr_t *eth;
 
-   if (!pkt_hdr->input_flags.eth)
+   if (!odp_packet_hdr_has_eth(pkt_hdr))
return 0;
 
eth = (const odph_ethhdr_t *)(pkt_addr + pkt_hdr->l2_offset);
diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index 85d4924..d9fe544 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -258,6 +258,16 @@ odp_buffer_t _odp_packet_to_buffer(odp_packet_t pkt);
 /* Convert a buffer handle to a packet handle */
 odp_packet_t _odp_packet_from_buffer(odp_buffer_t buf);
 
+static inline int odp_packet_hdr_has_l2(odp_packet_hdr_t *pkt_hdr)
+{
+   return pkt_hdr->input_flags.l2;
+}
+
+static inline int odp_packet_hdr_has_eth(odp_packet_hdr_t *pkt_hdr)
+{
+   return pkt_hdr->input_flags.eth;
+}
+
 int _odp_parse_common(odp_packet_hdr_t *pkt_hdr, const uint8_t *parseptr);
 
 int _odp_cls_parse(odp_packet_hdr_t *pkt_hdr, const uint8_t *parseptr);
diff --git a/platform/linux-generic/odp_classification.c 
b/platform/linux-generic/odp_classification.c
index da195ad..4551951 100644
--- a/platform/linux-generic/odp_classification.c
+++ b/platform/linux-generic/odp_classification.c
@@ -1003,8 +1003,8 @@ cos_t *match_qos_l2_cos(pmr_l2_cos_t *l2_cos, const 
uint8_t *pkt_addr,
const odph_vlanhdr_t *vlan;
uint16_t qos;
 
-   if (hdr->input_flags.l2 && hdr->input_flags.vlan &&
-   hdr->input_flags.eth) {
+   if (odp_packet_hdr_has_l2(hdr) && hdr->input_flags.vlan &&
+   odp_packet_hdr_has_eth(hdr)) {
eth = (const odph_ethhdr_t *)(pkt_addr + hdr->l2_offset);
vlan = (const odph_vlanhdr_t *)(>type);
qos = odp_be_to_cpu_16(vlan->tci);
-- 
1.9.1

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] pktio stats counters

[Zoltan Kiss]

I think the general question is: is it expected that the stats include 
the outgoing traffic of other, non-ODP applications using the same 
interface? Although I guess in most ODP implementations it's not 
possible to share an ODP interface, but generally it's not prohibited.



On 29/02/16 12:40, Zoltan Kiss wrote:

Yes, I've already have that, and some more switches, like "-multicast",
but it's impossible to predict all the services which will run on that
interface.

Zoli

On 19/02/16 08:45, Maxim Uvarov wrote:

In pktio_env for linux-generic we use disable arp:

ifconfig $iface -arp

In that case there is not dhcp and other traffic on veth.
Will it work for you?

Maxim.

On 02/18/16 21:06, Zoltan Kiss wrote:

Hi,

On ODP-DPDK this assert fails every now and then:

https://git.linaro.org/lng/odp.git/blob/refs/heads/api-next:/test/validation/pktio/pktio.c#l1193



The reason is that when you set the interfaces up, there are other
system services which can hook into the interface creation, and start
doing stuff. And it often means that they send out packets behind the
back of pktio[0], which is received by pktio[1], and that changes the
amount of received bytes there (but not on the sending side). E.g. on
my system there is mDNS, DHCP, NetBIOS so far which can ruin this test.
ODP-DPDK uses PCAP, which seems not to be able to catch all the
outgoing packets. I guess on linux-generic we use the kernel stats in
the unit tests which can do that.
Any idea how to solve this problem?

Regards,

Zoltan Kiss
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] pktio stats counters

[Zoltan Kiss]

Yes, I've already have that, and some more switches, like "-multicast", 
but it's impossible to predict all the services which will run on that 
interface.


Zoli

On 19/02/16 08:45, Maxim Uvarov wrote:

In pktio_env for linux-generic we use disable arp:

ifconfig $iface -arp

In that case there is not dhcp and other traffic on veth.
Will it work for you?

Maxim.

On 02/18/16 21:06, Zoltan Kiss wrote:

Hi,

On ODP-DPDK this assert fails every now and then:

https://git.linaro.org/lng/odp.git/blob/refs/heads/api-next:/test/validation/pktio/pktio.c#l1193


The reason is that when you set the interfaces up, there are other
system services which can hook into the interface creation, and start
doing stuff. And it often means that they send out packets behind the
back of pktio[0], which is received by pktio[1], and that changes the
amount of received bytes there (but not on the sending side). E.g. on
my system there is mDNS, DHCP, NetBIOS so far which can ruin this test.
ODP-DPDK uses PCAP, which seems not to be able to catch all the
outgoing packets. I guess on linux-generic we use the kernel stats in
the unit tests which can do that.
Any idea how to solve this problem?

Regards,

Zoltan Kiss
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close resources in odp_pktio_close()

[Zoltan Kiss]

On 25/02/16 15:53, Zoltan Kiss wrote:


pktio_entry already has a 'state' field, why not using that?



Sorry for the slow reply. The state field in pktio_entry is modified
outside dpdk

pktio code, so it cannot be used to track if rte_eth_dev_start() has
been actually
called. If rte_eth_dev_start() has not been called,
rte_eth_dev_close() cannot
be used in dpdk_close().

"state" is set to STATE_START only if rte_eth_dev_start() succeeded (and
everything else), and the stop function will call rte_eth_dev_close()
only if "state" is STATE_START. I think it's good enough for tracking



It is not the stop() function, which is calling rte_eth_dev_close().
It is close(). And because close() can be called only on a stopped
interface the "state" variable cannot be used to check if the
interface has been started.


I see. It seems DPDK has a different startup process, but I think it's
definitely a bug for them that close() crashes when you call it on an
interface never been started. Either they should document that the
application should track that, or even better, check "struct
rte_eth_dev" for that (dev->data->dev_started == 1) before closing.
Anyhow, we can work that around by checking ourselves, we don't need an
extra variable for that.


Oh wait, we can't work it around as that variable is not exposed. Then 
your solution remains, and patching it in DPDK later.


Zoli
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close resources in odp_pktio_close()

[Zoltan Kiss]

On 25/02/16 07:22, Elo, Matias (Nokia - FI/Espoo) wrote:




-Original Message-
From: EXT Zoltan Kiss [mailto:zoltan.k...@linaro.org]
Sent: Wednesday, February 24, 2016 9:10 PM
To: Elo, Matias (Nokia - FI/Espoo) <matias@nokia.com>; lng-
o...@lists.linaro.org
Subject: Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close
resources in odp_pktio_close()



On 24/02/16 07:36, Elo, Matias (Nokia - FI/Espoo) wrote:

-Original Message-
From: EXT Zoltan Kiss [mailto:zoltan.k...@linaro.org]
Sent: Friday, February 19, 2016 6:15 PM
To: Elo, Matias (Nokia - FI/Espoo) <matias@nokia.com>; lng-
o...@lists.linaro.org
Subject: Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close
resources in odp_pktio_close()

pktio_entry already has a 'state' field, why not using that?



Sorry for the slow reply. The state field in pktio_entry is modified outside 
dpdk

pktio code, so it cannot be used to track if rte_eth_dev_start() has been 
actually
called. If rte_eth_dev_start() has not been called, rte_eth_dev_close() cannot
be used in dpdk_close().

"state" is set to STATE_START only if rte_eth_dev_start() succeeded (and
everything else), and the stop function will call rte_eth_dev_close()
only if "state" is STATE_START. I think it's good enough for tracking



It is not the stop() function, which is calling rte_eth_dev_close(). It is close(). And 
because close() can be called only on a stopped interface the "state" variable 
cannot be used to check if the interface has been started.


I see. It seems DPDK has a different startup process, but I think it's 
definitely a bug for them that close() crashes when you call it on an 
interface never been started. Either they should document that the 
application should track that, or even better, check "struct 
rte_eth_dev" for that (dev->data->dev_started == 1) before closing.
Anyhow, we can work that around by checking ourselves, we don't need an 
extra variable for that.





-Matias



-Matias


On 15/02/16 10:50, Matias Elo wrote:

Free/close open resources in odp_pktio_close().

Reviewed-by: Petri Savolainen <petri.savolai...@nokia.com>
Signed-off-by: Matias Elo <matias@nokia.com>
---
platform/linux-generic/include/odp_packet_dpdk.h |  1 +
platform/linux-generic/pktio/dpdk.c  | 19 ++-
2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/platform/linux-generic/include/odp_packet_dpdk.h

b/platform/linux-generic/include/odp_packet_dpdk.h

index 9c8f7be..a085582 100644
--- a/platform/linux-generic/include/odp_packet_dpdk.h
+++ b/platform/linux-generic/include/odp_packet_dpdk.h
@@ -54,6 +54,7 @@ typedef struct {
uint16_t mtu; /**< maximum transmission unit */
/** DPDK packet pool name (pktpool_) */
char pool_name[IF_NAMESIZE + 8];
+   odp_bool_t started;   /**< DPDK device has been started */
uint8_t port_id;  /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash;  /**< Packet input hash protocol

*/

diff --git a/platform/linux-generic/pktio/dpdk.c b/platform/linux-

generic/pktio/dpdk.c

index 1082c59..c81bb43 100644
--- a/platform/linux-generic/pktio/dpdk.c
+++ b/platform/linux-generic/pktio/dpdk.c
@@ -217,8 +217,23 @@ static int dpdk_setup_port(pktio_entry_t

*pktio_entry)

return 0;
}

-static int dpdk_close(pktio_entry_t *pktio_entry ODP_UNUSED)
+static int dpdk_close(pktio_entry_t *pktio_entry)
{
+   pkt_dpdk_t *pkt_dpdk = _entry->s.pkt_dpdk;
+   unsigned idx;
+   unsigned i, j;
+
+   /* Free cache packets */
+   for (i = 0; i < PKTIO_MAX_QUEUES; i++) {
+   idx = pkt_dpdk->rx_cache[i].s.idx;
+
+   for (j = 0; j < pkt_dpdk->rx_cache[i].s.count; j++)
+   rte_pktmbuf_free(pkt_dpdk->rx_cache[i].s.pkt[idx++]);
+   }
+
+   if (pkt_dpdk->started)
+   rte_eth_dev_close(pkt_dpdk->port_id);
+
return 0;
}

@@ -515,6 +530,8 @@ static int dpdk_start(pktio_entry_t *pktio_entry)
ret, port_id);
return -1;
}
+   pkt_dpdk->started = 1;
+
return 0;
}



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close resources in odp_pktio_close()

[Zoltan Kiss]

On 24/02/16 07:36, Elo, Matias (Nokia - FI/Espoo) wrote:

-Original Message-
From: EXT Zoltan Kiss [mailto:zoltan.k...@linaro.org]
Sent: Friday, February 19, 2016 6:15 PM
To: Elo, Matias (Nokia - FI/Espoo) <matias@nokia.com>; lng-
o...@lists.linaro.org
Subject: Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close
resources in odp_pktio_close()

pktio_entry already has a 'state' field, why not using that?



Sorry for the slow reply. The state field in pktio_entry is modified outside 
dpdk pktio code, so it cannot be used to track if rte_eth_dev_start() has been 
actually called. If rte_eth_dev_start() has not been called, 
rte_eth_dev_close() cannot be used in dpdk_close().


"state" is set to STATE_START only if rte_eth_dev_start() succeeded (and 
everything else), and the stop function will call rte_eth_dev_close() 
only if "state" is STATE_START. I think it's good enough for tracking




-Matias


On 15/02/16 10:50, Matias Elo wrote:

Free/close open resources in odp_pktio_close().

Reviewed-by: Petri Savolainen <petri.savolai...@nokia.com>
Signed-off-by: Matias Elo <matias@nokia.com>
---
   platform/linux-generic/include/odp_packet_dpdk.h |  1 +
   platform/linux-generic/pktio/dpdk.c  | 19 ++-
   2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/platform/linux-generic/include/odp_packet_dpdk.h

b/platform/linux-generic/include/odp_packet_dpdk.h

index 9c8f7be..a085582 100644
--- a/platform/linux-generic/include/odp_packet_dpdk.h
+++ b/platform/linux-generic/include/odp_packet_dpdk.h
@@ -54,6 +54,7 @@ typedef struct {
uint16_t mtu; /**< maximum transmission unit */
/** DPDK packet pool name (pktpool_) */
char pool_name[IF_NAMESIZE + 8];
+   odp_bool_t started;   /**< DPDK device has been started */
uint8_t port_id;  /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash;  /**< Packet input hash protocol

*/

diff --git a/platform/linux-generic/pktio/dpdk.c b/platform/linux-

generic/pktio/dpdk.c

index 1082c59..c81bb43 100644
--- a/platform/linux-generic/pktio/dpdk.c
+++ b/platform/linux-generic/pktio/dpdk.c
@@ -217,8 +217,23 @@ static int dpdk_setup_port(pktio_entry_t

*pktio_entry)

return 0;
   }

-static int dpdk_close(pktio_entry_t *pktio_entry ODP_UNUSED)
+static int dpdk_close(pktio_entry_t *pktio_entry)
   {
+   pkt_dpdk_t *pkt_dpdk = _entry->s.pkt_dpdk;
+   unsigned idx;
+   unsigned i, j;
+
+   /* Free cache packets */
+   for (i = 0; i < PKTIO_MAX_QUEUES; i++) {
+   idx = pkt_dpdk->rx_cache[i].s.idx;
+
+   for (j = 0; j < pkt_dpdk->rx_cache[i].s.count; j++)
+   rte_pktmbuf_free(pkt_dpdk->rx_cache[i].s.pkt[idx++]);
+   }
+
+   if (pkt_dpdk->started)
+   rte_eth_dev_close(pkt_dpdk->port_id);
+
return 0;
   }

@@ -515,6 +530,8 @@ static int dpdk_start(pktio_entry_t *pktio_entry)
ret, port_id);
return -1;
}
+   pkt_dpdk->started = 1;
+
return 0;
   }



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 10/13] linux-generic: dpdk: add odp_pktio_input_queues_config()

[Zoltan Kiss]

On 22/02/16 09:07, Elo, Matias (Nokia - FI/Espoo) wrote:

-Original Message-
From: EXT Zoltan Kiss [mailto:zoltan.k...@linaro.org]
Sent: Friday, February 19, 2016 6:13 PM
To: Elo, Matias (Nokia - FI/Espoo) <matias@nokia.com>; lng-
o...@lists.linaro.org
Cc: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com>
Subject: Re: [lng-odp] [API-NEXT PATCH v4 10/13] linux-generic: dpdk: add
odp_pktio_input_queues_config()

Hi,

I'm implementing multiqueue for ODP-DPDK as well, where I'm taking a lot
of your code, so finally I'm doing a bigger review of this code:


Hi,

Great to have feedback. I'll hopefully have time to test and review you patches 
tomorrow.

-Matias


On 15/02/16 10:49, Matias Elo wrote:

@@ -309,6 +335,29 @@ static int odp_dpdk_pktio_init_local(void)
return 0;
   }

+static int dpdk_input_queues_config(pktio_entry_t *pktio_entry,
+   const odp_pktin_queue_param_t *p)
+{
+   odp_pktin_mode_t mode = pktio_entry->s.param.in_mode;
+   odp_bool_t lockless;
+
+   /**
+* Scheduler synchronizes input queue polls. Only single thread
+* at a time polls a queue */
+   if (mode == ODP_PKTIN_MODE_SCHED ||
+   p->op_mode == ODP_PKTIO_OP_MT_UNSAFE)
+   lockless = 1;
+   else
+   lockless = 0;
+
+   if (p->hash_enable && p->num_queues > 1)
+   pktio_entry->s.pkt_dpdk.hash = p->hash_proto;

We had a discussion about this when the API was introduced. The final
take for me was that this should not influence whether an actual hash is
generated, only the fact that hash is used to distribute packets between
queues. In DPDK you can't actually make difference between the two, so
even if these conditions are not met you have to generate something for
odp_packet_flow_hash().
I've simply added this else branch:

else
pktio_entry->s.pkt_dpdk.hash.all_bits = UINT32_MAX;


Good catch, I had missed the original API discussion. Instead of enabling all 
the hash protocols I'd rather go with a minimal default DPDK hash. ETH_RSS_IP 
seems like a good candidate for this. In the future odp_pktin_hash_proto_t may 
include more protocols and some of the combinations may not be supported by the 
DPDK drivers. I minimal default value would be more future proof and support 
more drivers.
Makes sense to not going all in. I would add TCP and UDP, as it is 
likely to be supported by everyone. This is the current default used in 
ODP-DPDK


This 'default hash' can be set in dpdk_setup_port() helper. I'll make a patch 
to implement this.




+
+   pktio_entry->s.pkt_dpdk.lockless_rx = lockless;
+
+   return 0;
+}
+


___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] locking in queue recv/send

[Zoltan Kiss]

Hi,

I've noticed that "struct pktio_entry" locks 'rxl' and 'txl' are not 
grabbed when you call odp_pktio_recv/send_queue() functions, only when 
you have a single queue, which goes to the old codepath. This is 
particularly dangerous when doing close, but other cases might have 
trouble with it too.
I think we should ditch rxl and txl and grab/release the per-queue locks 
in [un]lock_entry(). Maybe we should bring those locks into "struct 
pktio_entry"?
Plus we should mention at the ODP_PKTIO_OP_MT_UNSAFE description that 
the application is responsible for the synchronization of not jut just 
send/recv, but other pktio operations which can interfere. Particularly 
start/stop, close, term_*. Fortunately these status changing calls still 
sync with the status query calls (e.g. link status, mtu get) through the 
lock.


Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 13/13] linux-generic: dpdk: close resources in odp_pktio_close()

[Zoltan Kiss]

pktio_entry already has a 'state' field, why not using that?

On 15/02/16 10:50, Matias Elo wrote:

Free/close open resources in odp_pktio_close().

Reviewed-by: Petri Savolainen 
Signed-off-by: Matias Elo 
---
  platform/linux-generic/include/odp_packet_dpdk.h |  1 +
  platform/linux-generic/pktio/dpdk.c  | 19 ++-
  2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/platform/linux-generic/include/odp_packet_dpdk.h 
b/platform/linux-generic/include/odp_packet_dpdk.h
index 9c8f7be..a085582 100644
--- a/platform/linux-generic/include/odp_packet_dpdk.h
+++ b/platform/linux-generic/include/odp_packet_dpdk.h
@@ -54,6 +54,7 @@ typedef struct {
uint16_t mtu; /**< maximum transmission unit */
/** DPDK packet pool name (pktpool_) */
char pool_name[IF_NAMESIZE + 8];
+   odp_bool_t started;   /**< DPDK device has been started */
uint8_t port_id;  /**< DPDK port identifier */
unsigned min_rx_burst;/**< minimum RX burst size */
odp_pktin_hash_proto_t hash;  /**< Packet input hash protocol */
diff --git a/platform/linux-generic/pktio/dpdk.c 
b/platform/linux-generic/pktio/dpdk.c
index 1082c59..c81bb43 100644
--- a/platform/linux-generic/pktio/dpdk.c
+++ b/platform/linux-generic/pktio/dpdk.c
@@ -217,8 +217,23 @@ static int dpdk_setup_port(pktio_entry_t *pktio_entry)
return 0;
  }

-static int dpdk_close(pktio_entry_t *pktio_entry ODP_UNUSED)
+static int dpdk_close(pktio_entry_t *pktio_entry)
  {
+   pkt_dpdk_t *pkt_dpdk = _entry->s.pkt_dpdk;
+   unsigned idx;
+   unsigned i, j;
+
+   /* Free cache packets */
+   for (i = 0; i < PKTIO_MAX_QUEUES; i++) {
+   idx = pkt_dpdk->rx_cache[i].s.idx;
+
+   for (j = 0; j < pkt_dpdk->rx_cache[i].s.count; j++)
+   rte_pktmbuf_free(pkt_dpdk->rx_cache[i].s.pkt[idx++]);
+   }
+
+   if (pkt_dpdk->started)
+   rte_eth_dev_close(pkt_dpdk->port_id);
+
return 0;
  }

@@ -515,6 +530,8 @@ static int dpdk_start(pktio_entry_t *pktio_entry)
ret, port_id);
return -1;
}
+   pkt_dpdk->started = 1;
+
return 0;
  }



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 10/13] linux-generic: dpdk: add odp_pktio_input_queues_config()

[Zoltan Kiss]

Hi,

I'm implementing multiqueue for ODP-DPDK as well, where I'm taking a lot 
of your code, so finally I'm doing a bigger review of this code:


On 15/02/16 10:49, Matias Elo wrote:

@@ -309,6 +335,29 @@ static int odp_dpdk_pktio_init_local(void)
return 0;
  }

+static int dpdk_input_queues_config(pktio_entry_t *pktio_entry,
+   const odp_pktin_queue_param_t *p)
+{
+   odp_pktin_mode_t mode = pktio_entry->s.param.in_mode;
+   odp_bool_t lockless;
+
+   /**
+* Scheduler synchronizes input queue polls. Only single thread
+* at a time polls a queue */
+   if (mode == ODP_PKTIN_MODE_SCHED ||
+   p->op_mode == ODP_PKTIO_OP_MT_UNSAFE)
+   lockless = 1;
+   else
+   lockless = 0;
+
+   if (p->hash_enable && p->num_queues > 1)
+   pktio_entry->s.pkt_dpdk.hash = p->hash_proto;


We had a discussion about this when the API was introduced. The final 
take for me was that this should not influence whether an actual hash is 
generated, only the fact that hash is used to distribute packets between 
queues. In DPDK you can't actually make difference between the two, so 
even if these conditions are not met you have to generate something for 
odp_packet_flow_hash().

I've simply added this else branch:

else
pktio_entry->s.pkt_dpdk.hash.all_bits = UINT32_MAX;




+
+   pktio_entry->s.pkt_dpdk.lockless_rx = lockless;
+
+   return 0;
+}
+

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] pktio stats counters

[Zoltan Kiss]

Hi,

On ODP-DPDK this assert fails every now and then:

https://git.linaro.org/lng/odp.git/blob/refs/heads/api-next:/test/validation/pktio/pktio.c#l1193

The reason is that when you set the interfaces up, there are other 
system services which can hook into the interface creation, and start 
doing stuff. And it often means that they send out packets behind the 
back of pktio[0], which is received by pktio[1], and that changes the 
amount of received bytes there (but not on the sending side). E.g. on my 
system there is mDNS, DHCP, NetBIOS so far which can ruin this test.
ODP-DPDK uses PCAP, which seems not to be able to catch all the outgoing 
packets. I guess on linux-generic we use the kernel stats in the unit 
tests which can do that.

Any idea how to solve this problem?

Regards,

Zoltan Kiss
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] scheduler_test_wait_time() tolerance

[Zoltan Kiss]

On 17/02/16 16:59, Mike Holmes wrote:



On 17 February 2016 at 11:53, Ivan Khoronzhuk
<ivan.khoronz...@linaro.org <mailto:ivan.khoronz...@linaro.org>> wrote:



On 17.02.16 18:44, Mike Holmes wrote:

Does this tuning need to be documented in the implementer's guide ?

I thinks, no.


Really, per platform tuning is required but we dont mention it ?


It is not really a platform dependent stuff, this test is inherently 
unreliable, but we don't have a better idea, and adjusting the tolerance 
is just good enough.






On 17 February 2016 at 10:51, Zoltan Kiss
<zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>
<mailto:zoltan.k...@linaro.org <mailto:zoltan.k...@linaro.org>>>
wrote:



 On 17/02/16 15:24, Ivan Khoronzhuk wrote:

 Hi, Zoltan

     On 17.02.16 17:12, Zoltan Kiss wrote:

 Hi Ivan,

 I haven an another issue related to time API, which
is related to your
 recent patches, particularly this code:

 wait_time = odp_schedule_wait_time(ODP_TIME_SEC_IN_NS);
 ...
 /* check time correctness */
 start_time = odp_time_local();
 for (i = 1; i < 6; i++) {
   odp_schedule(, wait_time);
   printf("%d..", i);
 }
 end_time = odp_time_local();

 diff = odp_time_diff(end_time, start_time);
 ...
 upper_limit = odp_time_local_from_ns(5 *
ODP_TIME_SEC_IN_NS +
 ODP_WAIT_TOLERANCE);

 ...
 CU_ASSERT(odp_time_cmp(diff, upper_limit) <= 0);

 This assert fails every now and then on ODP-DPDK,
because although
 wait_time is 1 sec, and you call odp_schedule() 5
times, you can't
 really have any guarantees how long the delay lasts
between the
 subsequent calls, or how long the printf lasts (or
the function call
 overhead). I think we should come up with something
more accurate
 which doesn't produce false positives like this.

 Regards,

 Zoltan


 That's why I'm used WAIT_TOLERANCE in 20ms. That is
about 2 context
 switches.
 I've tested it on Keystone and on linux-generic. It was
enough.
 If you are testing it on system with more load it's
probably not enough.
 Time spent on printf and schedule calls is not
comparable with such time
 tolerance.
 Did you try to figure out the real delay it takes with
DPDK?


 Nope, it happens quite rarely in CI, and we don't have too
much information about the reasons. I assume it is a momentary
higher load on the system.

 Maybe we should increase it to be 3 or even 4 context
switches.
 Say 40ms, in order to be not so sensitive for some systems.
 #define ODP_WAIT_TOLERANCE(40 * ODP_TIME_MSEC_IN_NS)


 Yes, that might help.



 ___
 lng-odp mailing list
lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
<mailto:lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>>
https://lists.linaro.org/mailman/listinfo/lng-odp




--
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/>***│ *Open source software
for ARM SoCs
"Work should be fun and collborative, the rest follows"

__



--
Regards,
Ivan Khoronzhuk




--
Mike Holmes
Technical Manager - Linaro Networking Group
Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM SoCs
"Work should be fun and collborative, the rest follows"

__



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH] api: time: fix typo for cmp function

[Zoltan Kiss]

Signed-off-by: Zoltan Kiss <zoltan.k...@linaro.org>

On 17/02/16 15:34, Ivan Khoronzhuk wrote:

Signed-off-by: Ivan Khoronzhuk <ivan.khoronz...@linaro.org>
---
  include/odp/api/time.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/odp/api/time.h b/include/odp/api/time.h
index efc5478..85692ec 100644
--- a/include/odp/api/time.h
+++ b/include/odp/api/time.h
@@ -119,7 +119,9 @@ odp_time_t odp_time_global_from_ns(uint64_t ns);
   * @param t2Second time
   * @param t1First time
   *
- * @retval <0 if t2 < t1, >0 if t1 = t2, 1 if t2 > t1
+ * @retval <0 when t2 < t1
+ * @retval  0 when t2 == t1
+ * @retval >0 when t2 > t1
   */
  int odp_time_cmp(odp_time_t t2, odp_time_t t1);



___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] scheduler_test_wait_time() tolerance

[Zoltan Kiss]

On 17/02/16 15:24, Ivan Khoronzhuk wrote:

Hi, Zoltan

On 17.02.16 17:12, Zoltan Kiss wrote:

Hi Ivan,

I haven an another issue related to time API, which is related to your
recent patches, particularly this code:

wait_time = odp_schedule_wait_time(ODP_TIME_SEC_IN_NS);
...
/* check time correctness */
start_time = odp_time_local();
for (i = 1; i < 6; i++) {
 odp_schedule(, wait_time);
 printf("%d..", i);
}
end_time = odp_time_local();

diff = odp_time_diff(end_time, start_time);
...
upper_limit = odp_time_local_from_ns(5 * ODP_TIME_SEC_IN_NS +
ODP_WAIT_TOLERANCE);

...
CU_ASSERT(odp_time_cmp(diff, upper_limit) <= 0);

This assert fails every now and then on ODP-DPDK, because although
wait_time is 1 sec, and you call odp_schedule() 5 times, you can't
really have any guarantees how long the delay lasts between the
subsequent calls, or how long the printf lasts (or the function call
overhead). I think we should come up with something more accurate
which doesn't produce false positives like this.

Regards,

Zoltan


That's why I'm used WAIT_TOLERANCE in 20ms. That is about 2 context
switches.
I've tested it on Keystone and on linux-generic. It was enough.
If you are testing it on system with more load it's probably not enough.
Time spent on printf and schedule calls is not comparable with such time
tolerance.
Did you try to figure out the real delay it takes with DPDK?


Nope, it happens quite rarely in CI, and we don't have too much 
information about the reasons. I assume it is a momentary higher load on 
the system.



Maybe we should increase it to be 3 or even 4 context switches.
Say 40ms, in order to be not so sensitive for some systems.
#define ODP_WAIT_TOLERANCE(40 * ODP_TIME_MSEC_IN_NS)


Yes, that might help.




___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] scheduler_test_wait_time() tolerance

[Zoltan Kiss]

Hi Ivan,

I haven an another issue related to time API, which is related to your 
recent patches, particularly this code:


wait_time = odp_schedule_wait_time(ODP_TIME_SEC_IN_NS);
...
/* check time correctness */
start_time = odp_time_local();
for (i = 1; i < 6; i++) {
odp_schedule(, wait_time);
printf("%d..", i);
}
end_time = odp_time_local();

diff = odp_time_diff(end_time, start_time);
...
upper_limit = odp_time_local_from_ns(5 * ODP_TIME_SEC_IN_NS + 	 
ODP_WAIT_TOLERANCE);


...
CU_ASSERT(odp_time_cmp(diff, upper_limit) <= 0);

This assert fails every now and then on ODP-DPDK, because although 
wait_time is 1 sec, and you call odp_schedule() 5 times, you can't 
really have any guarantees how long the delay lasts between the 
subsequent calls, or how long the printf lasts (or the function call 
overhead). I think we should come up with something more accurate which 
doesn't produce false positives like this.


Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

[lng-odp] odp_time_cmp() return value

[Zoltan Kiss]

Hi,


I think the return value of this function is not entirely clear, 
probably it's a typo:


/**
 * Compare two times
 *
 * @param t2Second time
 * @param t1First time
 *
 * @retval <0 if t2 < t1, >0 if t1 = t2, 1 if t2 > t1
 */
int odp_time_cmp(odp_time_t t2, odp_time_t t1);


I guess the second case should be "=0 if t1 = t2". Otherwise returning 1 
would mean t2 either equal or bigger than t1. The linux-generic 
implementation works that way as well.


Probably it would be clearer to write:

 * @retval t2 < t1 then <0
 * @retval t1 == t2 then =0
 * @retval t2 > t1 then >0


Regards,

Zoltan
___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp

Re: [lng-odp] [API-NEXT PATCH v4 01/13] linux-generic: pktio: add separate functions for global and local init

[Zoltan Kiss]

On 15/02/16 10:49, Matias Elo wrote:

Add separate functions for performing global and local
pktio device initialization.

Signed-off-by: Matias Elo 
---
  platform/linux-generic/include/odp_packet_io_internal.h |  3 ++-
  platform/linux-generic/odp_packet_io.c  | 17 +++--
  platform/linux-generic/pktio/ipc.c  |  3 ++-
  platform/linux-generic/pktio/loop.c |  3 ++-
  platform/linux-generic/pktio/netmap.c   |  3 ++-
  platform/linux-generic/pktio/pcap.c |  2 ++
  platform/linux-generic/pktio/socket.c   |  3 ++-
  platform/linux-generic/pktio/socket_mmap.c  |  3 ++-
  platform/linux-generic/pktio/tap.c  |  3 ++-
  9 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_io_internal.h 
b/platform/linux-generic/include/odp_packet_io_internal.h
index 03128f1..eb7efa9 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -169,7 +169,8 @@ int is_free(pktio_entry_t *entry);

  typedef struct pktio_if_ops {
const char *name;
-   int (*init)(void);
+   int (*init_global)(void);
+   int (*init_local)(void);
int (*term)(void);
int (*open)(odp_pktio_t pktio, pktio_entry_t *pktio_entry,
const char *devname, odp_pool_t pool);
diff --git a/platform/linux-generic/odp_packet_io.c 
b/platform/linux-generic/odp_packet_io.c
index a8c1d87..7d53a8e 100644
--- a/platform/linux-generic/odp_packet_io.c
+++ b/platform/linux-generic/odp_packet_io.c
@@ -81,10 +81,12 @@ int odp_pktio_init_global(void)
}

for (pktio_if = 0; pktio_if_ops[pktio_if]; ++pktio_if) {
-   if (pktio_if_ops[pktio_if]->init)
-   if (pktio_if_ops[pktio_if]->init())
+   if (pktio_if_ops[pktio_if]->init_global)
+   if (pktio_if_ops[pktio_if]->init_global()) {
ODP_ERR("failed to initialized pktio type %d",
pktio_if);
+   return -1;


This fixes a separate issue, so at least please mention it in the commit 
message. Also, are we sure we want to do this?
I mean, even if a particular pktio fails to init we might want to keep 
up running. Maxim?




+   }
}

return 0;
@@ -92,6 +94,17 @@ int odp_pktio_init_global(void)

  int odp_pktio_init_local(void)
  {
+   int pktio_if;
+
+   for (pktio_if = 0; pktio_if_ops[pktio_if]; ++pktio_if) {
+   if (pktio_if_ops[pktio_if]->init_local)
+   if (pktio_if_ops[pktio_if]->init_local()) {
+   ODP_ERR("failed to initialized pktio type %d",
+   pktio_if);
+   return -1;
+   }
+   }
+
return 0;
  }

diff --git a/platform/linux-generic/pktio/ipc.c 
b/platform/linux-generic/pktio/ipc.c
index 52d3c8c..08a7934 100644
--- a/platform/linux-generic/pktio/ipc.c
+++ b/platform/linux-generic/pktio/ipc.c
@@ -713,7 +713,8 @@ static int ipc_close(pktio_entry_t *pktio_entry)
  }

  const pktio_if_ops_t ipc_pktio_ops = {
-   .init = NULL,
+   .init_global = NULL,
+   .init_local = NULL,
.term = NULL,
.open = ipc_pktio_open,
.close = ipc_close,
diff --git a/platform/linux-generic/pktio/loop.c 
b/platform/linux-generic/pktio/loop.c
index 34d769e..d5fce90 100644
--- a/platform/linux-generic/pktio/loop.c
+++ b/platform/linux-generic/pktio/loop.c
@@ -155,7 +155,8 @@ static int loopback_stats_reset(pktio_entry_t *pktio_entry 
ODP_UNUSED)

  const pktio_if_ops_t loopback_pktio_ops = {
.name = "loop",
-   .init = NULL,
+   .init_global = NULL,
+   .init_local = NULL,
.term = NULL,
.open = loopback_open,
.close = loopback_close,
diff --git a/platform/linux-generic/pktio/netmap.c 
b/platform/linux-generic/pktio/netmap.c
index 7009b97..27a5582 100644
--- a/platform/linux-generic/pktio/netmap.c
+++ b/platform/linux-generic/pktio/netmap.c
@@ -856,7 +856,8 @@ static int netmap_stats_reset(pktio_entry_t *pktio_entry)

  const pktio_if_ops_t netmap_pktio_ops = {
.name = "netmap",
-   .init = NULL,
+   .init_global = NULL,
+   .init_local = NULL,
.term = NULL,
.open = netmap_open,
.close = netmap_close,
diff --git a/platform/linux-generic/pktio/pcap.c 
b/platform/linux-generic/pktio/pcap.c
index ef42c11..56e603c 100644
--- a/platform/linux-generic/pktio/pcap.c
+++ b/platform/linux-generic/pktio/pcap.c
@@ -393,6 +393,8 @@ static int pcapif_stats(pktio_entry_t *pktio_entry,

  const pktio_if_ops_t pcap_pktio_ops = {
.name = "pcap",
+   .init_global = NULL,
+   .init_local = NULL,
.open = 

Re: [lng-odp] [API-NEXT PATCH v3 00/11] DPDK pktio implementation

[Zoltan Kiss]

I can confirm that these two changes were made

On 09/02/16 13:47, Matias Elo wrote:

V2:
- Check the number of mbuf segments in mbuf_to_pkt() (Zoltan Kiss)
- Copy DPDK RSS hash to ODP packet header in mbuf_to_pkt() (Zoltan Kiss)

___
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp