[lng-odp] [PATCH v1 1/1] linux-gen: add runtime configuration file
From: Matias Elo
Enables changing ODP runtime configuration options by using an optional
configuration file (libconfig). Path to the conf file is passed using
environment variable ODP_CONF_FILE. If ODP_CONF_FILE or a particular option
is not set, hardcoded default values are used intead. An example
configuration file is provided in config/odp-linux.conf.
Runtime configuration is initially used by DPDK pktio to set NIC options.
Adds new dependency to libconfig library.
Signed-off-by: Matias Elo
---
/** Email created from pull request 499 (matiaselo:dev/dpdk_dev_config)
** https://github.com/Linaro/odp/pull/499
** Patch: https://github.com/Linaro/odp/pull/499.patch
** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb
** Merge commit sha: ab13bcecea3972b5af189e9e5d6d4873790fb554
**/
.travis.yml| 3 +-
DEPENDENCIES | 2 +-
config/README | 8 +++
config/odp-linux.conf | 20 +++
m4/odp_libconfig.m4| 60 +
platform/linux-generic/Makefile.am | 4 ++
platform/linux-generic/include/odp_internal.h | 4 ++
.../linux-generic/include/odp_libconfig_internal.h | 29 ++
platform/linux-generic/include/odp_packet_dpdk.h | 8 +++
platform/linux-generic/libodp-linux.pc.in | 4 +-
platform/linux-generic/m4/configure.m4 | 1 +
platform/linux-generic/odp_init.c | 14 +
platform/linux-generic/odp_libconfig.c | 57
platform/linux-generic/pktio/dpdk.c| 63 --
platform/linux-generic/test/ring/Makefile.am | 1 +
15 files changed, 271 insertions(+), 7 deletions(-)
create mode 100644 config/odp-linux.conf
create mode 100644 m4/odp_libconfig.m4
create mode 100644 platform/linux-generic/include/odp_libconfig_internal.h
create mode 100644 platform/linux-generic/odp_libconfig.c
diff --git a/.travis.yml b/.travis.yml
index d9423adcc..4095516c9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -21,6 +21,7 @@ addons:
- gcc
- clang-3.8
- automake autoconf libtool libssl-dev graphviz mscgen
+- libconfig-dev
- codespell
- libpcap-dev
- libnuma-dev
@@ -254,7 +255,7 @@ script:
- make -j $(nproc)
- mkdir /dev/shm/odp
- if [ -z "$CROSS_ARCH" ] ; then
- sudo
LD_LIBRARY_PATH="$HOME/cunit-install/$CROSS_ARCH/lib:$LD_LIBRARY_PATH"
ODP_SHM_DIR=/dev/shm/odp make check ;
+ sudo ODP_CONFIG_FILE="`pwd`/config/odp-linux.conf"
LD_LIBRARY_PATH="$HOME/cunit-install/$CROSS_ARCH/lib:$LD_LIBRARY_PATH"
ODP_SHM_DIR=/dev/shm/odp make check ;
fi
- make install
diff --git a/DEPENDENCIES b/DEPENDENCIES
index 6f374ca92..b6765fecd 100644
--- a/DEPENDENCIES
+++ b/DEPENDENCIES
@@ -19,7 +19,7 @@ Prerequisites for building the OpenDataPlane (ODP) API
3. Required libraries
- Libraries currently required to link: openssl, libatomic
+ Libraries currently required to link: openssl, libatomic, libconfig
3.1 OpenSSL native compile
diff --git a/config/README b/config/README
index 3f4336103..415334255 100644
--- a/config/README
+++ b/config/README
@@ -1,2 +1,10 @@
ODP configuration options
-
+
+Runtime configuration options can be passed to an ODP instance by
+setting ODP_CONFIG_FILE environment variable to point to a libconfig
+format configuration file. An example configuration file with default
+values is provided in config/odp-linux.conf. If ODP_CONFIG_FILE is not
+found, hardcoded default values are used instead of any configuration
+file. A configuration file doesn't have to include all available
+options. The missing options are replaced with hardcoded default values.
diff --git a/config/odp-linux.conf b/config/odp-linux.conf
new file mode 100644
index 0..eb40f5212
--- /dev/null
+++ b/config/odp-linux.conf
@@ -0,0 +1,20 @@
+# ODP runtime configuration options
+#
+# A configuration file doesn't have to include all available
+# options. The missing options are replaced with hardcoded default
+# values.
+#
+# See libconfig syntax:
https://hyperrealm.github.io/libconfig/libconfig_manual.html#Configuration-Files
+
+# DPDK pktio options
+pktio_dpdk: {
+ # Default options
+ num_rx_desc = 128
+ num_tx_desc = 512
+ rx_drop_en = 0
+
+ # Driver specific options (use PMD names from DPDK)
+ net_ixgbe: {
+ rx_drop_en = 1
+ }
+}
diff --git a/m4/odp_libconfig.m4 b/m4/odp_libconfig.m4
new file mode 100644
index 0..e5b1d8acd
--- /dev/null
+++ b/m4/odp_libconfig.m4
@@ -0,0 +1,60 @@
+# ODP_LIBCONFIG([ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND])
+#
[lng-odp] [PATCH v1 0/1] linux-gen: add runtime configuration file
Enables changing ODP runtime configuration options by using an optional
configuration file (libconfig). Path to the conf file is passed using
environment variable ODP_CONF_FILE. If ODP_CONF_FILE or a particular option
is not set, hardcoded default values are used intead. An example
configuration file is provided in config/odp-linux.conf.
Runtime configuration is initially used by DPDK pktio to set NIC options.
Adds new dependency to libconfig library.
Signed-off-by: Matias Elo matias@nokia.com
github
/** Email created from pull request 499 (matiaselo:dev/dpdk_dev_config)
** https://github.com/Linaro/odp/pull/499
** Patch: https://github.com/Linaro/odp/pull/499.patch
** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb
** Merge commit sha: ab13bcecea3972b5af189e9e5d6d4873790fb554
**/
/github
checkpatch.pl
WARNING: 'intead' may be misspelled - perhaps 'instead'?
#10:
is not set, hardcoded default values are used intead. An example
WARNING: externs should be avoided in .c files
#394: FILE: platform/linux-generic/odp_libconfig.c:16:
+extern struct odp_global_data_s odp_global_data;
CHECK: Avoid CamelCase:
#503: FILE: platform/linux-generic/pktio/dpdk.c:141:
+ printf("DPDK interface (%s): %" PRIu16 "\n", dev_info->driver_name,
total: 0 errors, 2 warnings, 1 checks, 420 lines checked
to_send-p-000.patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
/checkpatch.pl
[lng-odp] [PATCH v7 2/2] validation: pool: verify pool data range
From: Michal Mazur
Allocate maximum number of packets from pool and verify that packet
data are located inside range returned by odp_pool_info.
Signed-off-by: Michal Mazur
---
/** Email created from pull request 495 (semihalf-mazur-michal:master)
** https://github.com/Linaro/odp/pull/495
** Patch: https://github.com/Linaro/odp/pull/495.patch
** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb
** Merge commit sha: 79aeba092a0c85e26786ff8efbaeb71608ae1fa3
**/
test/validation/api/pool/pool.c | 54 +
1 file changed, 54 insertions(+)
diff --git a/test/validation/api/pool/pool.c b/test/validation/api/pool/pool.c
index 34f973573..394c79497 100644
--- a/test/validation/api/pool/pool.c
+++ b/test/validation/api/pool/pool.c
@@ -217,6 +217,59 @@ static void pool_test_info_packet(void)
CU_ASSERT(odp_pool_destroy(pool) == 0);
}
+static void pool_test_info_data_range(void)
+{
+ odp_pool_t pool;
+ odp_pool_info_t info;
+ odp_pool_param_t param;
+ odp_packet_t pkt[PKT_NUM];
+ uint32_t i, num;
+ uintptr_t pool_len;
+
+ odp_pool_param_init(¶m);
+
+ param.type = ODP_POOL_PACKET;
+ param.pkt.num = PKT_NUM;
+ param.pkt.len = PKT_LEN;
+
+ pool = odp_pool_create(NULL, ¶m);
+ CU_ASSERT_FATAL(pool != ODP_POOL_INVALID);
+
+ CU_ASSERT_FATAL(odp_pool_info(pool, &info) == 0);
+
+ pool_len = info.max_data_addr - info.min_data_addr + 1;
+ CU_ASSERT(pool_len >= PKT_NUM * PKT_LEN);
+
+ num = 0;
+
+ for (i = 0; i < PKT_NUM; i++) {
+ pkt[num] = odp_packet_alloc(pool, PKT_LEN);
+ CU_ASSERT(pkt[num] != ODP_PACKET_INVALID);
+
+ if (pkt[num] != ODP_PACKET_INVALID)
+ num++;
+ }
+
+ for (i = 0; i < num; i++) {
+ uintptr_t pkt_data, pkt_data_end;
+ uint32_t offset = 0, seg_len;
+ uint32_t pkt_len = odp_packet_len(pkt[i]);
+
+ while (offset < pkt_len) {
+ pkt_data = (uintptr_t)odp_packet_offset(pkt[i], offset,
+ &seg_len, NULL);
+ pkt_data_end = pkt_data + seg_len - 1;
+ CU_ASSERT((pkt_data >= info.min_data_addr) &&
+ (pkt_data_end <= info.max_data_addr));
+ offset += seg_len;
+ }
+
+ odp_packet_free(pkt[i]);
+ }
+
+ CU_ASSERT(odp_pool_destroy(pool) == 0);
+}
+
odp_testinfo_t pool_suite[] = {
ODP_TEST_INFO(pool_test_create_destroy_buffer),
ODP_TEST_INFO(pool_test_create_destroy_packet),
@@ -225,6 +278,7 @@ odp_testinfo_t pool_suite[] = {
ODP_TEST_INFO(pool_test_alloc_packet_subparam),
ODP_TEST_INFO(pool_test_info_packet),
ODP_TEST_INFO(pool_test_lookup_info_print),
+ ODP_TEST_INFO(pool_test_info_data_range),
ODP_TEST_INFO_NULL,
};
[lng-odp] [PATCH v7 1/2] linux-generic: pool: Return address range in pool info
From: Michal Mazur Implement support in odp_pool_info function to provide address range of pool data available to application. Pull request of related API change: https://github.com/Linaro/odp/pull/200 Signed-off-by: Michal Mazur --- /** Email created from pull request 495 (semihalf-mazur-michal:master) ** https://github.com/Linaro/odp/pull/495 ** Patch: https://github.com/Linaro/odp/pull/495.patch ** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb ** Merge commit sha: 79aeba092a0c85e26786ff8efbaeb71608ae1fa3 **/ platform/linux-generic/odp_pool.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c index e5ba8982a..03578135c 100644 --- a/platform/linux-generic/odp_pool.c +++ b/platform/linux-generic/odp_pool.c @@ -693,6 +693,9 @@ int odp_pool_info(odp_pool_t pool_hdl, odp_pool_info_t *info) if (pool->params.type == ODP_POOL_PACKET) info->pkt.max_num = pool->num; + info->min_data_addr = (uintptr_t)pool->base_addr; + info->max_data_addr = (uintptr_t)pool->base_addr + pool->shm_size - 1; + return 0; }
[lng-odp] [PATCH v7 0/2] linux-generic: pool: Return address range in pool info
Implement support in odp_pool_info function to provide address range of pool data available to application. Similar change was already merged to caterpillar/linux-dpdk: #400 Pull request of related API change: #200 github /** Email created from pull request 495 (semihalf-mazur-michal:master) ** https://github.com/Linaro/odp/pull/495 ** Patch: https://github.com/Linaro/odp/pull/495.patch ** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb ** Merge commit sha: 79aeba092a0c85e26786ff8efbaeb71608ae1fa3 **/ /github checkpatch.pl total: 0 errors, 0 warnings, 0 checks, 9 lines checked to_send-p-000.patch has no obvious style problems and is ready for submission. total: 0 errors, 0 warnings, 0 checks, 66 lines checked to_send-p-001.patch has no obvious style problems and is ready for submission. /checkpatch.pl
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_ring_st_internal.h
line 78
@@ -0,0 +1,118 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef ODP_RING_ST_INTERNAL_H_
+#define ODP_RING_ST_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include
+#include
+
+/* Basic ring for single thread usage. Operations must be synchronized by using
+ * locks (or other means), when multiple threads use the same ring. */
+typedef struct {
+ uint32_t head;
+ uint32_t tail;
+ uint32_t mask;
+ uint32_t *data;
+
+} ring_st_t;
+
+/* Initialize ring. Ring size must be a power of two. */
+static inline void ring_st_init(ring_st_t *ring, uint32_t *data, uint32_t size)
+{
+ ring->head = 0;
+ ring->tail = 0;
+ ring->mask = size - 1;
+ ring->data = data;
+}
+
+/* Dequeue data from the ring head. Max_num is smaller than ring size.*/
+static inline uint32_t ring_st_deq_multi(ring_st_t *ring, uint32_t data[],
+uint32_t max_num)
+{
+ uint32_t head, tail, mask, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ num = tail - head;
+
+ /* Empty */
+ if (num == 0)
+ return 0;
+
+ if (num > max_num)
+ num = max_num;
+
+ idx = head & mask;
+
+ for (i = 0; i < num; i++) {
+ data[i] = ring->data[idx];
+ idx = (idx + 1) & mask;
+ }
+
+ ring->head = head + num;
+
+ return num;
+}
+
+/* Enqueue data into the ring tail. Num_data is smaller than ring size. */
+static inline uint32_t ring_st_enq_multi(ring_st_t *ring, const uint32_t
data[],
+uint32_t num_data)
+{
+ uint32_t head, tail, mask, size, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ size = mask + 1;
+ num = size - (tail - head);
Comment:
Please see above discussion. The comment stands.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Not true. See the comment below where it's clear that `head` and `tail` are
> free running and thus will eventually wrap as described above. If these were
> `uint64_t` variables you could argue that the wraps would take centuries and
> hence are nothing to worry about, but being `uint32_t` variables this should
> be expected to happen regularly on heavily used queues.
>
> `abs()` adds no overhead since compilers treat it as an intrinsic.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Note that neither `head` nor `tail` get masked when doing enqueues or
>> dequeues, so they will traverse the entire 32-bit range of the variable over
>> time. While the ring itself will never hold more than `size` entries, `head`
>> and `tail` will wrap around. When you index into the ring you do so via a
>> mask (`idx = head & mask;`), but calculating the number of elements in the
>> ring doesn't do this, which is why `abs()` is needed.
>>> Petri Savolainen(psavol) wrote:
>>> Plus this is done only once - in pool create phase.
Petri Savolainen(psavol) wrote:
No since ring size will be much smaller than 4 billion.
> Petri Savolainen(psavol) wrote:
> The point is that ring size will never be close to 4 billion entries.
> E.g. currently tail is always max 4096 larger than head. Your example
> above is based assumption of 4 billion entry ring. Overflow is avoided
> when ring size if <2 billion, as 32 bit indexes can be still used to
> calculate number of items correctly.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Agreed.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Agreed, this is good for now. Later we may wish to honor the
>>> user-requested queue `size` parameter.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Agreed.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Correct, as noted earlier. I withdraw that comment.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Presumably the compiler will see the overlap and optimize away the
>> redundancy, so I assume the performance impact will be nil here.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Since you're allowing `head` and `tail` to run over the entire
>>> 32-bit range, you're correct that you can completely fill the ring.
>>> I did miss that point. However, as shown above you still need this
>>> to be:
>>>
>>> ```
>>> num = size - abs(tail - head);
>>> ```
>>> To avoid problems at the 32-bit wrap boundary.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
You're computing in 32 bits not 8 bits, and your ring
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_ring_st_internal.h
line 46
@@ -0,0 +1,118 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef ODP_RING_ST_INTERNAL_H_
+#define ODP_RING_ST_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include
+#include
+
+/* Basic ring for single thread usage. Operations must be synchronized by using
+ * locks (or other means), when multiple threads use the same ring. */
+typedef struct {
+ uint32_t head;
+ uint32_t tail;
+ uint32_t mask;
+ uint32_t *data;
+
+} ring_st_t;
+
+/* Initialize ring. Ring size must be a power of two. */
+static inline void ring_st_init(ring_st_t *ring, uint32_t *data, uint32_t size)
+{
+ ring->head = 0;
+ ring->tail = 0;
+ ring->mask = size - 1;
+ ring->data = data;
+}
+
+/* Dequeue data from the ring head. Max_num is smaller than ring size.*/
+static inline uint32_t ring_st_deq_multi(ring_st_t *ring, uint32_t data[],
+uint32_t max_num)
+{
+ uint32_t head, tail, mask, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ num = tail - head;
Comment:
Not true. See the comment below where it's clear that `head` and `tail` are
free running and thus will eventually wrap as described above. If these were
`uint64_t` variables you could argue that the wraps would take centuries and
hence are nothing to worry about, but being `uint32_t` variables this should be
expected to happen regularly on heavily used queues.
`abs()` adds no overhead since compilers treat it as an intrinsic.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Note that neither `head` nor `tail` get masked when doing enqueues or
> dequeues, so they will traverse the entire 32-bit range of the variable over
> time. While the ring itself will never hold more than `size` entries, `head`
> and `tail` will wrap around. When you index into the ring you do so via a
> mask (`idx = head & mask;`), but calculating the number of elements in the
> ring doesn't do this, which is why `abs()` is needed.
>> Petri Savolainen(psavol) wrote:
>> Plus this is done only once - in pool create phase.
>>> Petri Savolainen(psavol) wrote:
>>> No since ring size will be much smaller than 4 billion.
Petri Savolainen(psavol) wrote:
The point is that ring size will never be close to 4 billion entries. E.g.
currently tail is always max 4096 larger than head. Your example above is
based assumption of 4 billion entry ring. Overflow is avoided when ring
size if <2 billion, as 32 bit indexes can be still used to calculate
number of items correctly.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Agreed.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Agreed, this is good for now. Later we may wish to honor the
>> user-requested queue `size` parameter.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Agreed.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Correct, as noted earlier. I withdraw that comment.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Presumably the compiler will see the overlap and optimize away the
> redundancy, so I assume the performance impact will be nil here.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Since you're allowing `head` and `tail` to run over the entire
>> 32-bit range, you're correct that you can completely fill the ring.
>> I did miss that point. However, as shown above you still need this
>> to be:
>>
>> ```
>> num = size - abs(tail - head);
>> ```
>> To avoid problems at the 32-bit wrap boundary.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> You're computing in 32 bits not 8 bits, and your ring size is less
>>> than 2^32 elements. Consider the following test program:
>>> ```
>>> #include
>>> #include
>>> #include
>>> #include
>>>
>>> void main() {
>>> uint32_t head[4] = {0, 1, 2, 3};
>>> uint32_t tail[4] = {0, 0x, 0xfffe, 0xfffd};
>>> uint32_t mask = 4095;
>>> uint32_t result;
>>> int i;
>>>
>>> for (i = 0; i < 4; i++) {
>>> printf("head = %" PRIu32 " tail = %" PRIu32 ":\n",
>>>head[i], tail[i]);
>>>
>>> result = tail[i] - head[i];
>>> printf("tail - head = %" PRIu32 "\n", result);
>>>
>>> result = (tail[i] - head[i]) & mask;
>>> printf("(tail - head) & mask = %" PRIu32 "\n", result);
>>>
>>>
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_ring_st_internal.h
line 62
@@ -0,0 +1,109 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef ODP_RING_ST_INTERNAL_H_
+#define ODP_RING_ST_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include
+#include
+
+/* Basic ring for single thread usage. Operations must be synchronized by using
+ * locks (or other means), when multiple threads use the same ring. */
+typedef struct {
+ uint32_t head;
+ uint32_t tail;
+ uint32_t mask;
+ uint32_t *data;
+
+} ring_st_t;
+
+/* Initialize ring. Ring size must be a power of two. */
+static inline void ring_st_init(ring_st_t *ring, uint32_t *data, uint32_t size)
+{
+ ring->head = 0;
+ ring->tail = 0;
+ ring->mask = size - 1;
+ ring->data = data;
+}
+
+/* Dequeue data from the ring head. Max_num is smaller than ring size.*/
+static inline uint32_t ring_st_deq_multi(ring_st_t *ring, uint32_t data[],
+uint32_t max_num)
+{
+ uint32_t head, tail, mask, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ num = tail - head;
+
+ /* Empty */
+ if (num == 0)
+ return 0;
+
+ if (num > max_num)
+ num = max_num;
+
+ idx = head & mask;
+
+ for (i = 0; i < num; i++) {
+ data[i] = ring->data[idx];
+ idx = (idx + 1) & mask;
+ }
+
+ ring->head = head + num;
Comment:
Note that neither `head` nor `tail` get masked when doing enqueues or dequeues,
so they will traverse the entire 32-bit range of the variable over time. While
the ring itself will never hold more than `size` entries, `head` and `tail`
will wrap around. When you index into the ring you do so via a mask (`idx =
head & mask;`), but calculating the number of elements in the ring doesn't do
this, which is why `abs()` is needed.
> Petri Savolainen(psavol) wrote:
> Plus this is done only once - in pool create phase.
>> Petri Savolainen(psavol) wrote:
>> No since ring size will be much smaller than 4 billion.
>>> Petri Savolainen(psavol) wrote:
>>> The point is that ring size will never be close to 4 billion entries. E.g.
>>> currently tail is always max 4096 larger than head. Your example above is
>>> based assumption of 4 billion entry ring. Overflow is avoided when ring
>>> size if <2 billion, as 32 bit indexes can be still used to calculate number
>>> of items correctly.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Agreed.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Agreed, this is good for now. Later we may wish to honor the
> user-requested queue `size` parameter.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Agreed.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Correct, as noted earlier. I withdraw that comment.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Presumably the compiler will see the overlap and optimize away the
redundancy, so I assume the performance impact will be nil here.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Since you're allowing `head` and `tail` to run over the entire 32-bit
> range, you're correct that you can completely fill the ring. I did
> miss that point. However, as shown above you still need this to be:
>
> ```
> num = size - abs(tail - head);
> ```
> To avoid problems at the 32-bit wrap boundary.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> You're computing in 32 bits not 8 bits, and your ring size is less
>> than 2^32 elements. Consider the following test program:
>> ```
>> #include
>> #include
>> #include
>> #include
>>
>> void main() {
>> uint32_t head[4] = {0, 1, 2, 3};
>> uint32_t tail[4] = {0, 0x, 0xfffe, 0xfffd};
>> uint32_t mask = 4095;
>> uint32_t result;
>> int i;
>>
>> for (i = 0; i < 4; i++) {
>> printf("head = %" PRIu32 " tail = %" PRIu32 ":\n",
>> head[i], tail[i]);
>>
>> result = tail[i] - head[i];
>> printf("tail - head = %" PRIu32 "\n", result);
>>
>> result = (tail[i] - head[i]) & mask;
>> printf("(tail - head) & mask = %" PRIu32 "\n", result);
>>
>> result = abs(tail[i] - head[i]);
>> printf("abs(tail - head) = %" PRIu32 "\n\n", result);
>> }
>> }
>> ```
>> in theory `tail - head` should be the number of elem
Re: [lng-odp] [PATCH API-NEXT v1] api: packet: data and segment length
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page: include/odp/api/spec/packet.h line 39 @@ -401,30 +401,39 @@ uint32_t odp_packet_buf_len(odp_packet_t pkt); /** * Packet data pointer * - * Returns the current packet data pointer. When a packet is received - * from packet input, this points to the first byte of the received - * packet. Packet level offsets are calculated relative to this position. + * Returns pointer to the first byte of packet data. When packet is segmented, + * only a portion of packet data follows the pointer. When unsure, use e.g. + * odp_packet_seg_len() to check the data length following the pointer. Packet + * level offsets are calculated relative to this position. * - * User can adjust the data pointer with head_push/head_pull (does not modify - * segmentation) and add_data/rem_data calls (may modify segmentation). + * When a packet is received from packet input, this points to the first byte + * of the received packet. Pool configuration parameters may be used to ensure + * that the first packet segment contains all/most of the data relevant to the + * application. + * + * User can adjust the data pointer with e.g. push_head/pull_head (does not + * modify segmentation) and extend_head/trunc_head (may modify segmentation) + * calls. * * @param pkt Packet handle * * @return Pointer to the packet data * - * @see odp_packet_l2_ptr(), odp_packet_seg_len() + * @see odp_packet_seg_len(), odp_packet_push_head(), odp_packet_extend_head() */ void *odp_packet_data(odp_packet_t pkt); /** - * Packet segment data length + * Packet data length following the data pointer * - * Returns number of data bytes following the current data pointer - * (odp_packet_data()) location in the segment. + * Returns number of data bytes (in the segment) following the current data + * pointer position. When unsure, use this function to check how many bytes Comment: Segments are inherently implementation-dependent, which is why portable applications should avoid processing packets in terms of segments. Applications only need be aware that segments may exist, meaning that packets may not be contiguously addressable. This is why a `seg_len` is returned on the various routines that provide addressability to packets. `odp_packet_data()` is the exception, and this was done as an efficiency measure for applications looking at headers contained at the start of the packet that are known to be contiguous because of the `min_seg_len` pool specification. > Petri Savolainen(psavol) wrote: > Yes. I didn't want to introduce a new limitation to segment/reference > implementation here. This patch just tries to make it clear that packets may > be segmented. > > E.g. Bill's reference implementation resulted packets that had first segment > length 0 and data pointer pointed to the second segment. It passed all > validation tests. From application point of view, it does not matter much if > spec allows empty segments to be linked into packet, although it's not very > intuitive and should be avoided when possible. >> Balasubramanian Manoharan(bala-manoharan) wrote: >> In the entire documentation you have avoided the term "first segment" is it >> by choice? >> IMO we could refer this as first segment valid data bytes >>> Petri Savolainen(psavol) wrote: >>> seg_len / push_head / extend_head are mentioned above. Packet_offset is not >>> specialized for handling first N bytes of packet, so it's not directly >>> related to these ones. Petri Savolainen(psavol) wrote: packet_offset is for different purpose (access data on arbitrary offset), these calls are optimized for the common case (offset zero). Also odp_packet_seg_data_len(), the new data_seg_len(), odp_packet_l2_ptr(), odp_packet_l3_ptr() and odp_packet_l4_ptr() output seg len, but we don't need to list all possible ways to get it. It's enough that the reader understands that a packet may have segments and segment length is different thing than total packet length > Dmitry Eremin-Solenikov(lumag) wrote: > And here too, please. >> Dmitry Eremin-Solenikov(lumag) wrote: >> odp_packet_seg_len() **or odp_packet_offset()**, if you don't mind. https://github.com/Linaro/odp/pull/497#discussion_r170275071 updated_at 2018-02-23 15:06:52
[lng-odp] [PATCH v6 1/2] linux-generic: pool: Return address range in pool info
From: Michal Mazur Implement support in odp_pool_info function to provide address range of pool data available to application. Pull request of related API change: https://github.com/Linaro/odp/pull/200 Signed-off-by: Michal Mazur --- /** Email created from pull request 495 (semihalf-mazur-michal:master) ** https://github.com/Linaro/odp/pull/495 ** Patch: https://github.com/Linaro/odp/pull/495.patch ** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb ** Merge commit sha: 0390589df2dbf51cfd601bc4baf1b06b571653bb **/ platform/linux-generic/odp_pool.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c index e5ba8982a..03578135c 100644 --- a/platform/linux-generic/odp_pool.c +++ b/platform/linux-generic/odp_pool.c @@ -693,6 +693,9 @@ int odp_pool_info(odp_pool_t pool_hdl, odp_pool_info_t *info) if (pool->params.type == ODP_POOL_PACKET) info->pkt.max_num = pool->num; + info->min_data_addr = (uintptr_t)pool->base_addr; + info->max_data_addr = (uintptr_t)pool->base_addr + pool->shm_size - 1; + return 0; }
[lng-odp] [PATCH v6 2/2] validation: pool: verify pool data range
From: Michal Mazur
Allocate maximum number of packets from pool and verify that packet
data are located inside range returned by odp_pool_info.
Signed-off-by: Michal Mazur
---
/** Email created from pull request 495 (semihalf-mazur-michal:master)
** https://github.com/Linaro/odp/pull/495
** Patch: https://github.com/Linaro/odp/pull/495.patch
** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb
** Merge commit sha: 0390589df2dbf51cfd601bc4baf1b06b571653bb
**/
test/validation/api/pool/pool.c | 46 +
1 file changed, 46 insertions(+)
diff --git a/test/validation/api/pool/pool.c b/test/validation/api/pool/pool.c
index 34f973573..f6ebb6886 100644
--- a/test/validation/api/pool/pool.c
+++ b/test/validation/api/pool/pool.c
@@ -217,6 +217,51 @@ static void pool_test_info_packet(void)
CU_ASSERT(odp_pool_destroy(pool) == 0);
}
+static void pool_test_info_data_range(void)
+{
+ odp_pool_t pool;
+ odp_pool_info_t info;
+ odp_pool_param_t param;
+ odp_packet_t pkt[PKT_NUM];
+ uint32_t i, num, seg_len;
+ uintptr_t pkt_data, pool_len;
+
+ odp_pool_param_init(¶m);
+
+ param.type = ODP_POOL_PACKET;
+ param.pkt.num = PKT_NUM;
+ param.pkt.len = PKT_LEN;
+
+ pool = odp_pool_create(NULL, ¶m);
+ CU_ASSERT_FATAL(pool != ODP_POOL_INVALID);
+
+ CU_ASSERT_FATAL(odp_pool_info(pool, &info) == 0);
+
+ pool_len = info.max_data_addr - info.min_data_addr + 1;
+ CU_ASSERT(pool_len >= PKT_NUM * PKT_LEN);
+
+ num = 0;
+
+ for (i = 0; i < PKT_NUM; i++) {
+ pkt[num] = odp_packet_alloc(pool, PKT_LEN);
+ CU_ASSERT(pkt[num] != ODP_PACKET_INVALID);
+
+ if (pkt[num] != ODP_PACKET_INVALID)
+ num++;
+ }
+
+ for (i = 0; i < num; i++) {
+ pkt_data = (uintptr_t)odp_packet_data(pkt[i]);
+ seg_len = odp_packet_seg_len(pkt[i]);
+ CU_ASSERT((pkt_data >= info.min_data_addr) &&
+ (pkt_data + seg_len - 1 <= info.max_data_addr));
+
+ odp_packet_free(pkt[i]);
+ }
+
+ CU_ASSERT(odp_pool_destroy(pool) == 0);
+}
+
odp_testinfo_t pool_suite[] = {
ODP_TEST_INFO(pool_test_create_destroy_buffer),
ODP_TEST_INFO(pool_test_create_destroy_packet),
@@ -225,6 +270,7 @@ odp_testinfo_t pool_suite[] = {
ODP_TEST_INFO(pool_test_alloc_packet_subparam),
ODP_TEST_INFO(pool_test_info_packet),
ODP_TEST_INFO(pool_test_lookup_info_print),
+ ODP_TEST_INFO(pool_test_info_data_range),
ODP_TEST_INFO_NULL,
};
[lng-odp] [PATCH v6 0/2] linux-generic: pool: Return address range in pool info
Implement support in odp_pool_info function to provide address range of pool data available to application. Similar change was already merged to caterpillar/linux-dpdk: #400 Pull request of related API change: #200 github /** Email created from pull request 495 (semihalf-mazur-michal:master) ** https://github.com/Linaro/odp/pull/495 ** Patch: https://github.com/Linaro/odp/pull/495.patch ** Base sha: 5a58bbf2bb331fd7dde2ebbc0430634ace6900fb ** Merge commit sha: 0390589df2dbf51cfd601bc4baf1b06b571653bb **/ /github checkpatch.pl total: 0 errors, 0 warnings, 0 checks, 9 lines checked to_send-p-000.patch has no obvious style problems and is ready for submission. total: 0 errors, 0 warnings, 0 checks, 58 lines checked to_send-p-001.patch has no obvious style problems and is ready for submission. /checkpatch.pl
Re: [lng-odp] [PATCH API-NEXT v1] api: packet: data and segment length
Petri Savolainen(psavol) replied on github web page: include/odp/api/spec/packet.h line 39 @@ -401,30 +401,39 @@ uint32_t odp_packet_buf_len(odp_packet_t pkt); /** * Packet data pointer * - * Returns the current packet data pointer. When a packet is received - * from packet input, this points to the first byte of the received - * packet. Packet level offsets are calculated relative to this position. + * Returns pointer to the first byte of packet data. When packet is segmented, + * only a portion of packet data follows the pointer. When unsure, use e.g. + * odp_packet_seg_len() to check the data length following the pointer. Packet + * level offsets are calculated relative to this position. * - * User can adjust the data pointer with head_push/head_pull (does not modify - * segmentation) and add_data/rem_data calls (may modify segmentation). + * When a packet is received from packet input, this points to the first byte + * of the received packet. Pool configuration parameters may be used to ensure + * that the first packet segment contains all/most of the data relevant to the + * application. + * + * User can adjust the data pointer with e.g. push_head/pull_head (does not + * modify segmentation) and extend_head/trunc_head (may modify segmentation) + * calls. * * @param pkt Packet handle * * @return Pointer to the packet data * - * @see odp_packet_l2_ptr(), odp_packet_seg_len() + * @see odp_packet_seg_len(), odp_packet_push_head(), odp_packet_extend_head() */ void *odp_packet_data(odp_packet_t pkt); /** - * Packet segment data length + * Packet data length following the data pointer * - * Returns number of data bytes following the current data pointer - * (odp_packet_data()) location in the segment. + * Returns number of data bytes (in the segment) following the current data + * pointer position. When unsure, use this function to check how many bytes Comment: Yes. I didn't want to introduce a new limitation to segment/reference implementation here. This patch just tries to make it clear that packets may be segmented. E.g. Bill's reference implementation resulted packets that had first segment length 0 and data pointer pointed to the second segment. It passed all validation tests. From application point of view, it does not matter much if spec allows empty segments to be linked into packet, although it's not very intuitive and should be avoided when possible. > Balasubramanian Manoharan(bala-manoharan) wrote: > In the entire documentation you have avoided the term "first segment" is it > by choice? > IMO we could refer this as first segment valid data bytes >> Petri Savolainen(psavol) wrote: >> seg_len / push_head / extend_head are mentioned above. Packet_offset is not >> specialized for handling first N bytes of packet, so it's not directly >> related to these ones. >>> Petri Savolainen(psavol) wrote: >>> packet_offset is for different purpose (access data on arbitrary offset), >>> these calls are optimized for the common case (offset zero). Also >>> odp_packet_seg_data_len(), the new data_seg_len(), odp_packet_l2_ptr(), >>> odp_packet_l3_ptr() and odp_packet_l4_ptr() output seg len, but we don't >>> need to list all possible ways to get it. It's enough that the reader >>> understands that a packet may have segments and segment length is different >>> thing than total packet length Dmitry Eremin-Solenikov(lumag) wrote: And here too, please. > Dmitry Eremin-Solenikov(lumag) wrote: > odp_packet_seg_len() **or odp_packet_offset()**, if you don't mind. https://github.com/Linaro/odp/pull/497#discussion_r170220664 updated_at 2018-02-23 10:45:55
Re: [lng-odp] [PATCH API-NEXT v1] api: packet: data and segment length
Balasubramanian Manoharan(bala-manoharan) replied on github web page: include/odp/api/spec/packet.h line 39 @@ -401,30 +401,39 @@ uint32_t odp_packet_buf_len(odp_packet_t pkt); /** * Packet data pointer * - * Returns the current packet data pointer. When a packet is received - * from packet input, this points to the first byte of the received - * packet. Packet level offsets are calculated relative to this position. + * Returns pointer to the first byte of packet data. When packet is segmented, + * only a portion of packet data follows the pointer. When unsure, use e.g. + * odp_packet_seg_len() to check the data length following the pointer. Packet + * level offsets are calculated relative to this position. * - * User can adjust the data pointer with head_push/head_pull (does not modify - * segmentation) and add_data/rem_data calls (may modify segmentation). + * When a packet is received from packet input, this points to the first byte + * of the received packet. Pool configuration parameters may be used to ensure + * that the first packet segment contains all/most of the data relevant to the + * application. + * + * User can adjust the data pointer with e.g. push_head/pull_head (does not + * modify segmentation) and extend_head/trunc_head (may modify segmentation) + * calls. * * @param pkt Packet handle * * @return Pointer to the packet data * - * @see odp_packet_l2_ptr(), odp_packet_seg_len() + * @see odp_packet_seg_len(), odp_packet_push_head(), odp_packet_extend_head() */ void *odp_packet_data(odp_packet_t pkt); /** - * Packet segment data length + * Packet data length following the data pointer * - * Returns number of data bytes following the current data pointer - * (odp_packet_data()) location in the segment. + * Returns number of data bytes (in the segment) following the current data + * pointer position. When unsure, use this function to check how many bytes Comment: In the entire documentation you have avoided the term "first segment" is it by choice? IMO we could refer this as first segment valid data bytes > Petri Savolainen(psavol) wrote: > seg_len / push_head / extend_head are mentioned above. Packet_offset is not > specialized for handling first N bytes of packet, so it's not directly > related to these ones. >> Petri Savolainen(psavol) wrote: >> packet_offset is for different purpose (access data on arbitrary offset), >> these calls are optimized for the common case (offset zero). Also >> odp_packet_seg_data_len(), the new data_seg_len(), odp_packet_l2_ptr(), >> odp_packet_l3_ptr() and odp_packet_l4_ptr() output seg len, but we don't >> need to list all possible ways to get it. It's enough that the reader >> understands that a packet may have segments and segment length is different >> thing than total packet length >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> And here too, please. Dmitry Eremin-Solenikov(lumag) wrote: odp_packet_seg_len() **or odp_packet_offset()**, if you don't mind. https://github.com/Linaro/odp/pull/497#discussion_r170204670 updated_at 2018-02-23 09:36:37
Re: [lng-odp] [PATCH v5] linux-generic: pool: Return address range in pool info
Petri Savolainen(psavol) replied on github web page:
test/validation/api/pool/pool.c
line 40
@@ -217,6 +217,50 @@ static void pool_test_info_packet(void)
CU_ASSERT(odp_pool_destroy(pool) == 0);
}
+static void pool_test_info_data_range(void)
+{
+ odp_pool_t pool;
+ odp_pool_info_t info;
+ odp_pool_param_t param;
+ odp_packet_t pkt[PKT_NUM];
+ uint32_t i, num;
+ uintptr_t pkt_data, pool_len;
+
+ odp_pool_param_init(¶m);
+
+ param.type = ODP_POOL_PACKET;
+ param.pkt.num = PKT_NUM;
+ param.pkt.len = PKT_LEN;
+
+ pool = odp_pool_create(NULL, ¶m);
+ CU_ASSERT_FATAL(pool != ODP_POOL_INVALID);
+
+ CU_ASSERT_FATAL(odp_pool_info(pool, &info) == 0);
+
+ pool_len = info.max_data_addr - info.min_data_addr + 1;
+ CU_ASSERT(pool_len >= PKT_NUM * PKT_LEN);
+
+ num = 0;
+
+ for (i = 0; i < PKT_NUM; i++) {
+ pkt[num] = odp_packet_alloc(pool, PKT_LEN);
+ CU_ASSERT(pkt[num] != ODP_PACKET_INVALID);
+
+ if (pkt[num] != ODP_PACKET_INVALID)
+ num++;
+ }
+
+ for (i = 0; i < num; i++) {
+ pkt_data = (uintptr_t)odp_packet_data(pkt[i]);
+ CU_ASSERT((pkt_data >= info.min_data_addr) &&
+ (pkt_data + PKT_LEN - 1 <= info.max_data_addr));
Comment:
It wrong to assume that entire packet data follows data pointer. Use
odp_packet_seg_len() instead of PKT_LEN here.
> semihalf-mazur-michal wrote
> Fixed in v4
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> I'd make this `CU_ASSERT()` rather than `CU_ASSERT_FATAL()` so all
>> discrepancies can be caught. `CU_ASSERT_FATAL()` is reserved for setup
>> failures that invalidate the entire test (_e.g.,_ not being able to create
>> the pool, `odp_pool_info()` reporting an error, etc.)
https://github.com/Linaro/odp/pull/495#discussion_r170193779
updated_at 2018-02-23 08:43:57
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_pool.c
line 28
@@ -296,7 +282,9 @@ static void init_buffers(pool_t *pool)
memset(buf_hdr, 0, (uintptr_t)data - (uintptr_t)buf_hdr);
/* Initialize buffer metadata */
- buf_hdr->index = i;
+ buf_hdr->index.u32= 0;
+ buf_hdr->index.pool = pool->pool_idx;
+ buf_hdr->index.buffer = i;
Comment:
Plus this is done only once - in pool create phase.
> Petri Savolainen(psavol) wrote:
> No since ring size will be much smaller than 4 billion.
>> Petri Savolainen(psavol) wrote:
>> The point is that ring size will never be close to 4 billion entries. E.g.
>> currently tail is always max 4096 larger than head. Your example above is
>> based assumption of 4 billion entry ring. Overflow is avoided when ring size
>> if <2 billion, as 32 bit indexes can be still used to calculate number of
>> items correctly.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Agreed.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Agreed, this is good for now. Later we may wish to honor the
user-requested queue `size` parameter.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Agreed.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Correct, as noted earlier. I withdraw that comment.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Presumably the compiler will see the overlap and optimize away the
>>> redundancy, so I assume the performance impact will be nil here.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Since you're allowing `head` and `tail` to run over the entire 32-bit
range, you're correct that you can completely fill the ring. I did
miss that point. However, as shown above you still need this to be:
```
num = size - abs(tail - head);
```
To avoid problems at the 32-bit wrap boundary.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> You're computing in 32 bits not 8 bits, and your ring size is less
> than 2^32 elements. Consider the following test program:
> ```
> #include
> #include
> #include
> #include
>
> void main() {
> uint32_t head[4] = {0, 1, 2, 3};
> uint32_t tail[4] = {0, 0x, 0xfffe, 0xfffd};
> uint32_t mask = 4095;
> uint32_t result;
> int i;
>
> for (i = 0; i < 4; i++) {
> printf("head = %" PRIu32 " tail = %" PRIu32 ":\n",
> head[i], tail[i]);
>
> result = tail[i] - head[i];
> printf("tail - head = %" PRIu32 "\n", result);
>
> result = (tail[i] - head[i]) & mask;
> printf("(tail - head) & mask = %" PRIu32 "\n", result);
>
> result = abs(tail[i] - head[i]);
> printf("abs(tail - head) = %" PRIu32 "\n\n", result);
> }
> }
> ```
> in theory `tail - head` should be the number of elements in the ring,
> in this case 0, 2, 4, and 6. But running this test program gives the
> following output:
> ```
> head = 0 tail = 0:
> tail - head = 0
> (tail - head) & mask = 0
> abs(tail - head) = 0
>
> head = 1 tail = 4294967295:
> tail - head = 4294967294
> (tail - head) & mask = 4094
> abs(tail - head) = 2
>
> head = 2 tail = 4294967294:
> tail - head = 4294967292
> (tail - head) & mask = 4092
> abs(tail - head) = 4
>
> head = 3 tail = 4294967293:
> tail - head = 4294967290
> (tail - head) & mask = 4090
> abs(tail - head) = 6
> ```
> Since you're allowing head to run free over the 32-bit range of the
> variable, when the 32-bits rolls over you'll get a large positive
> number, not the small one you need to stay within the ring bounds.
> The alternative is to mask `head` and `tail` as you increment them,
> but then you run into the effective range issue.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> OK
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> OK, the `ODP_ASSERT()` would still be useful for debugging.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
That functionality is not obvious from the name. It either implies
that one of the input arguments is written (not true here) or the
reader might assume that it is an expression without side-effect
and should be deleted (what I originally thought when reading it).
You should
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/include/odp_ring_st_internal.h
line 78
@@ -0,0 +1,118 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef ODP_RING_ST_INTERNAL_H_
+#define ODP_RING_ST_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include
+#include
+
+/* Basic ring for single thread usage. Operations must be synchronized by using
+ * locks (or other means), when multiple threads use the same ring. */
+typedef struct {
+ uint32_t head;
+ uint32_t tail;
+ uint32_t mask;
+ uint32_t *data;
+
+} ring_st_t;
+
+/* Initialize ring. Ring size must be a power of two. */
+static inline void ring_st_init(ring_st_t *ring, uint32_t *data, uint32_t size)
+{
+ ring->head = 0;
+ ring->tail = 0;
+ ring->mask = size - 1;
+ ring->data = data;
+}
+
+/* Dequeue data from the ring head. Max_num is smaller than ring size.*/
+static inline uint32_t ring_st_deq_multi(ring_st_t *ring, uint32_t data[],
+uint32_t max_num)
+{
+ uint32_t head, tail, mask, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ num = tail - head;
+
+ /* Empty */
+ if (num == 0)
+ return 0;
+
+ if (num > max_num)
+ num = max_num;
+
+ idx = head & mask;
+
+ for (i = 0; i < num; i++) {
+ data[i] = ring->data[idx];
+ idx = (idx + 1) & mask;
+ }
+
+ ring->head = head + num;
+
+ return num;
+}
+
+/* Enqueue data into the ring tail. Num_data is smaller than ring size. */
+static inline uint32_t ring_st_enq_multi(ring_st_t *ring, const uint32_t
data[],
+uint32_t num_data)
+{
+ uint32_t head, tail, mask, size, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ size = mask + 1;
+ num = size - (tail - head);
Comment:
No since ring size will be much smaller than 4 billion.
> Petri Savolainen(psavol) wrote:
> The point is that ring size will never be close to 4 billion entries. E.g.
> currently tail is always max 4096 larger than head. Your example above is
> based assumption of 4 billion entry ring. Overflow is avoided when ring size
> if <2 billion, as 32 bit indexes can be still used to calculate number of
> items correctly.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Agreed.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Agreed, this is good for now. Later we may wish to honor the user-requested
>>> queue `size` parameter.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Agreed.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Correct, as noted earlier. I withdraw that comment.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Presumably the compiler will see the overlap and optimize away the
>> redundancy, so I assume the performance impact will be nil here.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Since you're allowing `head` and `tail` to run over the entire 32-bit
>>> range, you're correct that you can completely fill the ring. I did miss
>>> that point. However, as shown above you still need this to be:
>>>
>>> ```
>>> num = size - abs(tail - head);
>>> ```
>>> To avoid problems at the 32-bit wrap boundary.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
You're computing in 32 bits not 8 bits, and your ring size is less
than 2^32 elements. Consider the following test program:
```
#include
#include
#include
#include
void main() {
uint32_t head[4] = {0, 1, 2, 3};
uint32_t tail[4] = {0, 0x, 0xfffe, 0xfffd};
uint32_t mask = 4095;
uint32_t result;
int i;
for (i = 0; i < 4; i++) {
printf("head = %" PRIu32 " tail = %" PRIu32 ":\n",
head[i], tail[i]);
result = tail[i] - head[i];
printf("tail - head = %" PRIu32 "\n", result);
result = (tail[i] - head[i]) & mask;
printf("(tail - head) & mask = %" PRIu32 "\n", result);
result = abs(tail[i] - head[i]);
printf("abs(tail - head) = %" PRIu32 "\n\n", result);
}
}
```
in theory `tail - head` should be the number of elements in the ring,
in this case 0, 2, 4, and 6. But running this test program gives the
following output:
```
head = 0 tail = 0:
tail - head = 0
>>>
Re: [lng-odp] [PATCH v2] Ring implementation of queues
Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/include/odp_ring_st_internal.h
line 46
@@ -0,0 +1,118 @@
+/* Copyright (c) 2018, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+#ifndef ODP_RING_ST_INTERNAL_H_
+#define ODP_RING_ST_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include
+#include
+
+/* Basic ring for single thread usage. Operations must be synchronized by using
+ * locks (or other means), when multiple threads use the same ring. */
+typedef struct {
+ uint32_t head;
+ uint32_t tail;
+ uint32_t mask;
+ uint32_t *data;
+
+} ring_st_t;
+
+/* Initialize ring. Ring size must be a power of two. */
+static inline void ring_st_init(ring_st_t *ring, uint32_t *data, uint32_t size)
+{
+ ring->head = 0;
+ ring->tail = 0;
+ ring->mask = size - 1;
+ ring->data = data;
+}
+
+/* Dequeue data from the ring head. Max_num is smaller than ring size.*/
+static inline uint32_t ring_st_deq_multi(ring_st_t *ring, uint32_t data[],
+uint32_t max_num)
+{
+ uint32_t head, tail, mask, idx;
+ uint32_t num, i;
+
+ head = ring->head;
+ tail = ring->tail;
+ mask = ring->mask;
+ num = tail - head;
Comment:
The point is that ring size will never be close to 4 billion entries. E.g.
currently tail is always max 4096 larger than head. Your example above is based
assumption of 4 billion entry ring. Overflow is avoided when ring size if <2
billion, as 32 bit indexes can be still used to calculate number of items
correctly.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Agreed.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Agreed, this is good for now. Later we may wish to honor the user-requested
>> queue `size` parameter.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Agreed.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Correct, as noted earlier. I withdraw that comment.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Presumably the compiler will see the overlap and optimize away the
> redundancy, so I assume the performance impact will be nil here.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Since you're allowing `head` and `tail` to run over the entire 32-bit
>> range, you're correct that you can completely fill the ring. I did miss
>> that point. However, as shown above you still need this to be:
>>
>> ```
>> num = size - abs(tail - head);
>> ```
>> To avoid problems at the 32-bit wrap boundary.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> You're computing in 32 bits not 8 bits, and your ring size is less than
>>> 2^32 elements. Consider the following test program:
>>> ```
>>> #include
>>> #include
>>> #include
>>> #include
>>>
>>> void main() {
>>> uint32_t head[4] = {0, 1, 2, 3};
>>> uint32_t tail[4] = {0, 0x, 0xfffe, 0xfffd};
>>> uint32_t mask = 4095;
>>> uint32_t result;
>>> int i;
>>>
>>> for (i = 0; i < 4; i++) {
>>> printf("head = %" PRIu32 " tail = %" PRIu32 ":\n",
>>>head[i], tail[i]);
>>>
>>> result = tail[i] - head[i];
>>> printf("tail - head = %" PRIu32 "\n", result);
>>>
>>> result = (tail[i] - head[i]) & mask;
>>> printf("(tail - head) & mask = %" PRIu32 "\n", result);
>>>
>>> result = abs(tail[i] - head[i]);
>>> printf("abs(tail - head) = %" PRIu32 "\n\n", result);
>>> }
>>> }
>>> ```
>>> in theory `tail - head` should be the number of elements in the ring,
>>> in this case 0, 2, 4, and 6. But running this test program gives the
>>> following output:
>>> ```
>>> head = 0 tail = 0:
>>> tail - head = 0
>>> (tail - head) & mask = 0
>>> abs(tail - head) = 0
>>>
>>> head = 1 tail = 4294967295:
>>> tail - head = 4294967294
>>> (tail - head) & mask = 4094
>>> abs(tail - head) = 2
>>>
>>> head = 2 tail = 4294967294:
>>> tail - head = 4294967292
>>> (tail - head) & mask = 4092
>>> abs(tail - head) = 4
>>>
>>> head = 3 tail = 4294967293:
>>> tail - head = 4294967290
>>> (tail - head) & mask = 4090
>>> abs(tail - head) = 6
>>> ```
>>> Since you're allowing head to run free over the 32-bit range of the
>>> variable, when the 32-bits rolls over you'll get a large positive
>>> number, not the small one you need to stay within the ring bounds. The
>>> alternative is to mask `head` and `tail` as you increment them, but
>>> then you run into the effective range issue.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
OK
>
Re: [lng-odp] [PATCH API-NEXT v1] api: packet: data and segment length
Petri Savolainen(psavol) replied on github web page: include/odp/api/spec/packet.h line 28 @@ -401,30 +401,39 @@ uint32_t odp_packet_buf_len(odp_packet_t pkt); /** * Packet data pointer * - * Returns the current packet data pointer. When a packet is received - * from packet input, this points to the first byte of the received - * packet. Packet level offsets are calculated relative to this position. + * Returns pointer to the first byte of packet data. When packet is segmented, + * only a portion of packet data follows the pointer. When unsure, use e.g. + * odp_packet_seg_len() to check the data length following the pointer. Packet + * level offsets are calculated relative to this position. * - * User can adjust the data pointer with head_push/head_pull (does not modify - * segmentation) and add_data/rem_data calls (may modify segmentation). + * When a packet is received from packet input, this points to the first byte + * of the received packet. Pool configuration parameters may be used to ensure + * that the first packet segment contains all/most of the data relevant to the + * application. + * + * User can adjust the data pointer with e.g. push_head/pull_head (does not + * modify segmentation) and extend_head/trunc_head (may modify segmentation) + * calls. * * @param pkt Packet handle * * @return Pointer to the packet data * - * @see odp_packet_l2_ptr(), odp_packet_seg_len() + * @see odp_packet_seg_len(), odp_packet_push_head(), odp_packet_extend_head() Comment: seg_len / push_head / extend_head are mentioned above. Packet_offset is not specialized for handling first N bytes of packet, so it's not directly related to these ones. > Petri Savolainen(psavol) wrote: > packet_offset is for different purpose (access data on arbitrary offset), > these calls are optimized for the common case (offset zero). Also > odp_packet_seg_data_len(), the new data_seg_len(), odp_packet_l2_ptr(), > odp_packet_l3_ptr() and odp_packet_l4_ptr() output seg len, but we don't > need to list all possible ways to get it. It's enough that the reader > understands that a packet may have segments and segment length is different > thing than total packet length >> Dmitry Eremin-Solenikov(lumag) wrote: >> And here too, please. >>> Dmitry Eremin-Solenikov(lumag) wrote: >>> odp_packet_seg_len() **or odp_packet_offset()**, if you don't mind. https://github.com/Linaro/odp/pull/497#discussion_r170183024 updated_at 2018-02-23 07:36:58
Re: [lng-odp] [PATCH API-NEXT v1] api: packet: data and segment length
Petri Savolainen(psavol) replied on github web page: include/odp/api/spec/packet.h line 9 @@ -401,30 +401,39 @@ uint32_t odp_packet_buf_len(odp_packet_t pkt); /** * Packet data pointer * - * Returns the current packet data pointer. When a packet is received - * from packet input, this points to the first byte of the received - * packet. Packet level offsets are calculated relative to this position. + * Returns pointer to the first byte of packet data. When packet is segmented, + * only a portion of packet data follows the pointer. When unsure, use e.g. + * odp_packet_seg_len() to check the data length following the pointer. Packet Comment: packet_offset is for different purpose (access data on arbitrary offset), these calls are optimized for the common case (offset zero). Also odp_packet_seg_data_len(), the new data_seg_len(), odp_packet_l2_ptr(), odp_packet_l3_ptr() and odp_packet_l4_ptr() output seg len, but we don't need to list all possible ways to get it. It's enough that the reader understands that a packet may have segments and segment length is different thing than total packet length > Dmitry Eremin-Solenikov(lumag) wrote: > And here too, please. >> Dmitry Eremin-Solenikov(lumag) wrote: >> odp_packet_seg_len() **or odp_packet_offset()**, if you don't mind. https://github.com/Linaro/odp/pull/497#discussion_r170182482 updated_at 2018-02-23 07:33:19
