| Message ID | 556AAF47.2030700@linaro.org |
|---|---|
| State | New |
| Headers | show |
If I understand your patch correctly, a timeout event is really a buffer with the timeout-specific header stored just after the buffer header (as before). You don't change the definition of odp_timeout_hdr_t and the implementation of odp_timeout_hdr(odp_buffer_t buf) has just moved from odp_timer_internal.h to odp_timer.c where is renamed to timeout_hdr_from_buf(). There is also a new function timeout_hdr(odp_timeout_t tmo) which helps with the conversion from timeout to timeout header. So you essential change seems to make the timeout event handle the same as a buffer event handle and not necessarily a pointer (as in the original code). I don't see why this is an important change (from a portability or structural point of view), can you please explain this? -- Ola On 31 May 2015 at 08:50, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 05/29/2015 07:26 AM, Ola Liljedahl wrote: > > On 13 May 2015 at 17:31, Ola Liljedahl <ola.liljedahl@linaro.org > > <mailto:ola.liljedahl@linaro.org>> wrote: > > > > On 13 May 2015 at 12:19, Maxim Uvarov <maxim.uvarov@linaro.org > > <mailto:maxim.uvarov@linaro.org>> wrote: > > > > Patch looks good. Validation test passed. Need one more review. > > > > Thanks. I did this primarily for KS2 being able to use the > > linux-generic timer implementation without any changes. I am waiting > > for Taras to verify this. > > > > Well it seems now that Taras is not going to verify that this patch > > solves his problems on KS2. I like the patch anyway and would like to > > see it merged. Any reviewers? > > Sorry for a long delay. > I had a slightly different approach in mind. > > ----8<----------------------------------------------------------------- > From: Taras Kondratiuk <taras.kondratiuk@linaro.org> > Date: Sat, 30 May 2015 23:26:19 -0700 > Subject: [PATCH] linux-generic: timer: convert odp_timeout_t to event > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > .../linux-generic/include/odp/plat/timer_types.h | 8 ++++-- > .../linux-generic/include/odp_timer_internal.h | 8 ------ > platform/linux-generic/odp_timer.c | 31 > ++++++++++++---------- > 3 files changed, 23 insertions(+), 24 deletions(-) > > diff --git a/platform/linux-generic/include/odp/plat/timer_types.h > b/platform/linux-generic/include/odp/plat/timer_types.h > index 006683e..d987096 100644 > --- a/platform/linux-generic/include/odp/plat/timer_types.h > +++ b/platform/linux-generic/include/odp/plat/timer_types.h > @@ -18,6 +18,10 @@ > extern "C" { > #endif > > +#include <odp/std_types.h> > +#include <odp/plat/strong_types.h> > +#include <odp/plat/event_types.h> > + > /** @addtogroup odp_timer > * @{ > **/ > @@ -32,9 +36,9 @@ typedef uint32_t odp_timer_t; > > #define ODP_TIMER_INVALID ((uint32_t)~0U) > > -typedef void *odp_timeout_t; > +typedef odp_handle_t odp_timeout_t; > > -#define ODP_TIMEOUT_INVALID NULL > +#define ODP_TIMEOUT_INVALID (odp_timeout_t)ODP_EVENT_INVALID > > /** > * @} > diff --git a/platform/linux-generic/include/odp_timer_internal.h > b/platform/linux-generic/include/odp_timer_internal.h > index 90af62c..db379ec 100644 > --- a/platform/linux-generic/include/odp_timer_internal.h > +++ b/platform/linux-generic/include/odp_timer_internal.h > @@ -40,12 +40,4 @@ typedef struct odp_timeout_hdr_stride { > } odp_timeout_hdr_stride; > > > -/** > - * Return the timeout header > - */ > -static inline odp_timeout_hdr_t *odp_timeout_hdr(odp_buffer_t buf) > -{ > - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); > -} > - > #endif > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index b7cb04f..39e76e1 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -80,7 +80,13 @@ static _odp_atomic_flag_t locks[NUM_LOCKS]; /* Multiple > locks per cache line! */ > > static odp_timeout_hdr_t *timeout_hdr_from_buf(odp_buffer_t buf) > { > - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); > + return (odp_timeout_hdr_t *)(void *)odp_buf_to_hdr(buf); > +} > + > +static odp_timeout_hdr_t *timeout_hdr(odp_timeout_t tmo) > +{ > + odp_buffer_t buf = > odp_buffer_from_event(odp_timeout_to_event(tmo)); > + return timeout_hdr_from_buf(buf); > } > > > /****************************************************************************** > @@ -422,7 +428,8 @@ static bool timer_reset(uint32_t idx, > } else { > /* We have a new timeout buffer which replaces any old one > */ > /* Fill in some (constant) header fields for timeout > events */ > - if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { > + if (odp_event_type(odp_buffer_to_event(*tmo_buf)) == > + ODP_EVENT_TIMEOUT) { > /* Convert from buffer to timeout hdr */ > odp_timeout_hdr_t *tmo_hdr = > timeout_hdr_from_buf(*tmo_buf); > @@ -567,7 +574,8 @@ static unsigned timer_expire(odp_timer_pool *tp, > uint32_t idx, uint64_t tick) > #endif > if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { > /* Fill in expiration tick for timeout events */ > - if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { > + if (odp_event_type(odp_buffer_to_event(tmo_buf)) == > + ODP_EVENT_TIMEOUT) { > /* Convert from buffer to timeout hdr */ > odp_timeout_hdr_t *tmo_hdr = > timeout_hdr_from_buf(tmo_buf); > @@ -811,19 +819,17 @@ odp_timeout_t odp_timeout_from_event(odp_event_t ev) > /* This check not mandated by the API specification */ > if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) > ODP_ABORT("Event not a timeout"); > - return > (odp_timeout_t)timeout_hdr_from_buf(odp_buffer_from_event(ev)); > + return (odp_timeout_t)ev; > } > > odp_event_t odp_timeout_to_event(odp_timeout_t tmo) > { > - odp_timeout_hdr_t *tmo_hdr = (odp_timeout_hdr_t *)tmo; > - odp_buffer_t buf = odp_hdr_to_buf(&tmo_hdr->buf_hdr); > - return odp_buffer_to_event(buf); > + return (odp_event_t)tmo; > } > > int odp_timeout_fresh(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > + const odp_timeout_hdr_t *hdr = timeout_hdr(tmo); > odp_timer_t hdl = hdr->timer; > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > @@ -836,20 +842,17 @@ int odp_timeout_fresh(odp_timeout_t tmo) > > odp_timer_t odp_timeout_timer(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->timer; > + return timeout_hdr(tmo)->timer; > } > > uint64_t odp_timeout_tick(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->expiration; > + return timeout_hdr(tmo)->expiration; > } > > void *odp_timeout_user_ptr(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->user_ptr; > + return timeout_hdr(tmo)->user_ptr; > } > > odp_timeout_t odp_timeout_alloc(odp_pool_t pool) > -- > 1.9.1 > >
Hi Ola, Maybe I'm wrong, but let me try to explain change in question. The main reason in this change is the lack of possibility to convert buf_hdr to buf_handle. The function odp_timeout_to_event requires this conversion. By and large it's not required the tmo_handle to be the tmo_header for linux-generic. It can be tmo_handle = buf_handle. In this case odp_timeout_to_event can avoid buf_hdr -> buf_handle conversion. After what it can be reused for keystone or other platforms that avoid buffer header to handle conversion. The second reason is function _odp_buffer_type(), On linux-generic buffer header holds a field for event type, in Keysonte, the h/w descriptor contains field for event type. So there is no reason to save it twice, in h/w descriptor and in buffer header. In keystone h/w descriptor = event handle. Holding event type in event is more logical. And Taras, probably, wanted to avoid implementing it's own _odp_buffet_type and replaced it on odp_event_type. Why this is problematic to implement on Kestone platform? It's not a problem, it's redundancy. From what I see, buf_handle = h/w descriptor pointer. H/w descriptor contains enough fields to hold buffer information, that's why actual buffer header is empty for now. It doesn't hold handle and event type, so it cannot be easily converted. Currently, there is no reason it this, especially if timeout handle can be equal to buffer handle. Am I right, Taras? But it's not a problem to add link on handle to buffer header, but why? On 01.06.15 15:12, Ola Liljedahl wrote: > If I understand your patch correctly, a timeout event is really a buffer > with the timeout-specific header stored just after the buffer header (as > before). You don't change the definition of odp_timeout_hdr_t and the > implementation of odp_timeout_hdr(odp_buffer_t buf) has just moved from > odp_timer_internal.h to odp_timer.c where is renamed > to timeout_hdr_from_buf(). There is also a new > function timeout_hdr(odp_timeout_t tmo) which helps with the conversion > from timeout to timeout header. > > So you essential change seems to make the timeout event handle the same > as a buffer event handle and not necessarily a pointer (as in the > original code). I don't see why this is an important change (from a > portability or structural point of view), can you please explain this? > > -- Ola > > > > On 31 May 2015 at 08:50, Taras Kondratiuk <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>> wrote: > > On 05/29/2015 07:26 AM, Ola Liljedahl wrote: > > On 13 May 2015 at 17:31, Ola Liljedahl <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org> > > <mailto:ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>>> wrote: > > > > On 13 May 2015 at 12:19, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > > <mailto:maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>>> wrote: > > > > Patch looks good. Validation test passed. Need one more review. > > > > Thanks. I did this primarily for KS2 being able to use the > > linux-generic timer implementation without any changes. I am waiting > > for Taras to verify this. > > > > Well it seems now that Taras is not going to verify that this patch > > solves his problems on KS2. I like the patch anyway and would like to > > see it merged. Any reviewers? > > Sorry for a long delay. > I had a slightly different approach in mind. > > ----8<----------------------------------------------------------------- > From: Taras Kondratiuk <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>> > Date: Sat, 30 May 2015 23:26:19 -0700 > Subject: [PATCH] linux-generic: timer: convert odp_timeout_t to event > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>> > --- > .../linux-generic/include/odp/plat/timer_types.h | 8 ++++-- > .../linux-generic/include/odp_timer_internal.h | 8 ------ > platform/linux-generic/odp_timer.c | 31 > ++++++++++++---------- > 3 files changed, 23 insertions(+), 24 deletions(-) > > diff --git a/platform/linux-generic/include/odp/plat/timer_types.h > b/platform/linux-generic/include/odp/plat/timer_types.h > index 006683e..d987096 100644 > --- a/platform/linux-generic/include/odp/plat/timer_types.h > +++ b/platform/linux-generic/include/odp/plat/timer_types.h > @@ -18,6 +18,10 @@ > extern "C" { > #endif > > +#include <odp/std_types.h> > +#include <odp/plat/strong_types.h> > +#include <odp/plat/event_types.h> > + > /** @addtogroup odp_timer > * @{ > **/ > @@ -32,9 +36,9 @@ typedef uint32_t odp_timer_t; > > #define ODP_TIMER_INVALID ((uint32_t)~0U) > > -typedef void *odp_timeout_t; > +typedef odp_handle_t odp_timeout_t; > > -#define ODP_TIMEOUT_INVALID NULL > +#define ODP_TIMEOUT_INVALID (odp_timeout_t)ODP_EVENT_INVALID > > /** > * @} > diff --git a/platform/linux-generic/include/odp_timer_internal.h > b/platform/linux-generic/include/odp_timer_internal.h > index 90af62c..db379ec 100644 > --- a/platform/linux-generic/include/odp_timer_internal.h > +++ b/platform/linux-generic/include/odp_timer_internal.h > @@ -40,12 +40,4 @@ typedef struct odp_timeout_hdr_stride { > } odp_timeout_hdr_stride; > > > -/** > - * Return the timeout header > - */ > -static inline odp_timeout_hdr_t *odp_timeout_hdr(odp_buffer_t buf) > -{ > - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); > -} > - > #endif > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index b7cb04f..39e76e1 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -80,7 +80,13 @@ static _odp_atomic_flag_t locks[NUM_LOCKS]; /* > Multiple locks per cache line! */ > > static odp_timeout_hdr_t *timeout_hdr_from_buf(odp_buffer_t buf) > { > - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); > + return (odp_timeout_hdr_t *)(void *)odp_buf_to_hdr(buf); > +} > + > +static odp_timeout_hdr_t *timeout_hdr(odp_timeout_t tmo) > +{ > + odp_buffer_t buf = > odp_buffer_from_event(odp_timeout_to_event(tmo)); > + return timeout_hdr_from_buf(buf); > } > > /****************************************************************************** > @@ -422,7 +428,8 @@ static bool timer_reset(uint32_t idx, > } else { > /* We have a new timeout buffer which replaces any > old one */ > /* Fill in some (constant) header fields for > timeout events */ > - if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { > + if (odp_event_type(odp_buffer_to_event(*tmo_buf)) == > + ODP_EVENT_TIMEOUT) { > /* Convert from buffer to timeout hdr */ > odp_timeout_hdr_t *tmo_hdr = > timeout_hdr_from_buf(*tmo_buf); > @@ -567,7 +574,8 @@ static unsigned timer_expire(odp_timer_pool *tp, > uint32_t idx, uint64_t tick) > #endif > if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { > /* Fill in expiration tick for timeout events */ > - if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { > + if (odp_event_type(odp_buffer_to_event(tmo_buf)) == > + ODP_EVENT_TIMEOUT) { > /* Convert from buffer to timeout hdr */ > odp_timeout_hdr_t *tmo_hdr = > timeout_hdr_from_buf(tmo_buf); > @@ -811,19 +819,17 @@ odp_timeout_t > odp_timeout_from_event(odp_event_t ev) > /* This check not mandated by the API specification */ > if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) > ODP_ABORT("Event not a timeout"); > - return > (odp_timeout_t)timeout_hdr_from_buf(odp_buffer_from_event(ev)); > + return (odp_timeout_t)ev; > } > > odp_event_t odp_timeout_to_event(odp_timeout_t tmo) > { > - odp_timeout_hdr_t *tmo_hdr = (odp_timeout_hdr_t *)tmo; > - odp_buffer_t buf = odp_hdr_to_buf(&tmo_hdr->buf_hdr); > - return odp_buffer_to_event(buf); > + return (odp_event_t)tmo; > } > > int odp_timeout_fresh(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > + const odp_timeout_hdr_t *hdr = timeout_hdr(tmo); > odp_timer_t hdl = hdr->timer; > odp_timer_pool *tp = handle_to_tp(hdl); > uint32_t idx = handle_to_idx(hdl, tp); > @@ -836,20 +842,17 @@ int odp_timeout_fresh(odp_timeout_t tmo) > > odp_timer_t odp_timeout_timer(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->timer; > + return timeout_hdr(tmo)->timer; > } > > uint64_t odp_timeout_tick(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->expiration; > + return timeout_hdr(tmo)->expiration; > } > > void *odp_timeout_user_ptr(odp_timeout_t tmo) > { > - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; > - return hdr->user_ptr; > + return timeout_hdr(tmo)->user_ptr; > } > > odp_timeout_t odp_timeout_alloc(odp_pool_t pool) > -- > 1.9.1 > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
diff --git a/platform/linux-generic/include/odp/plat/timer_types.h b/platform/linux-generic/include/odp/plat/timer_types.h index 006683e..d987096 100644 --- a/platform/linux-generic/include/odp/plat/timer_types.h +++ b/platform/linux-generic/include/odp/plat/timer_types.h @@ -18,6 +18,10 @@ extern "C" { #endif +#include <odp/std_types.h> +#include <odp/plat/strong_types.h> +#include <odp/plat/event_types.h> + /** @addtogroup odp_timer * @{ **/ @@ -32,9 +36,9 @@ typedef uint32_t odp_timer_t; #define ODP_TIMER_INVALID ((uint32_t)~0U) -typedef void *odp_timeout_t; +typedef odp_handle_t odp_timeout_t; -#define ODP_TIMEOUT_INVALID NULL +#define ODP_TIMEOUT_INVALID (odp_timeout_t)ODP_EVENT_INVALID /** * @} diff --git a/platform/linux-generic/include/odp_timer_internal.h b/platform/linux-generic/include/odp_timer_internal.h index 90af62c..db379ec 100644 --- a/platform/linux-generic/include/odp_timer_internal.h +++ b/platform/linux-generic/include/odp_timer_internal.h @@ -40,12 +40,4 @@ typedef struct odp_timeout_hdr_stride { } odp_timeout_hdr_stride; -/** - * Return the timeout header - */ -static inline odp_timeout_hdr_t *odp_timeout_hdr(odp_buffer_t buf) -{ - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); -} - #endif diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index b7cb04f..39e76e1 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -80,7 +80,13 @@ static _odp_atomic_flag_t locks[NUM_LOCKS]; /* Multiple locks per cache line! */ static odp_timeout_hdr_t *timeout_hdr_from_buf(odp_buffer_t buf) { - return (odp_timeout_hdr_t *)odp_buf_to_hdr(buf); + return (odp_timeout_hdr_t *)(void *)odp_buf_to_hdr(buf); +} + +static odp_timeout_hdr_t *timeout_hdr(odp_timeout_t tmo) +{ + odp_buffer_t buf = odp_buffer_from_event(odp_timeout_to_event(tmo)); + return timeout_hdr_from_buf(buf); } /****************************************************************************** @@ -422,7 +428,8 @@ static bool timer_reset(uint32_t idx, } else { /* We have a new timeout buffer which replaces any old one */ /* Fill in some (constant) header fields for timeout events */ - if (_odp_buffer_type(*tmo_buf) == ODP_EVENT_TIMEOUT) { + if (odp_event_type(odp_buffer_to_event(*tmo_buf)) == + ODP_EVENT_TIMEOUT) { /* Convert from buffer to timeout hdr */ odp_timeout_hdr_t *tmo_hdr = timeout_hdr_from_buf(*tmo_buf); @@ -567,7 +574,8 @@ static unsigned timer_expire(odp_timer_pool *tp, uint32_t idx, uint64_t tick) #endif if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) { /* Fill in expiration tick for timeout events */ - if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) { + if (odp_event_type(odp_buffer_to_event(tmo_buf)) == + ODP_EVENT_TIMEOUT) { /* Convert from buffer to timeout hdr */ odp_timeout_hdr_t *tmo_hdr = timeout_hdr_from_buf(tmo_buf); @@ -811,19 +819,17 @@ odp_timeout_t odp_timeout_from_event(odp_event_t ev) /* This check not mandated by the API specification */ if (odp_event_type(ev) != ODP_EVENT_TIMEOUT) ODP_ABORT("Event not a timeout"); - return (odp_timeout_t)timeout_hdr_from_buf(odp_buffer_from_event(ev)); + return (odp_timeout_t)ev; } odp_event_t odp_timeout_to_event(odp_timeout_t tmo) { - odp_timeout_hdr_t *tmo_hdr = (odp_timeout_hdr_t *)tmo; - odp_buffer_t buf = odp_hdr_to_buf(&tmo_hdr->buf_hdr); - return odp_buffer_to_event(buf); + return (odp_event_t)tmo; } int odp_timeout_fresh(odp_timeout_t tmo) { - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; + const odp_timeout_hdr_t *hdr = timeout_hdr(tmo); odp_timer_t hdl = hdr->timer; odp_timer_pool *tp = handle_to_tp(hdl); uint32_t idx = handle_to_idx(hdl, tp); @@ -836,20 +842,17 @@ int odp_timeout_fresh(odp_timeout_t tmo) odp_timer_t odp_timeout_timer(odp_timeout_t tmo) { - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; - return hdr->timer; + return timeout_hdr(tmo)->timer; } uint64_t odp_timeout_tick(odp_timeout_t tmo) { - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; - return hdr->expiration; + return timeout_hdr(tmo)->expiration; } void *odp_timeout_user_ptr(odp_timeout_t tmo) { - const odp_timeout_hdr_t *hdr = (odp_timeout_hdr_t *)tmo; - return hdr->user_ptr; + return timeout_hdr(tmo)->user_ptr; } odp_timeout_t odp_timeout_alloc(odp_pool_t pool)