Some more observations after having looked this over in a bit more detail: This patch in its current form is basically a massive rename from buffers to events and changing things like odp_buffer_pool_t to odp_pool_t. Presumably this is intended to be followed by additional patches that surface new capabilities that build on this revised nomenclature, but for now the advantages of this change are unclear. While a patch is good, it would be really helpful to have a description of what the envisioned target looks like so that the motivation behind this rename can be better appreciated.
Unless we also get API inline support into v1.0, there will be a performance hit here since as a result of the rename it seems a lot of applications will be making frequent calls to odp_buffer_to_event() and odp_buffer_from_event() that were not needed before. There are a few oddities. Why wasn't odp_buffer_pool.c renamed to odp_pool.c since it now implements odp_pool_create(), not odp_buffer_pool_create(), etc.? There also doesn't appear to be any way to manipulate events other than by first converting them to buffers. For example, when an event is obtained from odp_schedule() it can't be freed. Instead, you do an odp_buffer_from_event() followed by an odp_buffer_free() call. This seems awkward. The odp_pool_param_t (formerly odp_buffer_pool_param_t) is also incomplete since it's not clear how its intended to be used for the various event types. For example, there is a pkt section that has some items but odp_pool_create() still only references the buf section for creating packet events. And the tmo section is null. It seems very late in v1.0 to be contemplating this large a change, especially as it doesn't seem to be fully fleshed out yet. It would seem if we did something partial now applications would still face a step function conversion when the "second half" of the change was introduced later. So I'm wondering if it might be better if this were deferred to post-v1.0 to allow time for it to be fully developed and introduced as part of a later release? I think we can introduce new concepts in a reasonably backward-compatible manner with some planning. On Thu, Jan 15, 2015 at 1:58 PM, Bill Fischofer <[email protected]> wrote: > Actually, looks like none of the unit tests are updated by this patch, so > tests/validation do not compile with this patch applied. > > On Thu, Jan 15, 2015 at 1:48 PM, Bill Fischofer <[email protected] > > wrote: > >> Initial comment: >> >> Patch series applies, however appears incomplete as >> test/validation/odp_queue.c hasn't been updated: >> >> odp_queue.c: In function ‘init_queue_suite’: >> odp_queue.c:18:2: error: unknown type name ‘odp_buffer_pool_t’ >> odp_buffer_pool_t pool; >> ^ >> odp_queue.c:19:2: error: unknown type name ‘odp_buffer_pool_param_t’ >> odp_buffer_pool_param_t params; >> ^ >> odp_queue.c:21:8: error: request for member ‘buf_size’ in something not a >> structure or union >> params.buf_size = 0; >> ^ >> odp_queue.c:22:8: error: request for member ‘buf_align’ in something not >> a structure or union >> params.buf_align = ODP_CACHE_LINE_SIZE; >> ^ >> odp_queue.c:23:8: error: request for member ‘num_bufs’ in something not a >> structure or union >> params.num_bufs = 1024 * 10; >> ^ >> odp_queue.c:24:8: error: request for member ‘buf_type’ in something not a >> structure or union >> params.buf_type = ODP_BUFFER_TYPE_RAW; >> ^ >> odp_queue.c:24:21: error: ‘ODP_BUFFER_TYPE_RAW’ undeclared (first use in >> this function) >> params.buf_type = ODP_BUFFER_TYPE_RAW; >> ^ >> odp_queue.c:24:21: note: each undeclared identifier is reported only once >> for each function it appears in >> odp_queue.c:26:2: error: implicit declaration of function >> ‘odp_buffer_pool_create’ [-Werror=implicit-function-declaration] >> pool = odp_buffer_pool_create("msg_pool", ODP_SHM_NULL, ¶ms); >> ^ >> odp_queue.c:26:2: error: nested extern declaration of >> ‘odp_buffer_pool_create’ [-Werror=nested-externs] >> odp_queue.c:28:6: error: ‘ODP_BUFFER_POOL_INVALID’ undeclared (first use >> in this function) >> if (ODP_BUFFER_POOL_INVALID == pool) { >> ^ >> odp_queue.c: In function ‘test_odp_queue_sunnyday’: >> odp_queue.c:40:2: error: unknown type name ‘odp_buffer_pool_t’ >> odp_buffer_pool_t msg_pool; >> ^ >> odp_queue.c:68:2: error: implicit declaration of function >> ‘odp_buffer_pool_lookup’ [-Werror=implicit-function-declaration] >> msg_pool = odp_buffer_pool_lookup("msg_pool"); >> ^ >> odp_queue.c:68:2: error: nested extern declaration of >> ‘odp_buffer_pool_lookup’ [-Werror=nested-externs] >> cc1: all warnings being treated as errors >> Makefile:843: recipe for target 'odp_queue.o' failed >> make[2]: *** [odp_queue.o] Error 1 >> make[2]: Leaving directory '/home/bill/linaro/events/test/validation' >> Makefile:369: recipe for target 'all-recursive' failed >> make[1]: *** [all-recursive] Error 1 >> make[1]: Leaving directory '/home/bill/linaro/events/test' >> Makefile:454: recipe for target 'all-recursive' failed >> make: *** [all-recursive] Error 1 >> bill@ubuntu:~/linaro/events$ >> >> >> On Thu, Jan 15, 2015 at 9:40 AM, Petri Savolainen < >> [email protected]> wrote: >> >>> This patches introduces odp_event_t and replaces with that the usage of >>> odp_buffer_t as the super class for other "buffer types". What used to >>> be a >>> buffer type is now an event type. >>> >>> There are some lines over 80 char, since those are caused by temporary >>> event <-> buffer, packet -> event -> buffer conversions and should be >>> cleaned up >>> from the implementation anyway. >>> >>> Petri Savolainen (15): >>> api: event: Add odp_event_t >>> api: event: odp_schedule and odp_queue_enq >>> api: event: schedule_multi and queue_enq_multi >>> api: event: odp_queue_deq >>> api: event: odp_queue_deq_multi >>> api: buffer: Removed odp_buffer_type >>> api: packet: Removed odp_packet_to_buffer >>> api: packet: Removed odp_packet_from_buffer >>> api: timer: Use odp_event_t instead of odp_buffer_t >>> api: crypto: Use odp_event_t instead of odp_buffer_t >>> linux-generic: crypto: Use packet alloc for packet >>> api: buffer_pool: Rename odp_buffer_pool.h to odp_pool.h >>> api: pool: Rename pool params and remove buffer types >>> api: pool: Rename odp_buffer_pool_ to odp_pool_ >>> api: config: Renamed ODP_CONFIG_BUFFER_POOLS >>> >>> example/generator/odp_generator.c | 38 ++--- >>> example/ipsec/odp_ipsec.c | 70 ++++---- >>> example/ipsec/odp_ipsec_cache.c | 4 +- >>> example/ipsec/odp_ipsec_cache.h | 2 +- >>> example/ipsec/odp_ipsec_loop_db.c | 2 +- >>> example/ipsec/odp_ipsec_loop_db.h | 12 +- >>> example/ipsec/odp_ipsec_stream.c | 20 +-- >>> example/ipsec/odp_ipsec_stream.h | 2 +- >>> example/l2fwd/odp_l2fwd.c | 28 +-- >>> example/packet/odp_pktio.c | 28 +-- >>> example/timer/odp_timer_test.c | 64 +++---- >>> platform/linux-generic/Makefile.am | 4 +- >>> platform/linux-generic/include/api/odp.h | 3 +- >>> platform/linux-generic/include/api/odp_buffer.h | 40 +++-- >>> .../linux-generic/include/api/odp_buffer_pool.h | 177 >>> ------------------- >>> .../linux-generic/include/api/odp_classification.h | 2 +- >>> platform/linux-generic/include/api/odp_config.h | 4 +- >>> platform/linux-generic/include/api/odp_crypto.h | 16 +- >>> platform/linux-generic/include/api/odp_event.h | 59 +++++++ >>> platform/linux-generic/include/api/odp_packet.h | 29 ++-- >>> platform/linux-generic/include/api/odp_packet_io.h | 4 +- >>> .../linux-generic/include/api/odp_platform_types.h | 10 +- >>> platform/linux-generic/include/api/odp_pool.h | 189 >>> +++++++++++++++++++++ >>> platform/linux-generic/include/api/odp_queue.h | 32 ++-- >>> platform/linux-generic/include/api/odp_schedule.h | 32 ++-- >>> platform/linux-generic/include/api/odp_timer.h | 56 +++--- >>> .../linux-generic/include/odp_buffer_inlines.h | 6 +- >>> .../linux-generic/include/odp_buffer_internal.h | 20 ++- >>> .../include/odp_buffer_pool_internal.h | 22 +-- >>> .../linux-generic/include/odp_crypto_internal.h | 2 +- >>> .../linux-generic/include/odp_packet_internal.h | 8 +- >>> platform/linux-generic/include/odp_packet_socket.h | 10 +- >>> platform/linux-generic/odp_buffer.c | 12 +- >>> platform/linux-generic/odp_buffer_pool.c | 133 +++++++-------- >>> platform/linux-generic/odp_crypto.c | 29 ++-- >>> platform/linux-generic/odp_event.c | 19 +++ >>> platform/linux-generic/odp_packet.c | 34 ++-- >>> platform/linux-generic/odp_packet_io.c | 14 +- >>> platform/linux-generic/odp_packet_socket.c | 10 +- >>> platform/linux-generic/odp_queue.c | 18 +- >>> platform/linux-generic/odp_schedule.c | 48 +++--- >>> platform/linux-generic/odp_timer.c | 35 ++-- >>> test/performance/odp_scheduling.c | 105 +++++++----- >>> 43 files changed, 806 insertions(+), 646 deletions(-) >>> delete mode 100644 platform/linux-generic/include/api/odp_buffer_pool.h >>> create mode 100644 platform/linux-generic/include/api/odp_event.h >>> create mode 100644 platform/linux-generic/include/api/odp_pool.h >>> create mode 100644 platform/linux-generic/odp_event.c >>> >>> -- >>> 2.2.2 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> [email protected] >>> http://lists.linaro.org/mailman/listinfo/lng-odp >>> >> >> >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
