diff mbox

[ODP/PATCH] implement odp_timer_disarm_all()

Message ID 1397063130-10525-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov April 9, 2014, 5:05 p.m. UTC
Implement function to disarm all timers. Needed in case of
normal exit from application.
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/include/odp_internal.h |  1 +
 platform/linux-generic/source/odp_timer.c     | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Ola Liljedahl April 9, 2014, 7:38 p.m. UTC | #1
I don't understand. Isn't the ODP timer implementation a part of the
application (at least in linux-generic)? So if the application terminates,
all the Linux interval/per-process timers used by the ODP timer
implementation are removed by the kernel? I can't see anything on the man
page of timer_create() that the application has to reset these timers
before/when exiting.

And if you really want to disarm all timers why are you stopping if you
fail resetting one? Ignore the return value and just keep resetting them.
No return value, who is going to check that and what should the caller do
if this function calls an error? Print "sorry failed to reset timer, won't
exit"?
And shouldn't you install this function as an atexit() handler?



On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> Implement function to disarm all timers. Needed in case of
> normal exit from application.
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/include/odp_internal.h |  1 +
>  platform/linux-generic/source/odp_timer.c     | 24
> ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/platform/linux-generic/include/odp_internal.h
> b/platform/linux-generic/include/odp_internal.h
> index fb3be79..9b0769e 100644
> --- a/platform/linux-generic/include/odp_internal.h
> +++ b/platform/linux-generic/include/odp_internal.h
> @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>  int odp_schedule_init_local(void);
>
>  int odp_timer_init_global(void);
> +int odp_timer_disarm_all(void);
>
>  #ifdef __cplusplus
>  }
> diff --git a/platform/linux-generic/source/odp_timer.c
> b/platform/linux-generic/source/odp_timer.c
> index 6fb5025..98ffde3 100644
> --- a/platform/linux-generic/source/odp_timer.c
> +++ b/platform/linux-generic/source/odp_timer.c
> @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>         return 0;
>  }
>
> +int odp_timer_disarm_all(void)
> +{
> +       int timers;
> +       struct itimerspec ispec;
> +
> +       timers = odp_atomic_load_int(&odp_timer.num_timers);
> +
> +       ispec.it_interval.tv_sec  = 0;
> +       ispec.it_interval.tv_nsec = 0;
> +       ispec.it_value.tv_sec     = 0;
> +       ispec.it_value.tv_nsec    = 0;
> +
> +       for (; timers >= 0; timers--) {
> +               if (timer_settime(odp_timer.timer[timers].timerid,
> +                                 0, &ispec, NULL)) {
> +                       ODP_DBG("Timer reset failed\n");
> +                       return -1;
> +               }
> +               odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> +       }
> +
> +       return 0;
> +}
> +
>  odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
>                              uint64_t resolution, uint64_t min_tmo,
>                              uint64_t max_tmo)
> --
> 1.8.5.1.163.gd7aced9
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov April 10, 2014, 6:44 a.m. UTC | #2
On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> I don't understand. Isn't the ODP timer implementation a part of the 
> application (at least in linux-generic)? So if the application 
> terminates, all the Linux interval/per-process timers used by the ODP 
> timer implementation are removed by the kernel? I can't see anything 
> on the man page of timer_create() that the application has to reset 
> these timers before/when exiting.

Ola, I found that when did odp-snort. Sort uses DAQ (data acquisition 
library) as external library .so. It is used like plug-in.  I.e. snort 
calls dlopen("daq_odp.so"). daq_odp.so has implemented init(), 
shutdown() functions. On init() I call odp initialization which arms 
timer for timer_handler. Then I do some packet processing and press 
Ctrl-C. Snort calls shutdown(), then dlclose("daq_odp.so"), then prints 
some statistics about the packets. After dlclose() reference to 
timer_handler does not exist and timer arms on some unknown address. 
That leads to segfault.
>
> And if you really want to disarm all timers why are you stopping if 
> you fail resetting one? Ignore the return value and just keep 
> resetting them. No return value, who is going to check that and what 
> should the caller do if this function calls an error? Print "sorry 
> failed to reset timer, won't exit"?
Maybe it's better to use assert() there? Just if application was not 
able to cancel timer then we do something wrong and should crash there? 
Actually it is bug if timer was triggered and we can not cancel it.

 From my initial idea I thought to call odp_timer_disarm_all() several 
times until all timers will be canceled (no error code). I don't know 
but maybe there might be some timers which can't be stopped right now 
and after delay canceling is possible.

How do you think what is better here?
> And shouldn't you install this function as an atexit() handler?
>
For current odp examples it's not needed. Due to timers will be removed 
as program died. But we should think in future about some 
odp_global_shutdown()/odp_thread_shutdown() functions to free all 
allocated resources. Which in some cases might be useful.

Best regards,
Maxim.
>
> On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     Implement function to disarm all timers. Needed in case of
>     normal exit from application.
>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>>
>     ---
>      platform/linux-generic/include/odp_internal.h |  1 +
>      platform/linux-generic/source/odp_timer.c     | 24
>     ++++++++++++++++++++++++
>      2 files changed, 25 insertions(+)
>
>     diff --git a/platform/linux-generic/include/odp_internal.h
>     b/platform/linux-generic/include/odp_internal.h
>     index fb3be79..9b0769e 100644
>     --- a/platform/linux-generic/include/odp_internal.h
>     +++ b/platform/linux-generic/include/odp_internal.h
>     @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>      int odp_schedule_init_local(void);
>
>      int odp_timer_init_global(void);
>     +int odp_timer_disarm_all(void);
>
>      #ifdef __cplusplus
>      }
>     diff --git a/platform/linux-generic/source/odp_timer.c
>     b/platform/linux-generic/source/odp_timer.c
>     index 6fb5025..98ffde3 100644
>     --- a/platform/linux-generic/source/odp_timer.c
>     +++ b/platform/linux-generic/source/odp_timer.c
>     @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>             return 0;
>      }
>
>     +int odp_timer_disarm_all(void)
>     +{
>     +       int timers;
>     +       struct itimerspec ispec;
>     +
>     +       timers = odp_atomic_load_int(&odp_timer.num_timers);
>     +
>     +       ispec.it_interval.tv_sec  = 0;
>     +       ispec.it_interval.tv_nsec = 0;
>     +       ispec.it_value.tv_sec     = 0;
>     +       ispec.it_value.tv_nsec    = 0;
>     +
>     +       for (; timers >= 0; timers--) {
>     +               if (timer_settime(odp_timer.timer[timers].timerid,
>     +                                 0, &ispec, NULL)) {
>     +                       ODP_DBG("Timer reset failed\n");
>     +                       return -1;
>     +               }
>     + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>     +       }
>     +
>     +       return 0;
>     +}
>     +
>      odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t
>     pool,
>                                  uint64_t resolution, uint64_t min_tmo,
>                                  uint64_t max_tmo)
>     --
>     1.8.5.1.163.gd7aced9
>
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Savolainen, Petri (NSN - FI/Espoo) April 10, 2014, 7:08 a.m. UTC | #3
Hi,


If you want to delete a timer then prototype should be:

int odp_timer_delete(odp_timer_t timer);

Application knows which timers it have created, so no need to timer_delete_all API (that could be done in exit though, but that's implementation not API).

And you should call timer_delete() instead of only stopping the tick. And you should also free all other resources as well, like tmo (and potential user) buffers.

So, patch needs to be reworked pretty much.


-Petri


> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
> Sent: Thursday, April 10, 2014 9:44 AM
> To: Ola Liljedahl
> Cc: lng-odp-forward
> Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
> 
> On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> > I don't understand. Isn't the ODP timer implementation a part of the
> > application (at least in linux-generic)? So if the application
> > terminates, all the Linux interval/per-process timers used by the ODP
> > timer implementation are removed by the kernel? I can't see anything
> > on the man page of timer_create() that the application has to reset
> > these timers before/when exiting.
> 
> Ola, I found that when did odp-snort. Sort uses DAQ (data acquisition
> library) as external library .so. It is used like plug-in.  I.e. snort
> calls dlopen("daq_odp.so"). daq_odp.so has implemented init(),
> shutdown() functions. On init() I call odp initialization which arms
> timer for timer_handler. Then I do some packet processing and press
> Ctrl-C. Snort calls shutdown(), then dlclose("daq_odp.so"), then prints
> some statistics about the packets. After dlclose() reference to
> timer_handler does not exist and timer arms on some unknown address.
> That leads to segfault.
> >
> > And if you really want to disarm all timers why are you stopping if
> > you fail resetting one? Ignore the return value and just keep
> > resetting them. No return value, who is going to check that and what
> > should the caller do if this function calls an error? Print "sorry
> > failed to reset timer, won't exit"?
> Maybe it's better to use assert() there? Just if application was not
> able to cancel timer then we do something wrong and should crash there?
> Actually it is bug if timer was triggered and we can not cancel it.
> 
>  From my initial idea I thought to call odp_timer_disarm_all() several
> times until all timers will be canceled (no error code). I don't know
> but maybe there might be some timers which can't be stopped right now
> and after delay canceling is possible.
> 
> How do you think what is better here?
> > And shouldn't you install this function as an atexit() handler?
> >
> For current odp examples it's not needed. Due to timers will be removed
> as program died. But we should think in future about some
> odp_global_shutdown()/odp_thread_shutdown() functions to free all
> allocated resources. Which in some cases might be useful.
> 
> Best regards,
> Maxim.
> >
> > On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org
> > <mailto:maxim.uvarov@linaro.org>> wrote:
> >
> >     Implement function to disarm all timers. Needed in case of
> >     normal exit from application.
> >     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
> >     <mailto:maxim.uvarov@linaro.org>>
> >     ---
> >      platform/linux-generic/include/odp_internal.h |  1 +
> >      platform/linux-generic/source/odp_timer.c     | 24
> >     ++++++++++++++++++++++++
> >      2 files changed, 25 insertions(+)
> >
> >     diff --git a/platform/linux-generic/include/odp_internal.h
> >     b/platform/linux-generic/include/odp_internal.h
> >     index fb3be79..9b0769e 100644
> >     --- a/platform/linux-generic/include/odp_internal.h
> >     +++ b/platform/linux-generic/include/odp_internal.h
> >     @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> >      int odp_schedule_init_local(void);
> >
> >      int odp_timer_init_global(void);
> >     +int odp_timer_disarm_all(void);
> >
> >      #ifdef __cplusplus
> >      }
> >     diff --git a/platform/linux-generic/source/odp_timer.c
> >     b/platform/linux-generic/source/odp_timer.c
> >     index 6fb5025..98ffde3 100644
> >     --- a/platform/linux-generic/source/odp_timer.c
> >     +++ b/platform/linux-generic/source/odp_timer.c
> >     @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> >             return 0;
> >      }
> >
> >     +int odp_timer_disarm_all(void)
> >     +{
> >     +       int timers;
> >     +       struct itimerspec ispec;
> >     +
> >     +       timers = odp_atomic_load_int(&odp_timer.num_timers);
> >     +
> >     +       ispec.it_interval.tv_sec  = 0;
> >     +       ispec.it_interval.tv_nsec = 0;
> >     +       ispec.it_value.tv_sec     = 0;
> >     +       ispec.it_value.tv_nsec    = 0;
> >     +
> >     +       for (; timers >= 0; timers--) {
> >     +               if (timer_settime(odp_timer.timer[timers].timerid,
> >     +                                 0, &ispec, NULL)) {
> >     +                       ODP_DBG("Timer reset failed\n");
> >     +                       return -1;
> >     +               }
> >     + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> >     +       }
> >     +
> >     +       return 0;
> >     +}
> >     +
> >      odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t
> >     pool,
> >                                  uint64_t resolution, uint64_t
> min_tmo,
> >                                  uint64_t max_tmo)
> >     --
> >     1.8.5.1.163.gd7aced9
> >
> >
> >     _______________________________________________
> >     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
Maxim Uvarov April 10, 2014, 8:14 a.m. UTC | #4
On 04/10/2014 11:08 AM, Savolainen, Petri (NSN - FI/Espoo) wrote:
> Hi,
>
>
> If you want to delete a timer then prototype should be:
>
> int odp_timer_delete(odp_timer_t timer);
>
> Application knows which timers it have created, so no need to timer_delete_all API (that could be done in exit though, but that's implementation not API).
In my case application does not arm timer. odp_init_global() arms timer 
which needed to be deleted. So I need opposite function to 
odp_init_global() and not clean up what is seg faults my application.

>
> And you should call timer_delete() instead of only stopping the tick. And you should also free all other resources as well, like tmo (and potential user) buffers.
>
> So, patch needs to be reworked pretty much.
Ok. Agree. Lets define good function to delete timer. I will update the 
patch.

Maxim.
>
> -Petri
>
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
>> Sent: Thursday, April 10, 2014 9:44 AM
>> To: Ola Liljedahl
>> Cc: lng-odp-forward
>> Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>>
>> On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>>> I don't understand. Isn't the ODP timer implementation a part of the
>>> application (at least in linux-generic)? So if the application
>>> terminates, all the Linux interval/per-process timers used by the ODP
>>> timer implementation are removed by the kernel? I can't see anything
>>> on the man page of timer_create() that the application has to reset
>>> these timers before/when exiting.
>> Ola, I found that when did odp-snort. Sort uses DAQ (data acquisition
>> library) as external library .so. It is used like plug-in.  I.e. snort
>> calls dlopen("daq_odp.so"). daq_odp.so has implemented init(),
>> shutdown() functions. On init() I call odp initialization which arms
>> timer for timer_handler. Then I do some packet processing and press
>> Ctrl-C. Snort calls shutdown(), then dlclose("daq_odp.so"), then prints
>> some statistics about the packets. After dlclose() reference to
>> timer_handler does not exist and timer arms on some unknown address.
>> That leads to segfault.
>>> And if you really want to disarm all timers why are you stopping if
>>> you fail resetting one? Ignore the return value and just keep
>>> resetting them. No return value, who is going to check that and what
>>> should the caller do if this function calls an error? Print "sorry
>>> failed to reset timer, won't exit"?
>> Maybe it's better to use assert() there? Just if application was not
>> able to cancel timer then we do something wrong and should crash there?
>> Actually it is bug if timer was triggered and we can not cancel it.
>>
>>   From my initial idea I thought to call odp_timer_disarm_all() several
>> times until all timers will be canceled (no error code). I don't know
>> but maybe there might be some timers which can't be stopped right now
>> and after delay canceling is possible.
>>
>> How do you think what is better here?
>>> And shouldn't you install this function as an atexit() handler?
>>>
>> For current odp examples it's not needed. Due to timers will be removed
>> as program died. But we should think in future about some
>> odp_global_shutdown()/odp_thread_shutdown() functions to free all
>> allocated resources. Which in some cases might be useful.
>>
>> Best regards,
>> Maxim.
>>> On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org
>>> <mailto:maxim.uvarov@linaro.org>> wrote:
>>>
>>>      Implement function to disarm all timers. Needed in case of
>>>      normal exit from application.
>>>      Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>>      <mailto:maxim.uvarov@linaro.org>>
>>>      ---
>>>       platform/linux-generic/include/odp_internal.h |  1 +
>>>       platform/linux-generic/source/odp_timer.c     | 24
>>>      ++++++++++++++++++++++++
>>>       2 files changed, 25 insertions(+)
>>>
>>>      diff --git a/platform/linux-generic/include/odp_internal.h
>>>      b/platform/linux-generic/include/odp_internal.h
>>>      index fb3be79..9b0769e 100644
>>>      --- a/platform/linux-generic/include/odp_internal.h
>>>      +++ b/platform/linux-generic/include/odp_internal.h
>>>      @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>>>       int odp_schedule_init_local(void);
>>>
>>>       int odp_timer_init_global(void);
>>>      +int odp_timer_disarm_all(void);
>>>
>>>       #ifdef __cplusplus
>>>       }
>>>      diff --git a/platform/linux-generic/source/odp_timer.c
>>>      b/platform/linux-generic/source/odp_timer.c
>>>      index 6fb5025..98ffde3 100644
>>>      --- a/platform/linux-generic/source/odp_timer.c
>>>      +++ b/platform/linux-generic/source/odp_timer.c
>>>      @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>>>              return 0;
>>>       }
>>>
>>>      +int odp_timer_disarm_all(void)
>>>      +{
>>>      +       int timers;
>>>      +       struct itimerspec ispec;
>>>      +
>>>      +       timers = odp_atomic_load_int(&odp_timer.num_timers);
>>>      +
>>>      +       ispec.it_interval.tv_sec  = 0;
>>>      +       ispec.it_interval.tv_nsec = 0;
>>>      +       ispec.it_value.tv_sec     = 0;
>>>      +       ispec.it_value.tv_nsec    = 0;
>>>      +
>>>      +       for (; timers >= 0; timers--) {
>>>      +               if (timer_settime(odp_timer.timer[timers].timerid,
>>>      +                                 0, &ispec, NULL)) {
>>>      +                       ODP_DBG("Timer reset failed\n");
>>>      +                       return -1;
>>>      +               }
>>>      + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>>>      +       }
>>>      +
>>>      +       return 0;
>>>      +}
>>>      +
>>>       odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t
>>>      pool,
>>>                                   uint64_t resolution, uint64_t
>> min_tmo,
>>>                                   uint64_t max_tmo)
>>>      --
>>>      1.8.5.1.163.gd7aced9
>>>
>>>
>>>      _______________________________________________
>>>      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
Ola Liljedahl April 10, 2014, 8:50 a.m. UTC | #5
So you are not really killing the application, only unloading a shared
library (the application will continue to execute for a while). This is a
different situation. Of course when unloading a shared library, you must
first free all resources used by that shared library. If the DAQ library
uses ODP resources (e.g. timers), they must be freed in the DAQ shutdown
function.

