diff mbox

[PATCHv2] linux-generic: odp_timer: set user_ptr for cancelled timeout

Message ID 1426088205-17923-1-git-send-email-ola.liljedahl@linaro.org
State Accepted
Commit dd48199df5e1d55c3290ce01747edc596e35bd4f
Headers show

Commit Message

Ola Liljedahl March 11, 2015, 3:36 p.m. UTC
Ensure that the timeout user_ptr and timer fields are set when the
corresponding timer is immediately cancelled.
https://bugs.linaro.org/show_bug.cgi?id=1313

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
---
(This document/code contribution attached is provided under the terms of
agreement LES-LTM-21309)

Passes odp_timer validation with the new odp_timer_cancel() test from Petri.

v2:
Updated some comments in odp_timer.c to make the meaning clearer.

 platform/linux-generic/odp_timer.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Bill Fischofer March 11, 2015, 4:54 p.m. UTC | #1
I originally was going to point out the 'typo' but my first reaction was
that if should be of rather than in.  However, on reflection it seemed that
if was what was intended.  However, I agree that this revised wording is
clearer.


On Wed, Mar 11, 2015 at 10:36 AM, Ola Liljedahl <ola.liljedahl@linaro.org>
wrote:

> Ensure that the timeout user_ptr and timer fields are set when the
> corresponding timer is immediately cancelled.
> https://bugs.linaro.org/show_bug.cgi?id=1313
>
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>

Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org>


> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
>
> Passes odp_timer validation with the new odp_timer_cancel() test from
> Petri.
>
> v2:
> Updated some comments in odp_timer.c to make the meaning clearer.
>
>  platform/linux-generic/odp_timer.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/platform/linux-generic/odp_timer.c
> b/platform/linux-generic/odp_timer.c
> index 61a02b6..b7cb04f 100644
> --- a/platform/linux-generic/odp_timer.c
> +++ b/platform/linux-generic/odp_timer.c
> @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx,
>  #endif
>         } 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) {
> +                       /* Convert from buffer to timeout hdr */
> +                       odp_timeout_hdr_t *tmo_hdr =
> +                               timeout_hdr_from_buf(*tmo_buf);
> +                       tmo_hdr->timer = tp_idx_to_handle(tp, idx);
> +                       tmo_hdr->user_ptr = tp->timers[idx].user_ptr;
> +                       /* expiration field filled in when timer expires */
> +               }
> +               /* Else ignore buffers of other types */
>                 odp_buffer_t old_buf = ODP_BUFFER_INVALID;
>  #ifdef ODP_ATOMIC_U128
>                 tick_buf_t new, old;
> @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool *tp,
> uint32_t idx, uint64_t tick)
>         _odp_atomic_flag_clear(IDX2LOCK(idx));
>  #endif
>         if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) {
> -               /* Fill in metadata fields in system timeout buffer */
> +               /* Fill in expiration tick for timeout events */
>                 if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) {
>                         /* Convert from buffer to timeout hdr */
>                         odp_timeout_hdr_t *tmo_hdr =
>                                 timeout_hdr_from_buf(tmo_buf);
> -                       tmo_hdr->timer = tp_idx_to_handle(tp, idx);
>                         tmo_hdr->expiration = exp_tck;
> -                       tmo_hdr->user_ptr = tim->user_ptr;
> +                       /* timer and user_ptr fields filled in when timer
> +                        * was set */
>                 }
> -               /* Else ignore buffers of other types */
> +               /* Else ignore events of other types */
>                 /* Post the timeout to the destination queue */
>                 int rc = odp_queue_enq(tim->queue,
>                                        odp_buffer_to_event(tmo_buf));
> --
> 1.9.1
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov March 12, 2015, 8:11 a.m. UTC | #2
Merged,
Maxim.

