Petri this is quite well described but really should be part of the patch description (in the cover letter)? Then no need to have to ask why all this is done. I think we would like the validation suite (and everything else) to build as well... It's a bitch to change API's, I know. Even if the changes in some (more or less subjective) ways are not complete, if everything builds and runs then we know that ODP is in the same state as before the patch was applied. Cleaning up the implementation can and will always continue.
On 16 January 2015 at 10:25, Savolainen, Petri (NSN - FI/Espoo) <petri.savolai...@nsn.com> wrote: > > > > > 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 > _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org http://lists.linaro.org/mailman/listinfo/lng-odp