As Petri says, for each odp_create_timer call made by the module (the DAQ
library in this case), there must be an associated odp_remove_timer call
which frees all the resources allocated for that timer. If the timer is
valid, any errors when freeing Linux resources could be considered fatal
errors so just fprintf(stderr) and call abort(). Something is seriously
wrong if you can't free a Linux per-process timer that was earlier
allocated and associated for this ODP timer. This would be a bug that you
would normally like to investigate and correct. Thus abort() is the proper
way to terminate the execution.

There is actually a general learning here. We need functions for freeing
different ODP resources (buffer pools, queues, timers etc) because ODP can
be used in e.g. dynamically loaded libraries which are unloaded without the
application process terminating. The ODP timers used Linux per-process
timers to not freeing these resources lead to an immediate crash. Other ODP
resources might only be using memory but not freeing that memory would be a
memory leak and those are also bad.

Is it possible for ODP itself to keep track of all allocated resources and
free all of them using one call? (E.g. some shutdown call as you suggest
below). This would be more robust than trusting the user to free each
individual resource.

On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>
>> I don't understand. Isn't the ODP timer implementation a part of the
>> application (at least in linux-generic)? So if the application terminates,
>> all the Linux interval/per-process timers used by the ODP timer
>> implementation are removed by the kernel? I can't see anything on the man
>> page of timer_create() that the application has to reset these timers
>> before/when exiting.
>>
>
> Ola, I found that when did odp-snort. Sort uses DAQ (data acquisition
> library) as external library .so. It is used like plug-in.  I.e. snort
> calls dlopen("daq_odp.so"). daq_odp.so has implemented init(), shutdown()
> functions. On init() I call odp initialization which arms timer for
> timer_handler. Then I do some packet processing and press Ctrl-C. Snort
> calls shutdown(), then dlclose("daq_odp.so"), then prints some statistics
> about the packets. After dlclose() reference to timer_handler does not
> exist and timer arms on some unknown address. That leads to segfault.
>
>>
>> And if you really want to disarm all timers why are you stopping if you
>> fail resetting one? Ignore the return value and just keep resetting them.
>> No return value, who is going to check that and what should the caller do
>> if this function calls an error? Print "sorry failed to reset timer, won't
>> exit"?
>>
> Maybe it's better to use assert() there? Just if application was not able
> to cancel timer then we do something wrong and should crash there? Actually
> it is bug if timer was triggered and we can not cancel it.
>
> From my initial idea I thought to call odp_timer_disarm_all() several
> times until all timers will be canceled (no error code). I don't know but
> maybe there might be some timers which can't be stopped right now and after
> delay canceling is possible.
>
> How do you think what is better here?
>
>> And shouldn't you install this function as an atexit() handler?
>>
>>  For current odp examples it's not needed. Due to timers will be removed
> as program died. But we should think in future about some
> odp_global_shutdown()/odp_thread_shutdown() functions to free all
> allocated resources. Which in some cases might be useful.
>
> Best regards,
> Maxim.
>
>>
>> On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:
>> maxim.uvarov@linaro.org>> wrote:
>>
>>     Implement function to disarm all timers. Needed in case of
>>     normal exit from application.
>>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>     <mailto:maxim.uvarov@linaro.org>>
>>     ---
>>      platform/linux-generic/include/odp_internal.h |  1 +
>>      platform/linux-generic/source/odp_timer.c     | 24
>>     ++++++++++++++++++++++++
>>      2 files changed, 25 insertions(+)
>>
>>     diff --git a/platform/linux-generic/include/odp_internal.h
>>     b/platform/linux-generic/include/odp_internal.h
>>     index fb3be79..9b0769e 100644
>>     --- a/platform/linux-generic/include/odp_internal.h
>>     +++ b/platform/linux-generic/include/odp_internal.h
>>     @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>>      int odp_schedule_init_local(void);
>>
>>      int odp_timer_init_global(void);
>>     +int odp_timer_disarm_all(void);
>>
>>      #ifdef __cplusplus
>>      }
>>     diff --git a/platform/linux-generic/source/odp_timer.c
>>     b/platform/linux-generic/source/odp_timer.c
>>     index 6fb5025..98ffde3 100644
>>     --- a/platform/linux-generic/source/odp_timer.c
>>     +++ b/platform/linux-generic/source/odp_timer.c
>>     @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>>             return 0;
>>      }
>>
>>     +int odp_timer_disarm_all(void)
>>     +{
>>     +       int timers;
>>     +       struct itimerspec ispec;
>>     +
>>     +       timers = odp_atomic_load_int(&odp_timer.num_timers);
>>     +
>>     +       ispec.it_interval.tv_sec  = 0;
>>     +       ispec.it_interval.tv_nsec = 0;
>>     +       ispec.it_value.tv_sec     = 0;
>>     +       ispec.it_value.tv_nsec    = 0;
>>     +
>>     +       for (; timers >= 0; timers--) {
>>     +               if (timer_settime(odp_timer.timer[timers].timerid,
>>     +                                 0, &ispec, NULL)) {
>>     +                       ODP_DBG("Timer reset failed\n");
>>     +                       return -1;
>>     +               }
>>     + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>>     +       }
>>     +
>>     +       return 0;
>>     +}
>>     +
>>      odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t
>>     pool,
>>                                  uint64_t resolution, uint64_t min_tmo,
>>                                  uint64_t max_tmo)
>>     --
>>     1.8.5.1.163.gd7aced9
>>
>>
>>     _______________________________________________
>>     lng-odp mailing list
>>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>
Balasubramanian Manoharan April 10, 2014, 8:55 a.m. UTC | #6
Just to clarify, the behavior of this odp_timer_delete() call this will be
similar to shutting down of a timer.
Basically if an application issues odp_timer_delete() then he will have to
call timer_init_global() again in-order to start the timer.

In this case we can rename the API as odp_timer_shutdown()
And as  Ola is suggesting, we can have a Odp_shutdown_global() which can
internally call shutdown of different modules in the system.

Regards,
Bala


On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> So you are not really killing the application, only unloading a shared
> library (the application will continue to execute for a while). This is a
> different situation. Of course when unloading a shared library, you must
> first free all resources used by that shared library. If the DAQ library
> uses ODP resources (e.g. timers), they must be freed in the DAQ shutdown
> function.
>
> As Petri says, for each odp_create_timer call made by the module (the DAQ
> library in this case), there must be an associated odp_remove_timer call
> which frees all the resources allocated for that timer. If the timer is
> valid, any errors when freeing Linux resources could be considered fatal
> errors so just fprintf(stderr) and call abort(). Something is seriously
> wrong if you can't free a Linux per-process timer that was earlier
> allocated and associated for this ODP timer. This would be a bug that you
> would normally like to investigate and correct. Thus abort() is the proper
> way to terminate the execution.
>
> There is actually a general learning here. We need functions for freeing
> different ODP resources (buffer pools, queues, timers etc) because ODP can
> be used in e.g. dynamically loaded libraries which are unloaded without the
> application process terminating. The ODP timers used Linux per-process
> timers to not freeing these resources lead to an immediate crash. Other ODP
> resources might only be using memory but not freeing that memory would be a
> memory leak and those are also bad.
>
> Is it possible for ODP itself to keep track of all allocated resources and
> free all of them using one call? (E.g. some shutdown call as you suggest
> below). This would be more robust than trusting the user to free each
> individual resource.
>
> On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>> On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>>
>>> I don't understand. Isn't the ODP timer implementation a part of the
>>> application (at least in linux-generic)? So if the application terminates,
>>> all the Linux interval/per-process timers used by the ODP timer
>>> implementation are removed by the kernel? I can't see anything on the man
>>> page of timer_create() that the application has to reset these timers
>>> before/when exiting.
>>>
>>
>> Ola, I found that when did odp-snort. Sort uses DAQ (data acquisition
>> library) as external library .so. It is used like plug-in.  I.e. snort
>> calls dlopen("daq_odp.so"). daq_odp.so has implemented init(), shutdown()
>> functions. On init() I call odp initialization which arms timer for
>> timer_handler. Then I do some packet processing and press Ctrl-C. Snort
>> calls shutdown(), then dlclose("daq_odp.so"), then prints some statistics
>> about the packets. After dlclose() reference to timer_handler does not
>> exist and timer arms on some unknown address. That leads to segfault.
>>
>>>
>>> And if you really want to disarm all timers why are you stopping if you
>>> fail resetting one? Ignore the return value and just keep resetting them.
>>> No return value, who is going to check that and what should the caller do
>>> if this function calls an error? Print "sorry failed to reset timer, won't
>>> exit"?
>>>
>> Maybe it's better to use assert() there? Just if application was not able
>> to cancel timer then we do something wrong and should crash there? Actually
>> it is bug if timer was triggered and we can not cancel it.
>>
>> From my initial idea I thought to call odp_timer_disarm_all() several
>> times until all timers will be canceled (no error code). I don't know but
>> maybe there might be some timers which can't be stopped right now and after
>> delay canceling is possible.
>>
>> How do you think what is better here?
>>
>>> And shouldn't you install this function as an atexit() handler?
>>>
>>>  For current odp examples it's not needed. Due to timers will be removed
>> as program died. But we should think in future about some
>> odp_global_shutdown()/odp_thread_shutdown() functions to free all
>> allocated resources. Which in some cases might be useful.
>>
>> Best regards,
>> Maxim.
>>
>>>
>>> On 9 April 2014 19:05, Maxim Uvarov <maxim.uvarov@linaro.org <mailto:
>>> maxim.uvarov@linaro.org>> wrote:
>>>
>>>     Implement function to disarm all timers. Needed in case of
>>>     normal exit from application.
>>>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>>     <mailto:maxim.uvarov@linaro.org>>
>>>     ---
>>>      platform/linux-generic/include/odp_internal.h |  1 +
>>>      platform/linux-generic/source/odp_timer.c     | 24
>>>     ++++++++++++++++++++++++
>>>      2 files changed, 25 insertions(+)
>>>
>>>     diff --git a/platform/linux-generic/include/odp_internal.h
>>>     b/platform/linux-generic/include/odp_internal.h
>>>     index fb3be79..9b0769e 100644
>>>     --- a/platform/linux-generic/include/odp_internal.h
>>>     +++ b/platform/linux-generic/include/odp_internal.h
>>>     @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>>>      int odp_schedule_init_local(void);
>>>
>>>      int odp_timer_init_global(void);
>>>     +int odp_timer_disarm_all(void);
>>>
>>>      #ifdef __cplusplus
>>>      }
>>>     diff --git a/platform/linux-generic/source/odp_timer.c
>>>     b/platform/linux-generic/source/odp_timer.c
>>>     index 6fb5025..98ffde3 100644
>>>     --- a/platform/linux-generic/source/odp_timer.c
>>>     +++ b/platform/linux-generic/source/odp_timer.c
>>>     @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>>>             return 0;
>>>      }
>>>
>>>     +int odp_timer_disarm_all(void)
>>>     +{
>>>     +       int timers;
>>>     +       struct itimerspec ispec;
>>>     +
>>>     +       timers = odp_atomic_load_int(&odp_timer.num_timers);
>>>     +
>>>     +       ispec.it_interval.tv_sec  = 0;
>>>     +       ispec.it_interval.tv_nsec = 0;
>>>     +       ispec.it_value.tv_sec     = 0;
>>>     +       ispec.it_value.tv_nsec    = 0;
>>>     +
>>>     +       for (; timers >= 0; timers--) {
>>>     +               if (timer_settime(odp_timer.timer[timers].timerid,
>>>     +                                 0, &ispec, NULL)) {
>>>     +                       ODP_DBG("Timer reset failed\n");
>>>     +                       return -1;
>>>     +               }
>>>     + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>>>     +       }
>>>     +
>>>     +       return 0;
>>>     +}
>>>     +
>>>      odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t
>>>     pool,
>>>                                  uint64_t resolution, uint64_t min_tmo,
>>>                                  uint64_t max_tmo)
>>>     --
>>>     1.8.5.1.163.gd7aced9
>>>
>>>
>>>     _______________________________________________
>>>     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
>
>
Maxim Uvarov April 10, 2014, 8:59 a.m. UTC | #7
btw, do we always need timer tick? Maybe to post some flags to 
odp_init_global() to not init timers if they are not needed?

Maxim.

