From: ext Bill Fischofer [mailto:bill.fischo...@linaro.org] Sent: Thursday, January 15, 2015 11:26 PM To: Petri Savolainen Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [PATCH 00/15] Event introduction
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. This patch set introduces the new API structure. Implementation changes are minimal since it’s an implementation choice if buffer is used as the base class. Validation should be added and implementation cleaned where needed after this. As we have talked this already multiple times: - Event replaces buffer as the “base class” of things that can be transmitted over queues and scheduled - Event has minimum metadata (only event type == odp_event_type()) - Currently supported event types are: buffer, packet, timeout. Crypto completion event is likely to be the fourth one. - Buffer is just a another event (=raw buffer, no segmentation) - Benefit of the opaque base class is that different event types are more independent (e.g. do not have to implement odp_buffer_size(), odp_buffer_addr(), etc) - There are event types that do not carry data, but only metadata (e.g. timer timeout). Also in future, some event types may not be linked to a pool (user could not allocate, or ask for the pool) - Application cannot access one event type through multiple APIs (e.g. mix odp_buffer_xxx() and odp_packet_xxx() calls when accessing a packet) - Pools are not only for buffers, but for multiple event types (buffers, packets, timeouts, etc) 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. Performance is exactly the same as earlier. There are same number of handle conversions for packet, timeouts, etc (other than raw buffers). The conversion is still most likely a cast. I added some extra conversion into implementation to limit implementation changes (but performance of two type casts instead of one is the same). ev = odp_schedule(); pkt = odp_packet_from_event(ev) vs. buf = odp_schedule(); pkt = odp_packet_from_buffer(buf); 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. Since .c is implementation. I did minimal implementation change. Others can continue with implementation clean ups where needed. odp_event_free(odp_event_t* ev) can be added, but it would be mostly useful when application wants to blindly drop an event (without checking its type first). Event <-> buffer conversion is the feature. It promotes usage of correct types when accessing the event. Also (linux-generic) implementation should enforce type correctness and report a build (or run time) error if user tries to e.g. do odp_buffer_addr(event) or odp_packet_data(buf). 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’s incomplete to minimize implementation changes at this point. Those packet (and timeout) pool parametesr have to be defined and implementation changed accordingly. This type change enabled per event/pool type parameters, but does not implement those yet (== does not change the current pool param functionality). 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. Since it’s a conceptual and non-backward compatible change (“all events are buffers” vs “all evensts are events”), we should make the API change before 1.0. Implementations can be cleaned up under the covers anyway. From API point of view the missing things for v1.0 are: - Complete pool params - Introduce crypto completion event type (that seems to be on the way already) - Add odp_event_free() if needed (maybe not for 1.0) Two weeks should be enough for those. -Petri
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp
