[ODP/PATCHi,v2,1/2] timer:add cancel_tmo function

From: santosh shukla <santosh.shukla@linaro.org>

Signed-off-by: santosh shukla <santosh.shukla@linaro.org>
---
v2 change:
- added find and delete function so to delete only
  tmo not the entire list.

 platform/linux-generic/source/odp_timer.c |   62 ++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 2 deletions(-)

Hi,


On Thursday, 27 March 2014 22:20:04 UTC+2, Santosh Shukla wrote:

> From: santosh shukla <santosh...@linaro.org <javascript:>> 
>
> Signed-off-by: santosh shukla <santosh...@linaro.org <javascript:>> 
> --- 
> v2 change: 
> - added find and delete function so to delete only 
>   tmo not the entire list. 
>
>  platform/linux-generic/source/odp_timer.c |   62 
> ++++++++++++++++++++++++++++- 
>  1 file changed, 60 insertions(+), 2 deletions(-) 
>
> diff --git a/platform/linux-generic/source/odp_timer.c 
> b/platform/linux-generic/source/odp_timer.c 
> index 4bcc479..ce405a5 100644 
> --- a/platform/linux-generic/source/odp_timer.c 
> +++ b/platform/linux-generic/source/odp_timer.c 
> @@ -91,6 +91,66 @@ static timeout_t *rem_tmo(tick_t *tick) 
>          return tmo; 
>  } 
>   
> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle) 
> +{ 
> +        timeout_t *del; 
> + 
> +        for (; tmo; tmo = tmo->next) { 
>

Crashes on tmo->tmo_buf read, if tmo is not found from the list (or list is 
empty)
 

> +                if (tmo->tmo_buf == handle) 
> +                        break; 
> +        } 
> + 
> +        if (!tmo) { 
> +                ODP_ERR("Couldn't find the tmo handle (%d)\n", handle); 
> +                return -1; 
> +        } 
> + 
> +        del = tmo; 
> +        tmo = del->next ;


prev->next = tmo->next missing, which breaks the list...
 

>
> +        odp_buffer_free(del->tmo_buf); 
>

This leaks memory, since it does not free user proved buffer (if there's 
one). Actually, that buffer should be returned back to the user (who can 
decide to free or keep it).
 

> + 
> +        return 0; 
> +} 
> + 
> + 
> +/** 
> + * Cancel a timeout 
> + * 
> + * @param timer Timer 
> + * @param tmo   Timeout to cancel 
> + * 
> + * @return 0 if successful 
> + */


Doxygen documentation must be defined only once - in the header file, not 
here.
 

>
> +int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo) 
> +{ 
> +        int id; 
> +        uint64_t abs_tick; 
> +        timeout_t *new_tmo;


It's not new anymore... so naming is not accurate.
 

>
> +        tick_t *tick; 
> + 
> +        /* get id */ 
> +        id = timer - 1; 
> + 
> +        /* get tmo_buf to cancel */ 
> +        new_tmo = (timeout_t *)odp_buffer_addr(tmo); 
> +        new_tmo->tmo_tick = 0; /* reset tmo */


It's a race, if you modify tmo data before it's removed from the timer ring 
(notify function may have already processed it).
 

>
> +        abs_tick = new_tmo->tick;  /* get the absolute 
> +                                        tick setted by prev add_tmo call 
> */ 
> + 
> +        tick = &odp_timer.timer[id].tick[abs_tick]; 
>

tmo->tick is index, not absolute time. The functionality is OK, comment and 
variable naming are misleading.

-Petri
 

> + 
> +        odp_spinlock_lock(&tick->lock); 
> + 
> +        /* search and delete tmo from tick list */ 
> +        if (find_and_del_tmo(tick->list, tmo) != 0) {

+                ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx 
> %lu\n", id, tmo, abs_tick); 
> +                odp_spinlock_unlock(&tick->lock); 
> +                return -1; 
> +        } 
> +        odp_spinlock_unlock(&tick->lock); 
> + 
> +        return 0; 
> +} 
>   
>   
>  static void notify_function(union sigval sigval) 
> @@ -167,8 +227,6 @@ int odp_timer_init_global(void) 
>                  odp_spinlock_init(&odp_timer.timer[0].tick[i].lock); 
>   
>          timer_init(); 
> - 
> - 
>          return 0; 
>  } 
>   
> -- 
> 1.7.9.5 
>
>