On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> Just to clarify, the behavior of this odp_timer_delete() call this 
> will be similar to shutting down of a timer.
> Basically if an application issues odp_timer_delete() then he will 
> have to call timer_init_global() again in-order to start the timer.
>
> In this case we can rename the API as odp_timer_shutdown()
> And as  Ola is suggesting, we can have a Odp_shutdown_global() which 
> can internally call shutdown of different modules in the system.
>
> Regards,
> Bala
>
>
> On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org 
> <mailto:ola.liljedahl@linaro.org>> wrote:
>
>     So you are not really killing the application, only unloading a
>     shared library (the application will continue to execute for a
>     while). This is a different situation. Of course when unloading a
>     shared library, you must first free all resources used by that
>     shared library. If the DAQ library uses ODP resources (e.g.
>     timers), they must be freed in the DAQ shutdown function.
>
>     As Petri says, for each odp_create_timer call made by the module
>     (the DAQ library in this case), there must be an associated
>     odp_remove_timer call which frees all the resources allocated for
>     that timer. If the timer is valid, any errors when freeing Linux
>     resources could be considered fatal errors so just fprintf(stderr)
>     and call abort(). Something is seriously wrong if you can't free a
>     Linux per-process timer that was earlier allocated and associated
>     for this ODP timer. This would be a bug that you would normally
>     like to investigate and correct. Thus abort() is the proper way to
>     terminate the execution.
>
>     There is actually a general learning here. We need functions for
>     freeing different ODP resources (buffer pools, queues, timers etc)
>     because ODP can be used in e.g. dynamically loaded libraries which
>     are unloaded without the application process terminating. The ODP
>     timers used Linux per-process timers to not freeing these
>     resources lead to an immediate crash. Other ODP resources might
>     only be using memory but not freeing that memory would be a memory
>     leak and those are also bad.
>
>     Is it possible for ODP itself to keep track of all allocated
>     resources and free all of them using one call? (E.g. some shutdown
>     call as you suggest below). This would be more robust than
>     trusting the user to free each individual resource.
>
>     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>> wrote:
>
>         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>
>             I don't understand. Isn't the ODP timer implementation a
>             part of the application (at least in linux-generic)? So if
>             the application terminates, all the Linux
>             interval/per-process timers used by the ODP timer
>             implementation are removed by the kernel? I can't see
>             anything on the man page of timer_create() that the
>             application has to reset these timers before/when exiting.
>
>
>         Ola, I found that when did odp-snort. Sort uses DAQ (data
>         acquisition library) as external library .so. It is used like
>         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
>         has implemented init(), shutdown() functions. On init() I call
>         odp initialization which arms timer for timer_handler. Then I
>         do some packet processing and press Ctrl-C. Snort calls
>         shutdown(), then dlclose("daq_odp.so"), then prints some
>         statistics about the packets. After dlclose() reference to
>         timer_handler does not exist and timer arms on some unknown
>         address. That leads to segfault.
>
>
>             And if you really want to disarm all timers why are you
>             stopping if you fail resetting one? Ignore the return
>             value and just keep resetting them. No return value, who
>             is going to check that and what should the caller do if
>             this function calls an error? Print "sorry failed to reset
>             timer, won't exit"?
>
>         Maybe it's better to use assert() there? Just if application
>         was not able to cancel timer then we do something wrong and
>         should crash there? Actually it is bug if timer was triggered
>         and we can not cancel it.
>
>         From my initial idea I thought to call odp_timer_disarm_all()
>         several times until all timers will be canceled (no error
>         code). I don't know but maybe there might be some timers which
>         can't be stopped right now and after delay canceling is possible.
>
>         How do you think what is better here?
>
>             And shouldn't you install this function as an atexit()
>             handler?
>
>         For current odp examples it's not needed. Due to timers will
>         be removed as program died. But we should think in future
>         about some odp_global_shutdown()/odp_thread_shutdown()
>         functions to free all allocated resources. Which in some cases
>         might be useful.
>
>         Best regards,
>         Maxim.
>
>
>             On 9 April 2014 19:05, Maxim Uvarov
>             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>             <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>> wrote:
>
>                 Implement function to disarm all timers. Needed in case of
>                 normal exit from application.
>                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>
>                 <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>>
>                 ---
>                  platform/linux-generic/include/odp_internal.h |  1 +
>                  platform/linux-generic/source/odp_timer.c     | 24
>                 ++++++++++++++++++++++++
>                  2 files changed, 25 insertions(+)
>
>                 diff --git a/platform/linux-generic/include/odp_internal.h
>                 b/platform/linux-generic/include/odp_internal.h
>                 index fb3be79..9b0769e 100644
>                 --- a/platform/linux-generic/include/odp_internal.h
>                 +++ b/platform/linux-generic/include/odp_internal.h
>                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>                  int odp_schedule_init_local(void);
>
>                  int odp_timer_init_global(void);
>                 +int odp_timer_disarm_all(void);
>
>                  #ifdef __cplusplus
>                  }
>                 diff --git a/platform/linux-generic/source/odp_timer.c
>                 b/platform/linux-generic/source/odp_timer.c
>                 index 6fb5025..98ffde3 100644
>                 --- a/platform/linux-generic/source/odp_timer.c
>                 +++ b/platform/linux-generic/source/odp_timer.c
>                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>                         return 0;
>                  }
>
>                 +int odp_timer_disarm_all(void)
>                 +{
>                 +       int timers;
>                 +       struct itimerspec ispec;
>                 +
>                 +       timers =
>             odp_atomic_load_int(&odp_timer.num_timers);
>                 +
>                 +       ispec.it_interval.tv_sec  = 0;
>                 +       ispec.it_interval.tv_nsec = 0;
>                 +       ispec.it_value.tv_sec     = 0;
>                 +       ispec.it_value.tv_nsec    = 0;
>                 +
>                 +       for (; timers >= 0; timers--) {
>                 +               if
>             (timer_settime(odp_timer.timer[timers].timerid,
>                 +                                 0, &ispec, NULL)) {
>                 +                       ODP_DBG("Timer reset failed\n");
>                 +                       return -1;
>                 +               }
>                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>                 +       }
>                 +
>                 +       return 0;
>                 +}
>                 +
>                  odp_timer_t odp_timer_create(const char *name,
>             odp_buffer_pool_t
>                 pool,
>                                              uint64_t resolution,
>             uint64_t min_tmo,
>                                              uint64_t max_tmo)
>                 --
>                 1.8.5.1.163.gd7aced9
>
>
>                 _______________________________________________
>                 lng-odp mailing list
>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>             <mailto: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 <mailto:lng-odp@lists.linaro.org>
>     http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Balasubramanian Manoharan April 10, 2014, 9:10 a.m. UTC | #8
We can split the odp_timer_create() into two APIs one to configure the
timer odp_timer_configure() and another to start the timer
odp_timer_start(), the reason being when an Application is arming the timer
we need to get the current tick of the timer so that we can calculate after
how many ticks this timer needs to be fired.

If we decide to start the timer, during the first time arming the timer
then the first timeout will have a delay because of time lapse during the
starting.

Regards,
Bala


On 10 April 2014 14:29, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> btw, do we always need timer tick? Maybe to post some flags to
> odp_init_global() to not init timers if they are not needed?
>
> Maxim.
>
>
> On 04/10/2014 12:55 PM, Bala Manoharan wrote:
>
>> Just to clarify, the behavior of this odp_timer_delete() call this will
>> be similar to shutting down of a timer.
>> Basically if an application issues odp_timer_delete() then he will have
>> to call timer_init_global() again in-order to start the timer.
>>
>> In this case we can rename the API as odp_timer_shutdown()
>> And as  Ola is suggesting, we can have a Odp_shutdown_global() which can
>> internally call shutdown of different modules in the system.
>>
>> Regards,
>> Bala
>>
>>
>> On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org <mailto:
>> ola.liljedahl@linaro.org>> wrote:
>>
>>     So you are not really killing the application, only unloading a
>>     shared library (the application will continue to execute for a
>>     while). This is a different situation. Of course when unloading a
>>     shared library, you must first free all resources used by that
>>     shared library. If the DAQ library uses ODP resources (e.g.
>>     timers), they must be freed in the DAQ shutdown function.
>>
>>     As Petri says, for each odp_create_timer call made by the module
>>     (the DAQ library in this case), there must be an associated
>>     odp_remove_timer call which frees all the resources allocated for
>>     that timer. If the timer is valid, any errors when freeing Linux
>>     resources could be considered fatal errors so just fprintf(stderr)
>>     and call abort(). Something is seriously wrong if you can't free a
>>     Linux per-process timer that was earlier allocated and associated
>>     for this ODP timer. This would be a bug that you would normally
>>     like to investigate and correct. Thus abort() is the proper way to
>>     terminate the execution.
>>
>>     There is actually a general learning here. We need functions for
>>     freeing different ODP resources (buffer pools, queues, timers etc)
>>     because ODP can be used in e.g. dynamically loaded libraries which
>>     are unloaded without the application process terminating. The ODP
>>     timers used Linux per-process timers to not freeing these
>>     resources lead to an immediate crash. Other ODP resources might
>>     only be using memory but not freeing that memory would be a memory
>>     leak and those are also bad.
>>
>>     Is it possible for ODP itself to keep track of all allocated
>>     resources and free all of them using one call? (E.g. some shutdown
>>     call as you suggest below). This would be more robust than
>>     trusting the user to free each individual resource.
>>
>>     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
>>     <mailto:maxim.uvarov@linaro.org>> wrote:
>>
>>         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>>
>>             I don't understand. Isn't the ODP timer implementation a
>>             part of the application (at least in linux-generic)? So if
>>             the application terminates, all the Linux
>>             interval/per-process timers used by the ODP timer
>>             implementation are removed by the kernel? I can't see
>>             anything on the man page of timer_create() that the
>>             application has to reset these timers before/when exiting.
>>
>>
>>         Ola, I found that when did odp-snort. Sort uses DAQ (data
>>         acquisition library) as external library .so. It is used like
>>         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
>>         has implemented init(), shutdown() functions. On init() I call
>>         odp initialization which arms timer for timer_handler. Then I
>>         do some packet processing and press Ctrl-C. Snort calls
>>         shutdown(), then dlclose("daq_odp.so"), then prints some
>>         statistics about the packets. After dlclose() reference to
>>         timer_handler does not exist and timer arms on some unknown
>>         address. That leads to segfault.
>>
>>
>>             And if you really want to disarm all timers why are you
>>             stopping if you fail resetting one? Ignore the return
>>             value and just keep resetting them. No return value, who
>>             is going to check that and what should the caller do if
>>             this function calls an error? Print "sorry failed to reset
>>             timer, won't exit"?
>>
>>         Maybe it's better to use assert() there? Just if application
>>         was not able to cancel timer then we do something wrong and
>>         should crash there? Actually it is bug if timer was triggered
>>         and we can not cancel it.
>>
>>         From my initial idea I thought to call odp_timer_disarm_all()
>>         several times until all timers will be canceled (no error
>>         code). I don't know but maybe there might be some timers which
>>         can't be stopped right now and after delay canceling is possible.
>>
>>         How do you think what is better here?
>>
>>             And shouldn't you install this function as an atexit()
>>             handler?
>>
>>         For current odp examples it's not needed. Due to timers will
>>         be removed as program died. But we should think in future
>>         about some odp_global_shutdown()/odp_thread_shutdown()
>>         functions to free all allocated resources. Which in some cases
>>         might be useful.
>>
>>         Best regards,
>>         Maxim.
>>
>>
>>             On 9 April 2014 19:05, Maxim Uvarov
>>             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>>             <mailto:maxim.uvarov@linaro.org
>>
>>             <mailto:maxim.uvarov@linaro.org>>> wrote:
>>
>>                 Implement function to disarm all timers. Needed in case of
>>                 normal exit from application.
>>                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>             <mailto:maxim.uvarov@linaro.org>
>>                 <mailto:maxim.uvarov@linaro.org
>>
>>             <mailto:maxim.uvarov@linaro.org>>>
>>                 ---
>>                  platform/linux-generic/include/odp_internal.h |  1 +
>>                  platform/linux-generic/source/odp_timer.c     | 24
>>                 ++++++++++++++++++++++++
>>                  2 files changed, 25 insertions(+)
>>
>>                 diff --git a/platform/linux-generic/
>> include/odp_internal.h
>>                 b/platform/linux-generic/include/odp_internal.h
>>                 index fb3be79..9b0769e 100644
>>                 --- a/platform/linux-generic/include/odp_internal.h
>>                 +++ b/platform/linux-generic/include/odp_internal.h
>>                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>>                  int odp_schedule_init_local(void);
>>
>>                  int odp_timer_init_global(void);
>>                 +int odp_timer_disarm_all(void);
>>
>>                  #ifdef __cplusplus
>>                  }
>>                 diff --git a/platform/linux-generic/source/odp_timer.c
>>                 b/platform/linux-generic/source/odp_timer.c
>>                 index 6fb5025..98ffde3 100644
>>                 --- a/platform/linux-generic/source/odp_timer.c
>>                 +++ b/platform/linux-generic/source/odp_timer.c
>>                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>>                         return 0;
>>                  }
>>
>>                 +int odp_timer_disarm_all(void)
>>                 +{
>>                 +       int timers;
>>                 +       struct itimerspec ispec;
>>                 +
>>                 +       timers =
>>             odp_atomic_load_int(&odp_timer.num_timers);
>>                 +
>>                 +       ispec.it_interval.tv_sec  = 0;
>>                 +       ispec.it_interval.tv_nsec = 0;
>>                 +       ispec.it_value.tv_sec     = 0;
>>                 +       ispec.it_value.tv_nsec    = 0;
>>                 +
>>                 +       for (; timers >= 0; timers--) {
>>                 +               if
>>             (timer_settime(odp_timer.timer[timers].timerid,
>>                 +                                 0, &ispec, NULL)) {
>>                 +                       ODP_DBG("Timer reset failed\n");
>>                 +                       return -1;
>>                 +               }
>>                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>>                 +       }
>>                 +
>>                 +       return 0;
>>                 +}
>>                 +
>>                  odp_timer_t odp_timer_create(const char *name,
>>             odp_buffer_pool_t
>>                 pool,
>>                                              uint64_t resolution,
>>             uint64_t min_tmo,
>>                                              uint64_t max_tmo)
>>                 --
>>                 1.8.5.1.163.gd7aced9
>>
>>
>>                 _______________________________________________
>>                 lng-odp mailing list
>>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>             <mailto: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 <mailto:lng-odp@lists.linaro.org>
>>     http://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>
Savolainen, Petri (NSN - FI/Espoo) April 10, 2014, 9:16 a.m. UTC | #9
Hi,

In linux-generic timer create and tick start should called odp_timer_create. It's just half an implementation still... I'll continue work with that and other improvements discussed earlier.

The global init is internal to the implementation (linux-generic). So it's not in API (only timer_create is).

Yes, Ola ODP can track created timers (as any other resources) and can implement an internal timer_global_shutdown which can remove all resources.


-Petri


> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
> Sent: Thursday, April 10, 2014 12:00 PM
> To: Bala Manoharan; Ola Liljedahl
> Cc: lng-odp-forward
> Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
> 
> btw, do we always need timer tick? Maybe to post some flags to
> odp_init_global() to not init timers if they are not needed?
> 
> Maxim.
> 
> On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> > Just to clarify, the behavior of this odp_timer_delete() call this
> > will be similar to shutting down of a timer.
> > Basically if an application issues odp_timer_delete() then he will
> > have to call timer_init_global() again in-order to start the timer.
> >
> > In this case we can rename the API as odp_timer_shutdown()
> > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
> > can internally call shutdown of different modules in the system.
> >
> > Regards,
> > Bala
> >
> >
> > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
> > <mailto:ola.liljedahl@linaro.org>> wrote:
> >
> >     So you are not really killing the application, only unloading a
> >     shared library (the application will continue to execute for a
> >     while). This is a different situation. Of course when unloading a
> >     shared library, you must first free all resources used by that
> >     shared library. If the DAQ library uses ODP resources (e.g.
> >     timers), they must be freed in the DAQ shutdown function.
> >
> >     As Petri says, for each odp_create_timer call made by the module
> >     (the DAQ library in this case), there must be an associated
> >     odp_remove_timer call which frees all the resources allocated for
> >     that timer. If the timer is valid, any errors when freeing Linux
> >     resources could be considered fatal errors so just fprintf(stderr)
> >     and call abort(). Something is seriously wrong if you can't free
> a
> >     Linux per-process timer that was earlier allocated and associated
> >     for this ODP timer. This would be a bug that you would normally
> >     like to investigate and correct. Thus abort() is the proper way
> to
> >     terminate the execution.
> >
> >     There is actually a general learning here. We need functions for
> >     freeing different ODP resources (buffer pools, queues, timers etc)
> >     because ODP can be used in e.g. dynamically loaded libraries
> which
> >     are unloaded without the application process terminating. The ODP
> >     timers used Linux per-process timers to not freeing these
> >     resources lead to an immediate crash. Other ODP resources might
> >     only be using memory but not freeing that memory would be a
> memory
> >     leak and those are also bad.
> >
> >     Is it possible for ODP itself to keep track of all allocated
> >     resources and free all of them using one call? (E.g. some
> shutdown
> >     call as you suggest below). This would be more robust than
> >     trusting the user to free each individual resource.
> >
> >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
> >     <mailto:maxim.uvarov@linaro.org>> wrote:
> >
> >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> >
> >             I don't understand. Isn't the ODP timer implementation a
> >             part of the application (at least in linux-generic)? So
> if
> >             the application terminates, all the Linux
> >             interval/per-process timers used by the ODP timer
> >             implementation are removed by the kernel? I can't see
> >             anything on the man page of timer_create() that the
> >             application has to reset these timers before/when exiting.
> >
> >
> >         Ola, I found that when did odp-snort. Sort uses DAQ (data
> >         acquisition library) as external library .so. It is used like
> >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
> >         has implemented init(), shutdown() functions. On init() I
> call
> >         odp initialization which arms timer for timer_handler. Then I
> >         do some packet processing and press Ctrl-C. Snort calls
> >         shutdown(), then dlclose("daq_odp.so"), then prints some
> >         statistics about the packets. After dlclose() reference to
> >         timer_handler does not exist and timer arms on some unknown
> >         address. That leads to segfault.
> >
> >
> >             And if you really want to disarm all timers why are you
> >             stopping if you fail resetting one? Ignore the return
> >             value and just keep resetting them. No return value, who
> >             is going to check that and what should the caller do if
> >             this function calls an error? Print "sorry failed to
> reset
> >             timer, won't exit"?
> >
> >         Maybe it's better to use assert() there? Just if application
> >         was not able to cancel timer then we do something wrong and
> >         should crash there? Actually it is bug if timer was triggered
> >         and we can not cancel it.
> >
> >         From my initial idea I thought to call odp_timer_disarm_all()
> >         several times until all timers will be canceled (no error
> >         code). I don't know but maybe there might be some timers
> which
> >         can't be stopped right now and after delay canceling is
> possible.
> >
> >         How do you think what is better here?
> >
> >             And shouldn't you install this function as an atexit()
> >             handler?
> >
> >         For current odp examples it's not needed. Due to timers will
> >         be removed as program died. But we should think in future
> >         about some odp_global_shutdown()/odp_thread_shutdown()
> >         functions to free all allocated resources. Which in some
> cases
> >         might be useful.
> >
> >         Best regards,
> >         Maxim.
> >
> >
> >             On 9 April 2014 19:05, Maxim Uvarov
> >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
> >             <mailto:maxim.uvarov@linaro.org
> >             <mailto:maxim.uvarov@linaro.org>>> wrote:
> >
> >                 Implement function to disarm all timers. Needed in
> case of
> >                 normal exit from application.
> >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
> >             <mailto:maxim.uvarov@linaro.org>
> >                 <mailto:maxim.uvarov@linaro.org
> >             <mailto:maxim.uvarov@linaro.org>>>
> >                 ---
> >                  platform/linux-generic/include/odp_internal.h |  1 +
> >                  platform/linux-generic/source/odp_timer.c     | 24
> >                 ++++++++++++++++++++++++
> >                  2 files changed, 25 insertions(+)
> >
> >                 diff --git a/platform/linux-
> generic/include/odp_internal.h
> >                 b/platform/linux-generic/include/odp_internal.h
> >                 index fb3be79..9b0769e 100644
> >                 --- a/platform/linux-generic/include/odp_internal.h
> >                 +++ b/platform/linux-generic/include/odp_internal.h
> >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> >                  int odp_schedule_init_local(void);
> >
> >                  int odp_timer_init_global(void);
> >                 +int odp_timer_disarm_all(void);
> >
> >                  #ifdef __cplusplus
> >                  }
> >                 diff --git a/platform/linux-
> generic/source/odp_timer.c
> >                 b/platform/linux-generic/source/odp_timer.c
> >                 index 6fb5025..98ffde3 100644
> >                 --- a/platform/linux-generic/source/odp_timer.c
> >                 +++ b/platform/linux-generic/source/odp_timer.c
> >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> >                         return 0;
> >                  }
> >
> >                 +int odp_timer_disarm_all(void)
> >                 +{
> >                 +       int timers;
> >                 +       struct itimerspec ispec;
> >                 +
> >                 +       timers =
> >             odp_atomic_load_int(&odp_timer.num_timers);
> >                 +
> >                 +       ispec.it_interval.tv_sec  = 0;
> >                 +       ispec.it_interval.tv_nsec = 0;
> >                 +       ispec.it_value.tv_sec     = 0;
> >                 +       ispec.it_value.tv_nsec    = 0;
> >                 +
> >                 +       for (; timers >= 0; timers--) {
> >                 +               if
> >             (timer_settime(odp_timer.timer[timers].timerid,
> >                 +                                 0, &ispec, NULL)) {
> >                 +                       ODP_DBG("Timer reset
> failed\n");
> >                 +                       return -1;
> >                 +               }
> >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> >                 +       }
> >                 +
> >                 +       return 0;
> >                 +}
> >                 +
> >                  odp_timer_t odp_timer_create(const char *name,
> >             odp_buffer_pool_t
> >                 pool,
> >                                              uint64_t resolution,
> >             uint64_t min_tmo,
> >                                              uint64_t max_tmo)
> >                 --
> >                 1.8.5.1.163.gd7aced9
> >
> >
> >                 _______________________________________________
> >                 lng-odp mailing list
> >             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> >             <mailto: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 <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
Ola Liljedahl April 10, 2014, 12:21 p.m. UTC | #10
When a problem like this is detected ("Oh we need to clean up resources,
there's no support for that in ODP"), why don't write something about the
problem and post to the mailing list so we can have a discussion? I can't
check every patch being posted and understand the scope of the problem that
it tries to solve.

Requests for discussions should be easier to identify and be fewer as well.
When we actually agree on what is the problem and probably have discussed
some potential solutions then someone can code a patch and test it and
finally post that for review.

-- Ola



On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

> Hi,
>
> In linux-generic timer create and tick start should called
> odp_timer_create. It's just half an implementation still... I'll continue
> work with that and other improvements discussed earlier.
>
> The global init is internal to the implementation (linux-generic). So it's
> not in API (only timer_create is).
>
> Yes, Ola ODP can track created timers (as any other resources) and can
> implement an internal timer_global_shutdown which can remove all resources.
>
>
> -Petri
>
>
> > -----Original Message-----
> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
> > Sent: Thursday, April 10, 2014 12:00 PM
> > To: Bala Manoharan; Ola Liljedahl
> > Cc: lng-odp-forward
> > Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
> >
> > btw, do we always need timer tick? Maybe to post some flags to
> > odp_init_global() to not init timers if they are not needed?
> >
> > Maxim.
> >
> > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> > > Just to clarify, the behavior of this odp_timer_delete() call this
> > > will be similar to shutting down of a timer.
> > > Basically if an application issues odp_timer_delete() then he will
> > > have to call timer_init_global() again in-order to start the timer.
> > >
> > > In this case we can rename the API as odp_timer_shutdown()
> > > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
> > > can internally call shutdown of different modules in the system.
> > >
> > > Regards,
> > > Bala
> > >
> > >
> > > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
> > > <mailto:ola.liljedahl@linaro.org>> wrote:
> > >
> > >     So you are not really killing the application, only unloading a
> > >     shared library (the application will continue to execute for a
> > >     while). This is a different situation. Of course when unloading a
> > >     shared library, you must first free all resources used by that
> > >     shared library. If the DAQ library uses ODP resources (e.g.
> > >     timers), they must be freed in the DAQ shutdown function.
> > >
> > >     As Petri says, for each odp_create_timer call made by the module
> > >     (the DAQ library in this case), there must be an associated
> > >     odp_remove_timer call which frees all the resources allocated for
> > >     that timer. If the timer is valid, any errors when freeing Linux
> > >     resources could be considered fatal errors so just fprintf(stderr)
> > >     and call abort(). Something is seriously wrong if you can't free
> > a
> > >     Linux per-process timer that was earlier allocated and associated
> > >     for this ODP timer. This would be a bug that you would normally
> > >     like to investigate and correct. Thus abort() is the proper way
> > to
> > >     terminate the execution.
> > >
> > >     There is actually a general learning here. We need functions for
> > >     freeing different ODP resources (buffer pools, queues, timers etc)
> > >     because ODP can be used in e.g. dynamically loaded libraries
> > which
> > >     are unloaded without the application process terminating. The ODP
> > >     timers used Linux per-process timers to not freeing these
> > >     resources lead to an immediate crash. Other ODP resources might
> > >     only be using memory but not freeing that memory would be a
> > memory
> > >     leak and those are also bad.
> > >
> > >     Is it possible for ODP itself to keep track of all allocated
> > >     resources and free all of them using one call? (E.g. some
> > shutdown
> > >     call as you suggest below). This would be more robust than
> > >     trusting the user to free each individual resource.
> > >
> > >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
> > >     <mailto:maxim.uvarov@linaro.org>> wrote:
> > >
> > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> > >
> > >             I don't understand. Isn't the ODP timer implementation a
> > >             part of the application (at least in linux-generic)? So
> > if
> > >             the application terminates, all the Linux
> > >             interval/per-process timers used by the ODP timer
> > >             implementation are removed by the kernel? I can't see
> > >             anything on the man page of timer_create() that the
> > >             application has to reset these timers before/when exiting.
> > >
> > >
> > >         Ola, I found that when did odp-snort. Sort uses DAQ (data
> > >         acquisition library) as external library .so. It is used like
> > >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
> > >         has implemented init(), shutdown() functions. On init() I
> > call
> > >         odp initialization which arms timer for timer_handler. Then I
> > >         do some packet processing and press Ctrl-C. Snort calls
> > >         shutdown(), then dlclose("daq_odp.so"), then prints some
> > >         statistics about the packets. After dlclose() reference to
> > >         timer_handler does not exist and timer arms on some unknown
> > >         address. That leads to segfault.
> > >
> > >
> > >             And if you really want to disarm all timers why are you
> > >             stopping if you fail resetting one? Ignore the return
> > >             value and just keep resetting them. No return value, who
> > >             is going to check that and what should the caller do if
> > >             this function calls an error? Print "sorry failed to
> > reset
> > >             timer, won't exit"?
> > >
> > >         Maybe it's better to use assert() there? Just if application
> > >         was not able to cancel timer then we do something wrong and
> > >         should crash there? Actually it is bug if timer was triggered
> > >         and we can not cancel it.
> > >
> > >         From my initial idea I thought to call odp_timer_disarm_all()
> > >         several times until all timers will be canceled (no error
> > >         code). I don't know but maybe there might be some timers
> > which
> > >         can't be stopped right now and after delay canceling is
> > possible.
> > >
> > >         How do you think what is better here?
> > >
> > >             And shouldn't you install this function as an atexit()
> > >             handler?
> > >
> > >         For current odp examples it's not needed. Due to timers will
> > >         be removed as program died. But we should think in future
> > >         about some odp_global_shutdown()/odp_thread_shutdown()
> > >         functions to free all allocated resources. Which in some
> > cases
> > >         might be useful.
> > >
> > >         Best regards,
> > >         Maxim.
> > >
> > >
> > >             On 9 April 2014 19:05, Maxim Uvarov
> > >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
> > >             <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>> wrote:
> > >
> > >                 Implement function to disarm all timers. Needed in
> > case of
> > >                 normal exit from application.
> > >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>
> > >                 <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>>
> > >                 ---
> > >                  platform/linux-generic/include/odp_internal.h |  1 +
> > >                  platform/linux-generic/source/odp_timer.c     | 24
> > >                 ++++++++++++++++++++++++
> > >                  2 files changed, 25 insertions(+)
> > >
> > >                 diff --git a/platform/linux-
> > generic/include/odp_internal.h
> > >                 b/platform/linux-generic/include/odp_internal.h
> > >                 index fb3be79..9b0769e 100644
> > >                 --- a/platform/linux-generic/include/odp_internal.h
> > >                 +++ b/platform/linux-generic/include/odp_internal.h
> > >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> > >                  int odp_schedule_init_local(void);
> > >
> > >                  int odp_timer_init_global(void);
> > >                 +int odp_timer_disarm_all(void);
> > >
> > >                  #ifdef __cplusplus
> > >                  }
> > >                 diff --git a/platform/linux-
> > generic/source/odp_timer.c
> > >                 b/platform/linux-generic/source/odp_timer.c
> > >                 index 6fb5025..98ffde3 100644
> > >                 --- a/platform/linux-generic/source/odp_timer.c
> > >                 +++ b/platform/linux-generic/source/odp_timer.c
> > >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> > >                         return 0;
> > >                  }
> > >
> > >                 +int odp_timer_disarm_all(void)
> > >                 +{
> > >                 +       int timers;
> > >                 +       struct itimerspec ispec;
> > >                 +
> > >                 +       timers =
> > >             odp_atomic_load_int(&odp_timer.num_timers);
> > >                 +
> > >                 +       ispec.it_interval.tv_sec  = 0;
> > >                 +       ispec.it_interval.tv_nsec = 0;
> > >                 +       ispec.it_value.tv_sec     = 0;
> > >                 +       ispec.it_value.tv_nsec    = 0;
> > >                 +
> > >                 +       for (; timers >= 0; timers--) {
> > >                 +               if
> > >             (timer_settime(odp_timer.timer[timers].timerid,
> > >                 +                                 0, &ispec, NULL)) {
> > >                 +                       ODP_DBG("Timer reset
> > failed\n");
> > >                 +                       return -1;
> > >                 +               }
> > >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> > >                 +       }
> > >                 +
> > >                 +       return 0;
> > >                 +}
> > >                 +
> > >                  odp_timer_t odp_timer_create(const char *name,
> > >             odp_buffer_pool_t
> > >                 pool,
> > >                                              uint64_t resolution,
> > >             uint64_t min_tmo,
> > >                                              uint64_t max_tmo)
> > >                 --
> > >                 1.8.5.1.163.gd7aced9
> > >
> > >
> > >                 _______________________________________________
> > >                 lng-odp mailing list
> > >             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> > >             <mailto: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 <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
>
Ola Liljedahl April 10, 2014, 1:31 p.m. UTC | #11
Don't spend too much time and effort improving the current implementation...

I am preparing to post two timer-related header files, one for periodic
ticks (used for e.g. packet scheduling, the Cisco QoS case) and one for
(per-flow) supervision/retransmission timers (replacing the current API).
These API's satisfies some constraint I have on timers and function sin
general that are being called by the per-packet processing between flow
setup and flow teardown. E.g. these functions cannot spuriously fail,
arming/resetting a created timeout will always succeed (unless there is
some programming error, e.g. specifying an imvalid timeout). Errors are
only returned for the calls that are typically used when a flow is set up.
This is a much better place to handle resource exhaustion etc.


On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

> Hi,
>
> In linux-generic timer create and tick start should called
> odp_timer_create. It's just half an implementation still... I'll continue
> work with that and other improvements discussed earlier.
>
> The global init is internal to the implementation (linux-generic). So it's
> not in API (only timer_create is).
>
> Yes, Ola ODP can track created timers (as any other resources) and can
> implement an internal timer_global_shutdown which can remove all resources.
>
>
> -Petri
>
>
> > -----Original Message-----
> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
> > Sent: Thursday, April 10, 2014 12:00 PM
> > To: Bala Manoharan; Ola Liljedahl
> > Cc: lng-odp-forward
> > Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
> >
> > btw, do we always need timer tick? Maybe to post some flags to
> > odp_init_global() to not init timers if they are not needed?
> >
> > Maxim.
> >
> > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> > > Just to clarify, the behavior of this odp_timer_delete() call this
> > > will be similar to shutting down of a timer.
> > > Basically if an application issues odp_timer_delete() then he will
> > > have to call timer_init_global() again in-order to start the timer.
> > >
> > > In this case we can rename the API as odp_timer_shutdown()
> > > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
> > > can internally call shutdown of different modules in the system.
> > >
> > > Regards,
> > > Bala
> > >
> > >
> > > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
> > > <mailto:ola.liljedahl@linaro.org>> wrote:
> > >
> > >     So you are not really killing the application, only unloading a
> > >     shared library (the application will continue to execute for a
> > >     while). This is a different situation. Of course when unloading a
> > >     shared library, you must first free all resources used by that
> > >     shared library. If the DAQ library uses ODP resources (e.g.
> > >     timers), they must be freed in the DAQ shutdown function.
> > >
> > >     As Petri says, for each odp_create_timer call made by the module
> > >     (the DAQ library in this case), there must be an associated
> > >     odp_remove_timer call which frees all the resources allocated for
> > >     that timer. If the timer is valid, any errors when freeing Linux
> > >     resources could be considered fatal errors so just fprintf(stderr)
> > >     and call abort(). Something is seriously wrong if you can't free
> > a
> > >     Linux per-process timer that was earlier allocated and associated
> > >     for this ODP timer. This would be a bug that you would normally
> > >     like to investigate and correct. Thus abort() is the proper way
> > to
> > >     terminate the execution.
> > >
> > >     There is actually a general learning here. We need functions for
> > >     freeing different ODP resources (buffer pools, queues, timers etc)
> > >     because ODP can be used in e.g. dynamically loaded libraries
> > which
> > >     are unloaded without the application process terminating. The ODP
> > >     timers used Linux per-process timers to not freeing these
> > >     resources lead to an immediate crash. Other ODP resources might
> > >     only be using memory but not freeing that memory would be a
> > memory
> > >     leak and those are also bad.
> > >
> > >     Is it possible for ODP itself to keep track of all allocated
> > >     resources and free all of them using one call? (E.g. some
> > shutdown
> > >     call as you suggest below). This would be more robust than
> > >     trusting the user to free each individual resource.
> > >
> > >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
> > >     <mailto:maxim.uvarov@linaro.org>> wrote:
> > >
> > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> > >
> > >             I don't understand. Isn't the ODP timer implementation a
> > >             part of the application (at least in linux-generic)? So
> > if
> > >             the application terminates, all the Linux
> > >             interval/per-process timers used by the ODP timer
> > >             implementation are removed by the kernel? I can't see
> > >             anything on the man page of timer_create() that the
> > >             application has to reset these timers before/when exiting.
> > >
> > >
> > >         Ola, I found that when did odp-snort. Sort uses DAQ (data
> > >         acquisition library) as external library .so. It is used like
> > >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
> > >         has implemented init(), shutdown() functions. On init() I
> > call
> > >         odp initialization which arms timer for timer_handler. Then I
> > >         do some packet processing and press Ctrl-C. Snort calls
> > >         shutdown(), then dlclose("daq_odp.so"), then prints some
> > >         statistics about the packets. After dlclose() reference to
> > >         timer_handler does not exist and timer arms on some unknown
> > >         address. That leads to segfault.
> > >
> > >
> > >             And if you really want to disarm all timers why are you
> > >             stopping if you fail resetting one? Ignore the return
> > >             value and just keep resetting them. No return value, who
> > >             is going to check that and what should the caller do if
> > >             this function calls an error? Print "sorry failed to
> > reset
> > >             timer, won't exit"?
> > >
> > >         Maybe it's better to use assert() there? Just if application
> > >         was not able to cancel timer then we do something wrong and
> > >         should crash there? Actually it is bug if timer was triggered
> > >         and we can not cancel it.
> > >
> > >         From my initial idea I thought to call odp_timer_disarm_all()
> > >         several times until all timers will be canceled (no error
> > >         code). I don't know but maybe there might be some timers
> > which
> > >         can't be stopped right now and after delay canceling is
> > possible.
> > >
> > >         How do you think what is better here?
> > >
> > >             And shouldn't you install this function as an atexit()
> > >             handler?
> > >
> > >         For current odp examples it's not needed. Due to timers will
> > >         be removed as program died. But we should think in future
> > >         about some odp_global_shutdown()/odp_thread_shutdown()
> > >         functions to free all allocated resources. Which in some
> > cases
> > >         might be useful.
> > >
> > >         Best regards,
> > >         Maxim.
> > >
> > >
> > >             On 9 April 2014 19:05, Maxim Uvarov
> > >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
> > >             <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>> wrote:
> > >
> > >                 Implement function to disarm all timers. Needed in
> > case of
> > >                 normal exit from application.
> > >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>
> > >                 <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>>
> > >                 ---
> > >                  platform/linux-generic/include/odp_internal.h |  1 +
> > >                  platform/linux-generic/source/odp_timer.c     | 24
> > >                 ++++++++++++++++++++++++
> > >                  2 files changed, 25 insertions(+)
> > >
> > >                 diff --git a/platform/linux-
> > generic/include/odp_internal.h
> > >                 b/platform/linux-generic/include/odp_internal.h
> > >                 index fb3be79..9b0769e 100644
> > >                 --- a/platform/linux-generic/include/odp_internal.h
> > >                 +++ b/platform/linux-generic/include/odp_internal.h
> > >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> > >                  int odp_schedule_init_local(void);
> > >
> > >                  int odp_timer_init_global(void);
> > >                 +int odp_timer_disarm_all(void);
> > >
> > >                  #ifdef __cplusplus
> > >                  }
> > >                 diff --git a/platform/linux-
> > generic/source/odp_timer.c
> > >                 b/platform/linux-generic/source/odp_timer.c
> > >                 index 6fb5025..98ffde3 100644
> > >                 --- a/platform/linux-generic/source/odp_timer.c
> > >                 +++ b/platform/linux-generic/source/odp_timer.c
> > >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> > >                         return 0;
> > >                  }
> > >
> > >                 +int odp_timer_disarm_all(void)
> > >                 +{
> > >                 +       int timers;
> > >                 +       struct itimerspec ispec;
> > >                 +
> > >                 +       timers =
> > >             odp_atomic_load_int(&odp_timer.num_timers);
> > >                 +
> > >                 +       ispec.it_interval.tv_sec  = 0;
> > >                 +       ispec.it_interval.tv_nsec = 0;
> > >                 +       ispec.it_value.tv_sec     = 0;
> > >                 +       ispec.it_value.tv_nsec    = 0;
> > >                 +
> > >                 +       for (; timers >= 0; timers--) {
> > >                 +               if
> > >             (timer_settime(odp_timer.timer[timers].timerid,
> > >                 +                                 0, &ispec, NULL)) {
> > >                 +                       ODP_DBG("Timer reset
> > failed\n");
> > >                 +                       return -1;
> > >                 +               }
> > >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> > >                 +       }
> > >                 +
> > >                 +       return 0;
> > >                 +}
> > >                 +
> > >                  odp_timer_t odp_timer_create(const char *name,
> > >             odp_buffer_pool_t
> > >                 pool,
> > >                                              uint64_t resolution,
> > >             uint64_t min_tmo,
> > >                                              uint64_t max_tmo)
> > >                 --
> > >                 1.8.5.1.163.gd7aced9
> > >
> > >
> > >                 _______________________________________________
> > >                 lng-odp mailing list
> > >             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> > >             <mailto: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 <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
>
Savolainen, Petri (NSN - FI/Espoo) April 10, 2014, 1:42 p.m. UTC | #12
When? Why two APIs? Control of all tmo types (absolute/relative/periodic) should be similar.

-Petri


From: ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
Sent: Thursday, April 10, 2014 4:32 PM
To: Savolainen, Petri (NSN - FI/Espoo)
Cc: ext Maxim Uvarov; Bala Manoharan; lng-odp-forward
Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()

Don't spend too much time and effort improving the current implementation...
I am preparing to post two timer-related header files, one for periodic ticks (used for e.g. packet scheduling, the Cisco QoS case) and one for (per-flow) supervision/retransmission timers (replacing the current API). These API's satisfies some constraint I have on timers and function sin general that are being called by the per-packet processing between flow setup and flow teardown. E.g. these functions cannot spuriously fail, arming/resetting a created timeout will always succeed (unless there is some programming error, e.g. specifying an imvalid timeout). Errors are only returned for the calls that are typically used when a flow is set up. This is a much better place to handle resource exhaustion etc.

On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <petri.savolainen@nsn.com<mailto:petri.savolainen@nsn.com>> wrote:
Hi,

In linux-generic timer create and tick start should called odp_timer_create. It's just half an implementation still... I'll continue work with that and other improvements discussed earlier.

The global init is internal to the implementation (linux-generic). So it's not in API (only timer_create is).

Yes, Ola ODP can track created timers (as any other resources) and can implement an internal timer_global_shutdown which can remove all resources.


-Petri


> -----Original Message-----
> From: lng-odp-bounces@lists.linaro.org<mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-<mailto:lng-odp->
> bounces@lists.linaro.org<mailto:bounces@lists.linaro.org>] On Behalf Of ext Maxim Uvarov
> Sent: Thursday, April 10, 2014 12:00 PM
> To: Bala Manoharan; Ola Liljedahl
> Cc: lng-odp-forward
> Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>
> btw, do we always need timer tick? Maybe to post some flags to
> odp_init_global() to not init timers if they are not needed?
>
> Maxim.
>
> On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> > Just to clarify, the behavior of this odp_timer_delete() call this
> > will be similar to shutting down of a timer.
> > Basically if an application issues odp_timer_delete() then he will
> > have to call timer_init_global() again in-order to start the timer.
> >
> > In this case we can rename the API as odp_timer_shutdown()
> > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
> > can internally call shutdown of different modules in the system.
> >
> > Regards,
> > Bala
> >
> >
> > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org<mailto:ola.liljedahl@linaro.org>
> > <mailto:ola.liljedahl@linaro.org<mailto:ola.liljedahl@linaro.org>>> wrote:
> >
> >     So you are not really killing the application, only unloading a
> >     shared library (the application will continue to execute for a
> >     while). This is a different situation. Of course when unloading a
> >     shared library, you must first free all resources used by that
> >     shared library. If the DAQ library uses ODP resources (e.g.
> >     timers), they must be freed in the DAQ shutdown function.
> >
> >     As Petri says, for each odp_create_timer call made by the module
> >     (the DAQ library in this case), there must be an associated
> >     odp_remove_timer call which frees all the resources allocated for
> >     that timer. If the timer is valid, any errors when freeing Linux
> >     resources could be considered fatal errors so just fprintf(stderr)
> >     and call abort(). Something is seriously wrong if you can't free
> a
> >     Linux per-process timer that was earlier allocated and associated
> >     for this ODP timer. This would be a bug that you would normally
> >     like to investigate and correct. Thus abort() is the proper way
> to
> >     terminate the execution.
> >
> >     There is actually a general learning here. We need functions for
> >     freeing different ODP resources (buffer pools, queues, timers etc)
> >     because ODP can be used in e.g. dynamically loaded libraries
> which
> >     are unloaded without the application process terminating. The ODP
> >     timers used Linux per-process timers to not freeing these
> >     resources lead to an immediate crash. Other ODP resources might
> >     only be using memory but not freeing that memory would be a
> memory
> >     leak and those are also bad.
> >
> >     Is it possible for ODP itself to keep track of all allocated
> >     resources and free all of them using one call? (E.g. some
> shutdown
> >     call as you suggest below). This would be more robust than
> >     trusting the user to free each individual resource.
> >
> >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>
> >     <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>>> wrote:
> >
> >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> >
> >             I don't understand. Isn't the ODP timer implementation a
> >             part of the application (at least in linux-generic)? So
> if
> >             the application terminates, all the Linux
> >             interval/per-process timers used by the ODP timer
> >             implementation are removed by the kernel? I can't see
> >             anything on the man page of timer_create() that the
> >             application has to reset these timers before/when exiting.
> >
> >
> >         Ola, I found that when did odp-snort. Sort uses DAQ (data
> >         acquisition library) as external library .so. It is used like
> >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
> >         has implemented init(), shutdown() functions. On init() I
> call
> >         odp initialization which arms timer for timer_handler. Then I
> >         do some packet processing and press Ctrl-C. Snort calls
> >         shutdown(), then dlclose("daq_odp.so"), then prints some
> >         statistics about the packets. After dlclose() reference to
> >         timer_handler does not exist and timer arms on some unknown
> >         address. That leads to segfault.
> >
> >
> >             And if you really want to disarm all timers why are you
> >             stopping if you fail resetting one? Ignore the return
> >             value and just keep resetting them. No return value, who
> >             is going to check that and what should the caller do if
> >             this function calls an error? Print "sorry failed to
> reset
> >             timer, won't exit"?
> >
> >         Maybe it's better to use assert() there? Just if application
> >         was not able to cancel timer then we do something wrong and
> >         should crash there? Actually it is bug if timer was triggered
> >         and we can not cancel it.
> >
> >         From my initial idea I thought to call odp_timer_disarm_all()
> >         several times until all timers will be canceled (no error
> >         code). I don't know but maybe there might be some timers
> which
> >         can't be stopped right now and after delay canceling is
> possible.
> >
> >         How do you think what is better here?
> >
> >             And shouldn't you install this function as an atexit()
> >             handler?
> >
> >         For current odp examples it's not needed. Due to timers will
> >         be removed as program died. But we should think in future
> >         about some odp_global_shutdown()/odp_thread_shutdown()
> >         functions to free all allocated resources. Which in some
> cases
> >         might be useful.
> >
> >         Best regards,
> >         Maxim.
> >
> >
> >             On 9 April 2014 19:05, Maxim Uvarov
> >             <maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org> <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>>
> >             <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>
> >             <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>>>> wrote:
> >
> >                 Implement function to disarm all timers. Needed in
> case of
> >                 normal exit from application.
> >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>
> >             <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>>
> >                 <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>
> >             <mailto:maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>>>>
> >                 ---
> >                  platform/linux-generic/include/odp_internal.h |  1 +
> >                  platform/linux-generic/source/odp_timer.c     | 24
> >                 ++++++++++++++++++++++++
> >                  2 files changed, 25 insertions(+)
> >
> >                 diff --git a/platform/linux-
> generic/include/odp_internal.h
> >                 b/platform/linux-generic/include/odp_internal.h
> >                 index fb3be79..9b0769e 100644
> >                 --- a/platform/linux-generic/include/odp_internal.h
> >                 +++ b/platform/linux-generic/include/odp_internal.h
> >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> >                  int odp_schedule_init_local(void);
> >
> >                  int odp_timer_init_global(void);
> >                 +int odp_timer_disarm_all(void);
> >
> >                  #ifdef __cplusplus
> >                  }
> >                 diff --git a/platform/linux-
> generic/source/odp_timer.c
> >                 b/platform/linux-generic/source/odp_timer.c
> >                 index 6fb5025..98ffde3 100644
> >                 --- a/platform/linux-generic/source/odp_timer.c
> >                 +++ b/platform/linux-generic/source/odp_timer.c
> >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> >                         return 0;
> >                  }
> >
> >                 +int odp_timer_disarm_all(void)
> >                 +{
> >                 +       int timers;
> >                 +       struct itimerspec ispec;
> >                 +
> >                 +       timers =
> >             odp_atomic_load_int(&odp_timer.num_timers);
> >                 +
> >                 +       ispec.it_interval.tv_sec  = 0;
> >                 +       ispec.it_interval.tv_nsec = 0;
> >                 +       ispec.it_value.tv_sec     = 0;
> >                 +       ispec.it_value.tv_nsec    = 0;
> >                 +
> >                 +       for (; timers >= 0; timers--) {
> >                 +               if
> >             (timer_settime(odp_timer.timer[timers].timerid,
> >                 +                                 0, &ispec, NULL)) {
> >                 +                       ODP_DBG("Timer reset
> failed\n");
> >                 +                       return -1;
> >                 +               }
> >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> >                 +       }
> >                 +
> >                 +       return 0;
> >                 +}
> >                 +
> >                  odp_timer_t odp_timer_create(const char *name,
> >             odp_buffer_pool_t
> >                 pool,
> >                                              uint64_t resolution,
> >             uint64_t min_tmo,
> >                                              uint64_t max_tmo)
> >                 --
> >                 1.8.5.1.163.gd7aced9
> >
> >
> >                 _______________________________________________
> >                 lng-odp mailing list
> >             lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> <mailto:lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>>
> >             <mailto:lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>
> >             <mailto: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<mailto:lng-odp@lists.linaro.org> <mailto: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<mailto:lng-odp@lists.linaro.org>
> http://lists.linaro.org/mailman/listinfo/lng-odp
Ola Liljedahl April 10, 2014, 1:55 p.m. UTC | #13
They are quite different. You will see.
An implementation may however implement them together or one using the
other etc.


On 10 April 2014 15:42, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

>  When? Why two APIs? Control of all tmo types
> (absolute/relative/periodic) should be similar.
>
>
>
> -Petri
>
>
>
>
>
> *From:* ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
> *Sent:* Thursday, April 10, 2014 4:32 PM
> *To:* Savolainen, Petri (NSN - FI/Espoo)
> *Cc:* ext Maxim Uvarov; Bala Manoharan; lng-odp-forward
>
> *Subject:* Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>
>
>
> Don't spend too much time and effort improving the current
> implementation...
>
> I am preparing to post two timer-related header files, one for periodic
> ticks (used for e.g. packet scheduling, the Cisco QoS case) and one for
> (per-flow) supervision/retransmission timers (replacing the current API).
> These API's satisfies some constraint I have on timers and function sin
> general that are being called by the per-packet processing between flow
> setup and flow teardown. E.g. these functions cannot spuriously fail,
> arming/resetting a created timeout will always succeed (unless there is
> some programming error, e.g. specifying an imvalid timeout). Errors are
> only returned for the calls that are typically used when a flow is set up.
> This is a much better place to handle resource exhaustion etc.
>
>
>
> On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <
> petri.savolainen@nsn.com> wrote:
>
> Hi,
>
> In linux-generic timer create and tick start should called
> odp_timer_create. It's just half an implementation still... I'll continue
> work with that and other improvements discussed earlier.
>
> The global init is internal to the implementation (linux-generic). So it's
> not in API (only timer_create is).
>
> Yes, Ola ODP can track created timers (as any other resources) and can
> implement an internal timer_global_shutdown which can remove all resources.
>
>
>
> -Petri
>
>
> > -----Original Message-----
> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
>
> > Sent: Thursday, April 10, 2014 12:00 PM
> > To: Bala Manoharan; Ola Liljedahl
> > Cc: lng-odp-forward
> > Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
> >
>
> > btw, do we always need timer tick? Maybe to post some flags to
> > odp_init_global() to not init timers if they are not needed?
> >
> > Maxim.
> >
> > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
> > > Just to clarify, the behavior of this odp_timer_delete() call this
> > > will be similar to shutting down of a timer.
> > > Basically if an application issues odp_timer_delete() then he will
> > > have to call timer_init_global() again in-order to start the timer.
> > >
> > > In this case we can rename the API as odp_timer_shutdown()
> > > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
> > > can internally call shutdown of different modules in the system.
> > >
> > > Regards,
> > > Bala
> > >
> > >
> > > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
> > > <mailto:ola.liljedahl@linaro.org>> wrote:
> > >
> > >     So you are not really killing the application, only unloading a
> > >     shared library (the application will continue to execute for a
> > >     while). This is a different situation. Of course when unloading a
> > >     shared library, you must first free all resources used by that
> > >     shared library. If the DAQ library uses ODP resources (e.g.
> > >     timers), they must be freed in the DAQ shutdown function.
> > >
> > >     As Petri says, for each odp_create_timer call made by the module
> > >     (the DAQ library in this case), there must be an associated
> > >     odp_remove_timer call which frees all the resources allocated for
> > >     that timer. If the timer is valid, any errors when freeing Linux
> > >     resources could be considered fatal errors so just fprintf(stderr)
> > >     and call abort(). Something is seriously wrong if you can't free
> > a
> > >     Linux per-process timer that was earlier allocated and associated
> > >     for this ODP timer. This would be a bug that you would normally
> > >     like to investigate and correct. Thus abort() is the proper way
> > to
> > >     terminate the execution.
> > >
> > >     There is actually a general learning here. We need functions for
> > >     freeing different ODP resources (buffer pools, queues, timers etc)
> > >     because ODP can be used in e.g. dynamically loaded libraries
> > which
> > >     are unloaded without the application process terminating. The ODP
> > >     timers used Linux per-process timers to not freeing these
> > >     resources lead to an immediate crash. Other ODP resources might
> > >     only be using memory but not freeing that memory would be a
> > memory
> > >     leak and those are also bad.
> > >
> > >     Is it possible for ODP itself to keep track of all allocated
> > >     resources and free all of them using one call? (E.g. some
> > shutdown
> > >     call as you suggest below). This would be more robust than
> > >     trusting the user to free each individual resource.
> > >
> > >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
> > >     <mailto:maxim.uvarov@linaro.org>> wrote:
> > >
> > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
> > >
> > >             I don't understand. Isn't the ODP timer implementation a
> > >             part of the application (at least in linux-generic)? So
> > if
> > >             the application terminates, all the Linux
> > >             interval/per-process timers used by the ODP timer
> > >             implementation are removed by the kernel? I can't see
> > >             anything on the man page of timer_create() that the
> > >             application has to reset these timers before/when exiting.
> > >
> > >
> > >         Ola, I found that when did odp-snort. Sort uses DAQ (data
> > >         acquisition library) as external library .so. It is used like
> > >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
> > >         has implemented init(), shutdown() functions. On init() I
> > call
> > >         odp initialization which arms timer for timer_handler. Then I
> > >         do some packet processing and press Ctrl-C. Snort calls
> > >         shutdown(), then dlclose("daq_odp.so"), then prints some
> > >         statistics about the packets. After dlclose() reference to
> > >         timer_handler does not exist and timer arms on some unknown
> > >         address. That leads to segfault.
> > >
> > >
> > >             And if you really want to disarm all timers why are you
> > >             stopping if you fail resetting one? Ignore the return
> > >             value and just keep resetting them. No return value, who
> > >             is going to check that and what should the caller do if
> > >             this function calls an error? Print "sorry failed to
> > reset
> > >             timer, won't exit"?
> > >
> > >         Maybe it's better to use assert() there? Just if application
> > >         was not able to cancel timer then we do something wrong and
> > >         should crash there? Actually it is bug if timer was triggered
> > >         and we can not cancel it.
> > >
> > >         From my initial idea I thought to call odp_timer_disarm_all()
> > >         several times until all timers will be canceled (no error
> > >         code). I don't know but maybe there might be some timers
> > which
> > >         can't be stopped right now and after delay canceling is
> > possible.
> > >
> > >         How do you think what is better here?
> > >
> > >             And shouldn't you install this function as an atexit()
> > >             handler?
> > >
> > >         For current odp examples it's not needed. Due to timers will
> > >         be removed as program died. But we should think in future
> > >         about some odp_global_shutdown()/odp_thread_shutdown()
> > >         functions to free all allocated resources. Which in some
> > cases
> > >         might be useful.
> > >
> > >         Best regards,
> > >         Maxim.
> > >
> > >
> > >             On 9 April 2014 19:05, Maxim Uvarov
> > >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
> > >             <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>> wrote:
> > >
> > >                 Implement function to disarm all timers. Needed in
> > case of
> > >                 normal exit from application.
> > >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>
> > >                 <mailto:maxim.uvarov@linaro.org
> > >             <mailto:maxim.uvarov@linaro.org>>>
> > >                 ---
> > >                  platform/linux-generic/include/odp_internal.h |  1 +
> > >                  platform/linux-generic/source/odp_timer.c     | 24
> > >                 ++++++++++++++++++++++++
> > >                  2 files changed, 25 insertions(+)
> > >
> > >                 diff --git a/platform/linux-
> > generic/include/odp_internal.h
> > >                 b/platform/linux-generic/include/odp_internal.h
> > >                 index fb3be79..9b0769e 100644
> > >                 --- a/platform/linux-generic/include/odp_internal.h
> > >                 +++ b/platform/linux-generic/include/odp_internal.h
> > >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
> > >                  int odp_schedule_init_local(void);
> > >
> > >                  int odp_timer_init_global(void);
> > >                 +int odp_timer_disarm_all(void);
> > >
> > >                  #ifdef __cplusplus
> > >                  }
> > >                 diff --git a/platform/linux-
> > generic/source/odp_timer.c
> > >                 b/platform/linux-generic/source/odp_timer.c
> > >                 index 6fb5025..98ffde3 100644
> > >                 --- a/platform/linux-generic/source/odp_timer.c
> > >                 +++ b/platform/linux-generic/source/odp_timer.c
> > >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
> > >                         return 0;
> > >                  }
> > >
> > >                 +int odp_timer_disarm_all(void)
> > >                 +{
> > >                 +       int timers;
> > >                 +       struct itimerspec ispec;
> > >                 +
> > >                 +       timers =
> > >             odp_atomic_load_int(&odp_timer.num_timers);
> > >                 +
> > >                 +       ispec.it_interval.tv_sec  = 0;
> > >                 +       ispec.it_interval.tv_nsec = 0;
> > >                 +       ispec.it_value.tv_sec     = 0;
> > >                 +       ispec.it_value.tv_nsec    = 0;
> > >                 +
> > >                 +       for (; timers >= 0; timers--) {
> > >                 +               if
> > >             (timer_settime(odp_timer.timer[timers].timerid,
> > >                 +                                 0, &ispec, NULL)) {
> > >                 +                       ODP_DBG("Timer reset
> > failed\n");
> > >                 +                       return -1;
> > >                 +               }
> > >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
> > >                 +       }
> > >                 +
> > >                 +       return 0;
> > >                 +}
> > >                 +
> > >                  odp_timer_t odp_timer_create(const char *name,
> > >             odp_buffer_pool_t
> > >                 pool,
> > >                                              uint64_t resolution,
> > >             uint64_t min_tmo,
> > >                                              uint64_t max_tmo)
> > >                 --
> > >                 1.8.5.1.163.gd7aced9
> > >
> > >
> > >                 _______________________________________________
> > >                 lng-odp mailing list
> > >             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> > >             <mailto: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 <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
>
>
>
Maxim Uvarov June 16, 2014, 6:06 p.m. UTC | #14
What is the status of new timer implementation? I need function to disarm
timers. If it delays I want to apply my variant which can be rewritten in
future.

Thanks,
Maxim.


On 10 April 2014 17:55, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> They are quite different. You will see.
> An implementation may however implement them together or one using the
> other etc.
>
>
> On 10 April 2014 15:42, Savolainen, Petri (NSN - FI/Espoo) <
> petri.savolainen@nsn.com> wrote:
>
>>  When? Why two APIs? Control of all tmo types
>> (absolute/relative/periodic) should be similar.
>>
>>
>>
>> -Petri
>>
>>
>>
>>
>>
>> *From:* ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
>> *Sent:* Thursday, April 10, 2014 4:32 PM
>> *To:* Savolainen, Petri (NSN - FI/Espoo)
>> *Cc:* ext Maxim Uvarov; Bala Manoharan; lng-odp-forward
>>
>> *Subject:* Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>>
>>
>>
>> Don't spend too much time and effort improving the current
>> implementation...
>>
>> I am preparing to post two timer-related header files, one for periodic
>> ticks (used for e.g. packet scheduling, the Cisco QoS case) and one for
>> (per-flow) supervision/retransmission timers (replacing the current API).
>> These API's satisfies some constraint I have on timers and function sin
>> general that are being called by the per-packet processing between flow
>> setup and flow teardown. E.g. these functions cannot spuriously fail,
>> arming/resetting a created timeout will always succeed (unless there is
>> some programming error, e.g. specifying an imvalid timeout). Errors are
>> only returned for the calls that are typically used when a flow is set up.
>> This is a much better place to handle resource exhaustion etc.
>>
>>
>>
>> On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <
>> petri.savolainen@nsn.com> wrote:
>>
>> Hi,
>>
>> In linux-generic timer create and tick start should called
>> odp_timer_create. It's just half an implementation still... I'll continue
>> work with that and other improvements discussed earlier.
>>
>> The global init is internal to the implementation (linux-generic). So
>> it's not in API (only timer_create is).
>>
>> Yes, Ola ODP can track created timers (as any other resources) and can
>> implement an internal timer_global_shutdown which can remove all resources.
>>
>>
>>
>> -Petri
>>
>>
>> > -----Original Message-----
>> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
>>
>> > Sent: Thursday, April 10, 2014 12:00 PM
>> > To: Bala Manoharan; Ola Liljedahl
>> > Cc: lng-odp-forward
>> > Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>> >
>>
>> > btw, do we always need timer tick? Maybe to post some flags to
>> > odp_init_global() to not init timers if they are not needed?
>> >
>> > Maxim.
>> >
>> > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
>> > > Just to clarify, the behavior of this odp_timer_delete() call this
>> > > will be similar to shutting down of a timer.
>> > > Basically if an application issues odp_timer_delete() then he will
>> > > have to call timer_init_global() again in-order to start the timer.
>> > >
>> > > In this case we can rename the API as odp_timer_shutdown()
>> > > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
>> > > can internally call shutdown of different modules in the system.
>> > >
>> > > Regards,
>> > > Bala
>> > >
>> > >
>> > > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
>> > > <mailto:ola.liljedahl@linaro.org>> wrote:
>> > >
>> > >     So you are not really killing the application, only unloading a
>> > >     shared library (the application will continue to execute for a
>> > >     while). This is a different situation. Of course when unloading a
>> > >     shared library, you must first free all resources used by that
>> > >     shared library. If the DAQ library uses ODP resources (e.g.
>> > >     timers), they must be freed in the DAQ shutdown function.
>> > >
>> > >     As Petri says, for each odp_create_timer call made by the module
>> > >     (the DAQ library in this case), there must be an associated
>> > >     odp_remove_timer call which frees all the resources allocated for
>> > >     that timer. If the timer is valid, any errors when freeing Linux
>> > >     resources could be considered fatal errors so just fprintf(stderr)
>> > >     and call abort(). Something is seriously wrong if you can't free
>> > a
>> > >     Linux per-process timer that was earlier allocated and associated
>> > >     for this ODP timer. This would be a bug that you would normally
>> > >     like to investigate and correct. Thus abort() is the proper way
>> > to
>> > >     terminate the execution.
>> > >
>> > >     There is actually a general learning here. We need functions for
>> > >     freeing different ODP resources (buffer pools, queues, timers etc)
>> > >     because ODP can be used in e.g. dynamically loaded libraries
>> > which
>> > >     are unloaded without the application process terminating. The ODP
>> > >     timers used Linux per-process timers to not freeing these
>> > >     resources lead to an immediate crash. Other ODP resources might
>> > >     only be using memory but not freeing that memory would be a
>> > memory
>> > >     leak and those are also bad.
>> > >
>> > >     Is it possible for ODP itself to keep track of all allocated
>> > >     resources and free all of them using one call? (E.g. some
>> > shutdown
>> > >     call as you suggest below). This would be more robust than
>> > >     trusting the user to free each individual resource.
>> > >
>> > >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
>> > >     <mailto:maxim.uvarov@linaro.org>> wrote:
>> > >
>> > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>> > >
>> > >             I don't understand. Isn't the ODP timer implementation a
>> > >             part of the application (at least in linux-generic)? So
>> > if
>> > >             the application terminates, all the Linux
>> > >             interval/per-process timers used by the ODP timer
>> > >             implementation are removed by the kernel? I can't see
>> > >             anything on the man page of timer_create() that the
>> > >             application has to reset these timers before/when exiting.
>> > >
>> > >
>> > >         Ola, I found that when did odp-snort. Sort uses DAQ (data
>> > >         acquisition library) as external library .so. It is used like
>> > >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
>> > >         has implemented init(), shutdown() functions. On init() I
>> > call
>> > >         odp initialization which arms timer for timer_handler. Then I
>> > >         do some packet processing and press Ctrl-C. Snort calls
>> > >         shutdown(), then dlclose("daq_odp.so"), then prints some
>> > >         statistics about the packets. After dlclose() reference to
>> > >         timer_handler does not exist and timer arms on some unknown
>> > >         address. That leads to segfault.
>> > >
>> > >
>> > >             And if you really want to disarm all timers why are you
>> > >             stopping if you fail resetting one? Ignore the return
>> > >             value and just keep resetting them. No return value, who
>> > >             is going to check that and what should the caller do if
>> > >             this function calls an error? Print "sorry failed to
>> > reset
>> > >             timer, won't exit"?
>> > >
>> > >         Maybe it's better to use assert() there? Just if application
>> > >         was not able to cancel timer then we do something wrong and
>> > >         should crash there? Actually it is bug if timer was triggered
>> > >         and we can not cancel it.
>> > >
>> > >         From my initial idea I thought to call odp_timer_disarm_all()
>> > >         several times until all timers will be canceled (no error
>> > >         code). I don't know but maybe there might be some timers
>> > which
>> > >         can't be stopped right now and after delay canceling is
>> > possible.
>> > >
>> > >         How do you think what is better here?
>> > >
>> > >             And shouldn't you install this function as an atexit()
>> > >             handler?
>> > >
>> > >         For current odp examples it's not needed. Due to timers will
>> > >         be removed as program died. But we should think in future
>> > >         about some odp_global_shutdown()/odp_thread_shutdown()
>> > >         functions to free all allocated resources. Which in some
>> > cases
>> > >         might be useful.
>> > >
>> > >         Best regards,
>> > >         Maxim.
>> > >
>> > >
>> > >             On 9 April 2014 19:05, Maxim Uvarov
>> > >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>> > >             <mailto:maxim.uvarov@linaro.org
>> > >             <mailto:maxim.uvarov@linaro.org>>> wrote:
>> > >
>> > >                 Implement function to disarm all timers. Needed in
>> > case of
>> > >                 normal exit from application.
>> > >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>> > >             <mailto:maxim.uvarov@linaro.org>
>> > >                 <mailto:maxim.uvarov@linaro.org
>> > >             <mailto:maxim.uvarov@linaro.org>>>
>> > >                 ---
>> > >                  platform/linux-generic/include/odp_internal.h |  1 +
>> > >                  platform/linux-generic/source/odp_timer.c     | 24
>> > >                 ++++++++++++++++++++++++
>> > >                  2 files changed, 25 insertions(+)
>> > >
>> > >                 diff --git a/platform/linux-
>> > generic/include/odp_internal.h
>> > >                 b/platform/linux-generic/include/odp_internal.h
>> > >                 index fb3be79..9b0769e 100644
>> > >                 --- a/platform/linux-generic/include/odp_internal.h
>> > >                 +++ b/platform/linux-generic/include/odp_internal.h
>> > >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>> > >                  int odp_schedule_init_local(void);
>> > >
>> > >                  int odp_timer_init_global(void);
>> > >                 +int odp_timer_disarm_all(void);
>> > >
>> > >                  #ifdef __cplusplus
>> > >                  }
>> > >                 diff --git a/platform/linux-
>> > generic/source/odp_timer.c
>> > >                 b/platform/linux-generic/source/odp_timer.c
>> > >                 index 6fb5025..98ffde3 100644
>> > >                 --- a/platform/linux-generic/source/odp_timer.c
>> > >                 +++ b/platform/linux-generic/source/odp_timer.c
>> > >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>> > >                         return 0;
>> > >                  }
>> > >
>> > >                 +int odp_timer_disarm_all(void)
>> > >                 +{
>> > >                 +       int timers;
>> > >                 +       struct itimerspec ispec;
>> > >                 +
>> > >                 +       timers =
>> > >             odp_atomic_load_int(&odp_timer.num_timers);
>> > >                 +
>> > >                 +       ispec.it_interval.tv_sec  = 0;
>> > >                 +       ispec.it_interval.tv_nsec = 0;
>> > >                 +       ispec.it_value.tv_sec     = 0;
>> > >                 +       ispec.it_value.tv_nsec    = 0;
>> > >                 +
>> > >                 +       for (; timers >= 0; timers--) {
>> > >                 +               if
>> > >             (timer_settime(odp_timer.timer[timers].timerid,
>> > >                 +                                 0, &ispec, NULL)) {
>> > >                 +                       ODP_DBG("Timer reset
>> > failed\n");
>> > >                 +                       return -1;
>> > >                 +               }
>> > >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>> > >                 +       }
>> > >                 +
>> > >                 +       return 0;
>> > >                 +}
>> > >                 +
>> > >                  odp_timer_t odp_timer_create(const char *name,
>> > >             odp_buffer_pool_t
>> > >                 pool,
>> > >                                              uint64_t resolution,
>> > >             uint64_t min_tmo,
>> > >                                              uint64_t max_tmo)
>> > >                 --
>> > >                 1.8.5.1.163.gd7aced9
>> > >
>> > >
>> > >                 _______________________________________________
>> > >                 lng-odp mailing list
>> > >             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org
>> >
>> > >             <mailto: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 <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
>>
>>
>>
>
>
Ola Liljedahl June 16, 2014, 10:27 p.m. UTC | #15
The new SW reference implementation is pretty much done (if you don't mind
C++). I need to add spinlocks in some places to make it multicore-safe,
this should not take long, stay tuned for updates. But me providing a
proper patch might be the biggest problem... Perhaps I can get some help
here?

But doesn't the current implementation and API implement a cancel function?
Can't you use this?

-- Ola







On 16 June 2014 20:06, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> What is the status of new timer implementation? I need function to disarm
> timers. If it delays I want to apply my variant which can be rewritten in
> future.
>
> Thanks,
> Maxim.
>
>
> On 10 April 2014 17:55, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>
>> They are quite different. You will see.
>> An implementation may however implement them together or one using the
>> other etc.
>>
>>
>> On 10 April 2014 15:42, Savolainen, Petri (NSN - FI/Espoo) <
>> petri.savolainen@nsn.com> wrote:
>>
>>>  When? Why two APIs? Control of all tmo types
>>> (absolute/relative/periodic) should be similar.
>>>
>>>
>>>
>>> -Petri
>>>
>>>
>>>
>>>
>>>
>>> *From:* ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org]
>>> *Sent:* Thursday, April 10, 2014 4:32 PM
>>> *To:* Savolainen, Petri (NSN - FI/Espoo)
>>> *Cc:* ext Maxim Uvarov; Bala Manoharan; lng-odp-forward
>>>
>>> *Subject:* Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>>>
>>>
>>>
>>> Don't spend too much time and effort improving the current
>>> implementation...
>>>
>>> I am preparing to post two timer-related header files, one for periodic
>>> ticks (used for e.g. packet scheduling, the Cisco QoS case) and one for
>>> (per-flow) supervision/retransmission timers (replacing the current API).
>>> These API's satisfies some constraint I have on timers and function sin
>>> general that are being called by the per-packet processing between flow
>>> setup and flow teardown. E.g. these functions cannot spuriously fail,
>>> arming/resetting a created timeout will always succeed (unless there is
>>> some programming error, e.g. specifying an imvalid timeout). Errors are
>>> only returned for the calls that are typically used when a flow is set up.
>>> This is a much better place to handle resource exhaustion etc.
>>>
>>>
>>>
>>> On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo) <
>>> petri.savolainen@nsn.com> wrote:
>>>
>>> Hi,
>>>
>>> In linux-generic timer create and tick start should called
>>> odp_timer_create. It's just half an implementation still... I'll continue
>>> work with that and other improvements discussed earlier.
>>>
>>> The global init is internal to the implementation (linux-generic). So
>>> it's not in API (only timer_create is).
>>>
>>> Yes, Ola ODP can track created timers (as any other resources) and can
>>> implement an internal timer_global_shutdown which can remove all resources.
>>>
>>>
>>>
>>> -Petri
>>>
>>>
>>> > -----Original Message-----
>>> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>>> > bounces@lists.linaro.org] On Behalf Of ext Maxim Uvarov
>>>
>>> > Sent: Thursday, April 10, 2014 12:00 PM
>>> > To: Bala Manoharan; Ola Liljedahl
>>> > Cc: lng-odp-forward
>>> > Subject: Re: [lng-odp] [ODP/PATCH] implement odp_timer_disarm_all()
>>> >
>>>
>>> > btw, do we always need timer tick? Maybe to post some flags to
>>> > odp_init_global() to not init timers if they are not needed?
>>> >
>>> > Maxim.
>>> >
>>> > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
>>> > > Just to clarify, the behavior of this odp_timer_delete() call this
>>> > > will be similar to shutting down of a timer.
>>> > > Basically if an application issues odp_timer_delete() then he will
>>> > > have to call timer_init_global() again in-order to start the timer.
>>> > >
>>> > > In this case we can rename the API as odp_timer_shutdown()
>>> > > And as  Ola is suggesting, we can have a Odp_shutdown_global() which
>>> > > can internally call shutdown of different modules in the system.
>>> > >
>>> > > Regards,
>>> > > Bala
>>> > >
>>> > >
>>> > > On 10 April 2014 14:20, Ola Liljedahl <ola.liljedahl@linaro.org
>>> > > <mailto:ola.liljedahl@linaro.org>> wrote:
>>> > >
>>> > >     So you are not really killing the application, only unloading a
>>> > >     shared library (the application will continue to execute for a
>>> > >     while). This is a different situation. Of course when unloading a
>>> > >     shared library, you must first free all resources used by that
>>> > >     shared library. If the DAQ library uses ODP resources (e.g.
>>> > >     timers), they must be freed in the DAQ shutdown function.
>>> > >
>>> > >     As Petri says, for each odp_create_timer call made by the module
>>> > >     (the DAQ library in this case), there must be an associated
>>> > >     odp_remove_timer call which frees all the resources allocated for
>>> > >     that timer. If the timer is valid, any errors when freeing Linux
>>> > >     resources could be considered fatal errors so just
>>> fprintf(stderr)
>>> > >     and call abort(). Something is seriously wrong if you can't free
>>> > a
>>> > >     Linux per-process timer that was earlier allocated and associated
>>> > >     for this ODP timer. This would be a bug that you would normally
>>> > >     like to investigate and correct. Thus abort() is the proper way
>>> > to
>>> > >     terminate the execution.
>>> > >
>>> > >     There is actually a general learning here. We need functions for
>>> > >     freeing different ODP resources (buffer pools, queues, timers
>>> etc)
>>> > >     because ODP can be used in e.g. dynamically loaded libraries
>>> > which
>>> > >     are unloaded without the application process terminating. The ODP
>>> > >     timers used Linux per-process timers to not freeing these
>>> > >     resources lead to an immediate crash. Other ODP resources might
>>> > >     only be using memory but not freeing that memory would be a
>>> > memory
>>> > >     leak and those are also bad.
>>> > >
>>> > >     Is it possible for ODP itself to keep track of all allocated
>>> > >     resources and free all of them using one call? (E.g. some
>>> > shutdown
>>> > >     call as you suggest below). This would be more robust than
>>> > >     trusting the user to free each individual resource.
>>> > >
>>> > >     On 10 April 2014 08:44, Maxim Uvarov <maxim.uvarov@linaro.org
>>> > >     <mailto:maxim.uvarov@linaro.org>> wrote:
>>> > >
>>> > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>>> > >
>>> > >             I don't understand. Isn't the ODP timer implementation a
>>> > >             part of the application (at least in linux-generic)? So
>>> > if
>>> > >             the application terminates, all the Linux
>>> > >             interval/per-process timers used by the ODP timer
>>> > >             implementation are removed by the kernel? I can't see
>>> > >             anything on the man page of timer_create() that the
>>> > >             application has to reset these timers before/when
>>> exiting.
>>> > >
>>> > >
>>> > >         Ola, I found that when did odp-snort. Sort uses DAQ (data
>>> > >         acquisition library) as external library .so. It is used like
>>> > >         plug-in.  I.e. snort calls dlopen("daq_odp.so"). daq_odp.so
>>> > >         has implemented init(), shutdown() functions. On init() I
>>> > call
>>> > >         odp initialization which arms timer for timer_handler. Then I
>>> > >         do some packet processing and press Ctrl-C. Snort calls
>>> > >         shutdown(), then dlclose("daq_odp.so"), then prints some
>>> > >         statistics about the packets. After dlclose() reference to
>>> > >         timer_handler does not exist and timer arms on some unknown
>>> > >         address. That leads to segfault.
>>> > >
>>> > >
>>> > >             And if you really want to disarm all timers why are you
>>> > >             stopping if you fail resetting one? Ignore the return
>>> > >             value and just keep resetting them. No return value, who
>>> > >             is going to check that and what should the caller do if
>>> > >             this function calls an error? Print "sorry failed to
>>> > reset
>>> > >             timer, won't exit"?
>>> > >
>>> > >         Maybe it's better to use assert() there? Just if application
>>> > >         was not able to cancel timer then we do something wrong and
>>> > >         should crash there? Actually it is bug if timer was triggered
>>> > >         and we can not cancel it.
>>> > >
>>> > >         From my initial idea I thought to call odp_timer_disarm_all()
>>> > >         several times until all timers will be canceled (no error
>>> > >         code). I don't know but maybe there might be some timers
>>> > which
>>> > >         can't be stopped right now and after delay canceling is
>>> > possible.
>>> > >
>>> > >         How do you think what is better here?
>>> > >
>>> > >             And shouldn't you install this function as an atexit()
>>> > >             handler?
>>> > >
>>> > >         For current odp examples it's not needed. Due to timers will
>>> > >         be removed as program died. But we should think in future
>>> > >         about some odp_global_shutdown()/odp_thread_shutdown()
>>> > >         functions to free all allocated resources. Which in some
>>> > cases
>>> > >         might be useful.
>>> > >
>>> > >         Best regards,
>>> > >         Maxim.
>>> > >
>>> > >
>>> > >             On 9 April 2014 19:05, Maxim Uvarov
>>> > >             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org
>>> >
>>> > >             <mailto:maxim.uvarov@linaro.org
>>> > >             <mailto:maxim.uvarov@linaro.org>>> wrote:
>>> > >
>>> > >                 Implement function to disarm all timers. Needed in
>>> > case of
>>> > >                 normal exit from application.
>>> > >                 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>> > >             <mailto:maxim.uvarov@linaro.org>
>>> > >                 <mailto:maxim.uvarov@linaro.org
>>> > >             <mailto:maxim.uvarov@linaro.org>>>
>>> > >                 ---
>>> > >                  platform/linux-generic/include/odp_internal.h |  1 +
>>> > >                  platform/linux-generic/source/odp_timer.c     | 24
>>> > >                 ++++++++++++++++++++++++
>>> > >                  2 files changed, 25 insertions(+)
>>> > >
>>> > >                 diff --git a/platform/linux-
>>> > generic/include/odp_internal.h
>>> > >                 b/platform/linux-generic/include/odp_internal.h
>>> > >                 index fb3be79..9b0769e 100644
>>> > >                 --- a/platform/linux-generic/include/odp_internal.h
>>> > >                 +++ b/platform/linux-generic/include/odp_internal.h
>>> > >                 @@ -38,6 +38,7 @@ int odp_schedule_init_global(void);
>>> > >                  int odp_schedule_init_local(void);
>>> > >
>>> > >                  int odp_timer_init_global(void);
>>> > >                 +int odp_timer_disarm_all(void);
>>> > >
>>> > >                  #ifdef __cplusplus
>>> > >                  }
>>> > >                 diff --git a/platform/linux-
>>> > generic/source/odp_timer.c
>>> > >                 b/platform/linux-generic/source/odp_timer.c
>>> > >                 index 6fb5025..98ffde3 100644
>>> > >                 --- a/platform/linux-generic/source/odp_timer.c
>>> > >                 +++ b/platform/linux-generic/source/odp_timer.c
>>> > >                 @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>>> > >                         return 0;
>>> > >                  }
>>> > >
>>> > >                 +int odp_timer_disarm_all(void)
>>> > >                 +{
>>> > >                 +       int timers;
>>> > >                 +       struct itimerspec ispec;
>>> > >                 +
>>> > >                 +       timers =
>>> > >             odp_atomic_load_int(&odp_timer.num_timers);
>>> > >                 +
>>> > >                 +       ispec.it_interval.tv_sec  = 0;
>>> > >                 +       ispec.it_interval.tv_nsec = 0;
>>> > >                 +       ispec.it_value.tv_sec     = 0;
>>> > >                 +       ispec.it_value.tv_nsec    = 0;
>>> > >                 +
>>> > >                 +       for (; timers >= 0; timers--) {
>>> > >                 +               if
>>> > >             (timer_settime(odp_timer.timer[timers].timerid,
>>> > >                 +                                 0, &ispec, NULL)) {
>>> > >                 +                       ODP_DBG("Timer reset
>>> > failed\n");
>>> > >                 +                       return -1;
>>> > >                 +               }
>>> > >                 + odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>>> > >                 +       }
>>> > >                 +
>>> > >                 +       return 0;
>>> > >                 +}
>>> > >                 +
>>> > >                  odp_timer_t odp_timer_create(const char *name,
>>> > >             odp_buffer_pool_t
>>> > >                 pool,
>>> > >                                              uint64_t resolution,
>>> > >             uint64_t min_tmo,
>>> > >                                              uint64_t max_tmo)
>>> > >                 --
>>> > >                 1.8.5.1.163.gd7aced9
>>> > >
>>> > >
>>> > >                 _______________________________________________
>>> > >                 lng-odp mailing list
>>> > >             lng-odp@lists.linaro.org <mailto:
>>> lng-odp@lists.linaro.org>
>>> > >             <mailto: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 <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
>>>
>>>
>>>
>>
>>
>
Maxim Uvarov June 17, 2014, 6:53 a.m. UTC | #16
On 06/17/2014 02:27 AM, Ola Liljedahl wrote:
> The new SW reference implementation is pretty much done (if you don't 
> mind C++).
Hm, C++? Well let's see this first.

> I need to add spinlocks in some places to make it multicore-safe, this 
> should not take long, stay tuned for updates. But me providing a 
> proper patch might be the biggest problem... Perhaps I can get some 
> help here?
>
yes, sure please let me know if you have any problems with git. Will try 
to help you.
> But doesn't the current implementation and API implement a cancel 
> function? Can't you use this?
>
> -- Ola
>
No I can't. This timer is set in odp_init_global() and to cancel it 
access for odp internal api is needed.

Maxim.
>
>
>
>
>
>
> On 16 June 2014 20:06, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     What is the status of new timer implementation? I need function to
>     disarm timers. If it delays I want to apply my variant which can
>     be rewritten in future.
>
>     Thanks,
>     Maxim.
>
>
>     On 10 April 2014 17:55, Ola Liljedahl <ola.liljedahl@linaro.org
>     <mailto:ola.liljedahl@linaro.org>> wrote:
>
>         They are quite different. You will see.
>         An implementation may however implement them together or one
>         using the other etc.
>
>
>         On 10 April 2014 15:42, Savolainen, Petri (NSN - FI/Espoo)
>         <petri.savolainen@nsn.com <mailto:petri.savolainen@nsn.com>>
>         wrote:
>
>             When? Why two APIs? Control of all tmo types
>             (absolute/relative/periodic) should be similar.
>
>             -Petri
>
>             *From:*ext Ola Liljedahl [mailto:ola.liljedahl@linaro.org
>             <mailto:ola.liljedahl@linaro.org>]
>             *Sent:* Thursday, April 10, 2014 4:32 PM
>             *To:* Savolainen, Petri (NSN - FI/Espoo)
>             *Cc:* ext Maxim Uvarov; Bala Manoharan; lng-odp-forward
>
>
>             *Subject:* Re: [lng-odp] [ODP/PATCH] implement
>             odp_timer_disarm_all()
>
>             Don't spend too much time and effort improving the current
>             implementation...
>
>             I am preparing to post two timer-related header files, one
>             for periodic ticks (used for e.g. packet scheduling, the
>             Cisco QoS case) and one for (per-flow)
>             supervision/retransmission timers (replacing the current
>             API). These API's satisfies some constraint I have on
>             timers and function sin general that are being called by
>             the per-packet processing between flow setup and flow
>             teardown. E.g. these functions cannot spuriously fail,
>             arming/resetting a created timeout will always succeed
>             (unless there is some programming error, e.g. specifying
>             an imvalid timeout). Errors are only returned for the
>             calls that are typically used when a flow is set up. This
>             is a much better place to handle resource exhaustion etc.
>
>             On 10 April 2014 11:16, Savolainen, Petri (NSN - FI/Espoo)
>             <petri.savolainen@nsn.com
>             <mailto:petri.savolainen@nsn.com>> wrote:
>
>             Hi,
>
>             In linux-generic timer create and tick start should called
>             odp_timer_create. It's just half an implementation
>             still... I'll continue work with that and other
>             improvements discussed earlier.
>
>             The global init is internal to the implementation
>             (linux-generic). So it's not in API (only timer_create is).
>
>             Yes, Ola ODP can track created timers (as any other
>             resources) and can implement an internal
>             timer_global_shutdown which can remove all resources.
>
>
>
>             -Petri
>
>
>             > -----Original Message-----
>             > From: lng-odp-bounces@lists.linaro.org
>             <mailto:lng-odp-bounces@lists.linaro.org> [mailto:lng-odp-
>             <mailto:lng-odp->
>             > bounces@lists.linaro.org
>             <mailto:bounces@lists.linaro.org>] On Behalf Of ext Maxim
>             Uvarov
>
>             > Sent: Thursday, April 10, 2014 12:00 PM
>             > To: Bala Manoharan; Ola Liljedahl
>             > Cc: lng-odp-forward
>             > Subject: Re: [lng-odp] [ODP/PATCH] implement
>             odp_timer_disarm_all()
>             >
>
>             > btw, do we always need timer tick? Maybe to post some
>             flags to
>             > odp_init_global() to not init timers if they are not needed?
>             >
>             > Maxim.
>             >
>             > On 04/10/2014 12:55 PM, Bala Manoharan wrote:
>             > > Just to clarify, the behavior of this
>             odp_timer_delete() call this
>             > > will be similar to shutting down of a timer.
>             > > Basically if an application issues odp_timer_delete()
>             then he will
>             > > have to call timer_init_global() again in-order to
>             start the timer.
>             > >
>             > > In this case we can rename the API as odp_timer_shutdown()
>             > > And as  Ola is suggesting, we can have a
>             Odp_shutdown_global() which
>             > > can internally call shutdown of different modules in
>             the system.
>             > >
>             > > Regards,
>             > > Bala
>             > >
>             > >
>             > > On 10 April 2014 14:20, Ola Liljedahl
>             <ola.liljedahl@linaro.org <mailto:ola.liljedahl@linaro.org>
>             > > <mailto:ola.liljedahl@linaro.org
>             <mailto:ola.liljedahl@linaro.org>>> wrote:
>             > >
>             > >     So you are not really killing the application,
>             only unloading a
>             > >     shared library (the application will continue to
>             execute for a
>             > > while). This is a different situation. Of course when
>             unloading a
>             > >     shared library, you must first free all resources
>             used by that
>             > >     shared library. If the DAQ library uses ODP
>             resources (e.g.
>             > > timers), they must be freed in the DAQ shutdown function.
>             > >
>             > >     As Petri says, for each odp_create_timer call made
>             by the module
>             > >     (the DAQ library in this case), there must be an
>             associated
>             > > odp_remove_timer call which frees all the resources
>             allocated for
>             > >     that timer. If the timer is valid, any errors when
>             freeing Linux
>             > > resources could be considered fatal errors so just
>             fprintf(stderr)
>             > >     and call abort(). Something is seriously wrong if
>             you can't free
>             > a
>             > >     Linux per-process timer that was earlier allocated
>             and associated
>             > >     for this ODP timer. This would be a bug that you
>             would normally
>             > >     like to investigate and correct. Thus abort() is
>             the proper way
>             > to
>             > > terminate the execution.
>             > >
>             > >     There is actually a general learning here. We need
>             functions for
>             > > freeing different ODP resources (buffer pools, queues,
>             timers etc)
>             > > because ODP can be used in e.g. dynamically loaded
>             libraries
>             > which
>             > >     are unloaded without the application process
>             terminating. The ODP
>             > >     timers used Linux per-process timers to not
>             freeing these
>             > > resources lead to an immediate crash. Other ODP
>             resources might
>             > >     only be using memory but not freeing that memory
>             would be a
>             > memory
>             > >     leak and those are also bad.
>             > >
>             > >     Is it possible for ODP itself to keep track of all
>             allocated
>             > > resources and free all of them using one call? (E.g. some
>             > shutdown
>             > >     call as you suggest below). This would be more
>             robust than
>             > > trusting the user to free each individual resource.
>             > >
>             > >     On 10 April 2014 08:44, Maxim Uvarov
>             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>             > > <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>> wrote:
>             > >
>             > >         On 04/09/2014 11:38 PM, Ola Liljedahl wrote:
>             > >
>             > >   I don't understand. Isn't the ODP timer implementation a
>             > >   part of the application (at least in linux-generic)? So
>             > if
>             > >   the application terminates, all the Linux
>             > > interval/per-process timers used by the ODP timer
>             > >   implementation are removed by the kernel? I can't see
>             > >   anything on the man page of timer_create() that the
>             > >   application has to reset these timers before/when
>             exiting.
>             > >
>             > >
>             > > Ola, I found that when did odp-snort. Sort uses DAQ (data
>             > > acquisition library) as external library .so. It is
>             used like
>             > > plug-in.  I.e. snort calls dlopen("daq_odp.so").
>             daq_odp.so
>             > > has implemented init(), shutdown() functions. On init() I
>             > call
>             > > odp initialization which arms timer for timer_handler.
>             Then I
>             > >         do some packet processing and press Ctrl-C.
>             Snort calls
>             > > shutdown(), then dlclose("daq_odp.so"), then prints some
>             > > statistics about the packets. After dlclose() reference to
>             > > timer_handler does not exist and timer arms on some
>             unknown
>             > > address. That leads to segfault.
>             > >
>             > >
>             > >   And if you really want to disarm all timers why are you
>             > >   stopping if you fail resetting one? Ignore the return
>             > >   value and just keep resetting them. No return value, who
>             > >   is going to check that and what should the caller do if
>             > >   this function calls an error? Print "sorry failed to
>             > reset
>             > >   timer, won't exit"?
>             > >
>             > > Maybe it's better to use assert() there? Just if
>             application
>             > > was not able to cancel timer then we do something
>             wrong and
>             > > should crash there? Actually it is bug if timer was
>             triggered
>             > > and we can not cancel it.
>             > >
>             > > From my initial idea I thought to call
>             odp_timer_disarm_all()
>             > > several times until all timers will be canceled (no error
>             > > code). I don't know but maybe there might be some timers
>             > which
>             > > can't be stopped right now and after delay canceling is
>             > possible.
>             > >
>             > > How do you think what is better here?
>             > >
>             > >   And shouldn't you install this function as an atexit()
>             > >   handler?
>             > >
>             > > For current odp examples it's not needed. Due to
>             timers will
>             > >         be removed as program died. But we should
>             think in future
>             > > about some odp_global_shutdown()/odp_thread_shutdown()
>             > > functions to free all allocated resources. Which in some
>             > cases
>             > > might be useful.
>             > >
>             > > Best regards,
>             > > Maxim.
>             > >
>             > >
>             > >   On 9 April 2014 19:05, Maxim Uvarov
>             > >   <maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>
>             <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>
>             > >   <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>
>             > >   <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>>> wrote:
>             > >
>             > >       Implement function to disarm all timers. Needed in
>             > case of
>             > >       normal exit from application.
>             > >       Signed-off-by: Maxim Uvarov
>             <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>             > >   <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>
>             > >       <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>
>             > >   <mailto:maxim.uvarov@linaro.org
>             <mailto:maxim.uvarov@linaro.org>>>>
>             > >       ---
>             > >  platform/linux-generic/include/odp_internal.h |  1 +
>             > >  platform/linux-generic/source/odp_timer.c     | 24
>             > > ++++++++++++++++++++++++
>             > >        2 files changed, 25 insertions(+)
>             > >
>             > >       diff --git a/platform/linux-
>             > generic/include/odp_internal.h
>             > > b/platform/linux-generic/include/odp_internal.h
>             > >       index fb3be79..9b0769e 100644
>             > >       --- a/platform/linux-generic/include/odp_internal.h
>             > >       +++ b/platform/linux-generic/include/odp_internal.h
>             > >       @@ -38,6 +38,7 @@ int
>             odp_schedule_init_global(void);
>             > >        int odp_schedule_init_local(void);
>             > >
>             > >        int odp_timer_init_global(void);
>             > >       +int odp_timer_disarm_all(void);
>             > >
>             > >        #ifdef __cplusplus
>             > >        }
>             > >       diff --git a/platform/linux-
>             > generic/source/odp_timer.c
>             > > b/platform/linux-generic/source/odp_timer.c
>             > >       index 6fb5025..98ffde3 100644
>             > >       --- a/platform/linux-generic/source/odp_timer.c
>             > >       +++ b/platform/linux-generic/source/odp_timer.c
>             > >       @@ -217,6 +217,30 @@ int odp_timer_init_global(void)
>             > >               return 0;
>             > >        }
>             > >
>             > >       +int odp_timer_disarm_all(void)
>             > >       +{
>             > >       +       int timers;
>             > >       +       struct itimerspec ispec;
>             > >       +
>             > >       +       timers =
>             > > odp_atomic_load_int(&odp_timer.num_timers);
>             > >       +
>             > >       + ispec.it_interval.tv_sec  = 0;
>             > >       + ispec.it_interval.tv_nsec = 0;
>             > >       + ispec.it_value.tv_sec     = 0;
>             > >       + ispec.it_value.tv_nsec    = 0;
>             > >       +
>             > >       +       for (; timers >= 0; timers--) {
>             > >       +   if
>             > > (timer_settime(odp_timer.timer[timers].timerid,
>             > >       + 0, &ispec, NULL)) {
>             > >       + ODP_DBG("Timer reset
>             > failed\n");
>             > >       +           return -1;
>             > >       +   }
>             > >       +
>             odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
>             > >       +       }
>             > >       +
>             > >       +       return 0;
>             > >       +}
>             > >       +
>             > >        odp_timer_t odp_timer_create(const char *name,
>             > >   odp_buffer_pool_t
>             > >       pool,
>             > >  uint64_t resolution,
>             > >   uint64_t min_tmo,
>             > >  uint64_t max_tmo)
>             > >       --
>             > > 1.8.5.1.163.gd7aced9
>             > >
>             > >
>             > > _______________________________________________
>             > >       lng-odp mailing list
>             > > lng-odp@lists.linaro.org
>             <mailto:lng-odp@lists.linaro.org>
>             <mailto:lng-odp@lists.linaro.org
>             <mailto:lng-odp@lists.linaro.org>>
>             > >   <mailto:lng-odp@lists.linaro.org
>             <mailto:lng-odp@lists.linaro.org>
>             > >   <mailto: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
>             <mailto:lng-odp@lists.linaro.org>
>             <mailto: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 <mailto:lng-odp@lists.linaro.org>
>
>             > http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
diff mbox

Patch

diff --git a/platform/linux-generic/include/odp_internal.h b/platform/linux-generic/include/odp_internal.h
index fb3be79..9b0769e 100644
--- a/platform/linux-generic/include/odp_internal.h
+++ b/platform/linux-generic/include/odp_internal.h
@@ -38,6 +38,7 @@  int odp_schedule_init_global(void);
 int odp_schedule_init_local(void);
 
 int odp_timer_init_global(void);
+int odp_timer_disarm_all(void);
 
 #ifdef __cplusplus
 }
