[lng-odp] [Bug 2798] ODP random and other crypto functions (possibly) not thread safe?

[bugzilla-daemon]

https://bugs.linaro.org/show_bug.cgi?id=2798

Bill Fischofer  changed:

   What|Removed |Added

 Ever confirmed|0   |1
   Severity|enhancement |major
 Status|UNCONFIRMED |IN_PROGRESS
   Priority|--- |High
   Assignee|nikhil.agar...@linaro.org   |bill.fischo...@linaro.org
 CC||bill.fischo...@linaro.org

--- Comment #1 from Bill Fischofer  ---
Patch http://patches.opendataplane.org/patch/7807/ posted to address this
issue. Christophe: Please test this in your multithreading app since I've only
verified that it works in the basic random verification test.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[lng-odp] [API-NEXT PATCH] linux-generic: init: add openssl locking support for thread safety

[Bill Fischofer]

Resolve Bug https://bugs.linaro.org/show_bug.cgi?id=2798 by adding
OpenSSL callbacks for locking that use ticketlocks to provide
thread-safety for OpenSSL calls made by ODP components such as random
number generation.

Signed-off-by: Bill Fischofer 
---
 platform/linux-generic/include/odp_internal.h |  5 +++
 platform/linux-generic/odp_init.c | 63 +++
 2 files changed, 68 insertions(+)

