Message ID | 1397063130-10525-1-git-send-email-maxim.uvarov@linaro.org |
---|---|
State | New |
Headers | show |
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 >
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 > >
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
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
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 >> >> >> >
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 > >
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 > >
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 >> >> >> >
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
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 >
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 >
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
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 > > >
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 >> >> >> > >
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 >>> >>> >>> >> >> >
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 --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)
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(+)