We definitely need the validation tests to be updated as part of the initial patch, otherwise we have a patch that results in a non-buildable system, which violates our agreed-to patch rules.
Given that Anders' API restructure patch is at a more complete state and is needed to support API inlining in a portable manner, I suggest that we integrate that patch first and then rebase this one (in a complete form) on it. This would also ease the conversion issues for other implementations since this change would then ride on the platform-independent API structure that the former patch introduces. On Fri, Jan 16, 2015 at 4:52 AM, Savolainen, Petri (NSN - FI/Espoo) < [email protected]> wrote: > 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:[email protected]] > > 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) > > <[email protected]> wrote: > > > > > > > > > > > > > > > From: ext Bill Fischofer [mailto:[email protected]] > > > 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 > > > [email protected] > > > http://lists.linaro.org/mailman/listinfo/lng-odp > > > >
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