diff --git a/platform/linux-generic/include/odp_internal.h 
b/platform/linux-generic/include/odp_internal.h
index b313b1f..2280e3f 100644
--- a/platform/linux-generic/include/odp_internal.h
+++ b/platform/linux-generic/include/odp_internal.h
@@ -20,6 +20,8 @@ extern "C" {
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 
@@ -50,6 +52,8 @@ struct odp_global_data_s {
odp_cpumask_t control_cpus;
odp_cpumask_t worker_cpus;
int num_cpus_installed;
+   odp_shm_t openssl_shm;
+   odp_ticketlock_t **openssl_lock;
 };
 
 enum init_stage {
@@ -66,6 +70,7 @@ enum init_stage {
PKTIO_INIT,
TIMER_INIT,
CRYPTO_INIT,
+   OPENSSL_INIT,
CLASSIFICATION_INIT,
TRAFFIC_MNGR_INIT,
NAME_TABLE_INIT,
diff --git a/platform/linux-generic/odp_init.c 
b/platform/linux-generic/odp_init.c
index 1b0d8f8..6873bb5 100644
--- a/platform/linux-generic/odp_init.c
+++ b/platform/linux-generic/odp_init.c
@@ -6,6 +6,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -21,8 +23,30 @@
 #define _ODP_FILES_FMT "odp-%d-"
 #define _ODP_TMPDIR"/tmp"
 
+static inline int openssl_numlocks(void)
+{
+   return CRYPTO_num_locks();
+}
+
 struct odp_global_data_s odp_global_data;
 
+static unsigned long openssl_thread_id(void)
+{
+   return (unsigned long)odp_thread_id();
+}
+
+static void openssl_lock(int mode, int n,
+const char *file ODP_UNUSED,
+int line ODP_UNUSED)
+{
+   if (mode & CRYPTO_LOCK)
+   odp_ticketlock_lock((odp_ticketlock_t *)
+   &odp_global_data.openssl_lock[n]);
+   else
+   odp_ticketlock_unlock((odp_ticketlock_t *)
+ &odp_global_data.openssl_lock[n]);
+}
+
 /* remove all files staring with "odp-" from a directory "dir" */
 static int cleanup_files(const char *dirpath, int odp_pid)
 {
@@ -70,6 +94,8 @@ int odp_init_global(odp_instance_t *instance,
const odp_platform_init_t *platform_params ODP_UNUSED)
 {
char *hpdir;
+   int nlocks = openssl_numlocks();
+   int i;
 
memset(&odp_global_data, 0, sizeof(struct odp_global_data_s));
odp_global_data.main_pid = getpid();
@@ -162,6 +188,34 @@ int odp_init_global(odp_instance_t *instance,
}
stage = CRYPTO_INIT;
 
+   if (nlocks > 0) {
+   odp_global_data.openssl_shm =
+   odp_shm_reserve("ODP OpenSSL Mutexes",
+   nlocks * sizeof(odp_ticketlock_t),
+   sizeof(odp_ticketlock_t),
+   ODP_SHM_SW_ONLY);
+   if (odp_global_data.openssl_shm == ODP_SHM_INVALID) {
+   ODP_ERR("ODP OpenSSL init reserve failed.\n");
+   goto init_failed;
+   }
+
+   odp_global_data.openssl_lock =
+   odp_shm_addr(odp_global_data.openssl_shm);
+   if (odp_global_data.openssl_lock == NULL) {
+   odp_shm_free(odp_global_data.openssl_shm);
+   ODP_ERR("ODP OpenSSL init addr failed.\n");
+   goto init_failed;
+   }
+
+   for (i = 0; i < nlocks; i++)
+   odp_ticketlock_init((odp_ticketlock_t *)
+   &odp_global_data.openssl_lock[i]);
+
+   CRYPTO_set_id_callback(openssl_thread_id);
+   CRYPTO_set_locking_callback(openssl_lock);
+   }
+   stage = OPENSSL_INIT;
+
if (odp_classification_init_global()) {
ODP_ERR("ODP classification init failed.\n");
goto init_failed;
@@ -224,6 +278,15 @@ int _odp_term_global(enum init_stage stage)
}
/* Fall through */
 
+   case OPENSSL_INIT:
+   if (odp_global_data.openssl_lock) {
+   CRYPTO_set_locking_callback(NULL);
+   CRYPTO_set_id_callback(NULL);
+
+   odp_shm_free(odp_global_data.openssl_shm);
+   }
+   /* Fall through */
+
case CRYPTO_INIT:
if (odp_crypto_term_global()) {
ODP_ERR("ODP crypto term failed.\n");
-- 
2.7.4

[lng-odp] Internal component modularization

[Honnappa Nagarahalli]

Hi,
   As we had discussed earlier, the queue internal data structure
members are being accessed by other components. Modularizing the queue
component will make it easy to introduce different implementations of
queue.

I have written a small document generalizing the methods/conventions
we want to follow for any component. Please review the document and
provide any comments you may have. Future patches will follow the
conventions mentioned in this document.

https://docs.google.com/a/linaro.org/document/d/1i6K3Bm703dQybgGZTAVlE2T7PS-7vsD15pDUS1itUF0/edit?usp=sharing

Thank you,
Honnappa

[lng-odp] [API-NEXT PATCHv6 5/5] doc: userguide: add user documentation for packet references

[Bill Fischofer]

Signed-off-by: Bill Fischofer 
---
 doc/users-guide/users-guide-packet.adoc | 239 +++-
 1 file changed, 238 insertions(+), 1 deletion(-)

diff --git a/doc/users-guide/users-guide-packet.adoc 
b/doc/users-guide/users-guide-packet.adoc
index e3be23c..aecdca2 100644
--- a/doc/users-guide/users-guide-packet.adoc
+++ b/doc/users-guide/users-guide-packet.adoc
@@ -246,7 +246,7 @@ packet pool as the original packet.
 The opposite operation is performed by the `odp_packet_concat()` API. This API
 takes a destination and source packet as arguments and the result is that
 the source packet is concatenated to the destination packet and ceases to
-have any separete identity. Note that it is legal to concatenate a packet to
+have any separate identity. Note that it is legal to concatenate a packet to
 itself, in which case the result is a packet with double the length of the
 original packet.
 
@@ -282,3 +282,240 @@ larger than the underlying segment size. The call may 
also fail if the
 requested alignment is too high. Alignment limits will vary among different ODP
 implementations, however ODP requires that all implementations support
 requested alignments of at least 32 bytes.
+
+=== Packet References
+To support efficient multicast, retransmit, and related processing, ODP
+supports two additional types of packet manipulation: static and dynamic
+_references_. A reference is intended to be a lightweight mechanism for
+creating aliases to packets as well as to create packets that share data bytes
+with other packets to avoid unnecessary data copying.
+
+ Static References
+The simplest type of reference is the _static reference_. A static reference is
+created by the call:
+
+[source,c]
+-
+ref_pkt = odp_packet_ref_static(pkt);
+-
+
+If the reference fails, `ODP_PACKET_INVALID` is returned and `pkt`
+remains unchanged.
+
+The effect of this call is shown below:
+
+.Static Packet Reference
+image::refstatic.svg[align="center"]
+
+A static reference provides a simple and efficient means of creating an alias
+for a packet handle that prevents the packet itself from being freed until all
+references to it have been released via `odp_packet_free()` calls. This is
+useful, for example, to support retransmission processing, since as part of
+packet TX processing, `odp_pktout_send()` or `odp_tm_enq()` will free
+the packet after it has been transmitted.
+
+`odp_packet_ref_static()` might be used in a transmit routine wrapper
+function like:
+
+[source,c]
+-
+int xmit_pkt(odp_pktout_queue_t queue, odp_packet_t pkt)
+{
+   odp_packet_t ref = odp_packet_ref_static(pkt);
+   return ref == ODP_PACKET_INVALID ? -1 : odp_pktout_send(queue, ref, 1);
+}
+-
+
+This transmits a reference to `pkt` so that `pkt` is retained by the caller,
+which means that the caller is free to retransmit it if needed at a later
+time. When a higher level protocol (_e.g.,_ receipt of a TCP ACK packet)
+confirms that the transmission was successful, `pkt` can then be discarded via
+an `odp_packet_free()` call.
+
+The key characteristic of a static reference is that because there are
+multiple independent handles that refer to the same packet, the caller should
+treat the packet as read only following the creation of a static reference
+until all other references to it are freed. This is because all static
+references are simply aliases of the same packet, so if multiple threads were
+independently manipulating the packet this would lead to unpredictable race
+conditions.
+
+To assist in determining whether there are other references to a packet, ODP
+provides the API:
+
+[source,c]
+-
+int odp_packet_is_ref(odp_packet_t pkt);
+-
+
+that indicates whether other handles exist that share bytes with this
+packet. If this routine returns 0 then the caller can be assured that it is
+safe to modify it as this handle is the only reference to the packet.
+
+ Dynamic References
+While static references are convenient and efficient, they are limited by the
+need to be treated as read only. For example, consider an application that
+needs to _multicast_ a packet. Here the same packet needs to be sent to two or
+more different destinations. While the packet payload may be the same, each
+sent copy of the packet requires its own unique header to specify the
+destination that is to receive the packet.
+
+To address this need, ODP provides _dynamic references_. These are created
+by the call:
+
+[source,c]
+-
+ref_pkt = odp_packet_ref(pkt, offset);
+-
+
+The `offset` parameter specifies the byte offset into `pkt` at which the
+reference is to begin. This must be in the range
+0..`odp_packet_len(pkt)`-1. As before, if the reference is unable to be
+created `ODP_PACKET_INVALID` is returned and `pkt` is unchanged, otherwise the
+result is as shown below:
+
+.Dynamic Packet Reference
+image::ref.svg[align="center"]
+
+Following a successful reference creation, the bytes of `pkt` beginning at
+offset `

[lng-odp] [API-NEXT PATCHv6 2/5] linux-generic: packet: implement reference apis

[Bill Fischofer]

Implement the APIs:
- odp_packet_ref_static()
- odp_packet_ref()
- odp_packet_ref_pkt()
- odp_packet_is_ref()
- odp_packet_unshared_len()

This also involves functional upgrades to the existing packet manipulation
APIs to work with packet references as input arguments.

Signed-off-by: Bill Fischofer 
---
 .../linux-generic/include/odp_packet_internal.h|  73 ++-
 platform/linux-generic/odp_packet.c| 542 +
 2 files changed, 508 insertions(+), 107 deletions(-)

diff --git a/platform/linux-generic/include/odp_packet_internal.h 
b/platform/linux-generic/include/odp_packet_internal.h
index d09231e..ccb59b9 100644
--- a/platform/linux-generic/include/odp_packet_internal.h
+++ b/platform/linux-generic/include/odp_packet_internal.h
@@ -167,7 +167,7 @@ typedef struct {
  * packet_init(). Because of this any new fields added must be reviewed for
  * initialization requirements.
  */
-typedef struct {
+typedef struct odp_packet_hdr_t {
/* common buffer header */
odp_buffer_hdr_t buf_hdr;
 
@@ -178,6 +178,13 @@ typedef struct {
uint32_t headroom;
uint32_t tailroom;
 
+   /* Fields used to support packet references */
+   odp_atomic_u32_t ref_count;  /* Number of refs to this pkt/seg */
+   uint32_t unshared_len;   /* Offset that sharing starts at */
+   uint32_t ref_offset; /* Offset into base pkt for this ref */
+   uint32_t ref_len;/* frame_len at time this ref created */
+   struct odp_packet_hdr_t *ref_hdr; /* Ptr to the base pkt for this ref */
+
odp_pktio_t input;
 
/* Members below are not initialized by packet_init() */
@@ -200,6 +207,55 @@ static inline odp_packet_hdr_t 
*odp_packet_hdr(odp_packet_t pkt)
return (odp_packet_hdr_t *)buf_hdl_to_hdr((odp_buffer_t)pkt);
 }
 
+static inline odp_packet_hdr_t *odp_packet_last_hdr(odp_packet_t pkt,
+   uint32_t *offset)
+{
+   odp_packet_hdr_t *pkt_hdr = odp_packet_hdr(pkt);
+   odp_packet_hdr_t *prev_hdr = pkt_hdr;
+   uint32_t ref_offset = 0;
+
+   while (pkt_hdr->ref_hdr) {
+   ref_offset = pkt_hdr->ref_offset;
+   prev_hdr   = pkt_hdr;
+   pkt_hdr= pkt_hdr->ref_hdr;
+   }
+
+   if (offset) {
+   if (prev_hdr != pkt_hdr)
+   ref_offset += pkt_hdr->frame_len - prev_hdr->ref_len;
+   *offset = ref_offset;
+   }
+
+   return pkt_hdr;
+}
+
+static inline odp_packet_hdr_t *odp_packet_prev_hdr(odp_packet_hdr_t *pkt_hdr,
+   odp_packet_hdr_t *cur_hdr,
+   uint32_t *offset)
+{
+   uint32_t ref_offset = 0;
+   odp_packet_hdr_t *prev_hdr = pkt_hdr;
+
+   while (pkt_hdr->ref_hdr != cur_hdr) {
+   ref_offset = pkt_hdr->ref_offset;
+   prev_hdr   = pkt_hdr;
+   pkt_hdr= pkt_hdr->ref_hdr;
+   }
+
+   if (offset) {
+   if (prev_hdr != pkt_hdr)
+   ref_offset += pkt_hdr->frame_len - prev_hdr->ref_len;
+   *offset = ref_offset;
+   }
+
+   return pkt_hdr;
+}
+
+static inline odp_packet_t _odp_packet_hdl(odp_packet_hdr_t *pkt_hdr)
+{
+   return (odp_packet_t)odp_hdr_to_buf(&pkt_hdr->buf_hdr);
+}
+
 static inline void copy_packet_parser_metadata(odp_packet_hdr_t *src_hdr,
   odp_packet_hdr_t *dst_hdr)
 {
@@ -222,12 +278,25 @@ static inline void pull_tail(odp_packet_hdr_t *pkt_hdr, 
uint32_t len)
 
pkt_hdr->tailroom  += len;
pkt_hdr->frame_len -= len;
+   pkt_hdr->unshared_len -= len;
pkt_hdr->buf_hdr.seg[last].len -= len;
 }
 
 static inline uint32_t packet_len(odp_packet_hdr_t *pkt_hdr)
 {
-   return pkt_hdr->frame_len;
+   uint32_t pkt_len = 0;
+   uint32_t offset  = 0;
+
+   do {
+   pkt_len += pkt_hdr->frame_len - offset;
+   offset   = pkt_hdr->ref_offset;
+   if (pkt_hdr->ref_hdr)
+   offset += (pkt_hdr->ref_hdr->frame_len -
+  pkt_hdr->ref_len);
+   pkt_hdr  = pkt_hdr->ref_hdr;
+   } while (pkt_hdr);
+
+   return pkt_len;
 }
 
 static inline void packet_set_len(odp_packet_hdr_t *pkt_hdr, uint32_t len)
diff --git a/platform/linux-generic/odp_packet.c 
b/platform/linux-generic/odp_packet.c
index 58b6f32..5de3b8e 100644
--- a/platform/linux-generic/odp_packet.c
+++ b/platform/linux-generic/odp_packet.c
@@ -30,13 +30,34 @@ static inline odp_buffer_t buffer_handle(odp_packet_hdr_t 
*pkt_hdr)
return pkt_hdr->buf_hdr.handle.handle;
 }
 
+static inline uint32_t packet_ref_count(odp_packet_hdr_t *pkt_hdr)
+{
+   return odp_atomic_load_u32(&pkt_hdr->ref_count);
+}
+
+static inline void packet_ref_count_set(odp_packet_hdr_t *pkt_hdr, uint32_t n)
+{
+   odp_atomic_init_u3

[lng-odp] [API-NEXT PATCHv6 3/5] validation: packet: add packet reference tests

[Bill Fischofer]

Add validation tests for the new packet reference APIs:
- odp_packet_ref_static()
- odp_packet_ref()
- odp_packet_ref_pkt()
- odp_packet_is_ref()
- odp_packet_unshared_len()

Signed-off-by: Bill Fischofer 
---
 test/common_plat/validation/api/packet/packet.c | 368 +++-
 test/common_plat/validation/api/packet/packet.h |   2 +
 2 files changed, 366 insertions(+), 4 deletions(-)

diff --git a/test/common_plat/validation/api/packet/packet.c 
b/test/common_plat/validation/api/packet/packet.c
index cf11c01..0411d22 100644
--- a/test/common_plat/validation/api/packet/packet.c
+++ b/test/common_plat/validation/api/packet/packet.c
@@ -217,6 +217,7 @@ void packet_test_alloc_free(void)
params.pkt.num= 1;
 
pool = odp_pool_create("packet_pool_alloc", ¶ms);
+   CU_ASSERT_FATAL(pool != ODP_POOL_INVALID);
 
/* Allocate the only buffer from the pool */
packet = odp_packet_alloc(pool, packet_len);
@@ -1410,7 +1411,7 @@ void packet_test_concat_extend_trunc(void)
param.pkt.num = 100;
 
pool = odp_pool_create("packet_pool_concat", ¶m);
-   CU_ASSERT(packet_pool != ODP_POOL_INVALID);
+   CU_ASSERT_FATAL(packet_pool != ODP_POOL_INVALID);
 
pkt = odp_packet_alloc(pool, alloc_len);
CU_ASSERT_FATAL(pkt != ODP_PACKET_INVALID);
@@ -1429,6 +1430,7 @@ void packet_test_concat_extend_trunc(void)
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len + alloc_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
}
 
ret = odp_packet_extend_tail(&pkt, ext_len, NULL, NULL);
@@ -1436,12 +1438,14 @@ void packet_test_concat_extend_trunc(void)
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len + ext_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
 
ret = odp_packet_extend_head(&pkt, ext_len, NULL, NULL);
CU_ASSERT(ret >= 0);
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len + ext_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
 
pkt2 = odp_packet_alloc(pool, alloc_len);
CU_ASSERT_FATAL(pkt2 != ODP_PACKET_INVALID);
@@ -1454,18 +1458,21 @@ void packet_test_concat_extend_trunc(void)
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len + alloc_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
 
ret = odp_packet_trunc_head(&pkt, trunc_len, NULL, NULL);
CU_ASSERT(ret >= 0);
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len - trunc_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
 
ret = odp_packet_trunc_tail(&pkt, trunc_len, NULL, NULL);
CU_ASSERT(ret >= 0);
 
CU_ASSERT(odp_packet_len(pkt) == (cur_len - trunc_len));
cur_len = odp_packet_len(pkt);
+   CU_ASSERT(cur_len == odp_packet_unshared_len(pkt));
 
odp_packet_free(pkt);
 
@@ -1497,7 +1504,7 @@ void packet_test_extend_small(void)
param.pkt.num = 100;
 
pool = odp_pool_create("packet_pool_extend", ¶m);
-   CU_ASSERT(packet_pool != ODP_POOL_INVALID);
+   CU_ASSERT_FATAL(packet_pool != ODP_POOL_INVALID);
 
for (round = 0; round < 2; round++) {
pkt = odp_packet_alloc(pool, 1);
@@ -1533,6 +1540,7 @@ void packet_test_extend_small(void)
}
 
CU_ASSERT(odp_packet_len(pkt) == len);
+   CU_ASSERT(odp_packet_unshared_len(pkt) == len);
 
len = odp_packet_len(pkt);
 
@@ -1583,7 +1591,7 @@ void packet_test_extend_large(void)
param.pkt.num = 100;
 
pool = odp_pool_create("packet_pool_extend", ¶m);
-   CU_ASSERT(packet_pool != ODP_POOL_INVALID);
+   CU_ASSERT_FATAL(packet_pool != ODP_POOL_INVALID);
 
for (round = 0; round < 2 * num_div; round++) {
ext_len = len / div;
@@ -1694,7 +1702,7 @@ void packet_test_extend_mix(void)
param.pkt.num = 100;
 
pool = odp_pool_create("packet_pool_extend", ¶m);
-   CU_ASSERT(packet_pool != ODP_POOL_INVALID);
+   CU_ASSERT_FATAL(packet_pool != ODP_POOL_INVALID);
 
for (round = 0; round < 2; round++) {
small_count = 30;
@@ -1774,6 +1782,90 @@ void packet_test_extend_mix(void)
CU_ASSERT(odp_pool_destroy(pool) == 0);
 }
 
+void packet_test_extend_ref(void)
+{
+   odp_packet_t max_pkt, ref;
+   uint32_t hr, tr, max_len;
+
+   max_pkt = odp_packet_copy(segmented_test_packet,
+ odp_packet_pool(segmented_test_packet));
+   CU_ASSERT_FATAL(max_pkt != ODP_PACKET_INVALID);
+   max_len = odp_packet_len(max_pkt);
+
+   /* Maximize the max pkt */
+   hr = odp_packet_headroom(max_pkt);
+   tr = odp_packet_tailroom(max_pkt);
+   odp_packet_push_head(max_pkt, hr);
+   odp_packet_push_tail(max_pkt, tr);
+
+

[lng-odp] [API-NEXT PATCHv6 1/5] api: packet: add support for packet references

[Bill Fischofer]

Introduce three new APIs that support efficient sharing of portions of
packets.

odp_packet_ref_static() creates an alias for a base packet

odp_packet_ref() creates a reference to a base packet

odp_packet_ref_pkt() creates a reference to a base packet from a supplied
header packet

In addition, two other APIs simplify working with references

odp_packet_is_ref() says whether a packet is a reference

odp_packet_unshared_len() gives the sum of data lengths over all unshared
packet segments. These are the only bytes of the packet that may be
modified safely.

Signed-off-by: Bill Fischofer 
---
Changes in v6:
- Miscellaneous formatting corrections
- Minor documentation improvements

Changes in v5:
- Clarified that both input packets to odp_packet_ref_pkt() must be from the
  same pool and results are undefined if they are not.
- Clarified that input(s) to reference create APIs may be references and that
  implementations are free to perform partial or full packet copies if needed
  to support such compound references.
- Removed odp_packet_has_ref() and redefined odp_packet_is_ref() to indicate
  simply whether other handles exist that share bytes with this packet.
- Modified validation tests to reflect the above changes.

Changes in v4:
- Bug fixes
- Expand validation testing to cover extensions on packets with references

Changes in v3:
- Rebased to latest API-NEXT
- Bug fixes
- Eliminate concept of base packets, both references and referenced packets may
  have head extensions as needed
 include/odp/api/spec/packet.h | 172 +-
 1 file changed, 171 insertions(+), 1 deletion(-)

diff --git a/include/odp/api/spec/packet.h b/include/odp/api/spec/packet.h
index 4a86eba..9b6681c 100644
--- a/include/odp/api/spec/packet.h
+++ b/include/odp/api/spec/packet.h
@@ -256,6 +256,20 @@ uint32_t odp_packet_seg_len(odp_packet_t pkt);
 uint32_t odp_packet_len(odp_packet_t pkt);
 
 /**
+ * Packet unshared data len
+ *
+ * Returns the sum of data lengths over all unshared packet segments. These
+ * are the only bytes that should be modified by the caller. The rest of the
+ * packet should be treated as read only. odp_packet_unshared_len() will be
+ * equal to odp_packet_len() if odp_packet_is_ref() is 0.
+ *
+ * @param pkt Packet handle
+ *
+ * @return Packet unshared data length
+ */
+uint32_t odp_packet_unshared_len(odp_packet_t pkt);
+
+/**
  * Packet headroom length
  *
  * Returns the current packet level headroom length.
@@ -795,7 +809,7 @@ uint32_t odp_packet_seg_data_len(odp_packet_t pkt, 
odp_packet_seg_t seg);
  * data pointers. Otherwise, all old pointers remain valid.
  *
  * The resulting packet is always allocated from the same pool as
- * the destination packet. The source packet may have been allocate from
+ * the destination packet. The source packet may have been allocated from
  * any pool.
  *
  * On failure, both handles remain valid and packets are not modified.
@@ -848,6 +862,162 @@ int odp_packet_split(odp_packet_t *pkt, uint32_t len, 
odp_packet_t *tail);
 
 /*
  *
+ * References
+ * 
+ *
+ */
+
+/**
+ * Create a static reference to a packet
+ *
+ * A static reference is used to obtain an additional handle for referring to
+ * a packet so that the storage behind it is not freed until all references to
+ * the packet are freed. This can be used, for example, to support efficient
+ * retransmission processing.
+ *
+ * The intent of a static reference is that both the packet and the returned
+ * reference to it will be treated as read only after this call. Results are
+ * undefined if this restriction is not observed.
+ *
+ * Static references have restrictions but may have performance advantages on
+ * some platforms if the caller does not intend to modify the reference
+ * packet. If modification is needed (e.g., to prefix a unique header onto the
+ * packet) then odp_packet_ref() or odp_packet_ref_pkt() should be used.
+ *
+ * @param pktHandle of the packet for which a static reference is
+ *   to be created.
+ *
+ * @returnHandle of the static reference packet
+ * @retval ODP_PACKET_INVALID Operation failed. pkt remains unchanged.
+ */
+odp_packet_t odp_packet_ref_static(odp_packet_t pkt);
+
+/**
+ * Create a reference to a packet
+ *
+ * Create a dynamic reference to a packet starting at a specified byte
+ * offset. This reference may be used as an argument to header manipulation
+ * APIs to prefix a unique header onto the shared payload. The storage
+ * associated with the referenced packet is not freed until all references to
+ * it are freed, which permits easy multicasting or retransmission processing
+ * to be performed. Following a successful call, the shared portions of the
+ * referenced packet packet should be treated as read only. Results are
+ * undefined if this restriction is not observed.
+ *
+ * This operation logically prepends a zero-length header onto t

[lng-odp] [Bug 2494] Extend odp_pktio_capability_t to include minimal pool size required by pktio implementation.

[bugzilla-daemon]

https://bugs.linaro.org/show_bug.cgi?id=2494

--- Comment #13 from Bala Manoharan  ---
I have proposed an implementation change by which the API modification is not
required. Waiting for reply from Maceij

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[lng-odp] [PATCHv3 1/1] validation: pktio: fix invalid mac addr

[Balasubramanian Manoharan]

Fixes https://bugs.linaro.org/show_bug.cgi?id=2496

Signed-off-by: Balasubramanian Manoharan 
---
v3: Review comment incorporation

 test/common_plat/validation/api/pktio/pktio.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/test/common_plat/validation/api/pktio/pktio.c 
b/test/common_plat/validation/api/pktio/pktio.c
index 7c979fb..438b7e8 100644
--- a/test/common_plat/validation/api/pktio/pktio.c
+++ b/test/common_plat/validation/api/pktio/pktio.c
@@ -31,6 +31,8 @@
 #define PKTIN_TS_MAX_RES   100
 #define PKTIN_TS_CMP_RES   1
 
+#define PKTIO_SRC_MAC  {1, 2, 3, 4, 5, 6}
+#define PKTIO_DST_MAC  {6, 5, 4, 3, 2, 1}
 #undef DEBUG_STATS
 
 /** interface names used for testing */
@@ -245,7 +247,8 @@ static uint32_t pktio_init_packet(odp_packet_t pkt)
odph_udphdr_t *udp;
char *buf;
uint16_t seq;
-   uint8_t mac[ODP_PKTIO_MACADDR_MAXSIZE] = {0};
+   uint8_t src_mac[ODP_PKTIO_MACADDR_MAXSIZE] = PKTIO_SRC_MAC;
+   uint8_t dst_mac[ODP_PKTIO_MACADDR_MAXSIZE] = PKTIO_DST_MAC;
int pkt_len = odp_packet_len(pkt);
 
buf = odp_packet_data(pkt);
@@ -253,8 +256,8 @@ static uint32_t pktio_init_packet(odp_packet_t pkt)
/* Ethernet */
odp_packet_l2_offset_set(pkt, 0);
eth = (odph_ethhdr_t *)buf;
-   memcpy(eth->src.addr, mac, ODPH_ETHADDR_LEN);
-   memcpy(eth->dst.addr, mac, ODPH_ETHADDR_LEN);
+   memcpy(eth->src.addr, &src_mac, ODPH_ETHADDR_LEN);
+   memcpy(eth->dst.addr, &dst_mac, ODPH_ETHADDR_LEN);
eth->type = odp_cpu_to_be_16(ODPH_ETHTYPE_IPV4);
 
/* IP */
-- 
1.9.1

Re: [lng-odp] [API-NEXT PATCHv2] linux-gen: _ishm: fixing typos

[Bill Fischofer]

On Wed, Dec 21, 2016 at 6:55 AM, Christophe Milard
 wrote:
> Fixing a set of iritating typos. just in comments.
>
> Signed-off-by: Christophe Milard 

Reviewed-by: Bill Fischofer 

> ---
>
> Since V1:
> another typo found by Bill
>
>  platform/linux-generic/_ishm.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/platform/linux-generic/_ishm.c b/platform/linux-generic/_ishm.c
> index 449e357..a49c8e4 100644
> --- a/platform/linux-generic/_ishm.c
> +++ b/platform/linux-generic/_ishm.c
> @@ -75,7 +75,7 @@
>  /*
>   * Maximum number of internal shared memory blocks.
>   *
> - * This the the number of separate ISHM areas that can be reserved 
> concurrently
> + * This is the number of separate ISHM areas that can be reserved 
> concurrently
>   * (Note that freeing such blocks may take time, or possibly never happen
>   * if some of the block ownwers never procsync() after free). This number
>   * should take that into account)
> @@ -236,7 +236,7 @@ static void procsync(void);
>   * Take a piece of the preallocated virtual space to fit "size" bytes.
>   * (best fit). Size must be rounded up to an integer number of pages size.
>   * Possibly split the fragment to keep track of remaining space.
> - * Returns the allocated fragment (best_fragmnt) and the corresponding 
> address.
> + * Returns the allocated fragment (best_fragment) and the corresponding 
> address.
>   * External caller must ensure mutex before the call!
>   */
>  static void *alloc_fragment(uintptr_t size, int block_index, intptr_t align,
> @@ -282,11 +282,11 @@ static void *alloc_fragment(uintptr_t size, int 
> block_index, intptr_t align,
>
> /*
>  * if there is room between previous fragment and new one, (due to
> -* alignement requirement) then fragment (split) the space between
> +* alignment requirement) then fragment (split) the space between
>  * the end of the previous fragment and the beginning of the new one:
>  */
> if (border - (uintptr_t)(*best_fragmnt)->start > 0) {
> -   /* frangment space, i.e. take a new fragment descriptor... */
> +   /* fragment space, i.e. take a new fragment descriptor... */
> rem_fragmnt = ishm_ftbl->unused_fragmnts;
> if (!rem_fragmnt) {
> ODP_ERR("unable to get shmem fragment 
> descriptor!\n.");
> @@ -316,7 +316,7 @@ static void *alloc_fragment(uintptr_t size, int 
> block_index, intptr_t align,
> if (remainder == 0)
> return (*best_fragmnt)->start;
>
> -   /* otherwise, frangment space, i.e. take a new fragment descriptor... 
> */
> +   /* otherwise, fragment space, i.e. take a new fragment descriptor... 
> */
> rem_fragmnt = ishm_ftbl->unused_fragmnts;
> if (!rem_fragmnt) {
> ODP_ERR("unable to get shmem fragment descriptor!\n.");
> @@ -503,7 +503,7 @@ static void delete_file(ishm_block_t *block)
>   * performs the mapping, possibly allocating a fragment of the pre-reserved
>   * VA space if the _ODP_ISHM_SINGLE_VA flag was given.
>   * Sets fd, and returns the mapping address.
> - * This funstion will also set the _ODP_ISHM_SINGLE_VA flag if the alignment
> + * This function will also set the _ODP_ISHM_SINGLE_VA flag if the alignment
>   * requires it
>   * Mutex must be assured by the caller.
>   */
> @@ -724,7 +724,7 @@ static void procsync(void)
>
> last = ishm_proctable->nb_entries;
> while (i < last) {
> -   /* if the procecess sequence number doesn't match the main
> +   /* if the process sequence number doesn't match the main
>  * table seq number, this entry is obsolete
>  */
> block = 
> &ishm_tbl->block[ishm_proctable->entry[i].block_index];
> @@ -1033,7 +1033,7 @@ static int block_free(int block_index)
>  }
>
>  /*
> - * Free and unmap internal shared memory, intentified by its block number:
> + * Free and unmap internal shared memory, identified by its block number:
>   * return -1 on error. 0 if OK.
>   */
>  int _odp_ishm_free_by_index(int block_index)
> @@ -1049,7 +1049,7 @@ int _odp_ishm_free_by_index(int block_index)
>  }
>
>  /*
> - * free and unmap internal shared memory, intentified by its block name:
> + * free and unmap internal shared memory, identified by its block name:
>   * return -1 on error. 0 if OK.
>   */
>  int _odp_ishm_free_by_name(const char *name)
> @@ -1460,8 +1460,8 @@ static int do_odp_ishm_term_local(void)
>  * Go through the table of visible blocks for this process,
>  * decreasing the refcnt of each visible blocks, and issuing
>  * warning for those no longer referenced by any process.
> -* Note that non-referenced blocks are nor freeed: this is
> -* deliberate as this would imply that the sementic of the
> +* Note that non-

Re: [lng-odp] register_notifier

[Christophe Milard]

ok for me

On 5 January 2017 at 13:19, Francois Ozog  wrote:

> Monday is my busiest day (last call ends 10pm) but we can do a morning
> call: 11amCET?
>
> On 5 January 2017 at 12:28, Christophe Milard <
> christophe.mil...@linaro.org> wrote:
>
>> Hi Francois,
>>
>> Can we have a HO on monday ( I understood today was busy for you, FF and
>> I tomorrow is a swedish national holiday). If not, we can of course take in
>> on tuesday.
>>
>> The topics I'd like to discuss are:
>> 1) change of interface: pass a struct _param_t at register time
>> and get a  handle as a result.
>> I Hoped we could simplify that, but Petri has now enforced this on the
>> south interface as well (cf buddy allocator patch) and now we have an
>> inconsistant situation: I hence suggest to take this same approach at
>> register time as well.
>>
>> 2) The callback functions dicussed above (I am not 100% following you,
>> FF).
>>
>> 3) what should be done on HW removal: do we expect enumerators to
>> "disregister"/ be removed. I am assuming a case where we have device
>> dependant enumerator
>>
>> Christophe.
>>
>> On 5 January 2017 at 11:43, Francois Ozog 
>> wrote:
>>
>>> Hi Christophe,
>>>
>>> event are not just "new device" (in that case I would agree with you).
>>> Events can be:
>>> - "attention" (before "poweroff") for a specific device to orderly
>>> shutdown before poweroff (or remove if virtio-net device in a VM).
>>> - SFP insertion/removal of a specific port
>>> - Error condition (an internal watchdog may detect packet stall or I
>>> don't know)
>>>
>>> FF
>>>
>>> On 5 January 2017 at 09:13, Christophe Milard <
>>> christophe.mil...@linaro.org> wrote:
>>>
 Hi Francois,

 Following your suggestion, the driver API contains:
  /** Register event notifier function for hotplug events:
  */
 int (*register_notifier)(void (*event_handler) (void));

 But I an now wondering: Why this double callback?

 Wouldn't it be simpler to have the functions:

 odpdrv_enum_class_probe_request(): called by a enum class when it
 needs to refresh its enumerator list.
 (ODP would eventually call the enumerator class probe function after
 recieving the probe request), ... and...

 odpdrv_enum_probe_request(): called by an enumerator when it needs to
 refresh its device list. (ODP would eventually call the enumerator
 probe function after recieving the probe request)

 Christophe

>>>
>>>
>>>
>>> --
>>> [image: Linaro] 
>>> François-Frédéric Ozog | *Director Linaro Networking Group*
>>> T: +33.67221.6485
>>> francois.o...@linaro.org | Skype: ffozog
>>>
>>>
>>
>
>
> --
> [image: Linaro] 
> François-Frédéric Ozog | *Director Linaro Networking Group*
> T: +33.67221.6485
> francois.o...@linaro.org | Skype: ffozog
>
>

Re: [lng-odp] register_notifier

[Francois Ozog]

Monday is my busiest day (last call ends 10pm) but we can do a morning
call: 11amCET?

On 5 January 2017 at 12:28, Christophe Milard 
wrote:

> Hi Francois,
>
> Can we have a HO on monday ( I understood today was busy for you, FF and I
> tomorrow is a swedish national holiday). If not, we can of course take in
> on tuesday.
>
> The topics I'd like to discuss are:
> 1) change of interface: pass a struct _param_t at register time
> and get a  handle as a result.
> I Hoped we could simplify that, but Petri has now enforced this on the
> south interface as well (cf buddy allocator patch) and now we have an
> inconsistant situation: I hence suggest to take this same approach at
> register time as well.
>
> 2) The callback functions dicussed above (I am not 100% following you, FF).
>
> 3) what should be done on HW removal: do we expect enumerators to
> "disregister"/ be removed. I am assuming a case where we have device
> dependant enumerator
>
> Christophe.
>
> On 5 January 2017 at 11:43, Francois Ozog 
> wrote:
>
>> Hi Christophe,
>>
>> event are not just "new device" (in that case I would agree with you).
>> Events can be:
>> - "attention" (before "poweroff") for a specific device to orderly
>> shutdown before poweroff (or remove if virtio-net device in a VM).
>> - SFP insertion/removal of a specific port
>> - Error condition (an internal watchdog may detect packet stall or I
>> don't know)
>>
>> FF
>>
>> On 5 January 2017 at 09:13, Christophe Milard <
>> christophe.mil...@linaro.org> wrote:
>>
>>> Hi Francois,
>>>
>>> Following your suggestion, the driver API contains:
>>>  /** Register event notifier function for hotplug events:
>>>  */
>>> int (*register_notifier)(void (*event_handler) (void));
>>>
>>> But I an now wondering: Why this double callback?
>>>
>>> Wouldn't it be simpler to have the functions:
>>>
>>> odpdrv_enum_class_probe_request(): called by a enum class when it
>>> needs to refresh its enumerator list.
>>> (ODP would eventually call the enumerator class probe function after
>>> recieving the probe request), ... and...
>>>
>>> odpdrv_enum_probe_request(): called by an enumerator when it needs to
>>> refresh its device list. (ODP would eventually call the enumerator
>>> probe function after recieving the probe request)
>>>
>>> Christophe
>>>
>>
>>
>>
>> --
>> [image: Linaro] 
>> François-Frédéric Ozog | *Director Linaro Networking Group*
>> T: +33.67221.6485
>> francois.o...@linaro.org | Skype: ffozog
>>
>>
>


-- 
[image: Linaro] 
François-Frédéric Ozog | *Director Linaro Networking Group*
T: +33.67221.6485
francois.o...@linaro.org | Skype: ffozog

Re: [lng-odp] [API-NEXT PATCHv2] linux-gen: _ishm: fixing typos

[Christophe Milard]

Ping. Should be easy to review :-)

On 21 December 2016 at 13:55, Christophe Milard
 wrote:
> Fixing a set of iritating typos. just in comments.
>
> Signed-off-by: Christophe Milard 
> ---
>
> Since V1:
> another typo found by Bill
>
>  platform/linux-generic/_ishm.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/platform/linux-generic/_ishm.c b/platform/linux-generic/_ishm.c
> index 449e357..a49c8e4 100644
> --- a/platform/linux-generic/_ishm.c
> +++ b/platform/linux-generic/_ishm.c
> @@ -75,7 +75,7 @@
>  /*
>   * Maximum number of internal shared memory blocks.
>   *
> - * This the the number of separate ISHM areas that can be reserved 
> concurrently
> + * This is the number of separate ISHM areas that can be reserved 
> concurrently
>   * (Note that freeing such blocks may take time, or possibly never happen
>   * if some of the block ownwers never procsync() after free). This number
>   * should take that into account)
> @@ -236,7 +236,7 @@ static void procsync(void);
>   * Take a piece of the preallocated virtual space to fit "size" bytes.
>   * (best fit). Size must be rounded up to an integer number of pages size.
>   * Possibly split the fragment to keep track of remaining space.
> - * Returns the allocated fragment (best_fragmnt) and the corresponding 
> address.
> + * Returns the allocated fragment (best_fragment) and the corresponding 
> address.
>   * External caller must ensure mutex before the call!
>   */
>  static void *alloc_fragment(uintptr_t size, int block_index, intptr_t align,
> @@ -282,11 +282,11 @@ static void *alloc_fragment(uintptr_t size, int 
> block_index, intptr_t align,
>
> /*
>  * if there is room between previous fragment and new one, (due to
> -* alignement requirement) then fragment (split) the space between
> +* alignment requirement) then fragment (split) the space between
>  * the end of the previous fragment and the beginning of the new one:
>  */
> if (border - (uintptr_t)(*best_fragmnt)->start > 0) {
> -   /* frangment space, i.e. take a new fragment descriptor... */
> +   /* fragment space, i.e. take a new fragment descriptor... */
> rem_fragmnt = ishm_ftbl->unused_fragmnts;
> if (!rem_fragmnt) {
> ODP_ERR("unable to get shmem fragment 
> descriptor!\n.");
> @@ -316,7 +316,7 @@ static void *alloc_fragment(uintptr_t size, int 
> block_index, intptr_t align,
> if (remainder == 0)
> return (*best_fragmnt)->start;
>
> -   /* otherwise, frangment space, i.e. take a new fragment descriptor... 
> */
> +   /* otherwise, fragment space, i.e. take a new fragment descriptor... 
> */
> rem_fragmnt = ishm_ftbl->unused_fragmnts;
> if (!rem_fragmnt) {
> ODP_ERR("unable to get shmem fragment descriptor!\n.");
> @@ -503,7 +503,7 @@ static void delete_file(ishm_block_t *block)
>   * performs the mapping, possibly allocating a fragment of the pre-reserved
>   * VA space if the _ODP_ISHM_SINGLE_VA flag was given.
>   * Sets fd, and returns the mapping address.
> - * This funstion will also set the _ODP_ISHM_SINGLE_VA flag if the alignment
> + * This function will also set the _ODP_ISHM_SINGLE_VA flag if the alignment
>   * requires it
>   * Mutex must be assured by the caller.
>   */
> @@ -724,7 +724,7 @@ static void procsync(void)
>
> last = ishm_proctable->nb_entries;
> while (i < last) {
> -   /* if the procecess sequence number doesn't match the main
> +   /* if the process sequence number doesn't match the main
>  * table seq number, this entry is obsolete
>  */
> block = 
> &ishm_tbl->block[ishm_proctable->entry[i].block_index];
> @@ -1033,7 +1033,7 @@ static int block_free(int block_index)
>  }
>
>  /*
> - * Free and unmap internal shared memory, intentified by its block number:
> + * Free and unmap internal shared memory, identified by its block number:
>   * return -1 on error. 0 if OK.
>   */
>  int _odp_ishm_free_by_index(int block_index)
> @@ -1049,7 +1049,7 @@ int _odp_ishm_free_by_index(int block_index)
>  }
>
>  /*
> - * free and unmap internal shared memory, intentified by its block name:
> + * free and unmap internal shared memory, identified by its block name:
>   * return -1 on error. 0 if OK.
>   */
>  int _odp_ishm_free_by_name(const char *name)
> @@ -1460,8 +1460,8 @@ static int do_odp_ishm_term_local(void)
>  * Go through the table of visible blocks for this process,
>  * decreasing the refcnt of each visible blocks, and issuing
>  * warning for those no longer referenced by any process.
> -* Note that non-referenced blocks are nor freeed: this is
> -* deliberate as this would imply that the sementic of the
> +* Note that no

Re: [lng-odp] register_notifier

[Christophe Milard]

Hi Francois,

Can we have a HO on monday ( I understood today was busy for you, FF and I
tomorrow is a swedish national holiday). If not, we can of course take in
on tuesday.

The topics I'd like to discuss are:
1) change of interface: pass a struct _param_t at register time and
get a  handle as a result.
I Hoped we could simplify that, but Petri has now enforced this on the
south interface as well (cf buddy allocator patch) and now we have an
inconsistant situation: I hence suggest to take this same approach at
register time as well.

2) The callback functions dicussed above (I am not 100% following you, FF).

3) what should be done on HW removal: do we expect enumerators to
"disregister"/ be removed. I am assuming a case where we have device
dependant enumerator

Christophe.

On 5 January 2017 at 11:43, Francois Ozog  wrote:

> Hi Christophe,
>
> event are not just "new device" (in that case I would agree with you).
> Events can be:
> - "attention" (before "poweroff") for a specific device to orderly
> shutdown before poweroff (or remove if virtio-net device in a VM).
> - SFP insertion/removal of a specific port
> - Error condition (an internal watchdog may detect packet stall or I don't
> know)
>
> FF
>
> On 5 January 2017 at 09:13, Christophe Milard <
> christophe.mil...@linaro.org> wrote:
>
>> Hi Francois,
>>
>> Following your suggestion, the driver API contains:
>>  /** Register event notifier function for hotplug events:
>>  */
>> int (*register_notifier)(void (*event_handler) (void));
>>
>> But I an now wondering: Why this double callback?
>>
>> Wouldn't it be simpler to have the functions:
>>
>> odpdrv_enum_class_probe_request(): called by a enum class when it
>> needs to refresh its enumerator list.
>> (ODP would eventually call the enumerator class probe function after
>> recieving the probe request), ... and...
>>
>> odpdrv_enum_probe_request(): called by an enumerator when it needs to
>> refresh its device list. (ODP would eventually call the enumerator
>> probe function after recieving the probe request)
>>
>> Christophe
>>
>
>
>
> --
> [image: Linaro] 
> François-Frédéric Ozog | *Director Linaro Networking Group*
> T: +33.67221.6485
> francois.o...@linaro.org | Skype: ffozog
>
>

Re: [lng-odp] register_notifier

[Francois Ozog]

Hi Christophe,

event are not just "new device" (in that case I would agree with you).
Events can be:
- "attention" (before "poweroff") for a specific device to orderly shutdown
before poweroff (or remove if virtio-net device in a VM).
- SFP insertion/removal of a specific port
- Error condition (an internal watchdog may detect packet stall or I don't
know)

FF

On 5 January 2017 at 09:13, Christophe Milard 
wrote:

> Hi Francois,
>
> Following your suggestion, the driver API contains:
>  /** Register event notifier function for hotplug events:
>  */
> int (*register_notifier)(void (*event_handler) (void));
>
> But I an now wondering: Why this double callback?
>
> Wouldn't it be simpler to have the functions:
>
> odpdrv_enum_class_probe_request(): called by a enum class when it
> needs to refresh its enumerator list.
> (ODP would eventually call the enumerator class probe function after
> recieving the probe request), ... and...
>
> odpdrv_enum_probe_request(): called by an enumerator when it needs to
> refresh its device list. (ODP would eventually call the enumerator
> probe function after recieving the probe request)
>
> Christophe
>



-- 
[image: Linaro] 
François-Frédéric Ozog | *Director Linaro Networking Group*
T: +33.67221.6485
francois.o...@linaro.org | Skype: ffozog

Re: [lng-odp] [API-NEXT PATCHv2 3/4] linux-gen: add generic bitmaps and iterators

[Yi He]

O, missing this mail

Hi, Maxim, for the error and warnings it seems the checkpatch.pl cannot
recognize function pointers usage in macro definition, but it is a must in
this generic programming practice. For those checks are only one space or
parenthesize, trying to make code looks more clear and easy-reading.

Can we accept these?

Thanks and best regards, Yi

On 5 January 2017 at 03:16, Maxim Uvarov  wrote:

>
> Using patch: 0003-linux-gen-add-generic-bitmaps-and-iterators.patch
>   Trying to apply patch
>   Patch applied
> WARNING: Prefer ARRAY_SIZE(array)
> #77: FILE: platform/linux-generic/include/odp_bitmap_internal.h:30:
> +#define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0]))
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #201: FILE: platform/linux-generic/include/odp_bitmap_internal.h:154:
> +#define METHOD_DECLARE(_return, _name, _impl, ...) \
> +   _return (*_name)(struct _impl ## _bitmap_ ## iterator *this)
>
> WARNING: space prohibited between function name and open parenthesis '('
> #202: FILE: platform/linux-generic/include/odp_bitmap_internal.h:155:
> +   _return (*_name)(struct _impl ## _bitmap_ ## iterator *this)
>
> CHECK: No space is necessary after a cast
> #338: FILE: platform/linux-generic/include/odp_bitmap_internal.h:291:
> +   if (start >= (int) nbits)
>
> CHECK: No space is necessary after a cast
> #349: FILE: platform/linux-generic/include/odp_bitmap_internal.h:302:
> +   if (start >= (int) nbits)
>
> CHECK: No space is necessary after a cast
> #591: FILE: platform/linux-generic/odp_bitmap.c:218:
> +   this->_nbits = (int) *(this->_base.last);
>
> CHECK: Unnecessary parentheses around this->_base.last
> #591: FILE: platform/linux-generic/odp_bitmap.c:218:
> +   this->_nbits = (int) *(this->_base.last);
>
> total: 1 errors, 2 warnings, 4 checks, 649 lines checked
>
> NOTE: Ignored message types: BIT_MACRO COMPARISON_TO_NULL
> DEPRECATED_VARIABLE NEW_TYPEDEFS SPLIT_STRING SSCANF_TO_KSTRTO
>
> 0001-linux-gen-add-generic-bitmaps-and-iterators.patch has style
> problems, please review.
>
>
>
> On 12/09/16 13:51, Yi He wrote:
> > Add C++ template alike bitmap to allow
> > instantiate bitmap data structure of any size,
> > and provide iterators to help walking through
> > the bitmap objects.
> >
> > Signed-off-by: Yi He 
> > ---
> >  platform/linux-generic/Makefile.am |   2 +
> >  .../linux-generic/include/odp_bitmap_internal.h| 320
> +
> >  platform/linux-generic/odp_bitmap.c| 315
> 
> >  3 files changed, 637 insertions(+)
> >  create mode 100644 platform/linux-generic/include/odp_bitmap_internal.h
> >  create mode 100644 platform/linux-generic/odp_bitmap.c
> >
> > diff --git a/platform/linux-generic/Makefile.am
> b/platform/linux-generic/Makefile.am
> > index adbe24d..a6e62ef 100644
> > --- a/platform/linux-generic/Makefile.am
> > +++ b/platform/linux-generic/Makefile.am
> > @@ -128,6 +128,7 @@ noinst_HEADERS = \
> > ${srcdir}/include/odp_align_internal.h \
> > ${srcdir}/include/odp_atomic_internal.h \
> > ${srcdir}/include/odp_buffer_inlines.h \
> > +   ${srcdir}/include/odp_bitmap_internal.h \
> > ${srcdir}/include/odp_buffer_internal.h \
> > ${srcdir}/include/odp_classification_datamodel.h \
> > ${srcdir}/include/odp_classification_inlines.h \
> > @@ -171,6 +172,7 @@ __LIB__libodp_linux_la_SOURCES = \
> >  _ishmphy.c \
> >  odp_atomic.c \
> >  odp_barrier.c \
> > +odp_bitmap.c \
> >  odp_buffer.c \
> >  odp_byteorder.c \
> >  odp_classification.c \
> > diff --git a/platform/linux-generic/include/odp_bitmap_internal.h
> b/platform/linux-generic/include/odp_bitmap_internal.h
> > new file mode 100644
> > index 000..192c6f9
> > --- /dev/null
> > +++ b/platform/linux-generic/include/odp_bitmap_internal.h
> > @@ -0,0 +1,320 @@
> > +/* Copyright (c) 2016, Linaro Limited
> > + * All rights reserved.
> > + *
> > + * SPDX-License-Identifier: BSD-3-Clause
> > + */
> > +
> > +/**
> > + * @file
> > + *
> > + * ODP generic bitmap types and operations.
> > + */
> > +
> > +#ifndef ODP_BITMAP_INTERNAL_H_
> > +#define ODP_BITMAP_INTERNAL_H_
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Generate unique identifier for instantiated class */
> > +#define TOKENIZE(template, line) \
> > + template ## _ ## line ## _ ## __COUNTER__
> > +
> > +/* Array size in general */
> > +#define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0]))
> > +
> > +#define BITS_PER_BYTE(8)
> > +#define BITS_PER_LONG__WORDSIZE
> > +#define BYTES_PER_LONG   (BITS_PER_LONG / BITS_PER_BYTE)
> > +

Re: [lng-odp] [API-NEXT PATCHv9 1/5] drv: adding driver registration interface (stub)

[Yi He]

I've read and run tests again and my review still stands for this v9 series:

Reviewed-and-tested-by: Yi He 

Best Regards, Yi

On 3 January 2017 at 00:09, Christophe Milard 
wrote:

> The enumerator class, enumerator instance, devio and driver registration
> functions prototypes (and a draft of their parameters) are
> defined, the goal being to define the registration framework only.
>
> Signed-off-by: Christophe Milard 
> ---
>  include/odp/drv/spec/driver.h | 293 ++
> 
>  platform/Makefile.inc |   1 +
>  2 files changed, 294 insertions(+)
>  create mode 100644 include/odp/drv/spec/driver.h
>
> diff --git a/include/odp/drv/spec/driver.h b/include/odp/drv/spec/driver.h
> new file mode 100644
> index 000..abdd656
> --- /dev/null
> +++ b/include/odp/drv/spec/driver.h
> @@ -0,0 +1,293 @@
> +/* Copyright (c) 2017, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: BSD-3-Clause
> + */
> +
> +/**
> + * @file
> + *
> + * ODPDRV driver
> + */
> +
> +#ifndef ODPDRV_DRIVER_H_
> +#define ODPDRV_DRIVER_H_
> +#include 
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> +* @addtogroup odpdrv_driver
> +* @details
> +* enumerator and driver interface to ODP
> +*
> +*  1) ODP loads the different modules (i.e. it loads shared libraries,
> *.so).
> +* In the context of drivers, shared libraries may contain enumerators,
> +* drivers and devios. These register in step 2.
> +*
> +*  2)  odpdrv_enumr_class_register(int (probe*)()...)
> +*  --->
> +*  odpdrv_driver_register(int (probe*)()...)
> +*  --->
> +*  odpdrv_devio_register()
> +*  --->
> +*  A number of device_enumerator_classes are registered at the ODP
> startup.
> +*  Many classes are expected: static, ACPI, PCI, switchdev, virtual,
> DPAA2...
> +*  A number of drivers also register to ODP (passing their own probe
> function).
> +*  A number of device IO may also register to ODP (defining available
> devices
> +*  interfaces).
> +*
> +*  3)  ODP calls the probe function of each enumerator class
> +*  <---
> +*  odpdrv_emum_register(int (probe*)()...)
> +*  --->
> +*  --->
> +*  --->
> +*  odpdrv_devio_register(...)
> +*  --->
> +*  --->
> +*  --->
> +*  ODP calls the probe function of each registered enumerator_class.
> +*  This result in the enumerator_class registering some
> +*  enumerators (instances of the class) by calling
> +*  odpdrv_emumerator_register() for each instance.
> +*  A given enumerator_class may create many enumerators based on its
> platform:
> +*  For instance Linux defines a number of PCI domains that can be viewed
> as
> +*  multiple PCI enumerators. In addition, it could be considered that
> each PCI
> +*  root of each processor socket in a NUMA environment has its own PCI
> +*  enumerator.
> +*  For enumerator class PCI, there could be one instance for each PCI
> +*  domain.
> +*  Note that the probe function of a given class may be recalled at any
> time
> +*  (Hotplug)
> +*  The devios delivered with their enumerator may also register at this
> stage.
> +*
> +*  4) For each enumerator instance, odp calls the probe function to
> retrieve
> +*  the list of devices enumerated by the given enumerator instance.
> +*  Note that the probe function of a given enumerator may be recalled at
> any
> +*  time(Hotplug)
> +*
> +*  5) The driver framework calls the drivers probe(D,I) functions of the
> +*  drivers, with device D and devio I as parameter, assuming that:
> +*  -devio I was on the driver supported list of devio (and version
> matches)
> +*  -the devio I is registered and found its enumerator interface(E)
> api
> +*   (name and version)
> +*  -device D was enumerated by an enumerator providing interface E.
> +*  The return value of the driver probe function tells whether the driver
> +*  can handle the device or not.
> +*
> +* @{
> +*/
> +
> +/* Forward declarations for a top down description of structures */
> +typedef struct odpdrv_enumr_class_t odpdrv_enumr_class_t;
> +typedef struct odpdrv_enumr_t odpdrv_enumr_t;
> +typedef struct odpdrv_enumerated_dev_t odpdrv_enumerated_dev_t;
> +typedef struct odpdrv_devio_t odpdrv_devio_t;
> +typedef struct odpdrv_driver_t odpdrv_driver_t;
> +
> +/** Maximum size for driver and enumerator names: */
> +#define ODPDRV_NAME_SIZE 32
> +
> +/** Maximum size for the enumerator depen

[lng-odp] register_notifier

[Christophe Milard]

Hi Francois,

Following your suggestion, the driver API contains:
 /** Register event notifier function for hotplug events:
 */
int (*register_notifier)(void (*event_handler) (void));

But I an now wondering: Why this double callback?

Wouldn't it be simpler to have the functions:

odpdrv_enum_class_probe_request(): called by a enum class when it
needs to refresh its enumerator list.
(ODP would eventually call the enumerator class probe function after
recieving the probe request), ... and...

odpdrv_enum_probe_request(): called by an enumerator when it needs to
refresh its device list. (ODP would eventually call the enumerator
probe function after recieving the probe request)

Christophe