On 28 March 2014 00:53, Petri Savolainen <petri.savolainen@linaro.org> wrote:
> Hi,
>
>
> On Thursday, 27 March 2014 22:20:04 UTC+2, Santosh Shukla wrote:
>>
>> From: santosh shukla <santosh...@linaro.org>
>>
>> Signed-off-by: santosh shukla <santosh...@linaro.org>
>> ---
>> v2 change:
>> - added find and delete function so to delete only
>>   tmo not the entire list.
>>
>>  platform/linux-generic/source/odp_timer.c |   62
>> ++++++++++++++++++++++++++++-
>>  1 file changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/platform/linux-generic/source/odp_timer.c
>> b/platform/linux-generic/source/odp_timer.c
>> index 4bcc479..ce405a5 100644
>> --- a/platform/linux-generic/source/odp_timer.c
>> +++ b/platform/linux-generic/source/odp_timer.c
>> @@ -91,6 +91,66 @@ static timeout_t *rem_tmo(tick_t *tick)
>>          return tmo;
>>  }
>>
>> +static int find_and_del_tmo(timeout_t *tmo, odp_timer_tmo_t handle)
>> +{
>> +        timeout_t *del;
>> +
>> +        for (; tmo; tmo = tmo->next) {
>
>
> Crashes on tmo->tmo_buf read, if tmo is not found from the list (or list is
> empty)

if list empty then for loop likely to exit right.. and subsequent tmo
check lead to exit from function..
>
>>
>> +                if (tmo->tmo_buf == handle)
>> +                        break;
>> +        }
>> +
>> +        if (!tmo) {
>> +                ODP_ERR("Couldn't find the tmo handle (%d)\n", handle);
>> +                return -1;
>> +        }
>> +
>> +        del = tmo;
>> +        tmo = del->next ;
>
>
> prev->next = tmo->next missing, which breaks the list...
>

perhaps I should dump some test func result in commit log so to prove
correctness..
>>
>>
>> +        odp_buffer_free(del->tmo_buf);
>
>
> This leaks memory, since it does not free user proved buffer (if there's
> one). Actually, that buffer should be returned back to the user (who can
> decide to free or keep it).
>

User proved buffer?? which one,, is it tmo_buf or buf? I am confused
on understanding real use case for odp_buffer_t buf in timout_t {
structure.. are you saying that this buf may leak..if so then who
fills this buffer while arming.. and whats proposed method to free the
buffer?
>>
>> +
>> +        return 0;
>> +}
>> +
>> +
>> +/**
>> + * Cancel a timeout
>> + *
>> + * @param timer Timer
>> + * @param tmo   Timeout to cancel
>> + *
>> + * @return 0 if successful
>> + */
>
>
> Doxygen documentation must be defined only once - in the header file, not
> here.
>
>>
>>
>> +int odp_timer_cancel_tmo(odp_timer_t timer, odp_timer_tmo_t tmo)
>> +{
>> +        int id;
>> +        uint64_t abs_tick;
>> +        timeout_t *new_tmo;
>
>
> It's not new anymore... so naming is not accurate.
>
>>
>>
>> +        tick_t *tick;
>> +
>> +        /* get id */
>> +        id = timer - 1;
>> +
>> +        /* get tmo_buf to cancel */
>> +        new_tmo = (timeout_t *)odp_buffer_addr(tmo);
>> +        new_tmo->tmo_tick = 0; /* reset tmo */
>
>
> It's a race, if you modify tmo data before it's removed from the timer ring
> (notify function may have already processed it).
>
>>
>>
>> +        abs_tick = new_tmo->tick;  /* get the absolute
>> +                                        tick setted by prev add_tmo call
>> */
>> +
>> +        tick = &odp_timer.timer[id].tick[abs_tick];
>
>
> tmo->tick is index, not absolute time. The functionality is OK, comment and
> variable naming are misleading.
>
> -Petri
>
>>
>> +
>> +        odp_spinlock_lock(&tick->lock);
>> +
>> +        /* search and delete tmo from tick list */
>> +        if (find_and_del_tmo(tick->list, tmo) != 0) {
>>
>> +                ODP_ERR("cancel failed for tim id %d tmo id :%d tick idx
>> %lu\n", id, tmo, abs_tick);
>> +                odp_spinlock_unlock(&tick->lock);
>> +                return -1;
>> +        }
>> +        odp_spinlock_unlock(&tick->lock);
>> +
>> +        return 0;
>> +}
>>
>>
>>  static void notify_function(union sigval sigval)
>> @@ -167,8 +227,6 @@ int odp_timer_init_global(void)
>>                  odp_spinlock_init(&odp_timer.timer[0].tick[i].lock);
>>
>>          timer_init();
>> -
>> -
>>          return 0;
>>  }
>>
>> --
>> 1.7.9.5
>>
>