On 03/11/15 19:54, Bill Fischofer wrote:
> I originally was going to point out the 'typo' but my first reaction 
> was that if should be of rather than in.  However, on reflection it 
> seemed that if was what was intended.  However, I agree that this 
> revised wording is clearer.
>
>
> On Wed, Mar 11, 2015 at 10:36 AM, Ola Liljedahl 
> <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>> wrote:
>
>     Ensure that the timeout user_ptr and timer fields are set when the
>     corresponding timer is immediately cancelled.
>     https://bugs.linaro.org/show_bug.cgi?id=1313
>
>     Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org
>     <mailto:ola.liljedahl@linaro.org>>
>
>
> Reviewed-by: Bill Fischofer <bill.fischofer@linaro.org 
> <mailto:bill.fischofer@linaro.org>>
>
>     ---
>     (This document/code contribution attached is provided under the
>     terms of
>     agreement LES-LTM-21309)
>
>     Passes odp_timer validation with the new odp_timer_cancel() test
>     from Petri.
>
>     v2:
>     Updated some comments in odp_timer.c to make the meaning clearer.
>
>      platform/linux-generic/odp_timer.c | 18 ++++++++++++++----
>      1 file changed, 14 insertions(+), 4 deletions(-)
>
>     diff --git a/platform/linux-generic/odp_timer.c
>     b/platform/linux-generic/odp_timer.c
>     index 61a02b6..b7cb04f 100644
>     --- a/platform/linux-generic/odp_timer.c
>     +++ b/platform/linux-generic/odp_timer.c
>     @@ -421,6 +421,16 @@ static bool timer_reset(uint32_t idx,
>      #endif
>             } 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) {
>     +                       /* Convert from buffer to timeout hdr */
>     +                       odp_timeout_hdr_t *tmo_hdr =
>     +  timeout_hdr_from_buf(*tmo_buf);
>     +                       tmo_hdr->timer = tp_idx_to_handle(tp, idx);
>     +                       tmo_hdr->user_ptr = tp->timers[idx].user_ptr;
>     +                       /* expiration field filled in when timer
>     expires */
>     +               }
>     +               /* Else ignore buffers of other types */
>                     odp_buffer_t old_buf = ODP_BUFFER_INVALID;
>      #ifdef ODP_ATOMIC_U128
>                     tick_buf_t new, old;
>     @@ -556,16 +566,16 @@ static unsigned timer_expire(odp_timer_pool
>     *tp, uint32_t idx, uint64_t tick)
>             _odp_atomic_flag_clear(IDX2LOCK(idx));
>      #endif
>             if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) {
>     -               /* Fill in metadata fields in system timeout buffer */
>     +               /* Fill in expiration tick for timeout events */
>                     if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) {
>                             /* Convert from buffer to timeout hdr */
>                             odp_timeout_hdr_t *tmo_hdr =
>     timeout_hdr_from_buf(tmo_buf);
>     -                       tmo_hdr->timer = tp_idx_to_handle(tp, idx);
>                             tmo_hdr->expiration = exp_tck;
>     -                       tmo_hdr->user_ptr = tim->user_ptr;
>     +                       /* timer and user_ptr fields filled in
>     when timer
>     +                        * was set */
>                     }
>     -               /* Else ignore buffers of other types */
>     +               /* Else ignore events of other types */
>                     /* Post the timeout to the destination queue */
>                     int rc = odp_queue_enq(tim->queue,
>      odp_buffer_to_event(tmo_buf));
>     --
>     1.9.1
>
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto: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
diff mbox

Patch

diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
index 61a02b6..b7cb04f 100644
--- a/platform/linux-generic/odp_timer.c
+++ b/platform/linux-generic/odp_timer.c
@@ -421,6 +421,16 @@  static bool timer_reset(uint32_t idx,
 #endif
 	} 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) {
+			/* Convert from buffer to timeout hdr */
+			odp_timeout_hdr_t *tmo_hdr =
+				timeout_hdr_from_buf(*tmo_buf);
+			tmo_hdr->timer = tp_idx_to_handle(tp, idx);
+			tmo_hdr->user_ptr = tp->timers[idx].user_ptr;
+			/* expiration field filled in when timer expires */
+		}
+		/* Else ignore buffers of other types */
 		odp_buffer_t old_buf = ODP_BUFFER_INVALID;
 #ifdef ODP_ATOMIC_U128
 		tick_buf_t new, old;
@@ -556,16 +566,16 @@  static unsigned timer_expire(odp_timer_pool *tp, uint32_t idx, uint64_t tick)
 	_odp_atomic_flag_clear(IDX2LOCK(idx));
 #endif
 	if (odp_likely(tmo_buf != ODP_BUFFER_INVALID)) {
-		/* Fill in metadata fields in system timeout buffer */
+		/* Fill in expiration tick for timeout events */
 		if (_odp_buffer_type(tmo_buf) == ODP_EVENT_TIMEOUT) {
 			/* Convert from buffer to timeout hdr */
 			odp_timeout_hdr_t *tmo_hdr =
 				timeout_hdr_from_buf(tmo_buf);
-			tmo_hdr->timer = tp_idx_to_handle(tp, idx);
 			tmo_hdr->expiration = exp_tck;
-			tmo_hdr->user_ptr = tim->user_ptr;
+			/* timer and user_ptr fields filled in when timer
+			 * was set */
 		}
-		/* Else ignore buffers of other types */
+		/* Else ignore events of other types */
 		/* Post the timeout to the destination queue */
 		int rc = odp_queue_enq(tim->queue,
 				       odp_buffer_to_event(tmo_buf));