Yes, agree validation needs to be updated (soon or at the same time). Because the patch set is out now, someone could have picked that up and started that work already yesterday. We just need to decide if we merge this for 0.9 and if so, who updates validation suite and how those are merged. This patch set builds and runs for the default target.
./bootstrap ./configure make -Petri > -----Original Message----- > From: ext Ola Liljedahl [mailto:ola.liljed...@linaro.org] > Sent: Friday, January 16, 2015 12:43 PM > To: Savolainen, Petri (NSN - FI/Espoo) > Cc: ext Bill Fischofer; LNG ODP Mailman List > Subject: Re: [lng-odp] [PATCH 00/15] Event introduction > > 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