diff --git a/platform/linux-generic/source/odp_timer.c b/platform/linux-generic/source/odp_timer.c
index 6fb5025..98ffde3 100644
--- a/platform/linux-generic/source/odp_timer.c
+++ b/platform/linux-generic/source/odp_timer.c
@@ -217,6 +217,30 @@  int odp_timer_init_global(void)
 	return 0;
 }
 
+int odp_timer_disarm_all(void)
+{
+	int timers;
+	struct itimerspec ispec;
+
+	timers = odp_atomic_load_int(&odp_timer.num_timers);
+
+	ispec.it_interval.tv_sec  = 0;
+	ispec.it_interval.tv_nsec = 0;
+	ispec.it_value.tv_sec     = 0;
+	ispec.it_value.tv_nsec    = 0;
+
+	for (; timers >= 0; timers--) {
+		if (timer_settime(odp_timer.timer[timers].timerid,
+				  0, &ispec, NULL)) {
+			ODP_DBG("Timer reset failed\n");
+			return -1;
+		}
+		odp_atomic_fetch_sub_int(&odp_timer.num_timers, 1);
+	}
+
+	return 0;
+}
+
 odp_timer_t odp_timer_create(const char *name, odp_buffer_pool_t pool,
 			     uint64_t resolution, uint64_t min_tmo,
 			     uint64_t max_tmo)